Perform an initial fetch when starting the app - #4
Conversation
WalkthroughAdds background sync task tracking to App, spawns sync in background on startup and on 'r' key, tracks JoinHandle, handles task completion in the renderer to update status/errors and reload data, and overlays a modal SyncingDialog during loading/syncing. Introduces and exports the SyncingDialog component and adjusts dialog exports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/ui/app.rs (1)
36-37: Tighten the JoinHandle type and plan for graceful shutdown
- Prefer the already-imported SyncStatus over the fully-qualified crate::sync::SyncStatus for consistency and readability.
- Optional: introduce a type alias to reduce verbosity.
- Optional: abort any in-flight sync on shutdown to avoid a background task lingering after the UI exits.
Apply this minimal consistency diff:
- pub sync_task: Option<JoinHandle<anyhow::Result<crate::sync::SyncStatus>>>, + pub sync_task: Option<JoinHandle<anyhow::Result<SyncStatus>>>,Optional alias and Drop (outside the selected lines):
// near the top of this file, after imports type SyncTaskHandle = JoinHandle<anyhow::Result<SyncStatus>>; // field could then be: // pub sync_task: Option<SyncTaskHandle>, // ensure background task doesn't outlive the app impl Drop for App { fn drop(&mut self) { if let Some(handle) = &self.sync_task { handle.abort(); } } }src/ui/components/dialogs/syncing_dialog.rs (2)
25-31: Clarify quit instructions in the dialog copyUsers can also quit with Ctrl+C per event handling. Suggest updating the message for consistency.
- Line::from(Span::raw("Press q to quit")), + Line::from(Span::raw("Press q or Ctrl+C to quit")),
43-69: Clamp percentages to avoid underflow for invalid inputsIf percent_x or percent_y > 100, (100 - percent) underflows (u16). It’s safe with current callers (50, 25) but trivial to harden.
- fn centered_rect(percent_x: u16, percent_y: u16, r: Rect) -> Rect { - let popup_layout = Layout::default() + fn centered_rect(percent_x: u16, percent_y: u16, r: Rect) -> Rect { + let px = percent_x.min(100); + let py = percent_y.min(100); + let popup_layout = Layout::default() .direction(Direction::Vertical) .constraints( [ - Constraint::Percentage((100 - percent_y) / 2), - Constraint::Percentage(percent_y), - Constraint::Percentage((100 - percent_y) / 2), + Constraint::Percentage((100 - py) / 2), + Constraint::Percentage(py), + Constraint::Percentage((100 - py) / 2), ] .as_ref(), ) .split(r); - let horizontal = Layout::default() + let horizontal = Layout::default() .direction(Direction::Horizontal) .constraints( [ - Constraint::Percentage((100 - percent_x) / 2), - Constraint::Percentage(percent_x), - Constraint::Percentage((100 - percent_x) / 2), + Constraint::Percentage((100 - px) / 2), + Constraint::Percentage(px), + Constraint::Percentage((100 - px) / 2), ] .as_ref(), ) .split(popup_layout[1]); horizontal[1] }src/ui/renderer.rs (3)
35-37: Confirm loading UX is surfaced during local loadYou render the SyncingDialog when app.loading || app.syncing. Please confirm App::load_local_data sets app.loading true/false internally; otherwise users may not see the “Loading local data” overlay during the initial load.
89-109: Simplify JoinHandle handling and reload only on successTwo tweaks:
- Avoid borrowing Option then mutating it; compute finished first, then take(). This sidesteps subtle borrow patterns.
- Only reload local data on a successful sync. Errors already surface via error_message; skipping a reload avoids unnecessary I/O after a failed sync.
Apply this diff:
- if let Some(handle_ref) = app.sync_task.as_ref() { - if handle_ref.is_finished() { - if let Some(handle) = app.sync_task.take() { - match handle.await { - Ok(Ok(status)) => { - app.last_sync_status = status; - app.load_local_data(sync_service).await; - } - Ok(Err(e)) => { - app.error_message = Some(format!("Sync failed: {e}")); - } - Err(join_err) => { - app.error_message = Some(format!("Sync task error: {join_err}")); - } - } - app.syncing = false; - } - } - } + if app + .sync_task + .as_ref() + .map(|h| h.is_finished()) + .unwrap_or(false) + { + if let Some(handle) = app.sync_task.take() { + match handle.await { + Ok(Ok(status)) => { + let should_reload = matches!(status, crate::sync::SyncStatus::Success { .. }); + app.last_sync_status = status; + if should_reload { + app.load_local_data(sync_service).await; + } + } + Ok(Err(e)) => { + app.error_message = Some(format!("Sync failed: {e}")); + } + Err(join_err) => { + app.error_message = Some(format!("Sync task error: {join_err}")); + } + } + app.syncing = false; + } + }If you prefer not to use a fully-qualified path in matches!, add this import at the top of the file:
use crate::sync::SyncStatus;
38-49: Guard against double-spawning sync tasks in renderer.rsAdd the same
app.sync_task.is_none()check you have inevents.rsto the initial-sync branch insrc/ui/renderer.rs, so a rapid manual trigger can’t stomp on the initialJoinHandle.Locations to update:
- src/ui/renderer.rs, in the
Ok(false)arm of thematch sync_service.has_local_data().awaitProposed diff:
--- a/src/ui/renderer.rs +++ b/src/ui/renderer.rs @@ 38,9 +38,13 @@ impl Renderer { // If local DB is empty, start an initial sync in the background match sync_service.has_local_data().await { Ok(false) => { - app.syncing = true; - let svc = sync_service.clone(); - app.sync_task = Some(tokio::spawn(async move { svc.force_sync().await })); + // Avoid overwriting an existing sync task + if app.sync_task.is_none() { + app.syncing = true; + let svc = sync_service.clone(); + app.sync_task = Some(tokio::spawn(async move { svc.force_sync().await })); + } } Ok(true) => {} Err(e) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/ui/app.rs(3 hunks)src/ui/components/dialogs/mod.rs(1 hunks)src/ui/components/dialogs/syncing_dialog.rs(1 hunks)src/ui/components/mod.rs(1 hunks)src/ui/events.rs(1 hunks)src/ui/renderer.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/ui/components/dialogs/syncing_dialog.rs (1)
src/ui/app.rs (2)
new(60-97)default(52-54)
src/ui/app.rs (3)
src/sync.rs (6)
sync(157-177)SyncService(25-324)SyncStats(328-333)perform_sync(180-235)SyncService(11-15)SyncStatus(18-23)src/main.rs (1)
sync(3-3)src/lib.rs (1)
sync(3-3)
src/ui/events.rs (1)
src/sync.rs (1)
SyncService(25-324)
src/ui/renderer.rs (5)
src/ui/components/dialogs/syncing_dialog.rs (1)
render(16-41)src/ui/components/dialogs/error_dialog.rs (1)
render(18-34)src/ui/components/status_bar.rs (1)
render(17-45)src/sync.rs (5)
SyncService(25-324)SyncService(11-15)SyncStats(328-333)perform_sync(180-235)SyncStatus(18-23)src/storage.rs (1)
LocalStorage(160-688)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test (beta)
- GitHub Check: Security Audit
🔇 Additional comments (9)
src/ui/app.rs (2)
7-7: JoinHandle import — looks goodImports the Tokio JoinHandle needed for background sync orchestration.
84-84: Initialization to None — OKsync_task starts empty and will be set when a background sync is spawned.
src/ui/components/dialogs/mod.rs (2)
7-7: New syncing_dialog module wiring — OKModule is declared alongside the existing dialogs.
14-14: Re-export SyncingDialog — OKPublic re-export enables ergonomic use at crate::ui::components::SyncingDialog.
src/ui/events.rs (1)
238-243: Background sync behavior verifiedAll checks pass:
- In
src/ui/renderer.rs, afterhandle_ref.is_finished(), the code callsapp.sync_task.take(), awaits the handle, updatesapp.last_sync_statusand reloads data on success, handles both service errors and panics by settingapp.error_message, and finally setsapp.syncing = false.SyncServiceinsrc/sync.rsis#[derive(Clone)].No further changes needed.
src/ui/components/mod.rs (1)
10-12: Public re-exports updated — OKSyncingDialog is now available to consumers; grouping remains consistent with other dialog exports.
src/ui/components/dialogs/syncing_dialog.rs (1)
15-41: Rendering logic and modal overlay — OKCentered layout, Clear overlay, and simple status messaging are implemented cleanly.
src/ui/renderer.rs (2)
15-16: Importing SyncingDialog into the renderer is correctGood integration of the new modal component into the renderer’s import list.
128-131: Syncing/Loading overlay is correctly integratedThe modal overlay condition matches the new state flags and will provide clear feedback during initial load and background sync.
1 participant
Summary by CodeRabbit
New Features
Improvements