Repository navigation
Preserve complete chronological notification history - #7875
azooz2003-bit wants to merge 95 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughNotification handling now preserves stable UUIDs and acceptance timestamps, orders feed entries deterministically, reports explicit queue backpressure outcomes, merges restored session state, tracks external-banner ownership, and updates control-socket listing, UI projections, tests, documentation, localization, and project wiring. ChangesNotification queue and delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalController
participant AgentNotificationDelivery
participant TerminalMutationBus
participant TerminalNotificationStore
participant Workspace
TerminalController->>AgentNotificationDelivery: enqueue notification
AgentNotificationDelivery->>TerminalMutationBus: return delivery outcome
TerminalMutationBus->>TerminalNotificationStore: deliver stable id and acceptedAt
Workspace->>TerminalNotificationStore: merge restored notifications
TerminalNotificationStore->>TerminalNotificationStore: deduplicate and order chronologically
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (7 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 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 SummaryThis PR preserves complete notification history while separating feed order from external delivery state. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (60): Last reviewed commit: "Fix agent resume liveness test initializ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/TerminalController.swift (1)
12318-12328: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale
coalesces=0in the enqueue debug log.Coalescing was removed from
enqueueNotification, so the hardcodedcoalesces=0in this DEBUG log is now misleading. Drop it to avoid confusion when tracing the enqueue path.🧹 Proposed cleanup
cmuxDebugLog( - "socket.notifyTargetAsync.enqueue workspace=\(tabId.uuidString.prefix(8)) surface=\(surfaceId.uuidString.prefix(8)) titleLen=\(title.count) subtitleLen=\(subtitle.count) bodyLen=\(body.count) coalesces=0" + "socket.notifyTargetAsync.enqueue workspace=\(tabId.uuidString.prefix(8)) surface=\(surfaceId.uuidString.prefix(8)) titleLen=\(title.count) subtitleLen=\(subtitle.count) bodyLen=\(body.count)" )🤖 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/TerminalController.swift` around lines 12318 - 12328, Remove the obsolete hardcoded “coalesces=0” field from the DEBUG log in the notification enqueue path within the surrounding notification method, while preserving the workspace, surface, and length details.cmuxTests/WorkspaceManualUnreadTests.swift (1)
525-575: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the remaining test that still expects reordering.
markLatestNotificationAsOldestUnreadnow preserves feed order (Lines 1394-1398), buttestMarkLatestNotificationAsOldestUnreadAppendsWhenNoOtherUnreadNotificationsRemainat Lines 1182-1219 still expects the old append behavior. The suite will fail.- XCTAssertEqual(store.notifications.map(\.id), [readNotificationId, currentNotificationId]) - XCTAssertFalse(store.notifications.last?.isRead ?? true) + XCTAssertEqual(store.notifications.map(\.id), [currentNotificationId, readNotificationId]) + XCTAssertFalse(store.notifications.first?.isRead ?? true)🤖 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/WorkspaceManualUnreadTests.swift` around lines 525 - 575, Update testMarkLatestNotificationAsOldestUnreadAppendsWhenNoOtherUnreadNotificationsRemain to expect the existing chronological feed order after markLatestNotificationAsOldestUnread, rather than expecting the marked notification to be appended. Adjust its notification ID/order assertions to match the preserved order while retaining the unread-state verification.Sources/TerminalNotificationStore.swift (1)
858-872: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake cooldown reservations ownership-aware across asynchronous completions.
Reservations are written before policy evaluation. If two same-key notifications are in flight, a later/replayed notification can record first, then an earlier completion reaches the duplicate guard and restores its stale
previousDate, erasing the committed cooldown. TheacceptedAtcommits can also move a newer cooldown date backward when completion order differs.Track a reservation owner/generation and only restore or commit while that operation still owns the slot; keep the stored cooldown date monotonic. Add a reversed-completion regression test.
Also applies to: 881-940, 1098-1104, 1119-1164
🤖 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/TerminalNotificationStore.swift` around lines 858 - 872, Make cooldown reservations ownership-aware across asynchronous completion paths in the notification store. Update makeCooldownReservation and the related duplicate-guard, restoration, and acceptedAt commit logic (including the additional referenced sections) to track a reservation owner/generation, only restore or commit when the completing operation still owns the slot, and never move a stored cooldown date backward. Add a regression test covering reversed completion order for same-key notifications.
🤖 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/CmuxEventPublishing.swift`:
- Around line 299-301: Update the debug label in publishNotificationChanges to
reference the actual deduplication methods, recordNotification and
restoreSessionNotifications, instead of the nonexistent
TerminalNotificationStore.recordAndRestoreDeduplication identifier.
---
Outside diff comments:
In `@cmuxTests/WorkspaceManualUnreadTests.swift`:
- Around line 525-575: Update
testMarkLatestNotificationAsOldestUnreadAppendsWhenNoOtherUnreadNotificationsRemain
to expect the existing chronological feed order after
markLatestNotificationAsOldestUnread, rather than expecting the marked
notification to be appended. Adjust its notification ID/order assertions to
match the preserved order while retaining the unread-state verification.
In `@Sources/TerminalController.swift`:
- Around line 12318-12328: Remove the obsolete hardcoded “coalesces=0” field
from the DEBUG log in the notification enqueue path within the surrounding
notification method, while preserving the workspace, surface, and length
details.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 858-872: Make cooldown reservations ownership-aware across
asynchronous completion paths in the notification store. Update
makeCooldownReservation and the related duplicate-guard, restoration, and
acceptedAt commit logic (including the additional referenced sections) to track
a reservation owner/generation, only restore or commit when the completing
operation still owns the slot, and never move a stored cooldown date backward.
Add a regression test covering reversed completion order for same-key
notifications.
🪄 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: ee8227dc-e4fa-4fcf-a898-9e8c8c0cf459
📒 Files selected for processing (10)
Sources/CmuxEventPublishing.swiftSources/TerminalController.swiftSources/TerminalNotificationQueue.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/NotificationChronologyTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TerminalNotificationQueueTests.swiftcmuxTests/WorkspaceManualUnreadTests.swiftdocs/notifications.md
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmuxTests/NotificationChronologyTests.swift`:
- Around line 234-263: Update
reversedCooldownCompletionKeepsNewestReservationMonotonic to commit the later
reservation before the earlier one, asserting the stored date remains laterDate
after each completion; then restore the earlier reservation only if needed to
exercise its completion/rollback path and verify dates[key] still equals
laterDate.
🪄 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: 1043f4ce-3e13-4297-b6ab-856e85a8f4ac
📒 Files selected for processing (9)
Sources/AppDelegate+NotificationNavSeams.swiftSources/CmuxEventPublishing.swiftSources/NotificationCooldownReservations.swiftSources/TerminalController.swiftSources/TerminalNotificationQueue.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/NotificationChronologyTests.swiftcmuxTests/WorkspaceManualUnreadTests.swift
|
Addressed the CodeRabbit algorithmic-complexity finding in 9337ae7. |
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 (2)
cmuxTests/TerminalNotificationQueueTests.swift (1)
304-307: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid asserting feed order from back-to-back wall-clock timestamps.
TerminalNotificationQueue.enqueueNotificationcreates each notification withacceptedAt: Date(). If two enqueues receive the same timestamp, store ordering falls back to UUID text, so these["Second", "First"]/["Fresh unrelated", "Stale unrelated"]assertions can fail nondeterministically. Use an injected test clock, or compare the retained rows as a set when chronology is not the behavior under test.As per coding guidelines, tests must avoid real wall-clock dependencies and use injected virtual clocks for time-driven behavior.
Also applies to: 355-359, 424-428
🤖 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/TerminalNotificationQueueTests.swift` around lines 304 - 307, Update the affected TerminalNotificationQueue tests to avoid relying on Date() ordering: inject and advance a deterministic test clock before enqueueing notifications when chronology is under test, or compare retained notification rows without order when chronology is irrelevant. Apply this consistently to the assertions around workspaceNotifications, the unrelated notification assertions, and the additional occurrence near lines 424-428; preserve the existing title/content expectations.Source: Coding guidelines
Sources/TerminalNotificationStore.swift (1)
385-410: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRebuild the unread-navigation cache when deferred IDs change.
The cache is rebuilt only from
notifications.didSet, but operations such asmarkLatestNotificationAsOldestUnreadupdatedeferredUnreadNavigationIdsafterward. The requested navigation order therefore remains stale until another notification mutation.Proposed fix
- private var deferredUnreadNavigationIds: [UUID] = [] + private var deferredUnreadNavigationIds: [UUID] = [] { + didSet { + guard oldValue != deferredUnreadNavigationIds else { return } + rebuildUnreadNavigationNotifications() + } + }🤖 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/TerminalNotificationStore.swift` around lines 385 - 410, Update every mutation of deferredUnreadNavigationIds, especially markLatestNotificationAsOldestUnread, to call rebuildUnreadNavigationNotifications() after changing the IDs so notificationsForUnreadNavigation immediately reflects the new navigation order. Keep the existing notifications.didSet rebuild behavior unchanged.
🤖 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 `@cmuxTests/NotificationChronologyTests.swift`:
- Around line 63-107: Update lateOlderRecordDoesNotReplaceNewerExternalDelivery
to save the current AppFocusState.overrideIsFocused value, set a deterministic
unfocused override before adding notifications, and restore the original value
in the existing defer cleanup alongside the store handlers and notifications.
In `@Sources/NotificationCooldownReservations.swift`:
- Around line 3-13: Mark both TerminalNotificationStore.externalBannerTransition
and notificationSortPrecedes as nonisolated, preserving their pure comparison
behavior while allowing callers to invoke them without inheriting the `@MainActor`
isolation boundary.
In `@Sources/TerminalController.swift`:
- Around line 12328-12329: Replace the synchronous v2MainSync call in the
backpressure closure with an asynchronous capacity signal or continuation that
lets the socket worker yield without blocking on MainActor. Preserve the
existing TerminalMutationBus.shared.drainForBackpressure behavior while ensuring
queue saturation cannot require a synchronous main-actor round trip.
In `@Sources/TerminalController`+ControlNotificationContext.swift:
- Around line 4-25: Remove the
TerminalMutationBus.shared.discardPendingNotifications call from
deliverNotificationSynchronously so accepted pending notifications remain
preserved. Keep synchronous delivery routed through
TerminalNotificationStore.shared.addNotification, allowing acceptedAt ordering
to place the new notification correctly without target-level coalescing.
---
Outside diff comments:
In `@cmuxTests/TerminalNotificationQueueTests.swift`:
- Around line 304-307: Update the affected TerminalNotificationQueue tests to
avoid relying on Date() ordering: inject and advance a deterministic test clock
before enqueueing notifications when chronology is under test, or compare
retained notification rows without order when chronology is irrelevant. Apply
this consistently to the assertions around workspaceNotifications, the unrelated
notification assertions, and the additional occurrence near lines 424-428;
preserve the existing title/content expectations.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 385-410: Update every mutation of deferredUnreadNavigationIds,
especially markLatestNotificationAsOldestUnread, to call
rebuildUnreadNavigationNotifications() after changing the IDs so
notificationsForUnreadNavigation immediately reflects the new navigation order.
Keep the existing notifications.didSet rebuild behavior 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: ASSERTIVE
Plan: Pro
Run ID: 9c24d086-8324-47e7-884d-26ff3013341f
📒 Files selected for processing (9)
Sources/NotificationCooldownReservations.swiftSources/TerminalController+ControlNotificationContext.swiftSources/TerminalController+ControlSidebarContextSupport.swiftSources/TerminalController.swiftSources/TerminalNotificationQueue.swiftSources/TerminalNotificationStore.swiftcmuxTests/NotificationChronologyTests.swiftcmuxTests/TerminalNotificationClearAllTests.swiftcmuxTests/TerminalNotificationQueueTests.swift
…onological-feed # Conflicts: # Sources/TerminalController.swift
…onological-feed # Conflicts: # Resources/Localizable.xcstrings # Sources/TerminalNotificationPolicyInFlightStore.swift # Sources/TerminalNotificationStore.swift # cmux.xcodeproj/project.pbxproj
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
* test(ios): cover reconnect overlap cleanup * fix(ios): retire superseded reconnect sessions * test(ios): isolate saved dev Mac instances * fix(ios): enforce build compatibility boundaries * test(ios): cover startup status auth race * fix(ios): reuse connect token for identity check * test(auth): preserve selected team during refresh outage * fix(auth): keep selected team effective during startup * test(auth): keep cached sessions restoring until ready * fix(ios): wait for auth restore before reconnect * test(ios): cover compatibility review regressions * fix(ios): address compatibility review findings * test(ios): use deterministic compatibility timestamps --------- Co-authored-by: cmux reload-cloud <cmux-reload-cloud@users.noreply.github.com>
# Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/IOSBuildScopedPairedMacStore.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
| activeDatesByOwner: [:] | ||
| ) | ||
| state.activeDatesByOwner[reservation.owner] = acceptedAt | ||
| stateByKey[key] = state |
There was a problem hiding this comment.
This writes an active reservation into the same timestamp used by the admission gate. If notification A reserves the key while its asynchronous policy check runs, notification B arriving within the interval is rejected immediately. When A later fails policy and restores its reservation, B has already been discarded even though no notification committed the cooldown. Keep provisional reservations separate from the committed timestamp, or retain arrivals blocked only by provisional state for reevaluation.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
lawrencecchen
added
the
stale-revisit
3 participants
Summary
Product contract
An accepted notification appears exactly once. Its initial acceptance timestamp and UUID never change during policy evaluation, queueing, replay, reconnect, or restore. The feed sorts by acceptance timestamp descending and UUID text ascending, so late older arrivals insert at their chronological position and simultaneous timestamps remain deterministic. Duplicate UUIDs are idempotent replays. Corrupt restored arrays with repeated UUIDs preserve the first row deterministically. Read, open, unread navigation, and banner supersession do not reorder or delete feed history. Session replacement remaps known workspace and surface identities while retaining unmapped identities. Queue saturation rejects before acceptance with an explicit retry result; an accepted event is not dropped.
Acceptance coverage
Verification
0b16a20eb413f1b21594ccec2ae8b633247c64a2cmux-assets/task-notification-chronological-feed/nfeed5/tests/The canonical upstream agent-notification host run passed 35/36 feature-relevant cases after isolated reruns. The remaining unchanged PID telemetry test fails before product resolution because its shell subprocess does not execute its SIGUSR1 trap or write the marker on this host; its isolated diagnostic is retained in
final-upstream-pid-telemetry-isolated.xcresult.Regression provenance remains split into test-only and repair commits where applicable. Runtime dogfood approval is required before merge.
Summary by CodeRabbit
notification.listnow uses a context-aware snapshot suitable for unread navigation.