Offline notes queue: capture while offline, send to an agent once online - #6905
austinywang wants to merge 41 commits into
Conversation
Implements #6483: capture notes while offline, persist them across app restarts, and hand them to an agent once connectivity is restored, with clear pending / sending / sent / failed status per note. Engine (fully unit-tested): - OfflineNote model + OfflineNotesStore: a Codable queue persisted to Application Support/cmux/offline-notes.json (atomic writes, reloaded on launch). NWPathMonitor-backed reachability drains pending notes through an injectable OfflineNoteDispatching seam whenever connectivity is (re)gained; failed notes are preserved and retryable. Notes stranded as `.sending` by a crash are normalized back to `.pending` on load. Delivery: - OfflineNoteAgentDispatcher stages each note non-destructively into the active workspace's agent composer (TextBox) draft — the cleanest native hand-off; the protocol seam lets this evolve without touching the queue. UI: - A beta-gated "Notes" right-sidebar mode (off by default, like Feed/Dock) to capture notes and watch their status, with retry/send/clear actions. Rows receive value snapshots + closures only (snapshot-boundary rule). All user-facing strings localized (en + ja). New files wired into project.pbxproj; pbxproj normalized and test-wiring lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 an opt-in Notes sidebar mode with offline note capture, queued delivery, a dedicated panel UI, launch-time startup, settings/search entries, and CLI/sidebar routing updates. ChangesOffline Notes
Sequence Diagram(s)sequenceDiagram
participant User
participant OfflineNotesPanelView
participant OfflineNotesStore
participant OfflineNoteAgentDispatcher
participant Workspace
participant focusedTerminalPanel
User->>OfflineNotesPanelView: submit draft
OfflineNotesPanelView->>OfflineNotesStore: addNote(draft, workspaceID)
OfflineNotesStore->>OfflineNotesStore: persist queue state
OfflineNotesStore->>OfflineNoteAgentDispatcher: dispatch(note)
OfflineNoteAgentDispatcher->>Workspace: resolveVisibleWorkspace(workspaceID)
OfflineNoteAgentDispatcher->>focusedTerminalPanel: restoreSessionTextBoxDraft(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (18 passed)
✨ 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 |
Greptile SummaryAdds a beta-gated offline notes queue: notes captured while offline are persisted as a bounded JSON array in Application Support, survive app restarts, and are staged into the captured workspace's agent composer once connectivity is restored. The engine (
Confidence Score: 4/5Safe to merge with the localization gap addressed; the queue engine, persistence, and agent dispatch are well-implemented and well-tested. The new strings added for the Notes feature are only translated into English and Japanese, leaving 18 of the 20 locales the string catalog already supports without translations. Users in those locales will see English text for every Notes UI surface — status badges, error messages, the empty-state subtitle, and beta-settings labels. Everything else — persistence, flush ordering, bounded growth, workspace binding, and termination durability — is solid and covered by tests. Resources/Localizable.xcstrings needs translations added for ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant for all 24 new offlineNotes.* and related keys. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User captures note] --> B{isOnline?}
B -- Yes --> C[addNote → scheduleFlush]
B -- No --> D[addNote → persist to disk]
D --> E[NWPathMonitor: connectivity regained]
E --> F[handleReachabilityChange → scheduleFlush]
C --> G[flush]
F --> G
G --> H{isFlushing?}
H -- Yes --> I[no-op]
H -- No --> J{pending notes?}
J -- No --> K[defer: pruneSent + persist]
J -- Yes --> L[mark .sending]
L --> M[dispatcher.dispatch note]
M --> N{success?}
N -- Yes --> O[mark .staged]
N -- noActiveWorkspace --> P[revert .pending, defer this pass]
N -- other error --> Q[mark .failed + lastError]
O --> R[persist + waitForPendingPersist]
P --> R
Q --> R
R --> J
K --> S[done]
R --> S
T[AppDelegate: applicationWillTerminate] --> U{hasInstance?}
U -- Yes --> V[flushPendingPersistOnTermination: writeQueue.asyncAndWait]
U -- No --> W[skip]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User captures note] --> B{isOnline?}
B -- Yes --> C[addNote → scheduleFlush]
B -- No --> D[addNote → persist to disk]
D --> E[NWPathMonitor: connectivity regained]
E --> F[handleReachabilityChange → scheduleFlush]
C --> G[flush]
F --> G
G --> H{isFlushing?}
H -- Yes --> I[no-op]
H -- No --> J{pending notes?}
J -- No --> K[defer: pruneSent + persist]
J -- Yes --> L[mark .sending]
L --> M[dispatcher.dispatch note]
M --> N{success?}
N -- Yes --> O[mark .staged]
N -- noActiveWorkspace --> P[revert .pending, defer this pass]
N -- other error --> Q[mark .failed + lastError]
O --> R[persist + waitForPendingPersist]
P --> R
Q --> R
R --> J
K --> S[done]
R --> S
T[AppDelegate: applicationWillTerminate] --> U{hasInstance?}
U -- Yes --> V[flushPendingPersistOnTermination: writeQueue.asyncAndWait]
U -- No --> W[skip]
Reviews (33): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
Only the four files the offline-notes wiring pushed over their budget are bumped to their current line counts; untouched entries keep their accepted origin values to minimize the budget diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/MainWindowFocusController.swift`:
- Line 120: Wire a real focus endpoint for Notes mode: MainWindowFocusController
currently treats .notes like a generic host-backed mode, but no host is recorded
for it and the focus check for that mode always fails, so
focusRightSidebarInActiveMainWindow(mode: .notes, ...) can’t actually move
keyboard focus into Notes. Update the mode handling in MainWindowFocusController
so .notes has its own valid focus target/host (or a special-case path that can
succeed), and make the corresponding focus eligibility logic return true for the
Notes UI instead of always rejecting it.
In `@Sources/OfflineNoteAgentDispatcher.swift`:
- Around line 52-57: The terminal target selection in terminalPanel(in:) is
nondeterministic because it falls back to the first TerminalPanel from
workspace.panels.values when focusedTerminalPanel is nil. Replace that
collection-order fallback with a reliable structured current-target source of
truth for the composer route, and if no valid active terminal can be determined,
fail closed rather than picking an arbitrary pane.
In `@Sources/OfflineNotesPanelView.swift`:
- Around line 237-240: The relative-time formatting in the OfflineNotesPanelView
row is allocating a new RelativeDateTimeFormatter on every body recompute, which
should be avoided in hot-path list rendering. Update the relativeTime computed
property to use a cached formatter or move the formatted string into the row
snapshot/model so the Row render path does not create a formatter per note. Use
the existing OfflineNotesPanelView and relativeTime symbol to locate the change.
In `@Sources/OfflineNotesStore.swift`:
- Around line 311-315: The fallback error handling in OfflineNotesStore’s note
update path is exposing raw dispatcher details through the user-visible
lastError field. Update the catch blocks around note(id:) / failed.status
handling to use a fixed localized fallback string for lastError instead of
Self.describe(error) or error.localizedDescription, and keep the detailed error
only in private diagnostics/logging. Apply the same change in both affected
catch sites so the notes UI never surfaces upstream vendor or backend text.
- Around line 160-166: OfflineNotesStore is introducing ambient runtime state as
a process-global singleton and ObservableObject/@Published owner, which
conflicts with the SwiftUI state ownership rules. Remove the shared
singleton/global store pattern from OfflineNotesStore and move note/online state
ownership into an explicit coordinator or model that is injected into the panel,
using `@Observable/value-snapshot` style state instead of `@Published`. Update the
panel and any callers to receive this state through dependency injection rather
than accessing OfflineNotesStore.shared.
- Around line 174-183: The OfflineNotesStore persistence work is happening on
the MainActor during initialization and flush/mutation paths, which can block
responsiveness. Move the JSON load/parse and rewrite/encode work out of
OfflineNotesStore into a dedicated actor/service (or detached task-backed
repository), using OfflineNotesStore and flush-related helpers as the handoff
points. Keep only snapshot publication and any UI/process-launch coordination on
`@MainActor`, and hop back there explicitly after the background persistence
completes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0ae59ddd-e6ad-4be9-8e47-d268b23e5c3a
📒 Files selected for processing (15)
Resources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/MainWindowFocusController.swiftSources/OfflineNoteAgentDispatcher.swiftSources/OfflineNotesPanelView.swiftSources/OfflineNotesStore.swiftSources/RightSidebarMode+Availability.swiftSources/RightSidebarPanelView.swiftSources/RightSidebarToolPanel.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OfflineNotesStoreTests.swift
- Add the Notes beta flag as an editable Settings row: a `rightSidebarNotes` key in BetaFeaturesCatalogSection and a `notesRow` in BetaFeaturesSection (searchAnchorID setting:betaFeatures:notes, localized on/off subtitles), so the Settings search result and command-palette toggle now resolve to a real, editable row. Register the anchor in SettingsRowAnchorResolutionTests. - Coalesce OfflineNotesStore flush persistence to a single atomic write per pass (in-memory status transitions + one persist on completion) so a reconnect with a large backlog does bounded main-actor disk work; a status lost mid-flush is recovered as pending on next launch. - Localize the new betaFeatures:notes search alias and the row subtitles (en + ja) in Localizable.xcstrings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit/Greptile review follow-ups: - Convert OfflineNotesStore to @observable (drop ObservableObject/@Published/ Combine), matching the codebase migration direction (e.g. PaneMemoryGuardrail). Panel consumes it via a plain reference; SwiftUI tracks the reads in body. - Move persistence off the main actor: writes are coalesced (latest snapshot wins) and encoded + atomically written in a nonisolated task. Add waitForPendingPersist() (also useful at app termination). - Dispatcher: drop the nondeterministic first-terminal fallback; target only the workspace's focused terminal and fail closed (note stays queued) otherwise. - Mask unknown dispatcher errors behind a fixed localized message; keep raw detail in DEBUG diagnostics only. - Wire a real focus endpoint for Notes mode (route through the sidebar host so command-palette/shortcut focus lands in the panel). - Cache the RelativeDateTimeFormatter and JSON coders instead of allocating per render / per write. - Localize the generic dispatch-error message (en + ja). Bump MainWindowFocusController budget for the added comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex) follow-ups on the new queue: - Bound app-lifetime growth: truncate a single note to maxNoteLength (100k chars) and auto-evict the oldest already-sent notes beyond maxRetainedSentNotes (200) after add/flush. Pending and failed notes are never auto-evicted, so unsent work is preserved. - Migrate OfflineNotesStoreTests from XCTest to Swift Testing (import Testing, @Suite/@test, #expect/#require) per the repo's new-test policy, and add coverage for note truncation and oldest-sent eviction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex) follow-ups: - P1: pending/failed notes are now bounded too. addNote reclaims old sent notes then applies backpressure — it refuses new captures once the queue reaches maxTotalNotes (1000) instead of growing memory/disk without bound. Existing unsent work is never silently evicted. - P2: persist each note's terminal state (off the main actor, awaited) before dispatching the next one, so a crash/kill mid-flush can't replay a note that was already staged into the composer. Add a backpressure test; trim a doc comment to keep the file under the length budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex) follow-ups on the dispatcher: - P1 (wrong-workspace delivery): each note now records the workspace it was captured in (OfflineNote.workspaceID, set from the Notes panel). The default dispatcher delivers only to that workspace — found across open windows — and fails closed (note stays queued/retryable) if it no longer exists, instead of staging into whatever workspace is active minutes later at flush time. - P2 (silent hidden draft): staging now makes the composer visible (isActive: true) without stealing focus, so the user actually sees the note delivered into the captured workspace's agent composer. Add a capture-binding test. Trim doc comments to stay under the length budget; refresh the now-accurate flush persistence doc comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex) follow-ups: - addNote now schedules a flush when the store is already online, so a note captured while connected is handed off immediately instead of sitting pending until the next reconnect or manual Send (the isFlushing guard prevents dupes). - Start OfflineNotesStore at app launch when the Notes beta is enabled, so notes persisted from a previous session are delivered once connectivity returns even if the user never opens the Notes sidebar this session. Add an online-capture test; bump AppDelegate length budget for the launch hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex): the off-main fire-and-forget persist could lose a note if the user captured it and quit before the write drained. Split persistence: - Discrete user mutations (capture/delete/retry/clear) now write synchronously, so a note is durable the moment it is captured — an immediate quit can't lose it (one small atomic write; negligible main-actor cost). - flush() still writes off the main actor (awaited per note) so draining a backlog stays bounded on the main actor while remaining crash-durable. Removes the coalescing writer; updates the restart test (capture is now durable synchronously). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex P1): the shared static JSONEncoder was used by both the synchronous main-actor persist() and the detached off-main flush writer, so a capture/delete during a backlog flush could enter encoder.encode concurrently with the detached write and corrupt/crash. JSONEncoder/JSONDecoder are mutable Foundation reference types, so create a fresh one per write/load instead of sharing a process-wide instance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex P2): the synchronous capture write could rewrite ~100 MB on the main actor because the caps allowed 1,000 notes of 100,000 chars. Per Codex's "reduce the retained payload bounds" option, tighten the caps to a note-sized queue (maxNoteLength 4,000, maxTotalNotes 200, maxRetainedSentNotes 100) so the worst-case backing file is well under 1 MB and the durable sync capture write stays a sub-millisecond, bounded main-actor cost. Flush still writes off-main. Also document why dispatch stages into the composer rather than auto-submitting: the reconnect flush is automatic/background, so submitting note text into the focused terminal could run it unreviewed (e.g. as a shell command); staging keeps delivery safe and visible for the user to review and send. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/OfflineNotesStore.swift (1)
321-361: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize all note-file writes through one persistence path.
persist()andpersistOffMainAndWait()both write the same file independently. Whileflush()is suspended waiting for the detached snapshot write, the main actor can still rundeleteNote,retry, orclearSent, and those methods immediatelypersist()a newer state. If the older detached write finishes afterward, it overwrites that newer file with stale contents, so the next launch can resurrect deleted notes or revert retry state. Route both paths through one serialized writer/actor and only await that shared drain fromflush().Also applies to: 421-434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OfflineNotesStore.swift` around lines 321 - 361, flush() and the file-writing methods are racing because persist() and persistOffMainAndWait() write the same notes snapshot independently. Update OfflineNotesStore so all note-file saves go through one serialized persistence path/actor, and have flush() await that shared writer instead of a separate detached write. Make sure deleteNote, retry, clearSent, and flush all funnel through the same persistence entry point to prevent stale writes from overwriting newer state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/OfflineNoteAgentDispatcher.swift`:
- Around line 60-69: The workspace lookup in defaultResolveWorkspace(_:), which
is called repeatedly from flush(), rescans AppDelegate.mainWindowContexts and
each tabManager.tabs for every queued note. Refactor the note dispatch path so
workspace IDs are resolved once per flush by building an index or adding an O(1)
lookup on AppDelegate, then reuse that mapping for each note instead of
repeating the full collection scan.
- Around line 56-64: The default routing in defaultResolveWorkspace(_:) is
falling back to activeTabManagerForCommands()?.selectedWorkspace when
workspaceID is missing, which can misroute legacy queued notes based on current
UI state. Update OfflineNoteAgentDispatcher’s defaultResolveWorkspace(_:) to
fail closed by returning nil whenever the workspace binding is absent, or handle
migration before dispatch, so routing always uses the authoritative workspace
source of truth.
In `@Sources/RightSidebarPanelView.swift`:
- Around line 433-434: The Notes panel is bypassing the view’s injected
workspace identifier by reading tabManager.selectedWorkspace?.id directly, which
breaks consistency with the .dock case. Update the .notes branch in
RightSidebarPanelView to pass the existing workspaceId property into
OfflineNotesPanelView, matching the other panel cases and keeping the view
dependent on its explicit prop.
---
Outside diff comments:
In `@Sources/OfflineNotesStore.swift`:
- Around line 321-361: flush() and the file-writing methods are racing because
persist() and persistOffMainAndWait() write the same notes snapshot
independently. Update OfflineNotesStore so all note-file saves go through one
serialized persistence path/actor, and have flush() await that shared writer
instead of a separate detached write. Make sure deleteNote, retry, clearSent,
and flush all funnel through the same persistence entry point to prevent stale
writes from overwriting newer state.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 26588572-6160-4901-95b0-c3bec06d12ea
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (14)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/MainWindowFocusController.swiftSources/OfflineNoteAgentDispatcher.swiftSources/OfflineNotesPanelView.swiftSources/OfflineNotesStore.swiftSources/RightSidebarPanelView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OfflineNotesStoreTests.swift
Autoreview (Codex) on the merge commit: - P2 (race): the prior split-write design let an older off-main flush write land after a newer synchronous capture write, overwriting it. Replace both paths with a single serialized coalescing off-main writer that always persists the latest snapshot (writes never overwrite newer state, one write at a time, O(1) main-actor snapshot). Quit-durability is guaranteed by a synchronous flushPendingPersistOnTermination() called from applicationWillTerminate. - P3 (organization): split the domain types into focused files — OfflineNote, OfflineNoteDispatching, OfflineNotesReachability — leaving OfflineNotesStore as just the queue/store (364 lines). Bump AppDelegate budget for the termination hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex P2): the synchronous termination write could be overwritten by a still-in-flight older async write. Route every disk write through a single serial DispatchQueue so writes have a total order — the newest snapshot is always last to land, and the termination write (dispatched sync, last in the serial order) can no longer be overwritten by an earlier async write. Keeps the main-actor cost to an O(1) snapshot + enqueue, per-note flush durability via an enqueued barrier, and fresh per-write coders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview (Codex P2 / shared-behavior policy): the app-side socket parser accepted `notes`, but the bundled CLI still whitelisted only files/find/vault/ sessions/feed/dock, so `cmux right-sidebar set notes` (and the `notes` alias) were rejected before reaching the app. Add `notes` to the CLI mode whitelist and normalizer (isRightSidebarCLIMode / normalizedRightSidebarCLIArgument), the shortcut-action case, and the usage/error help; localize the updated error (en + ja); and cover `from(cliArgument: "notes")` in the mode-parser test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rage cmux-policy (Aziz) triage: - File-organization: split the companion types into their own files — OfflineNoteStatus, OfflineNoteDispatchError, and OfflineNotesNetworkReachability (leaving the protocols/struct one-per-file). - Test framework: keep FileExplorerStateModePersistenceTests (XCTest) untouched and add the new `from(cliArgument: "notes")` coverage in the Swift Testing OfflineNotesStoreTests suite instead. - Document why persistence uses a serial DispatchQueue (ordered I/O executor + synchronous terminate flush) rather than an actor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/OfflineNotesStoreTests.swift (1)
196-212: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert unique note IDs in the reentrancy test.
dispatcher.dispatched.count == 2still passes if one note is dispatched twice and the other is skipped. Assert the dispatched IDs match the queued note IDs so this test actually proves “each note once.”Suggested fix
_ = await (firstFlush, secondFlush) await waitUntil { store.notes.allSatisfy { $0.status == .sent } } `#expect`(dispatcher.dispatched.count == 2) + `#expect`(Set(dispatcher.dispatched.map(\.id)).count == 2) + `#expect`(Set(dispatcher.dispatched.map(\.id)) == Set(store.notes.map(\.id)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/OfflineNotesStoreTests.swift` around lines 196 - 212, The reentrancy test in concurrentFlushesDispatchEachNoteOnce only checks the total dispatch count, so it can miss duplicate dispatches of the same note. Update the assertion in this test to verify the dispatched note IDs are exactly the two queued note IDs from addNote("a") and addNote("b"), using the dispatcher.dispatched values, so the test proves each note is dispatched once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 2003-2005: The quit-time flush in AppDelegate is incorrectly gated
by the mutable RightSidebarBetaFeatureSettings.isNotesEnabled() flag, which can
skip the final persistence for an already-used OfflineNotesStore. Update the
termination path to call
OfflineNotesStore.shared.flushPendingPersistOnTermination() based on store
usage/instantiation state rather than the current beta toggle, and use the
OfflineNotesStore and flushPendingPersistOnTermination symbols to locate the
shutdown logic.
In `@Sources/OfflineNotesStore.swift`:
- Around line 277-295: The persistence pipeline in OfflineNotesStore does not
surface write failures, so flush and addNote can continue as if durability
succeeded. Update persist() and writeToDisk/writeQueue handling so the async
write reports success or failure back to callers, and change
waitForPendingPersist() to return or throw that result instead of only draining
the queue. Then have flush() and addNote() check the reported outcome and fail
closed when the disk write breaks, using the existing OfflineNotesStore symbols
to wire the error path through.
---
Outside diff comments:
In `@cmuxTests/OfflineNotesStoreTests.swift`:
- Around line 196-212: The reentrancy test in
concurrentFlushesDispatchEachNoteOnce only checks the total dispatch count, so
it can miss duplicate dispatches of the same note. Update the assertion in this
test to verify the dispatched note IDs are exactly the two queued note IDs from
addNote("a") and addNote("b"), using the dispatcher.dispatched values, so the
test proves each note is dispatched once.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 05c1b490-d38a-4499-bf1d-b8a0e8c390f2
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (13)
CLI/CMUXCLI+ThemeSupport.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/OfflineNote.swiftSources/OfflineNoteDispatchError.swiftSources/OfflineNoteDispatching.swiftSources/OfflineNoteStatus.swiftSources/OfflineNotesNetworkReachability.swiftSources/OfflineNotesReachability.swiftSources/OfflineNotesStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OfflineNotesStoreTests.swift
Autoreview (Codex P2): the launch-time flush can run before windows are restored, so the dispatcher found no workspace and the notes were marked failed (stranded, needing manual retry). Distinguish transient unavailability from real failure: - No window at all (launch race / all windows closed) -> noActiveWorkspace, which the store treats as transient: revert the note to pending and stop the pass (retry on the next flush). Notes are never stranded as failed. - A window exists but the captured workspace is gone / has no terminal -> noComposerTarget -> failed (retryable from the UI). - Flush when the Notes panel appears, so pending notes are delivered once a window is up even if an earlier flush ran too early. Adds a no-window transient test; updates the dispatcher (hasAnyWindow seam). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/OfflineNotesStore.swift (1)
295-308: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersistence failures are still swallowed and not surfaced to callers.
writeToDiskdiscards write errors (DEBUG-only log) andwaitForPendingPersist()only drainswriteQueuewithout reporting whether the write succeeded. If the disk write fails (permissions, disk-full),addNoteandflush()still treat capture/hand-off as durable, so queued notes can be lost or replayed on restart despite the durability contract. Consider having the write path record a last-write error thatwaitForPendingPersist()returns/throws so delivery fails closed when persistence is broken.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OfflineNotesStore.swift` around lines 295 - 308, Persistence errors from OfflineNotesStore.writeToDisk are currently swallowed, so addNote and flush can continue as if the hand-off was durable; update persist() and waitForPendingPersist() to record the last write failure and surface it back to callers by returning/throwing from waitForPendingPersist(). Use the existing OfflineNotesStore helpers (persist, writeToDisk, waitForPendingPersist, flush) to ensure queued notes fail closed when a disk write fails instead of silently proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/OfflineNotesStore.swift`:
- Around line 295-308: Persistence errors from OfflineNotesStore.writeToDisk are
currently swallowed, so addNote and flush can continue as if the hand-off was
durable; update persist() and waitForPendingPersist() to record the last write
failure and surface it back to callers by returning/throwing from
waitForPendingPersist(). Use the existing OfflineNotesStore helpers (persist,
writeToDisk, waitForPendingPersist, flush) to ensure queued notes fail closed
when a disk write fails instead of silently proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2296fe44-741f-43e9-b274-7b32090b21b9
📒 Files selected for processing (4)
Sources/OfflineNoteAgentDispatcher.swiftSources/OfflineNotesPanelView.swiftSources/OfflineNotesStore.swiftcmuxTests/OfflineNotesStoreTests.swift
Autoreview (Codex) follow-ups on delivery lifecycle: - Stranded-pending: hook a retry to window availability — flush on applicationDidBecomeActive (gated), so pending notes are delivered once windows are up even if Notes is never opened. - Hidden-workspace "sent": the dispatcher now delivers only when the captured workspace is the currently-visible (selected) workspace; otherwise it signals a transient condition so the note stays pending and is retried when that workspace becomes visible — a note is never marked sent into a hidden draft. - flush() now defers (skips, doesn't break on) transient notes so a not-yet- visible workspace can't block delivery of other notes, with no spin. Bump AppDelegate budget for the activation hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The store's init declared @MainActor-isolated defaults (OfflineNoteAgentDispatcher(), OfflineNotesNetworkReachability()) as default arguments, which are evaluated in a nonisolated context — a main-actor isolation error that broke the app build (SwiftEmitModule for cmux_DEV). Make those parameters optional (nil) and build the concrete defaults inside the init body, which runs on the main actor. Tests still inject fakes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/OfflineNotesStore.swift (1)
341-347: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't treat a load failure as an empty queue.
Line 342 returns
[]for any read or decode error. A partial or corrupt file will make every queued note disappear in memory, and the next successful persist can overwrite the original file permanently. Preserve/quarantine the unreadable file and surface a recoverable load failure instead of silently starting from empty.Based on PR objectives, notes are expected to survive app restarts.
♻️ Duplicate comments (2)
Sources/OfflineNoteAgentDispatcher.swift (1)
69-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when a queued note has no workspace binding.
Line 71 falls back to the currently selected workspace when
workspaceIDis missing, so a legacy queued note can be staged into whatever workspace happens to be active at reconnect. That breaks the capture-routing contract; migrate unbound notes before dispatch or returnnilhere instead of guessing.Suggested fix
- guard let workspaceID else { - return appDelegate.activeTabManagerForCommands()?.selectedWorkspace - } + guard let workspaceID else { return nil }As per path instructions, correctness-critical routing should use “a single authoritative, structured source of truth” and fail closed when that signal is missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OfflineNoteAgentDispatcher.swift` around lines 69 - 73, The fallback in defaultResolveVisibleWorkspace(_:) currently guesses the workspace by using the active selected workspace when workspaceID is missing, which can route a queued note incorrectly. Change this logic to fail closed: if the note has no bound workspace, return nil instead of falling back to AppDelegate.shared.activeTabManagerForCommands()?.selectedWorkspace. If legacy unbound notes need support, migrate or bind them before dispatch in the caller path rather than inside this resolver.Source: Path instructions
Sources/OfflineNotesStore.swift (1)
301-314: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
waitForPendingPersist()report write failures.Lines 301-338 only guarantee queue ordering. They do not tell
addNote()orflush()whether the queued write actually succeeded. If disk-full or permission errors occur, capture still looks successful andflush()can continue as if the terminal state was durable, which breaks the persisted-queue contract. Have the write path latch/return aResultand makewaitForPendingPersist()throw it so callers fail closed when persistence breaks.Based on PR objectives, notes must be stored locally and survive app restarts.
Also applies to: 326-338
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OfflineNotesStore.swift` around lines 301 - 314, The current `persist()`/`waitForPendingPersist()` path only waits for queue ordering and never surfaces disk write errors, so `addNote()` and `flush()` can continue after a failed write. Update `OfflineNotesStore` so the queued write in `persist()` captures success or failure as a `Result`, then make `waitForPendingPersist()` throw that stored failure instead of just resuming after the queue drains. Ensure the callers of `waitForPendingPersist()` in the note persistence flow handle the thrown error and fail closed when `writeToDisk` does not succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/OfflineNoteAgentDispatcher.swift`:
- Around line 69-73: The fallback in defaultResolveVisibleWorkspace(_:)
currently guesses the workspace by using the active selected workspace when
workspaceID is missing, which can route a queued note incorrectly. Change this
logic to fail closed: if the note has no bound workspace, return nil instead of
falling back to
AppDelegate.shared.activeTabManagerForCommands()?.selectedWorkspace. If legacy
unbound notes need support, migrate or bind them before dispatch in the caller
path rather than inside this resolver.
In `@Sources/OfflineNotesStore.swift`:
- Around line 301-314: The current `persist()`/`waitForPendingPersist()` path
only waits for queue ordering and never surfaces disk write errors, so
`addNote()` and `flush()` can continue after a failed write. Update
`OfflineNotesStore` so the queued write in `persist()` captures success or
failure as a `Result`, then make `waitForPendingPersist()` throw that stored
failure instead of just resuming after the queue drains. Ensure the callers of
`waitForPendingPersist()` in the note persistence flow handle the thrown error
and fail closed when `writeToDisk` does not succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a553ed1-f06b-49d9-8cb6-8fa5d9a93942
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/OfflineNoteAgentDispatcher.swiftSources/OfflineNotesStore.swift
makeStore declared `dispatcher: FakeDispatcher = FakeDispatcher()` — a @mainactor init in a nonisolated default-argument context, which broke the cmuxTests module build. Default it to nil and build the fake in the helper body (same fix as the store init). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolved conflicts in 5 files: - Sources/AppDelegate.swift: kept origin's saveSessionSnapshot/flushPendingSaves relocation; retained the offline-notes quit-durability hook, now gated on OfflineNotesStore.hasInstance (set from init) rather than the live beta toggle so a note captured before Notes was toggled off is still flushed, without force-creating the store for users who never opened Notes. - Sources/OfflineNotesStore.swift: added `hasInstance` (set in init; shared uses the proven simple `= OfflineNotesStore()` form) to decouple quit durability from the beta flag. - Sources/RightSidebarPanelView.swift: kept origin's .dock refactor + the .notes panel case. - cmux.xcodeproj/project.pbxproj: union of both sides' new file entries. - Resources/Localizable.xcstrings: programmatic 3-way JSON merge — 23 offlineNotes keys added, 3 mode-list strings updated to include "notes"; en+ja parity. Also refreshed .github/swift-file-length-budget.tsv via --write-budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
The offline-notes length cap uses String.prefix(maxNoteLength), which counts grapheme clusters. A single grapheme cluster can hold unboundedly many combining scalars, so a combining-mark-heavy paste (or "Zalgo" text) passes the cap essentially untrimmed and every persist re-encodes an arbitrarily large payload, breaking the documented bounded-file guarantee. These tests fail against the current grapheme-count cap (added here first, per the two-commit regression policy); the scalar-count fix lands in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 30 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The note-size limit used `String.prefix(maxNoteLength)`, which counts extended grapheme clusters. A single grapheme cluster can hold unboundedly many combining scalars, so a pasted "Zalgo"/combining-mark blob passed the cap essentially untrimmed and was then re-encoded on every persist — breaking the store's bounded-file guarantee and its cheap-synchronous-write invariant. Cap on Unicode scalars via `boundedNoteText(_:)`, truncating on a scalar boundary so the result is always valid UTF-8 and each note is bounded to <= 4 x maxNoteLength UTF-8 bytes. Re-apply the cap in `load()` so an oversized note written by an older build cannot reappear unbounded and be rewritten on every subsequent persist. Regression tests (added in the previous commit) cover both the capture and load paths; they fail on the old grapheme cap (50001 scalars retained) and pass here (4000). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review findings on the offline-notes queue: - Scope the queue file by bundle identifier (offline-notes-<bundleId>.json), mirroring ClosedItemHistory. Production, staging, and tagged side-by-side debug builds are separate processes with their own store and serial write queue; the previous fixed "offline-notes.json" was shared across all variants, so concurrent captures were last-writer-wins JSON rewrites that dropped the other app's notes and exposed one variant's queued text in another. Adds regression coverage that two bundle ids resolve to distinct files and that path-hostile characters are sanitized to a single valid component. - Fix OfflineNotesNetworkReachability.stop(): it cancelled the monitor but left `started` true, so a later start() no-op'd on the guard and never resumed monitoring (NWPathMonitor is also terminal after cancel()). Reset state and swap in a fresh monitor so stop()/start() cycles work. - Defer OfflineNotesStore.shared.start() off the immediate launch path via DispatchQueue.main.async: start() lazily instantiates the store, which reads and decodes the queue file synchronously, so keeping it off the launch stack preserves window/bootstrap responsiveness. The terminate flush is gated on hasInstance, so nothing is lost if the app quits before it runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The load path re-caps each note's text but does not enforce the queue's size bounds (maxTotalNotes / maxRetainedSentNotes) that the write path maintains. A stale, corrupt, or hand-edited offline-notes file can therefore put an unbounded number of notes into the long-lived @observable store, driving unbounded sidebar rows, O(n) count scans, and full-array JSON rewrites on every persist. These two tests fail against the current load() (which returns the full decoded array): one asserts the queue is trimmed to maxTotalNotes keeping the newest captures, the other asserts the oldest sent notes are evicted down to maxRetainedSentNotes while pending work is preserved. The fix follows in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The write path caps captures (maxTotalNotes / maxRetainedSentNotes), but load() trusted the entire decoded array, so a stale, corrupt, or hand-edited offline-notes file could put an unbounded number of notes into the long-lived @observable store — driving unbounded sidebar rows, O(n) count scans, and full-array JSON rewrites on every persist, despite maxTotalNotes documenting a hard queue bound. Mirror the write-path policy on load: - Extract the sent-note eviction into a pure `prunedSentNotes(_:)` shared by the live `pruneSentNotes()` and a new `boundedForLoad(_:)`. - `boundedForLoad(_:)` evicts the oldest sent notes beyond maxRetainedSentNotes, then keeps the newest maxTotalNotes captures. - Skip a file larger than `maxPersistedFileBytes` (a cheap stat) before `Data(contentsOf:)`/decode so a pathological file can't balloon memory or block the @mainactor init before the bounds can trim it. The two load-bounding tests added in the previous commit now pass; a third asserts the file-size ceiling boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adding `.notes` to `RightSidebarMode.availableModes()` made `testCommandPaletteIncludesDefaultRightSidebarModes` (which asserts exactly 3 default modes) depend on the ambient `rightSidebar.beta.notes.enabled` UserDefaults key, but the test only cleared feed+dock and the `withSavedBetaFeatureDefaults` helper only saved/restored those two. On any dev/CI machine where the notes beta key was already true, the test would see 4 available modes and fail. Clear the notes key alongside feed+dock in the default-modes test, save and restore it in the helper, and assert the notes contribution is absent for symmetry with the existing feed/dock nil checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
…ux-notes-should-queue-agent
…ux-notes-should-queue-agent # Conflicts: # .github/swift-file-length-budget.tsv
Fixes #6483
What
Adds an offline notes queue: users can capture notes while offline, the notes are stored locally and survive app restarts, and once connectivity returns cmux hands each note to an agent. Every note shows a clear status — pending / sending / sent / failed.
How it works
Engine (
OfflineNotesStore, fully unit-tested)OfflineNoteis aCodablevalue persisted as a JSON array underApplication Support/cmux/offline-notes.json(atomic writes, reloaded on launch → survives restarts).NWPathMonitor-backed reachability monitor drains pending notes whenever connectivity is (re)gained. Draining is one-at-a-time with a reentrancy guard so overlapping flushes never double-send..sendingby a crash/relaunch are normalized back to.pendingon load.OfflineNotesReachabilityMonitoring,OfflineNoteDispatching), so the queue logic is exercised deterministically in tests.Delivery (
OfflineNoteAgentDispatcher)UI (
OfflineNotesPanelView)Tests
cmuxTests/OfflineNotesStoreTests.swiftcovers: persistence across store instances (restart), whitespace ignored,.sending→.pendingnormalization on load, offline does not flush, regaining connectivity flushes pending → sent, failure → failed → retry → sent, concurrent-flush reentrancy, andclearSent.This is a feature (not a bug fix), so tests land with the implementation rather than as a separate red/green pair.
Localization & wiring
Resources/Localizable.xcstrings(en + ja).project.pbxproj;normalize-pbxproj.py --checkandlint-pbxproj-test-wiring.shpass.Notes / scope
🤖 Generated with Claude Code
Summary by cubic
Adds an offline notes queue so you can capture notes while offline, keep them across restarts, and auto-send to an agent once back online or immediately when captured online. Addresses #6483 and adds a beta right‑sidebar Notes mode with clear pending/sending/staged/failed states.
New Features
OfflineNotesStore(@Observable) with per-note status and retry. JSON in Application Support, namespaced per bundle ID; single serialized writer (ordered, off-main) with a termination flush; normalizes.sendingto.pendingon load; uses.stagedfor delivered notes (legacy.sentsupported).NWPathMonitor, immediate dispatch when captured online, queue starts at app launch (beta‑gated) with startup deferred; flush runs when Notes opens, on app activation, and on workspace selection so deferred notes deliver when their workspace becomes visible.OfflineNoteAgentDispatcher: delivers only to the currently visible workspace the note was captured in; otherwise the note stays pending. Stages text visibly and non‑destructively into that workspace’s focused terminal agent composer; localized errors; fails closed if no target.cmux right-sidebar set notes). All strings localized (en/ja); CLI and remote help now listnotes, and Settings search includesnotes.Bug Fixes
offline-notes-<bundleId>.json) with safe path sanitization to prevent cross‑bundle overwrites.stop()now fully resets state and swaps in a fresh monitor sostart()can resume; prevents losing connectivity updates after a stop/start cycle.Written for commit 141b2c7. Summary will update on new commits.
Summary by CodeRabbit