Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-document editor session management and a DocEditor route; introduces startup progress reporting and an app-startup clock; increases SQLite busy/acquire timeouts to 90s; routes FFI repo stop/shutdown through the shared runtime; adds platform graceful shutdown wiring; refactors bottom navigation into a floating bar/menu and many UI/navigation wiring changes. Changes
Sequence Diagram(s)sequenceDiagram
participant AppFfi as AppFfiCtx
participant Rt as Rt::boot
participant Progress as ProgressRepo
participant InitRepo as InitRepo
participant UI as Compose UI
AppFfi->>AppFfi: init_app_startup_clock() (OnceLock)
AppFfi->>Rt: Rt::boot(startup_progress_task_id)
Rt->>Progress: emit_startup_progress_status(stage begin)
Rt->>InitRepo: ensure_plug_init_dispatches(..., startup_task_id, total_started)
InitRepo->>Progress: report_boot_init_stage(...)
Rt->>Progress: emit_startup_progress_status(stage complete)
Rt->>UI: signal boot completed
sequenceDiagram
participant UI as DrawerScreen
participant Store as DocEditorStoreViewModel
participant Repo as DrawerRepoFfi
participant Ctrl as EditorSessionController
UI->>Store: selectDoc(docId)
Store->>Repo: getBundle(docId, "main")
Repo-->>Store: DocBundle
Store->>Store: createOrRefreshSession(docId)
Store->>Ctrl: bindDoc(doc, bundle, allowOverwrite=false)
Ctrl->>Ctrl: compute draftFingerprint & decide bind/save
Store-->>UI: selectedDocId & selectedController
UI->>UI: navigate to DocEditor
sequenceDiagram
participant Desktop as Desktop entry
participant App as App Composable
participant Shutdown as shutdownAppContainer
participant UI as App UI
Desktop->>Desktop: receive TERM/INT or window close
Desktop->>App: set shutdownRequested=true
App->>Shutdown: shutdownAppContainer()
Shutdown->>Shutdown: stop repos, flush, close FFI (via fcx.do_on_rt where applicable)
Shutdown-->>App: onShutdownCompleted()
App->>UI: show "Shutting down..." overlay
Desktop->>Desktop: exitApplication()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt (1)
96-115:⚠️ Potential issue | 🟡 MinorPotential unhandled error from
tablesRepo.setWindow().Per coding guidelines, errors should crash the program or show a toast if non-critical. The
setWindow()call could potentially fail (network, database, etc.), but the error is silently swallowed within the coroutine. Consider adding error handling to show a toast on failure.Suggested error handling
private fun persistCaptureMode(mode: CaptureMode) { viewModelScope.launch { + try { val state = tablesVm.tablesState.value // ... existing logic ... windowId?.let { id -> state.windows[id]?.let { window -> tablesRepo.setWindow(id, window.copy(lastCaptureMode = mode)) } } + } catch (e: Exception) { + // Mode persistence failed - non-critical, mode still changed locally + println("Failed to persist capture mode: ${e.message}") + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt` around lines 96 - 115, The persistCaptureMode coroutine silently swallows failures from tablesRepo.setWindow; wrap the body of viewModelScope.launch in a try/catch (inside persistCaptureMode) and handle exceptions from tablesRepo.setWindow: on non-recoverable errors rethrow or call a crash/report path, otherwise display a user-facing toast/error via your UI helper (e.g., the app's toast/snackbar service) and log the exception with context (mentioning persistCaptureMode, selectedTableId, windowId) so failures are not lost. Ensure the catch only surrounds the async work (inside viewModelScope.launch) and preserves structured concurrency semantics.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt (1)
682-689:⚠️ Potential issue | 🟠 MajorHonor
enabledin the narrow rail click handler.This branch still calls
item.onActivate()whenitem.enabled == false, so disabled destinations remain reachable in narrow mode. The wide-mode path already does the right guard.Suggested fix
NavigationRailItem( selected = isSelected, onClick = { scope.launch { - item.onActivate() + if (item.enabled) { + item.onActivate() + } } }, enabled = item.enabled,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt` around lines 682 - 689, The narrow-mode NavigationRailItem currently invokes item.onActivate() unconditionally; change the click handler in the NavigationRailItem used for narrow rail so it respects item.enabled by either setting onClick to null when item.enabled is false or guarding the lambda (e.g., if (item.enabled) scope.launch { item.onActivate() })—update the NavigationRailItem instance that has onClick = { scope.launch { item.onActivate() } } to include this enabled check so disabled destinations are not activated.src/daybook_core/rt.rs (1)
723-834:⚠️ Potential issue | 🟠 Major
Nonehere still reports into the startup task.The new callers at Line 1506 and Line 1551 pass
None, butInitRepo::report_boot_init_stage()falls back to the repo-levelstartup_progress_task_idwhen the override is absent. That means non-boot dispatches can keep appendingrt init per-boot: ...updates to the startup task long after boot has finished. IfNoneis supposed to disable reporting outside boot, this needs an explicit “disabled” state or a guard that skipsreport_boot_init_stage()when no boot context was supplied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_core/rt.rs` around lines 723 - 834, The ensure_plug_init_dispatches function currently always calls init_repo.report_boot_init_stage(...) even when startup_progress_task_id is None; change it so that when startup_progress_task_id is None you do not call InitRepo::report_boot_init_stage (i.e., guard each report_boot_init_stage invocation in ensure_plug_init_dispatches behind a check like if startup_progress_task_id.is_some()), or alternatively extend init::BootInitProgressContext with an explicit disabled flag and pass that through and make InitRepo::report_boot_init_stage respect it; update all three places in ensure_plug_init_dispatches where report_boot_init_stage is invoked so non-boot callers that pass None no longer append to the startup task.
🧹 Nitpick comments (6)
src/daybook_core/local_state.rs (1)
124-131: Consider unifying SQLite timeout values across the codebase.This change correctly applies the new timeout constants. However, other files like
progress.rs(5000ms via PRAGMA) andsync.rs(5s) still use shorter timeouts. Consider a follow-up to unify these values or document why different components need different timeout thresholds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_core/local_state.rs` around lines 124 - 131, The new constants SQLITE_BUSY_TIMEOUT and SQLITE_POOL_ACQUIRE_TIMEOUT are applied here but other places (e.g., the PRAGMA busy_timeout in progress.rs and the 5s timeout in sync.rs) still use different values; update those call sites to reference the shared constants (replace hard-coded 5000/5s values and PRAGMA usage with SQLITE_BUSY_TIMEOUT/SQLITE_POOL_ACQUIRE_TIMEOUT as appropriate) or add a brief comment explaining why a different timeout is required so the timeout policy is unified and discoverable across LocalState (the SqlitePoolOptions setup) and the other modules.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.kt (1)
331-336: Make the amount summary pick its latest entry explicitly too.The timeline is now sorted by
sequence, but the "Progress" block still comes fromlatestAmountEntry(), which depends on the rawupdatesorder. That leaves this screen with two different ordering rules. UsingmaxByOrNull { it.sequence }for amount entries as well would make the summary and timeline stay in sync even if repository ordering changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.kt` around lines 331 - 336, The amount summary is still chosen from latestAmountEntry() which relies on raw updates order; change it to pick the amount entry by sequence so it matches timelineUpdates ordering: when computing the summary, filter updates for ProgressUpdateDeets.Amount and select the one with the highest sequence using maxByOrNull { it.sequence } (instead of relying on latestAmountEntry() or list order) so the "Progress" block and timeline use the same ordering rule; update references to latestAmountEntry() accordingly in ProgressScreen.src/am_utils_rs/repo.rs (1)
29-29: Centralize the SQLite acquire timeout.This 90-second constant is now duplicated here and in
src/daybook_core/repo.rs. Pulling it into one shared location avoids the two repo bootstrap paths drifting apart later.Also applies to: 154-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/am_utils_rs/repo.rs` at line 29, The constant SQLITE_POOL_ACQUIRE_TIMEOUT (currently defined as std::time::Duration::from_secs(90) in repo.rs) is duplicated; remove the local definition and reference a single shared constant instead. Create or use an existing shared module (e.g., a common/constants or config module) that exposes SQLITE_POOL_ACQUIRE_TIMEOUT and update the usages in functions/constructors that rely on it (including the repo bootstrap paths and the other occurrence in daybook_core::repo.rs) to import and use that shared symbol so both code paths read the same centralized value.src/daybook_core/app.rs (1)
7-8: Centralize these SQLite timeout constants.The same 90-second values are now being wired in multiple pool builders in this PR, so future tuning can drift silently. A shared timeout module would keep
SqlCtx, local state, and repo boot paths in sync.Also applies to: 32-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_core/app.rs` around lines 7 - 8, Centralize the duplicated 90s SQLite timeouts by extracting the two constants (currently defined as SQLITE_POOL_ACQUIRE_TIMEOUT and SQLITE_BUSY_TIMEOUT in app.rs) into a single shared module or constants file (e.g., sqlite::timeouts or config::sqlite_timeouts) and update all callers — the SqlCtx pool builder, any local state pool builders, and repo boot/initialization code that set busy/acquire timeouts — to import and use those centralized constants instead of duplicating the values; remove the duplicated definitions in app.rs and other files so all code refers to the single source of truth.src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt (1)
31-33: Signal handler installation failure is logged but not surfaced to user.Per coding guidelines, errors should generally crash or show a toast rather than being silently logged. However, signal handler installation is a soft requirement (the app functions without it via window close), so this catch-and-log may be intentional. If this is by design, consider adding a brief comment explaining why silent failure is acceptable here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt` around lines 31 - 33, The onFailure handler that logs signal installation errors (the lambda using onFailure { error -> println("[APP_SHUTDOWN] failed to install signal handler name=$signalName err=${error.message}") }) should either surface the failure to the user or document why silent logging is acceptable; update the handler so it shows a brief user-visible notification (e.g., a toast/dialog) when installation fails, or if the silent fallback is intentional, replace the println with a concise comment above the onFailure explaining that signal handler installation is optional and failures are intentionally non-fatal so normal window close behavior remains and no user interruption is required.src/daybook_core/rt/init.rs (1)
394-409: Add stage context before propagatingadd_updatefailures.If this bubbles up today, the caller only gets the generic progress-repo error. Wrapping it with
plug_id/init_key/stagewould make startup failures much easier to diagnose.Suggested fix
self.progress_repo .add_update( &task_id, crate::progress::ProgressUpdate { at: jiff::Timestamp::now(), @@ }, ) - .await + .await + .wrap_err_with(|| { + format!( + "report_boot_init_stage failed for {plug_id}/{init_key} stage={stage}" + ) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_core/rt/init.rs` around lines 394 - 409, When calling self.progress_repo.add_update(...).await, capture and wrap any error with context that includes plug_id, init_key and stage (and optionally task_id) before returning so callers don't just get the generic progress-repo error; replace the bare .await return with an error-mapping that appends a message like "failed to add_update for {plug_id}/{init_key} {stage}" (using map_err or ? with context/anyhow) referencing the add_update call, task_id, plug_id, init_key and stage to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 17: Replace the misspelled word "sanboxing" with "sandboxing" in the
README entry that currently reads "Agent sanboxing/orchestration." so the phrase
becomes "Agent sandboxing/orchestration."; update any identical occurrences of
"sanboxing" elsewhere in the README to "sandboxing" to keep docs consistent.
In `@src/daybook_compose/composeApp/build.gradle.kts`:
- Around line 390-401: The existence check for the Rust shared library (the File
constructed as sourceSoFile using cargoTargetDir and sourceLibPath) is currently
executed during task configuration; move that check into the task's execution
phase by placing it inside a doFirst { ... } block of the Sync task registered
with tasks.register<Sync>(taskName) (after dependsOn(buildTaskName) and before
from(sourceSoFile)/into(destDir)), so the check runs at execution time and will
succeed if buildTaskName produces the file.
In
`@src/daybook_compose/composeApp/src/androidMain/kotlin/org/example/daybook/MainActivity.kt`:
- Around line 114-118: The ON_DESTROY handler in MainActivity.kt sets
shutdownRequested but if the composition is already tearing down the
LaunchedEffect(shutdownRequested, ...) may never run and DisposableEffect
fallback is disabled; enable the safety net by setting autoShutdownOnDispose =
true (the variable referenced in App.kt) so the DisposableEffect onDispose calls
shutdownAppContainer() when composition is disposed, or alternatively move the
shutdownRequested toggle earlier than the Lifecycle.Event.ON_DESTROY check so
LaunchedEffect can observe it; update the autoShutdownOnDispose flag or adjust
where shutdownRequested is set to ensure shutdownAppContainer() always runs.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt`:
- Around line 916-948: If a partial FFI load succeeds (SyncRepoFfi.load) but a
subsequent load (RtFfi.load) throws, the code currently sets initState =
AppInitState.Error without closing the already-created syncRepo and other
partial resources; modify the try/catch so that in the non-cancellation catch
path you explicitly close/cleanup any partially initialized FFIs (at least call
the syncRepo's close/dispose method created by SyncRepoFfi.load and any other
resources from current that were newly assigned) before assigning
AppInitState.Error, mirroring the cleanup behavior elsewhere (reference
SyncRepoFfi.load, RtFfi.load, initState, AppInitState.Error, and
current/DisposableEffect/autoShutdownOnDispose to locate the logic). Ensure
cleanup runs even if multiple partial objects exist and does not rethrow
cancellation exceptions.
- Around line 817-857: The startup progress is being marked complete via
startupProgress.complete(…) (STARTUP_PROGRESS_TASK_ID) before the runtime is
loaded, causing UI to show "startup complete" while RtFfi.load() is still
booting; either delay calling startupProgress.complete until after
RtFfi.load(...) returns successfully (move the complete(...) call to after the
withStartupStage block that runs RtFfi.load) or create a separate progress task
for the runtime boot and use that new task id for RtFfi.load; update references
around withStartupStage("warmUpTablesRepo", ...), the warmUpTablesRepo call, and
the code that invokes RtFfi.load to ensure the final complete() corresponds to
the actual finished work.
- Around line 1007-1009: The viewModel() calls for DrawerViewModel and
DocEditorStoreViewModel are unkeyed and will be reused across different
AppContainer instances; change the calls to pass a stable key derived from the
active container (e.g., appContainer identifier such as
appContainer.ffiCtx.toString() or repo id) so the ViewModels are recreated when
the repo/container changes: update the two calls where viewModel {
DrawerViewModel(appContainer.drawerRepo) } and viewModel {
DocEditorStoreViewModel(appContainer.drawerRepo) } to use viewModel(key =
containerKey) { ... } with a container-specific key.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt`:
- Around line 61-64: The init block launches a fire-and-forget coroutine that
calls ffiRegisterListener(listener) and assigns listenerRegistration, but
onCleared() can run before that coroutine completes, leaving a stray listener;
tie the registration coroutine to the ViewModel lifecycle by capturing the Job
from viewModelScope.launch (e.g. registerJob) and canceling it in onCleared(),
and/or make the coroutine check isActive before assigning listenerRegistration
so it never assigns after cancellation; update references to
ffiRegisterListener, listenerRegistration, viewModelScope.launch, listener, and
onCleared to implement this cancellation/guard so the listener cannot outlive
the ViewModel.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.kt`:
- Around line 467-478: The pinned Surface header (controlled by
showPinnedTimelineHeader in ProgressScreen.kt) is drawn over the LazyColumn and
hides the first timeline row; fix by either converting the header into a
LazyColumn stickyHeader or by adding a matching top inset to the LazyColumn (use
contentPadding = PaddingValues(top = headerHeight)) when
showPinnedTimelineHeader is true so the first item is pushed below the Surface;
reference the Surface block and the LazyColumn declaration in ProgressScreen.kt
and ensure the header height used for padding matches the Surface’s measured
height.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt`:
- Around line 389-416: The drag-release path currently activates items without
checking their enabled state; update the checks in the highlightedMenuItem
handling and the navBarFeatureControllers/prominentButtonControllers loops to
verify the target is enabled before invoking actions. Specifically, when
resolving feature via allMenuItems.find for highlightedMenuItem, ensure
feature.enabled (or equivalent) is true before calling feature.onActivate();
likewise, in navBarFeatureControllers -> navBarFeatures and
prominentButtonControllers -> prominentButtons, confirm feature.enabled or
button.enabled is true before launching feature.onActivate() or
button.onClick(), preserving the existing activationReadyMenuItem and
ctrl.ready.value guards.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/DocEditor.kt`:
- Around line 103-108: The current code computes a single document-wide
saveStatus (using saveStatus derived from state.saveError and state.isSaving)
and renders it in every FacetHeader, causing each facet to show the same save
icon; either move that indicator out of FacetHeader and render it once at the
editor level, or change the state shape and plumbing to be facet-specific (e.g.,
add per-facet saving flags and errors such as facetSaveErrors:
Map<FacetId,Error> and facetIsSaving: Set<FacetId>), compute a facet-specific
status inside the FacetHeader rendering path (use facetId to look up
facetIsSaving or facetSaveErrors) and pass only that status to FacetHeader
instead of the shared saveStatus; update usages where saveStatus is referenced
(search for saveStatus, state.saveError, state.isSaving, and FacetHeader) to
follow the chosen approach.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/helpers.kt`:
- Around line 9-15: The helper moved the entire caller block onto
Dispatchers.IO; revert so only AppFfiCtx.init()/close() run on IO: in
withAppFfiCtx, call AppFfiCtx.init() inside withContext(Dispatchers.IO) to
obtain gcx, then invoke block(gcx) outside that withContext so it runs on the
caller dispatcher, and finally ensure gcx.close() runs back on Dispatchers.IO
(e.g., in a withContext(Dispatchers.IO) in the finally). Keep references to
withAppFfiCtx, AppFfiCtx.init(), block(gcx), gcx.close(), and Dispatchers.IO
when making the change.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`:
- Around line 1082-1101: The composable is blocking rendering on
CameraQrAnalyzerFfi.load() even when useNativePreviewQr (calculated from
cameraPreviewFfi.supportsNativeQrAnalysis()) indicates native preview scanning
is available; change the logic so the UI does not return early when native
preview QR is supported. Keep the LaunchedEffect that asynchronously starts
analyzer = CameraQrAnalyzerFfi.load(), but only show the initializing
progress/early return when useNativePreviewQr is false and analyzerReady ==
null; otherwise allow the composable to render and proceed without waiting for
analyzer. Reference symbols: useNativePreviewQr,
cameraPreviewFfi.supportsNativeQrAnalysis(), analyzer/analyzerReady,
CameraQrAnalyzerFfi.load(), LaunchedEffect.
In
`@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt`:
- Line 19: The import and use of the internal API sun.misc.Signal should be
removed or made robust: either delete the import and all signal-handler
installation code (so the app relies on Runtime.addShutdownHook() and the window
close handler in onCloseRequest) or, if you decide to keep Signal, do not
swallow installation errors — catch the exception, log it with details and
re-throw or propagate it so failures are visible; locate the code that
references sun.misc.Signal and the try/catch around its installation, then
remove those lines or replace the catch block with logging that includes the
exception and re-throwing the exception.
---
Outside diff comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt`:
- Around line 96-115: The persistCaptureMode coroutine silently swallows
failures from tablesRepo.setWindow; wrap the body of viewModelScope.launch in a
try/catch (inside persistCaptureMode) and handle exceptions from
tablesRepo.setWindow: on non-recoverable errors rethrow or call a crash/report
path, otherwise display a user-facing toast/error via your UI helper (e.g., the
app's toast/snackbar service) and log the exception with context (mentioning
persistCaptureMode, selectedTableId, windowId) so failures are not lost. Ensure
the catch only surrounds the async work (inside viewModelScope.launch) and
preserves structured concurrency semantics.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt`:
- Around line 682-689: The narrow-mode NavigationRailItem currently invokes
item.onActivate() unconditionally; change the click handler in the
NavigationRailItem used for narrow rail so it respects item.enabled by either
setting onClick to null when item.enabled is false or guarding the lambda (e.g.,
if (item.enabled) scope.launch { item.onActivate() })—update the
NavigationRailItem instance that has onClick = { scope.launch {
item.onActivate() } } to include this enabled check so disabled destinations are
not activated.
In `@src/daybook_core/rt.rs`:
- Around line 723-834: The ensure_plug_init_dispatches function currently always
calls init_repo.report_boot_init_stage(...) even when startup_progress_task_id
is None; change it so that when startup_progress_task_id is None you do not call
InitRepo::report_boot_init_stage (i.e., guard each report_boot_init_stage
invocation in ensure_plug_init_dispatches behind a check like if
startup_progress_task_id.is_some()), or alternatively extend
init::BootInitProgressContext with an explicit disabled flag and pass that
through and make InitRepo::report_boot_init_stage respect it; update all three
places in ensure_plug_init_dispatches where report_boot_init_stage is invoked so
non-boot callers that pass None no longer append to the startup task.
---
Nitpick comments:
In `@src/am_utils_rs/repo.rs`:
- Line 29: The constant SQLITE_POOL_ACQUIRE_TIMEOUT (currently defined as
std::time::Duration::from_secs(90) in repo.rs) is duplicated; remove the local
definition and reference a single shared constant instead. Create or use an
existing shared module (e.g., a common/constants or config module) that exposes
SQLITE_POOL_ACQUIRE_TIMEOUT and update the usages in functions/constructors that
rely on it (including the repo bootstrap paths and the other occurrence in
daybook_core::repo.rs) to import and use that shared symbol so both code paths
read the same centralized value.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.kt`:
- Around line 331-336: The amount summary is still chosen from
latestAmountEntry() which relies on raw updates order; change it to pick the
amount entry by sequence so it matches timelineUpdates ordering: when computing
the summary, filter updates for ProgressUpdateDeets.Amount and select the one
with the highest sequence using maxByOrNull { it.sequence } (instead of relying
on latestAmountEntry() or list order) so the "Progress" block and timeline use
the same ordering rule; update references to latestAmountEntry() accordingly in
ProgressScreen.
In
`@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt`:
- Around line 31-33: The onFailure handler that logs signal installation errors
(the lambda using onFailure { error -> println("[APP_SHUTDOWN] failed to install
signal handler name=$signalName err=${error.message}") }) should either surface
the failure to the user or document why silent logging is acceptable; update the
handler so it shows a brief user-visible notification (e.g., a toast/dialog)
when installation fails, or if the silent fallback is intentional, replace the
println with a concise comment above the onFailure explaining that signal
handler installation is optional and failures are intentionally non-fatal so
normal window close behavior remains and no user interruption is required.
In `@src/daybook_core/app.rs`:
- Around line 7-8: Centralize the duplicated 90s SQLite timeouts by extracting
the two constants (currently defined as SQLITE_POOL_ACQUIRE_TIMEOUT and
SQLITE_BUSY_TIMEOUT in app.rs) into a single shared module or constants file
(e.g., sqlite::timeouts or config::sqlite_timeouts) and update all callers — the
SqlCtx pool builder, any local state pool builders, and repo boot/initialization
code that set busy/acquire timeouts — to import and use those centralized
constants instead of duplicating the values; remove the duplicated definitions
in app.rs and other files so all code refers to the single source of truth.
In `@src/daybook_core/local_state.rs`:
- Around line 124-131: The new constants SQLITE_BUSY_TIMEOUT and
SQLITE_POOL_ACQUIRE_TIMEOUT are applied here but other places (e.g., the PRAGMA
busy_timeout in progress.rs and the 5s timeout in sync.rs) still use different
values; update those call sites to reference the shared constants (replace
hard-coded 5000/5s values and PRAGMA usage with
SQLITE_BUSY_TIMEOUT/SQLITE_POOL_ACQUIRE_TIMEOUT as appropriate) or add a brief
comment explaining why a different timeout is required so the timeout policy is
unified and discoverable across LocalState (the SqlitePoolOptions setup) and the
other modules.
In `@src/daybook_core/rt/init.rs`:
- Around line 394-409: When calling self.progress_repo.add_update(...).await,
capture and wrap any error with context that includes plug_id, init_key and
stage (and optionally task_id) before returning so callers don't just get the
generic progress-repo error; replace the bare .await return with an
error-mapping that appends a message like "failed to add_update for
{plug_id}/{init_key} {stage}" (using map_err or ? with context/anyhow)
referencing the add_update call, task_id, plug_id, init_key and stage to locate
the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f87f138-70bb-4ffa-8193-452e32912c01
⛔ Files ignored due to path filters (4)
docs/DEVDOC/todo.mdis excluded by!docs/DEVDOC/**and included by**/*src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/core/daybook_core.ktis excluded by!src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/**and included by**/*src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/daybook_ffi.ktis excluded by!src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/**and included by**/*src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/types/daybook_types.ktis excluded by!src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/uniffi/**and included by**/*
📒 Files selected for processing (46)
README.mdsrc/am_utils_rs/repo.rssrc/daybook_compose/composeApp/build.gradle.ktssrc/daybook_compose/composeApp/src/androidMain/kotlin/org/example/daybook/MainActivity.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ConfigViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DrawerViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/CaptureNavActions.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/drawer/DrawerScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FeatureItem.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/Features.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FloatingBottomBar.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/RevealBottomSheetScaffold.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/DocEditor.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/editor/EditorSessionController.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/facets.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/helpers.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.ktsrc/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.ktsrc/daybook_compose/composeApp/src/iosMain/kotlin/org/example/daybook/MainViewController.ktsrc/daybook_compose/composeApp/src/wasmJsMain/kotlin/org/example/daybook/main.ktsrc/daybook_core/app.rssrc/daybook_core/drawer/cache.rssrc/daybook_core/index/doc_blobs.rssrc/daybook_core/local_state.rssrc/daybook_core/repo.rssrc/daybook_core/rt.rssrc/daybook_core/rt/init.rssrc/daybook_core/test_support.rssrc/daybook_ffi/ffi.rssrc/daybook_ffi/repos/config.rssrc/daybook_ffi/repos/dispatch.rssrc/daybook_ffi/repos/drawer.rssrc/daybook_ffi/repos/plugs.rssrc/daybook_ffi/repos/progress.rssrc/daybook_ffi/repos/sync.rssrc/daybook_ffi/repos/tables.rssrc/daybook_ffi/rt.rssrc/utils_rs/lib.rsx/build-a-dayb.ts
💤 Files with no reviewable changes (2)
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DrawerViewModel.kt
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/facets.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt (1)
267-279: Consider extracting route mapping to a shared location.This
isFeatureRouteSelectedfunction duplicates the feature-key-to-route mapping that exists implicitly inFeatures.kt. If new features are added, both places need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt` around lines 267 - 279, Extract the feature-to-route mapping into a single shared source (e.g., add a function or map in Features.kt such as FeatureKeys.toRoute() or a FEATURE_ROUTE_MAP) and update isFeatureRouteSelected to call that shared mapper instead of duplicating the when block; locate the mapping currently in isFeatureRouteSelected and replace it by delegating to the new shared symbol (FeatureKeys.toRoute or FEATURE_ROUTE_MAP[featureKey]) so adding new features requires only one change.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt (1)
119-128: Redundant catch-and-rethrow.The catch block catches
FfiExceptiononly to rethrow it, which has no effect. Either remove the try-catch entirely, or add meaningful handling (e.g., logging, updating error state).🔧 Suggested simplification
private suspend fun refreshDoc(docId: String) { val entry = sessions[docId] ?: return - try { - val bundle = drawerRepo.getBundle(docId, "main") - entry.controller.bindDoc(bundle?.doc, bundle) - entry.lastTouchedMs = nowMs() - } catch (e: FfiException) { - throw e - } + val bundle = drawerRepo.getBundle(docId, "main") + entry.controller.bindDoc(bundle?.doc, bundle) + entry.lastTouchedMs = nowMs() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt` around lines 119 - 128, The try/catch in refreshDoc is redundant because it only catches FfiException to rethrow it; remove the try/catch and let exceptions propagate, or if you prefer handling, catch FfiException and perform meaningful work (e.g., log via a logger or update an error state) before rethrowing; locate the refreshDoc function (uses sessions[docId], drawerRepo.getBundle(docId, "main"), and entry.controller.bindDoc(bundle?.doc, bundle)) and either delete the entire try/catch block or replace it with a catch that calls your logging/error-update routine then rethrows.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt (1)
358-384: Consider extracting duplicate default-parent fetching logic.Lines 358-382 and 420-444 both fetch
defaultCloneParentDir()with nearly identical error handling. While the duplication serves different purposes (initial load vs. recovery for blank path), extracting a shared helper function would reduce maintenance burden.♻️ Suggested extraction (optional)
// Add as a private suspend function private suspend fun fetchDefaultParentDir(): Result<String> { return try { val defaultParent = withAppFfiCtx { gcx -> gcx.defaultCloneParentDir().trim() } Result.success(defaultParent) } catch (error: Throwable) { if (error is CancellationException) throw error Result.failure(error) } }Then use in both LaunchedEffects to reduce duplication.
Also applies to: 420-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt` around lines 358 - 384, Extract the duplicate logic that calls withAppFfiCtx { gcx.defaultCloneParentDir().trim() } and handles CancellationException into a shared private suspend helper (e.g., fetchDefaultParentDir(): Result<String>) and replace both LaunchedEffect blocks that currently call withAppFfiCtx directly (the ones updating onCreateRepoUiStateChange with CreateRepoUiState.Editing) to call this helper; ensure the helper returns Result.success on the trimmed string and Result.failure on non-cancellation throwables so callers can set parentPath or errorMessage consistently.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt (1)
85-93: Route the cycle path throughsetCaptureMode().
cycleCaptureMode()is duplicating the same state update + persistence path thatsetCaptureMode()already owns. Reusing the setter keeps future validation or telemetry changes in one place.♻️ Suggested cleanup
fun cycleCaptureMode() { val next = when (_captureMode.value) { CaptureMode.TEXT -> CaptureMode.CAMERA CaptureMode.CAMERA -> CaptureMode.MIC CaptureMode.MIC -> CaptureMode.TEXT } - _captureMode.value = next - persistCaptureMode(next) + setCaptureMode(next) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt` around lines 85 - 93, cycleCaptureMode() duplicates state update and persistence logic already handled by setCaptureMode(); replace the manual updates in cycleCaptureMode() by computing the next CaptureMode (using _captureMode.value) and then call setCaptureMode(next) so all validation/telemetry/persistence remains centralized in setCaptureMode (remove the direct assignments to _captureMode.value and the call to persistCaptureMode(next)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt`:
- Around line 115-132: The catch block for FfiException currently assigns a
static string to the MutableStateFlow `_message`, which conflates identical
consecutive emissions; replace the `_message` MutableStateFlow with a
MutableSharedFlow<String>(extraBufferCapacity = 1, replay = 0) (exposed as a
SharedFlow `message`) and change the handler in the catch for
`persistCaptureMode` to emit the error string into `_message` (use
`_message.emit(...)` or `_message.tryEmit(...)`), remove any `clearMessage()`
calls, and update the UI collector (the `LaunchedEffect(vm)` block) to collect
from `vm.message` and show the snackbar for each emission; locate these changes
around the `persistCaptureMode` catch, `_message`/`message` declarations,
`clearMessage()` implementation, and the `LaunchedEffect(vm)` snackbar collector
to implement the fix.
In
`@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt`:
- Around line 52-54: The current onDispose block uses runCatching {
Runtime.getRuntime().removeShutdownHook(hook) } which swallows exceptions
silently; change it to capture failures and log the exception details instead
(e.g., use runCatching { Runtime.getRuntime().removeShutdownHook(hook)
}.onFailure { logger.warn("Failed to remove shutdown hook", it) } or similar),
so any IllegalStateException or other errors are recorded; update the onDispose
block and reference Runtime.getRuntime().removeShutdownHook(hook), runCatching
and onDispose when making the change.
---
Nitpick comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt`:
- Around line 85-93: cycleCaptureMode() duplicates state update and persistence
logic already handled by setCaptureMode(); replace the manual updates in
cycleCaptureMode() by computing the next CaptureMode (using _captureMode.value)
and then call setCaptureMode(next) so all validation/telemetry/persistence
remains centralized in setCaptureMode (remove the direct assignments to
_captureMode.value and the call to persistCaptureMode(next)).
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt`:
- Around line 119-128: The try/catch in refreshDoc is redundant because it only
catches FfiException to rethrow it; remove the try/catch and let exceptions
propagate, or if you prefer handling, catch FfiException and perform meaningful
work (e.g., log via a logger or update an error state) before rethrowing; locate
the refreshDoc function (uses sessions[docId], drawerRepo.getBundle(docId,
"main"), and entry.controller.bindDoc(bundle?.doc, bundle)) and either delete
the entire try/catch block or replace it with a catch that calls your
logging/error-update routine then rethrows.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt`:
- Around line 267-279: Extract the feature-to-route mapping into a single shared
source (e.g., add a function or map in Features.kt such as FeatureKeys.toRoute()
or a FEATURE_ROUTE_MAP) and update isFeatureRouteSelected to call that shared
mapper instead of duplicating the when block; locate the mapping currently in
isFeatureRouteSelected and replace it by delegating to the new shared symbol
(FeatureKeys.toRoute or FEATURE_ROUTE_MAP[featureKey]) so adding new features
requires only one change.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`:
- Around line 358-384: Extract the duplicate logic that calls withAppFfiCtx {
gcx.defaultCloneParentDir().trim() } and handles CancellationException into a
shared private suspend helper (e.g., fetchDefaultParentDir(): Result<String>)
and replace both LaunchedEffect blocks that currently call withAppFfiCtx
directly (the ones updating onCreateRepoUiStateChange with
CreateRepoUiState.Editing) to call this helper; ensure the helper returns
Result.success on the trimmed string and Result.failure on non-cancellation
throwables so callers can set parentPath or errorMessage consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ff47726-25d4-4255-b17d-412dae83f032
📒 Files selected for processing (16)
README.mdsrc/daybook_cli/main.rssrc/daybook_compose/composeApp/build.gradle.ktssrc/daybook_compose/composeApp/src/androidMain/kotlin/org/example/daybook/MainActivity.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/progress/ProgressScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/ui/DocEditor.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/helpers.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.ktsrc/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.ktsrc/daybook_core/rt/init.rs
✅ Files skipped from review due to trivial changes (2)
- README.md
- src/daybook_compose/composeApp/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/helpers.kt
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt (1)
1090-1108: Minor optimization opportunity: Skip analyzer loading when native QR is supported.The
LaunchedEffectunconditionally loads the analyzer even whenuseNativePreviewQris true and the analyzer won't be used. Consider gating the load:LaunchedEffect(Unit) { - if (analyzer == null) { + if (!useNativePreviewQr && analyzer == null) { analyzer = withContext(Dispatchers.IO) { CameraQrAnalyzerFfi.load() } } }This avoids unnecessary FFI initialization when native preview scanning is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt` around lines 1090 - 1108, The LaunchedEffect currently always initializes the FFI analyzer even when native preview QR is enabled; modify the launch block so it only loads the analyzer when it will actually be used (i.e., when analyzer == null AND useNativePreviewQr is false). Constrain the existing LaunchedEffect that calls CameraQrAnalyzerFfi.load() to check useNativePreviewQr before invoking load(), leaving the rest of the logic (setting analyzer, using Dispatchers.IO) unchanged.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt (1)
135-137: Remove redundant catch block.This catch-and-rethrow serves no purpose—uncaught exceptions propagate naturally. This is dead code.
🧹 Proposed fix
_message.tryEmit("Failed to persist capture mode") - } catch (t: Throwable) { - throw t } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt` around lines 135 - 137, Remove the redundant catch block in CaptureScreen.kt that catches Throwable and immediately rethrows it; delete the entire "catch (t: Throwable) { throw t }" block and adjust the surrounding try/catch/braces so the try's code remains intact (no functional changes otherwise) — locate the block inside the CaptureScreen composable/function and remove only that catch clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt`:
- Line 30: The sessions map is not thread-safe; replace the mutableMapOf with a
java.util.concurrent.ConcurrentHashMap<String, DocEditorSessionEntry> (update
the declaration of sessions) and update all compound operations: use
sessions.computeIfAbsent(...) in createSession/selectDoc to atomically
create/look up entries, avoid check-then-act races, and in evictIdleSessions
iterate safely by collecting stale keys then calling sessions.remove(key) (or
use entrySet().removeIf(...) on the ConcurrentHashMap) instead of relying on
unsynchronized reads; alternatively, if you prefer serialized mutations, route
all accesses through a single CoroutineDispatcher or guard mutations with a
Mutex used by onDrawerEvent, selectDoc, attachHost, detachHost,
evictIdleSessions and createSession.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt`:
- Around line 266-268: isFeatureRouteSelected() uses
routeForFeatureKey(featureKey) to determine selection but routeForFeatureKey()
lacks a mapping for FeatureKeys.CloneShare, so any CloneShare FeatureItem never
becomes selected and its reselect branch never executes; fix by either adding an
explicit case for FeatureKeys.CloneShare in routeForFeatureKey() (the function
referenced) returning the correct route string, or change the data model so
FeatureItem carries its destination route and update isFeatureRouteSelected() to
use that route instead of re-deriving it; update references in
isFeatureRouteSelected, routeForFeatureKey, FeatureKeys.CloneShare and
FeatureItem accordingly.
- Around line 226-263: Replace the custom clickable container with a selectable
so accessibility exposes tab semantics: in the Box where clickable(...) is used,
switch to Modifier.selectable(selected = selected, onClick = onClick,
interactionSource = outerInteraction, indication = null, role = Role.Tab)
(keeping the same enabled flag and padding), so the component (Box / Column /
icon() / label()) reports the selected state as a tab to screen readers; ensure
Role is imported and the selected boolean used is the one passed into this
composable.
---
Nitpick comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.kt`:
- Around line 135-137: Remove the redundant catch block in CaptureScreen.kt that
catches Throwable and immediately rethrows it; delete the entire "catch (t:
Throwable) { throw t }" block and adjust the surrounding try/catch/braces so the
try's code remains intact (no functional changes otherwise) — locate the block
inside the CaptureScreen composable/function and remove only that catch clause.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`:
- Around line 1090-1108: The LaunchedEffect currently always initializes the FFI
analyzer even when native preview QR is enabled; modify the launch block so it
only loads the analyzer when it will actually be used (i.e., when analyzer ==
null AND useNativePreviewQr is false). Constrain the existing LaunchedEffect
that calls CameraQrAnalyzerFfi.load() to check useNativePreviewQr before
invoking load(), leaving the rest of the logic (setting analyzer, using
Dispatchers.IO) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f782a435-d641-47ae-8251-32e333f29da7
📒 Files selected for processing (6)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/Features.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.ktsrc/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/Features.kt
Comment on lines
+266
to
+268
| private fun isFeatureRouteSelected(featureKey: String, currentRoute: String?): Boolean { | ||
| val targetRoute = routeForFeatureKey(featureKey) | ||
| return targetRoute != null && targetRoute == currentRoute |
There was a problem hiding this comment.
The new route-based selection breaks for unmapped feature keys.
isFeatureRouteSelected() now controls both the highlight state and the reselect branch, but routeForFeatureKey() in src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/Features.kt:19-28 still has no case for FeatureKeys.CloneShare declared in src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FeatureItem.kt:8-15. Any CloneShare item rendered here will stay permanently unselected, and the selected-path logic at Lines 190-194 will never run for it. Please make the key→route mapping exhaustive, or carry the destination route on FeatureItem instead of re-deriving it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt`
around lines 266 - 268, isFeatureRouteSelected() uses
routeForFeatureKey(featureKey) to determine selection but routeForFeatureKey()
lacks a mapping for FeatureKeys.CloneShare, so any CloneShare FeatureItem never
becomes selected and its reselect branch never executes; fix by either adding an
explicit case for FeatureKeys.CloneShare in routeForFeatureKey() (the function
referenced) returning the correct route string, or change the data model so
FeatureItem carries its destination route and update isFeatureRouteSelected() to
use that route instead of re-deriving it; update references in
isFeatureRouteSelected, routeForFeatureKey, FeatureKeys.CloneShare and
FeatureItem accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt (2)
52-57: Minor TOCTOU race between check and update of selection.The check
if (_selectedDocId.value == event.id)followed by the assignments is non-atomic. IfselectDoc(newDocId)runs between the check and the clear, the selection could be incorrectly reset. Practical impact is low since a deleted doc can't be meaningfully used anyway.If you want to tighten this, you could use
compareAndSetor route the entire handler through the main dispatcher as the previous review suggested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt` around lines 52 - 57, The deletion handler for DrawerEvent.DocDeleted has a TOCTOU: it checks _selectedDocId.value == event.id then clears _selectedDocId and _selectedController, so a concurrent selectDoc(newDocId) can race and be lost; fix by performing an atomic conditional clear (e.g., use _selectedDocId.compareAndSet(event.id, null) and only then set _selectedController.value = null) or ensure the whole handler runs on the main dispatcher/single-threaded context so selection updates are serialized (apply this change inside the DrawerEvent.DocDeleted branch where sessions.remove(event.id) is called).
22-26: Consider using atomic types for thread-safe field access.While
ConcurrentHashMapensures thread-safe map operations, the mutablevarfields (hostCount,lastTouchedMs) are modified via non-atomic read-modify-write operations (e.g.,entry.hostCount += 1at line 97). If coroutines ever interleave unexpectedly, this could cause lost updates.Using
AtomicIntegerandAtomicLongwould make the thread-safety guarantees explicit:🛡️ Suggested defensive change
+import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicLong + private data class DocEditorSessionEntry( val controller: EditorSessionController, - var hostCount: Int = 0, - var lastTouchedMs: Long = Clock.System.now().toEpochMilliseconds() + val hostCount: AtomicInteger = AtomicInteger(0), + val lastTouchedMs: AtomicLong = AtomicLong(Clock.System.now().toEpochMilliseconds()) )Then update usages:
entry.hostCount += 1→entry.hostCount.incrementAndGet()entry.hostCount = (entry.hostCount - 1).coerceAtLeast(0)→entry.hostCount.updateAndGet { (it - 1).coerceAtLeast(0) }entry.lastTouchedMs = nowMs()→entry.lastTouchedMs.set(nowMs())entry.hostCount > 0→entry.hostCount.get() > 0now - entry.lastTouchedMs→now - entry.lastTouchedMs.get()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt` around lines 22 - 26, DocEditorSessionEntry currently uses mutable vars hostCount and lastTouchedMs which are updated non-atomically; change hostCount to java.util.concurrent.atomic.AtomicInteger and lastTouchedMs to java.util.concurrent.atomic.AtomicLong in the DocEditorSessionEntry definition and update all usages: replace increments/decrements and reads on entry.hostCount with AtomicInteger methods (incrementAndGet(), updateAndGet{...}, get()) and replace writes/reads to entry.lastTouchedMs with AtomicLong.set(...) and get(); ensure code paths that check hostCount > 0 and compute now - lastTouchedMs use get() and get() respectively so updates are atomic and safe with the ConcurrentHashMap and coroutines.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt (1)
618-636: Consider adding a timeout or error feedback for camera initialization.If
cameraPreviewFfiremains null indefinitely (e.g., camera initialization fails silently in the parent), the user will be stuck on "Initializing camera…" with no way to recover or understand what's wrong. Consider adding a timeout that shows an error message or a back/retry option after a reasonable duration.💡 Suggested enhancement
} else { + var initTimedOut by remember { mutableStateOf(false) } + LaunchedEffect(Unit) { + delay(10_000) // 10 second timeout + if (cameraPreviewFfi == null) initTimedOut = true + } if (cameraPreviewFfi == null) { Box(modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center) { Column( horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.spacedBy(12.dp) ) { - CircularProgressIndicator() - Text("Initializing camera…", style = MaterialTheme.typography.bodyMedium) + if (initTimedOut) { + Text("Camera initialization timed out.", style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.error) + } else { + CircularProgressIndicator() + Text("Initializing camera…", style = MaterialTheme.typography.bodyMedium) + } } } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt` around lines 618 - 636, The current UI shows an indefinite loading state when cameraPreviewFfi is null; add a timeout and error feedback so users can retry or navigate away: in the composable that reads cameraPreviewFfi, create a local state (e.g., cameraInitFailed: Boolean) and use a LaunchedEffect tied to cameraPreviewFfi to start a coroutine with a delay (e.g., 5–10s) that sets cameraInitFailed = true if cameraPreviewFfi is still null; update the UI branch to show an error message with a Retry button that invokes the existing camera re-init mechanism (or calls a provided retry lambda) and a Back button that calls navController.popBackStack(WelcomeRoute.CloneUrl, false) and/or updates onCloneUiStateChange as appropriate; ensure CloneQrScannerScreen is still rendered when cameraPreviewFfi becomes non-null and clear the timeout state on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`:
- Around line 1091-1095: Wrap the CameraQrAnalyzerFfi.load() call inside
LaunchedEffect in a try/catch: when analyzer is null and !useNativePreviewQr,
call withContext(Dispatchers.IO) { try { CameraQrAnalyzerFfi.load() } catch (e:
Throwable) { analyzerError = e; log the error } } and ensure analyzer is only
assigned on success; then update the existing loading UI condition to also check
analyzerError and show an appropriate error message/state to allow fallback to
manual URL entry (refer to symbols: LaunchedEffect, analyzer,
useNativePreviewQr, CameraQrAnalyzerFfi.load, withContext, Dispatchers.IO,
analyzerError).
---
Nitpick comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.kt`:
- Around line 52-57: The deletion handler for DrawerEvent.DocDeleted has a
TOCTOU: it checks _selectedDocId.value == event.id then clears _selectedDocId
and _selectedController, so a concurrent selectDoc(newDocId) can race and be
lost; fix by performing an atomic conditional clear (e.g., use
_selectedDocId.compareAndSet(event.id, null) and only then set
_selectedController.value = null) or ensure the whole handler runs on the main
dispatcher/single-threaded context so selection updates are serialized (apply
this change inside the DrawerEvent.DocDeleted branch where
sessions.remove(event.id) is called).
- Around line 22-26: DocEditorSessionEntry currently uses mutable vars hostCount
and lastTouchedMs which are updated non-atomically; change hostCount to
java.util.concurrent.atomic.AtomicInteger and lastTouchedMs to
java.util.concurrent.atomic.AtomicLong in the DocEditorSessionEntry definition
and update all usages: replace increments/decrements and reads on
entry.hostCount with AtomicInteger methods (incrementAndGet(),
updateAndGet{...}, get()) and replace writes/reads to entry.lastTouchedMs with
AtomicLong.set(...) and get(); ensure code paths that check hostCount > 0 and
compute now - lastTouchedMs use get() and get() respectively so updates are
atomic and safe with the ConcurrentHashMap and coroutines.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`:
- Around line 618-636: The current UI shows an indefinite loading state when
cameraPreviewFfi is null; add a timeout and error feedback so users can retry or
navigate away: in the composable that reads cameraPreviewFfi, create a local
state (e.g., cameraInitFailed: Boolean) and use a LaunchedEffect tied to
cameraPreviewFfi to start a coroutine with a delay (e.g., 5–10s) that sets
cameraInitFailed = true if cameraPreviewFfi is still null; update the UI branch
to show an error message with a Retry button that invokes the existing camera
re-init mechanism (or calls a provided retry lambda) and a Back button that
calls navController.popBackStack(WelcomeRoute.CloneUrl, false) and/or updates
onCloneUiStateChange as appropriate; ensure CloneQrScannerScreen is still
rendered when cameraPreviewFfi becomes non-null and clear the timeout state on
success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb2e79d0-66fe-4b6b-8a7f-379e4c0e2691
📒 Files selected for processing (4)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/DocEditorStoreViewModel.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/capture/screens/CaptureScreen.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt
Comment on lines
+1091
to
+1095
| LaunchedEffect(Unit) { | ||
| if (analyzer == null && !useNativePreviewQr) { | ||
| analyzer = withContext(Dispatchers.IO) { CameraQrAnalyzerFfi.load() } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for analyzer load failure.
If CameraQrAnalyzerFfi.load() throws, the exception will propagate uncaught, potentially crashing the app. Since the user can fall back to manual URL entry, this is a recoverable failure that should show an error message rather than crash.
🛡️ Proposed fix
+ var analyzerError by remember { mutableStateOf<String?>(null) }
LaunchedEffect(Unit) {
if (analyzer == null && !useNativePreviewQr) {
- analyzer = withContext(Dispatchers.IO) { CameraQrAnalyzerFfi.load() }
+ try {
+ analyzer = withContext(Dispatchers.IO) { CameraQrAnalyzerFfi.load() }
+ } catch (error: Throwable) {
+ if (error is CancellationException) throw error
+ analyzerError = "Failed to initialize QR analyzer: ${error.message ?: error::class.simpleName}"
+ }
}
}Then update the loading UI check to also display analyzerError when set.
As per coding guidelines: "In Kotlin or UI code, if an error occurs that can't be handled, crash the program or show a toast if non-critical."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/welcome/welcome.kt`
around lines 1091 - 1095, Wrap the CameraQrAnalyzerFfi.load() call inside
LaunchedEffect in a try/catch: when analyzer is null and !useNativePreviewQr,
call withContext(Dispatchers.IO) { try { CameraQrAnalyzerFfi.load() } catch (e:
Throwable) { analyzerError = e; log the error } } and ensure analyzer is only
assigned on success; then update the existing loading UI condition to also check
analyzerError and show an appropriate error message/state to allow fallback to
manual URL entry (refer to symbols: LaunchedEffect, analyzer,
useNativePreviewQr, CameraQrAnalyzerFfi.load, withContext, Dispatchers.IO,
analyzerError).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt (1)
927-971:⚠️ Potential issue | 🟠 MajorThe compact drawer cannot switch away from Progress.
selectedPanestarts at1, and the only tab rendered here is permanently selected and setsselectedPane = 1again. That makes theelsebranch with the table title andTabSelectionListunreachable, so this drawer loses its second pane entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt` around lines 927 - 971, selectedPane is initialized to 1 but the TabRow renders a single always-selected Tab that sets selectedPane back to 1, making the else branch unreachable; update the TabRow/Tab rendering to reflect both panes and bind selection to selectedPane (e.g., use TabRow(selectedTabIndex = selectedPane - 1) and render two Tab items with selected = (selectedPane == 1) / selected = (selectedPane == 2) and onClick handlers that set selectedPane = 1 or selectedPane = 2 respectively), so ProgressList and the alternate pane with selectedTable/TabSelectionList can be toggled correctly.src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt (1)
1215-1233:⚠️ Potential issue | 🟠 MajorUse pane B's virtual size in the
regimeBfallback branch.This branch is clamping the discrete B pane with
resolveDiscreteTargetSizeDp(regimeB, virtualSizeDpA). That makes the snap decision depend on the wrong side of the divider, so drags near the rail threshold can resolve to the wrong final weights.Suggested fix
- val targetSizeDpA = resolveDiscreteTargetSizeDp(regimeB, virtualSizeDpA) - val targetWeightA = sizeDpToWeight(targetSizeDpA, totalSizeDp, totalWeight) - val newWeightA = - targetWeightA - .coerceAtLeast( - SidebarLayoutConstants.MIN_PANE_WEIGHT - ).coerceAtMost( - totalCurrentWeight - SidebarLayoutConstants.MIN_PANE_WEIGHT - ) - val newWeightB = (totalCurrentWeight - newWeightA).coerceAtLeast( - SidebarLayoutConstants.MIN_PANE_WEIGHT - ) + val targetSizeDpB = resolveDiscreteTargetSizeDp(regimeB, virtualSizeDpB) + val targetWeightB = sizeDpToWeight(targetSizeDpB, totalSizeDp, totalWeight) + val newWeightB = + targetWeightB + .coerceAtLeast( + SidebarLayoutConstants.MIN_PANE_WEIGHT + ).coerceAtMost( + totalCurrentWeight - SidebarLayoutConstants.MIN_PANE_WEIGHT + ) + val newWeightA = (totalCurrentWeight - newWeightB).coerceAtLeast( + SidebarLayoutConstants.MIN_PANE_WEIGHT + ) weightMap[keyA] = newWeightA weightMap[keyB] = newWeightB🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt` around lines 1215 - 1233, The fallback branch for PaneSizeRegime.Discrete is using the wrong side's virtual size: change the call resolveDiscreteTargetSizeDp(regimeB, virtualSizeDpA) to use virtualSizeDpB so the snap decision uses pane B's virtual size; then compute targetWeightB = sizeDpToWeight(targetSizeDpB, totalSizeDp, totalWeight) (or similarly named variable) and derive newWeightB = targetWeightB.coerceAtLeast(SidebarLayoutConstants.MIN_PANE_WEIGHT) and newWeightA = (totalCurrentWeight - newWeightB).coerceAtLeast(SidebarLayoutConstants.MIN_PANE_WEIGHT), updating weightMap[keyA]/weightMap[keyB] accordingly (refer to resolveDiscreteTargetSizeDp, sizeDpToWeight, weightMap, keyA, keyB, totalCurrentWeight).src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt (1)
748-788:⚠️ Potential issue | 🟠 MajorTreat bootstrap timeouts as failures, not generic coroutine cancellation.
withTimeout(20_000)throwsTimeoutCancellationException, which is a subclass ofCancellationException. The catch block at lines 783–784 rethrows allCancellationExceptions, so a repo-config or repo-usability timeout (lines 754, 770) leaves the app stuck inLoadinginstead of transitioning to the error state at line 787.Suggested fix
} catch (throwable: Throwable) { - if (throwable is CancellationException) throw throwable + if (throwable is CancellationException && + throwable !is kotlinx.coroutines.TimeoutCancellationException + ) { + throw throwable + } cloneSourceUrlPendingOpen = null println("[APP_INIT] stage=bootstrap failed err=${throwable.message}") initState = AppInitState.Error(throwable) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt` around lines 748 - 788, The catch in the LaunchedEffect(initAttempt) block currently rethrows all CancellationException, which causes withTimeout(20_000) timeouts (TimeoutCancellationException) to bypass the error path; update the catch so it only rethrows genuine external cancellations (i.e., if throwable is CancellationException and NOT a TimeoutCancellationException) and treat TimeoutCancellationException like other failures: clear cloneSourceUrlPendingOpen, log the timeout, and set initState = AppInitState.Error(throwable). Locate the withTimeout(20_000) usages around ffiServices.getRepoConfig() and ffiServices.isRepoUsable() and modify the catch logic in that same LaunchedEffect to distinguish TimeoutCancellationException from other CancellationException cases.
♻️ Duplicate comments (1)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt (1)
917-983:⚠️ Potential issue | 🟠 MajorClose the already-open base container when deferred runtime boot fails.
If
SyncRepoFfi.load()orRtFfi.load()fails here, this catch closesloadedSyncRepobut leavescurrent's already-open repos/FFI handles alive before switchinginitStatetoError. TheReadybranch'sDisposableEffectexplicitly skips cleanup whenautoShutdownOnDisposeisfalse, so those resources are leaked on that path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt` around lines 917 - 983, The catch block must fully tear down the already-open base container resources on failure of SyncRepoFfi.load or RtFfi.load; update the catch to, after handling CancellationException, run a scoped cleanup (withContext(Dispatchers.IO) and runCatching) that closes loadedSyncRepo and also closes the open handles on current (e.g., current.syncRepo?.close(), current.rtFfi?.close(), and any other repo/FFI handles held on current such as current.drawerRepo, current.plugsRepo, current.dispatchRepo, current.blobsRepo, current.progressRepo) before adding the FAILED progress update and setting initState = AppInitState.Error(throwable); if the codebase exposes a single cleanup method on the container (e.g., current.close() or current.shutdown()), prefer calling that instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt`:
- Around line 1189-1222: The shutdownAppContainer flow needs to catch and
aggregate failures so one exception doesn't abort remaining cleanup; update
shutdownAppContainer to wrap each call (syncRepo.stop(), rtFfi.stop(),
appContainer.progressRepo.stop(), and every close() call on drawerRepo,
tablesRepo, dispatchRepo, progressRepo, rtFfi, plugsRepo, configRepo, blobsRepo,
syncRepo, cameraPreviewFfi, ffiCtx) in try/catch blocks (keeping the existing
withContext(Dispatchers.IO) scopes for stop() calls), collect thrown Throwables
into a mutable list, continue executing remaining stops/closes on error, and
after all attempts rethrow a composite exception (e.g., first exception with
others added as suppressed) if any failures were recorded so callers still get
an error but cleanup is resilient.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt`:
- Around line 189-193: The new tabsSheetState is never activated because
sheetContent is never set to SheetContent.TABS and tabsSheetState.show()/open
path is missing; locate the UI action that should open the tabs (places that
currently call menuSheetState.show() or set menuCloseDragEnabled) and add logic
to set sheetContent = SheetContent.TABS and call tabsSheetState.show() (and
ensure ensureValidAnchor()/hide() usage mirrors the MENU flow); update any
handlers that previously only touched menuSheetState to branch on the intended
target sheet and open/close tabsSheetState accordingly so the TABS sheet can
become visible.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FloatingBottomBar.kt`:
- Around line 204-245: The enableDragToClose flag is ignored because
dragModifier is always applied; update the code to only attach the draggable
modifier when enableDragToClose is true: wrap creation/use of dragModifier or
the .then(dragModifier) calls with a conditional (e.g., val effectiveModifier =
if (enableDragToClose) Modifier.then(dragModifier) else Modifier) and use
effectiveModifier for the Surface modifier and for the bottom drag area modifier
(the other place around the barHeight/drag area), so when enableDragToClose is
false the sheetState draggable handlers are not wired.
In
`@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt`:
- Around line 37-39: installSignalHandler("INT") and
installSignalHandler("TERM") are being called directly in the application
composition body which can run on recomposition; move these calls into a
one-time side effect by wrapping them in LaunchedEffect(Unit) (matching the
pattern already used later) so the handlers are registered only once; locate the
calls to installSignalHandler in main and wrap them inside a
LaunchedEffect(Unit) block.
---
Outside diff comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt`:
- Around line 748-788: The catch in the LaunchedEffect(initAttempt) block
currently rethrows all CancellationException, which causes withTimeout(20_000)
timeouts (TimeoutCancellationException) to bypass the error path; update the
catch so it only rethrows genuine external cancellations (i.e., if throwable is
CancellationException and NOT a TimeoutCancellationException) and treat
TimeoutCancellationException like other failures: clear
cloneSourceUrlPendingOpen, log the timeout, and set initState =
AppInitState.Error(throwable). Locate the withTimeout(20_000) usages around
ffiServices.getRepoConfig() and ffiServices.isRepoUsable() and modify the catch
logic in that same LaunchedEffect to distinguish TimeoutCancellationException
from other CancellationException cases.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt`:
- Around line 927-971: selectedPane is initialized to 1 but the TabRow renders a
single always-selected Tab that sets selectedPane back to 1, making the else
branch unreachable; update the TabRow/Tab rendering to reflect both panes and
bind selection to selectedPane (e.g., use TabRow(selectedTabIndex = selectedPane
- 1) and render two Tab items with selected = (selectedPane == 1) / selected =
(selectedPane == 2) and onClick handlers that set selectedPane = 1 or
selectedPane = 2 respectively), so ProgressList and the alternate pane with
selectedTable/TabSelectionList can be toggled correctly.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.kt`:
- Around line 1215-1233: The fallback branch for PaneSizeRegime.Discrete is
using the wrong side's virtual size: change the call
resolveDiscreteTargetSizeDp(regimeB, virtualSizeDpA) to use virtualSizeDpB so
the snap decision uses pane B's virtual size; then compute targetWeightB =
sizeDpToWeight(targetSizeDpB, totalSizeDp, totalWeight) (or similarly named
variable) and derive newWeightB =
targetWeightB.coerceAtLeast(SidebarLayoutConstants.MIN_PANE_WEIGHT) and
newWeightA = (totalCurrentWeight -
newWeightB).coerceAtLeast(SidebarLayoutConstants.MIN_PANE_WEIGHT), updating
weightMap[keyA]/weightMap[keyB] accordingly (refer to
resolveDiscreteTargetSizeDp, sizeDpToWeight, weightMap, keyA, keyB,
totalCurrentWeight).
---
Duplicate comments:
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt`:
- Around line 917-983: The catch block must fully tear down the already-open
base container resources on failure of SyncRepoFfi.load or RtFfi.load; update
the catch to, after handling CancellationException, run a scoped cleanup
(withContext(Dispatchers.IO) and runCatching) that closes loadedSyncRepo and
also closes the open handles on current (e.g., current.syncRepo?.close(),
current.rtFfi?.close(), and any other repo/FFI handles held on current such as
current.drawerRepo, current.plugsRepo, current.dispatchRepo, current.blobsRepo,
current.progressRepo) before adding the FAILED progress update and setting
initState = AppInitState.Error(throwable); if the codebase exposes a single
cleanup method on the container (e.g., current.close() or current.shutdown()),
prefer calling that instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f32f597-f951-4623-8ed8-db17f916d1b8
📒 Files selected for processing (9)
src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/dockable/DockableDivider.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/Features.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FloatingBottomBar.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/SidebarMenuButton.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.ktsrc/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/expanded.ktsrc/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt
✅ Files skipped from review due to trivial changes (1)
- src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/CenterNavBarContent.kt
Comment on lines
+1189
to
+1222
| private suspend fun shutdownAppContainer(appContainer: AppContainer) { | ||
| println("[APP_SHUTDOWN] flushing to disk: begin") | ||
| val rtFfi = appContainer.rtFfi | ||
| val syncRepo = appContainer.syncRepo | ||
| if (syncRepo != null) { | ||
| withContext(Dispatchers.IO) { | ||
| println("[APP_SHUTDOWN] flushing to disk: stopping sync repo") | ||
| syncRepo.stop() | ||
| } | ||
| } | ||
| if (rtFfi != null) { | ||
| withContext(Dispatchers.IO) { | ||
| println("[APP_SHUTDOWN] flushing to disk: stopping runtime repo") | ||
| rtFfi.stop() | ||
| } | ||
| } | ||
| withContext(Dispatchers.IO) { | ||
| println("[APP_SHUTDOWN] flushing to disk: stopping progress repo") | ||
| appContainer.progressRepo.stop() | ||
| } | ||
| println("[APP_SHUTDOWN] flushing to disk: closing repo handles") | ||
| appContainer.drawerRepo.close() | ||
| appContainer.tablesRepo.close() | ||
| appContainer.dispatchRepo.close() | ||
| appContainer.progressRepo.close() | ||
| rtFfi?.close() | ||
| appContainer.plugsRepo.close() | ||
| appContainer.configRepo.close() | ||
| appContainer.blobsRepo.close() | ||
| syncRepo?.close() | ||
| appContainer.cameraPreviewFfi.close() | ||
| appContainer.ffiCtx.close() | ||
| println("[APP_SHUTDOWN] flushing to disk: complete") | ||
| } |
There was a problem hiding this comment.
Make shutdown cleanup resilient to partial failures.
A single exception from syncRepo.stop(), rtFfi.stop(), or any later close() aborts the rest of this method. In the desktop flow that means shutdownDone never flips, so the blocking shutdown overlay can stay up and onShutdownCompleted() never runs. Try every stop/close, collect failures, and only rethrow after the full cleanup attempt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/App.kt`
around lines 1189 - 1222, The shutdownAppContainer flow needs to catch and
aggregate failures so one exception doesn't abort remaining cleanup; update
shutdownAppContainer to wrap each call (syncRepo.stop(), rtFfi.stop(),
appContainer.progressRepo.stop(), and every close() call on drawerRepo,
tablesRepo, dispatchRepo, progressRepo, rtFfi, plugsRepo, configRepo, blobsRepo,
syncRepo, cameraPreviewFfi, ffiCtx) in try/catch blocks (keeping the existing
withContext(Dispatchers.IO) scopes for stop() calls), collect thrown Throwables
into a mutable list, continue executing remaining stops/closes on error, and
after all attempts rethrow a composite exception (e.g., first exception with
others added as suppressed) if any failures were recorded so callers still get
an error but cleanup is resilient.
Comment on lines
+189
to
193
| val tabsSheetState = rememberRevealBottomSheetState(initiallyVisible = false) | ||
| val menuSheetState = rememberRevealBottomSheetState(initiallyVisible = false) | ||
| var sheetContent by remember { mutableStateOf(SheetContent.MENU) } | ||
| var menuCloseDragEnabled by remember { mutableStateOf(false) } | ||
| val leftDrawerState = rememberDrawerState(initialValue = DrawerValue.Closed) |
There was a problem hiding this comment.
The TABS sheet no longer has any activation path.
This split introduces tabsSheetState, but the file never switches sheetContent to SheetContent.TABS or opens tabsSheetState; every in-file reference to that state is hide()/ensureValidAnchor(). As written, the scaffolded tabs sheet cannot become visible with any content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/compact.kt`
around lines 189 - 193, The new tabsSheetState is never activated because
sheetContent is never set to SheetContent.TABS and tabsSheetState.show()/open
path is missing; locate the UI action that should open the tabs (places that
currently call menuSheetState.show() or set menuCloseDragEnabled) and add logic
to set sheetContent = SheetContent.TABS and call tabsSheetState.show() (and
ensure ensureValidAnchor()/hide() usage mirrors the MENU flow); update any
handlers that previously only touched menuSheetState to branch on the intended
target sheet and open/close tabsSheetState accordingly so the TABS sheet can
become visible.
Comment on lines
+204
to
+245
| val dragModifier = | ||
| Modifier.draggable( | ||
| state = | ||
| rememberDraggableState { dragAmount -> | ||
| val total = maxMenuHeightPx | ||
| val boundedProgress = sheetState.progress.coerceIn(0f, maxAnchor) | ||
| val currentVisible = total * (boundedProgress / maxAnchor).coerceIn(0f, 1f) | ||
| val nextVisible = (currentVisible - dragAmount).coerceIn(0f, total) | ||
| val nextProgress = ((nextVisible / total) * maxAnchor).coerceIn(0f, maxAnchor) | ||
| sheetState.setProgressImmediate(nextProgress) | ||
| }, | ||
| orientation = Orientation.Vertical, | ||
| onDragStopped = { velocityY -> | ||
| if (velocityY > flingCloseThreshold) { | ||
| scope.launch { | ||
| val anim = Animatable(sheetState.progress.coerceIn(0f, maxAnchor)) | ||
| anim.animateTo(0f, animationSpec = tween(durationMillis = 200)) { | ||
| sheetState.setProgressImmediate(value) | ||
| } | ||
| sheetState.hideInstant() | ||
| onDismiss() | ||
| } | ||
| } else { | ||
| sheetState.settle(velocityY) { settledProgress -> | ||
| if (settledProgress <= 0f) { | ||
| onDismiss() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| val barDragAreaHeight = barHeight + FloatingBarDefaults.verticalPadding * 2 | ||
| val surfaceHeight = targetHeight.coerceAtLeast(1.dp) | ||
|
|
||
| Surface( | ||
| modifier = | ||
| Modifier | ||
| .align(Alignment.BottomCenter) | ||
| .fillMaxWidth() | ||
| .padding(bottom = FloatingBarDefaults.verticalPadding) | ||
| .height(surfaceHeight) | ||
| .then(dragModifier), |
There was a problem hiding this comment.
Wire enableDragToClose into the draggable modifiers.
This flag is currently ignored: both the sheet surface and the bottom drag area always attach dragModifier. In compact.kt, the caller flips enableDragToClose off while it is doing drag-to-arm menu activation, so the sheet can still move/close during the period that is supposed to be disabled.
Suggested fix
- val dragModifier =
- Modifier.draggable(
- state =
- rememberDraggableState { dragAmount ->
- val total = maxMenuHeightPx
- val boundedProgress = sheetState.progress.coerceIn(0f, maxAnchor)
- val currentVisible = total * (boundedProgress / maxAnchor).coerceIn(0f, 1f)
- val nextVisible = (currentVisible - dragAmount).coerceIn(0f, total)
- val nextProgress = ((nextVisible / total) * maxAnchor).coerceIn(0f, maxAnchor)
- sheetState.setProgressImmediate(nextProgress)
- },
- orientation = Orientation.Vertical,
- onDragStopped = { velocityY ->
- if (velocityY > flingCloseThreshold) {
- scope.launch {
- val anim = Animatable(sheetState.progress.coerceIn(0f, maxAnchor))
- anim.animateTo(0f, animationSpec = tween(durationMillis = 200)) {
- sheetState.setProgressImmediate(value)
- }
- sheetState.hideInstant()
- onDismiss()
- }
- } else {
- sheetState.settle(velocityY) { settledProgress ->
- if (settledProgress <= 0f) {
- onDismiss()
- }
- }
- }
- }
- )
+ val dragModifier =
+ if (enableDragToClose) {
+ Modifier.draggable(
+ state =
+ rememberDraggableState { dragAmount ->
+ val total = maxMenuHeightPx
+ val boundedProgress = sheetState.progress.coerceIn(0f, maxAnchor)
+ val currentVisible = total * (boundedProgress / maxAnchor).coerceIn(0f, 1f)
+ val nextVisible = (currentVisible - dragAmount).coerceIn(0f, total)
+ val nextProgress = ((nextVisible / total) * maxAnchor).coerceIn(0f, maxAnchor)
+ sheetState.setProgressImmediate(nextProgress)
+ },
+ orientation = Orientation.Vertical,
+ onDragStopped = { velocityY ->
+ if (velocityY > flingCloseThreshold) {
+ scope.launch {
+ val anim = Animatable(sheetState.progress.coerceIn(0f, maxAnchor))
+ anim.animateTo(0f, animationSpec = tween(durationMillis = 200)) {
+ sheetState.setProgressImmediate(value)
+ }
+ sheetState.hideInstant()
+ onDismiss()
+ }
+ } else {
+ sheetState.settle(velocityY) { settledProgress ->
+ if (settledProgress <= 0f) {
+ onDismiss()
+ }
+ }
+ }
+ }
+ )
+ } else {
+ Modifier
+ }Also applies to: 321-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/commonMain/kotlin/org/example/daybook/tables/FloatingBottomBar.kt`
around lines 204 - 245, The enableDragToClose flag is ignored because
dragModifier is always applied; update the code to only attach the draggable
modifier when enableDragToClose is true: wrap creation/use of dragModifier or
the .then(dragModifier) calls with a conditional (e.g., val effectiveModifier =
if (enableDragToClose) Modifier.then(dragModifier) else Modifier) and use
effectiveModifier for the Surface modifier and for the bottom drag area modifier
(the other place around the barHeight/drag area), so when enableDragToClose is
false the sheetState draggable handlers are not wired.
Comment on lines
37
to
+39
| fun main() = application { | ||
| installSignalHandler("INT") | ||
| installSignalHandler("TERM") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.ktRepository: dman-os/townframe
Length of output: 4963
🌐 Web query:
Compose Desktop "application" function recomposition behavior
💡 Result:
In Compose Multiplatform for Desktop (also known as Compose Desktop), the "application" refers to the application top-level composable function from androidx.compose.ui.window.application. This function creates an ApplicationScope where you define your app's top-level UI, typically by composing one or more Window composables. The application lambda is a @Composable scope that follows standard Compose recomposition rules: it re-executes when state read within it changes, following intelligent recomposition where Compose skips unchanged subtrees. Key behaviors: - Initial composition happens once on app launch. - Multiple recompositions can occur at startup due to window sizing/initialization. For example, windows may recompose twice: first with 0.dp size during layout, then with actual size after packing (fixed in later versions like 1.4.0-alpha01-dev985). - Window content recomposes on state changes (e.g., WindowState like size/position), user resizes, or observed State updates. - Each Window has its own Composition and Recomposer, scoped to the window (similar to Android's window-scoped Recomposers), so changes in one window don't trigger others unless shared state is read. - Use rememberWindowState for persistent state across recompositions. - To avoid excessive recompositions, hoist state outside composables (e.g., create ViewModels/root components in main before application), use remember, and perform navigation/side effects outside composition. singleWindowApplication is a convenience wrapper that internally uses application with one Window; it has similar behavior but limits to one window. Sources confirm standard Compose semantics apply, with desktop-specific triggers like window events causing targeted recompositions.
Citations:
- 1: https://oboe.com/learn/compose-multiplatform-desktop-ui-1xm6spc
- 2: https://kotlinlang.org/docs/multiplatform/compose-desktop-components.html
- 3: https://kotlinlang.org/docs/multiplatform/compose-multiplatform-new-project.html
- 4: Child gets recomposed more than once at startup in Compose Desktop arkivanov/Decompose#56
- 5: https://developer.android.com/develop/ui/compose/lifecycle
- 6: Window is recomposed 2 times at the start of the application JetBrains/compose-multiplatform#2951
- 7: https://kotlinlang.org/docs/multiplatform/compose-desktop-top-level-windows-management.html
- 8: https://oboe.com/learn/compose-multiplatform-desktop-ui-1xm6spc/desktop-state-lifecycle-2
- 9: https://android.googlesource.com/platform/frameworks/support/+/b5fe88c71231ae9b955b5dd50e583afbaa82b1fe/compose/desktop/desktop/samples/src/jvmMain/kotlin/androidx/compose/desktop/examples/windowapi/Examples.jvm.kt
- 10: https://kotlinlang.org/docs/multiplatform/compose-lifecycle.html
🌐 Web query:
Java sun.misc.Signal.handle idempotent repeated calls behavior
💡 Result:
sun.misc.Signal.handle is idempotent for repeated calls with the same signal and handler. Each call registers (or re-registers) the provided handler, replacing any previous one for that signal, and returns the prior handler. No exception is thrown for repeated registrations of the same handler, unlike attempts to register on VM-reserved signals which throw IllegalArgumentException. The method is static synchronized and delegates to jdk.internal.misc.Signal.handle, which uses a native method (handle0 in older versions) to set the OS-level signal handler. The native implementation treats re-registration as a replacement operation, overwriting the previous handler without error for user signals. Example from source (OpenJDK master): public static synchronized SignalHandler handle(Signal sig, SignalHandler handler) { jdk.internal.misc.Signal.Handler oldHandler = jdk.internal.misc.Signal.handle(sig.iSignal, InternalMiscHandler.of(sig, handler)); return SunMiscHandler.of(sig.iSignal, oldHandler); } Older versions (JDK8) explicitly show: long oldH = handle0(sig.number, newH); if (oldH == -1) { throw new IllegalArgumentException(...); } // Only for VM/OS conflict, not repeats Repeated calls with the same handler simply return the current (same) handler each time, confirming idempotence.
Citations:
- 1: https://github.com/openjdk/jdk/blob/master/src/jdk.unsupported/share/classes/sun/misc/Signal.java
- 2: https://github.com/frohoff/jdk8u-jdk/blob/master/src/share/classes/sun/misc/Signal.java
- 3: https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/signals_posix.cpp
- 4: https://hg.openjdk.org/jdk8u/jdk8u/jdk/file/tip/src/share/classes/sun/misc/Signal.java
Move signal handler installation into a one-time effect.
These calls execute directly in the application {} composition body, which can recompose multiple times during initialization and window lifecycle events. Each recomposition re-registers the handlers. While Signal.handle() is idempotent (repeated calls safely replace the previous handler), this pattern violates Compose best practices for side effects. Wrap in LaunchedEffect(Unit) to match the pattern already used in this file (see line 70).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/daybook_compose/composeApp/src/desktopMain/kotlin/org/example/daybook/main.kt`
around lines 37 - 39, installSignalHandler("INT") and
installSignalHandler("TERM") are being called directly in the application
composition body which can run on recomposition; move these calls into a
one-time side effect by wrapping them in LaunchedEffect(Unit) (matching the
pattern already used later) so the handlers are registered only once; locate the
calls to installSignalHandler in main and wrap them inside a
LaunchedEffect(Unit) block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Refactor
Documentation