Repository navigation
Preserve unread indicators across session restore - #4130
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughSession snapshots now persist workspace- and panel-level unread indicators (including a restored-unread set). Autosave fingerprint includes notification-derived flags. Restore re-applies unread indicators and updates notificationStore; detach/attach and dismissal flows, UI unread visibility, and tests are updated accordingly. ChangesUnread Indicator Persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (15 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 |
Greptile SummaryThis PR persists workspace and panel unread indicators in session snapshots and restores them as restored indicators (distinct from manual unread) after session reload, so badges survive app restarts. It also includes unread state in the autosave fingerprint and keeps all read/clear flows consistent with the new indicator type.
Confidence Score: 4/5Safe to merge with one targeted fix in markLatestNotificationAsOldestUnread — all other clear paths handle restoredUnreadWorkspaceIds correctly. markLatestNotificationAsOldestUnread explicitly clears setWorkspaceManualUnread(false) to prevent double-counting when a notification takes over as the unread representative, but omits the parallel setWorkspaceRestoredUnread(false) call. Since restoredUnreadWorkspaceIds feeds into unreadCount via the same OR formula, the sidebar badge is inflated by 1 whenever this function fires for a workspace carrying a restored indicator. All other clear paths (terminal interaction, markRead, clearAll, markAllRead, clearNotifications) correctly drain the restored indicator — this function was simply not updated to match. Sources/TerminalNotificationStore.swift — markLatestNotificationAsOldestUnread at line 1430 needs a matching setWorkspaceRestoredUnread(false, forTabId: tabId) call. Important Files Changed
Sequence DiagramsequenceDiagram
participant S as Session Save
participant W as Workspace
participant NS as TerminalNotificationStore
participant R as Session Restore
Note over S,NS: Snapshot path
S->>W: sessionSnapshot()
W->>NS: hasManualUnread(forTabId:)
NS-->>W: isManuallyUnread
W->>NS: hasUnreadNotification + hasRestoredUnreadIndicator
NS-->>W: hasUnreadIndicator
W-->>S: SessionWorkspaceSnapshot(isManuallyUnread, hasUnreadIndicator)
W->>W: sessionPanelSnapshot() per panel
Note over R,NS: Restore path
R->>W: restoreSessionSnapshot(snapshot)
W->>W: restoreWorkspaceManualUnread(isManuallyUnread)
W->>NS: markUnread or clearManualUnread
W->>NS: restoreUnreadIndicator or clearRestoredUnreadIndicator
R->>W: restorePanelSnapshot per panel
W->>W: restorePanelUnreadIndicator(panelId)
Note over W,NS: Clear paths
W->>NS: dismissNotificationOnTerminalInteraction
NS->>W: clearManualUnread(panelId)
NS->>W: clearRestoredUnreadIndicator(panelId)
NS->>NS: clearRestoredUnreadIndicator(forTabId:)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func restoreWorkspaceUnreadIndicator(_ hasUnreadIndicator: Bool) { | ||
| guard let notificationStore = AppDelegate.shared?.notificationStore else { return } | ||
| if hasUnreadIndicator { | ||
| notificationStore.markUnread(forTabId: id) | ||
| } else { | ||
| _ = notificationStore.clearManualUnread(forTabId: id) | ||
| } | ||
| syncUnreadBadgeStateForAllPanels() | ||
| } |
There was a problem hiding this comment.
When a workspace was unread solely because of panel notification unreads (not a workspace-level manual mark), restoreWorkspaceUnreadIndicator(true) calls notificationStore.markUnread(forTabId: id), adding the workspace to manualUnreadWorkspaceIds. The panels are simultaneously restored as panel-level manual unreads via markPanelUnread. After restore, markPanelRead calls clearManualUnread(panelId:) and notificationStore.markRead(tabId:surfaceId:) — neither of those touches manualUnreadWorkspaceIds — so the workspace remains stuck as unread at the store level even after the user has dismissed every panel's indicator.
The snapshot should distinguish between "workspace was explicitly marked unread by the user" (notificationStore.hasManualUnread(forTabId: id)) and "workspace was unread because panels had notifications" (already captured by the panel-level hasUnreadIndicator). Using workspaceIsUnread for the workspace snapshot conflates both signals and triggers the spurious extra manual-unread on restore.
| if hasUnreadIndicator { | ||
| notificationStore.markUnread(forTabId: id) | ||
| } else { | ||
| _ = notificationStore.clearManualUnread(forTabId: id) |
There was a problem hiding this comment.
_ = prefix is unnecessary because clearManualUnread(forTabId:) is already annotated @discardableResult. Drop it for consistency with every other call site.
| _ = notificationStore.clearManualUnread(forTabId: id) | |
| notificationStore.clearManualUnread(forTabId: id) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 (1)
Sources/Workspace.swift (1)
887-891:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnread restore is being promoted to manual unread state.
At Line 887 and Line 933, indicator restoration writes manual unread state (
markPanelUnread/markUnread(forTabId:)) even when the snapshot value may be notification-derived. This changes semantics after restore (e.g., sticky unread/manual badges and representative workspace badge behavior diverging from pre-restore notification state).Also applies to: 930-936
🤖 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/Workspace.swift` around lines 887 - 891, The restore path is incorrectly promoting notification-derived unread indicators into manual unread state; update the logic where snapshot values are applied (references: markPanelUnread, clearManualUnread, and markUnread(forTabId:)) so that you only call markPanelUnread / markUnread(forTabId:) when the snapshot explicitly indicates a manual unread (snapshot.isManuallyUnread == true). If snapshot.hasUnreadIndicator is true but snapshot.isManuallyUnread is false, restore the indicator without setting manual state (i.e., do not call markPanelUnread/markUnread; instead restore the non-manual indicator or leave as-is). Also ensure clearManualUnread(panelId:) is only called when removing a previously manual unread. Apply this change at both affected spots indicated in the diff.
🤖 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/WorkspaceManualUnreadTests.swift`:
- Around line 388-431: The test
testSessionAutosaveFingerprintChangesWhenUnreadIndicatorsChange leaves residual
manual/unread state between assertions; reset all relevant state before each
fingerprint capture by clearing notifications
(store.replaceNotificationsForTesting([])) and also clearing manual/focused
unread markers — e.g., remove workspace manual unread IDs
(workspace.manualUnreadPanelIds.removeAll() or
workspace.clearPanelUnread(panelId)), clear focused read indicators via the
store API (store.clearFocusedReadIndicator(forTabId:surfaceId:) or equivalent),
and clear workspace-level unread (workspace.clearUnread() or equivalent) so each
XCTAssertNotEqual compares the cleanFingerprint to a single, isolated
unread-indicator change.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 887-891: The restore path is incorrectly promoting
notification-derived unread indicators into manual unread state; update the
logic where snapshot values are applied (references: markPanelUnread,
clearManualUnread, and markUnread(forTabId:)) so that you only call
markPanelUnread / markUnread(forTabId:) when the snapshot explicitly indicates a
manual unread (snapshot.isManuallyUnread == true). If
snapshot.hasUnreadIndicator is true but snapshot.isManuallyUnread is false,
restore the indicator without setting manual state (i.e., do not call
markPanelUnread/markUnread; instead restore the non-manual indicator or leave
as-is). Also ensure clearManualUnread(panelId:) is only called when removing a
previously manual unread. Apply this change at both affected spots indicated in
the diff.
🪄 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: 5c6895e2-2541-4aef-8459-48b823bff407
📒 Files selected for processing (4)
Sources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/WorkspaceManualUnreadTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 (2)
Sources/WorkspaceContentView.swift (2)
301-309:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd onChange handler for restored unread panels.
Lines 234-235 and 457-458 now include
workspace.restoredUnreadPanelIdsin the unread indicator logic, but there is no corresponding.onChange(of: workspace.restoredUnreadPanelIds)handler to trigger badge sync. Changes to the restored unread set won't update the UI.🔔 Proposed fix to add reactivity for restored unread
.onChange(of: workspace.manualUnreadPanelIds) { _, _ in syncBonsplitNotificationBadges() } +.onChange(of: workspace.restoredUnreadPanelIds) { _, _ in + syncBonsplitNotificationBadges() +} .onChange(of: isWorkspaceManuallyUnread) { _, _ in syncBonsplitNotificationBadges() }🤖 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/WorkspaceContentView.swift` around lines 301 - 309, The view is missing an onChange handler for workspace.restoredUnreadPanelIds so updates to the restored-unread set won't call syncBonsplitNotificationBadges(); add an .onChange(of: workspace.restoredUnreadPanelIds) { _, _ in syncBonsplitNotificationBadges() } entry alongside the existing .onChange(of: workspace.manualUnreadPanelIds), .onChange(of: isWorkspaceManuallyUnread), and .onChange(of: workspaceManualUnreadPanelId) so changes to workspace.restoredUnreadPanelIds trigger the same badge-sync logic.
340-375:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInclude restored unread panels in badge sync.
Line 356 only checks
manualUnread.contains($0)when computingisManuallyUnread, but lines 234-235 and 457-458 check bothworkspace.manualUnreadPanelIdsandworkspace.restoredUnreadPanelIds. This inconsistency will cause badge state to diverge from the rendered unread indicators: panels with restored unread will show the unread ring in the main content but won't have their tab badges synced.🐛 Proposed fix to include restored unread in badge sync
private func syncBonsplitNotificationBadges() { let manualUnread = workspace.manualUnreadPanelIds + let restoredUnread = workspace.restoredUnreadPanelIds let isWorkspaceManuallyUnread = notificationStore.hasManualUnread(forTabId: workspace.id) let workspaceManualUnreadPanelId = workspace.representativePanelIdForWorkspaceManualUnread() for paneId in workspace.bonsplitController.allPaneIds { for tab in workspace.bonsplitController.tabs(inPane: paneId) { let panelId = workspace.panelIdFromSurfaceId(tab.id) let expectedKind = panelId.flatMap { workspace.panelKind(panelId: $0) } let expectedPinned = panelId.map { workspace.isPanelPinned($0) } ?? false let shouldShow = panelId.map { Workspace.shouldShowUnreadIndicator( hasUnreadNotification: notificationStore.hasVisibleNotificationIndicator( forTabId: workspace.id, surfaceId: $0 ), - isManuallyUnread: manualUnread.contains($0), + isManuallyUnread: manualUnread.contains($0) || restoredUnread.contains($0), isWorkspaceManuallyUnread: isWorkspaceManuallyUnread, isWorkspaceManualUnreadRepresentative: workspaceManualUnreadPanelId == $0 ) } ?? false🤖 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/WorkspaceContentView.swift` around lines 340 - 375, The badge sync misses panels marked as restored unread; in syncBonsplitNotificationBadges compute isManuallyUnread by checking both workspace.manualUnreadPanelIds and workspace.restoredUnreadPanelIds (e.g., replace manualUnread.contains($0) with a check that tests membership in either set or precompute a combined set), then pass that result to Workspace.shouldShowUnreadIndicator so restored unread panels get their tab badges updated consistently with the main content.
♻️ Duplicate comments (1)
cmuxTests/WorkspaceManualUnreadTests.swift (1)
449-453: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winComplete the state reset helper to clear all unread indicator types.
The
resetUnreadState()helper partially addresses a previous review concern by clearing panel manual-unread and restored-unread indicators. However, it still doesn't reset:
- Focused read indicators (set on line 473 via
store.setFocusedReadIndicator)- Workspace-level manual unread (set on line 477 via
store.markUnread)Without clearing these, each assertion after the first compares against accumulated baseline state rather than a fully clean state. While the test remains functionally valid (it still verifies each indicator type affects the fingerprint), full isolation would make the test more precise and robust.
♻️ Suggested enhancement to resetUnreadState
func resetUnreadState() { store.replaceNotificationsForTesting([]) workspace.clearManualUnread(panelId: panelId) workspace.clearRestoredUnreadIndicator(panelId: panelId) + store.markRead(forTabId: workspace.id) // Clear workspace-level manual unread + // If API exists: store.clearFocusedReadIndicator(forTabId: workspace.id, surfaceId: panelId) }🤖 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 449 - 453, resetUnreadState currently clears only panel manual-unread and restored-unread; extend it to also remove focused-read and workspace-level manual-unread state by invoking the corresponding store cleanup APIs that mirror the setters used in the test (i.e., call the store API that clears focused read indicators after store.setFocusedReadIndicator and call the store API that clears workspace/manual unread after store.markUnread); keep the existing workspace.clearManualUnread(panelId:) and workspace.clearRestoredUnreadIndicator(panelId:) calls and add these two clear calls inside resetUnreadState so each test starts from a fully clean unread 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/Workspace.swift`:
- Around line 8523-8553: clearManualUnread(panelId:) only removes manual unread
state and leaves restored-only badges intact; update its behavior so the
user-facing "Mark as Read" action actually clears restored unread indicators
too. Inside clearManualUnread(panelId:) call
clearRestoredUnreadIndicatorState(panelId:) (or otherwise remove panelId from
restoredUnreadPanelIds) in addition to clearManualUnreadState(panelId:), and
ensure you still call syncUnreadBadgeStateForPanel(panelId) once after changes;
alternatively, migrate callers of clearManualUnread(panelId:) to use
markPanelRead(_:) instead — choose one approach and update references to
clearManualUnreadState(panelId:), clearRestoredUnreadIndicatorState(panelId:),
markPanelRead(_:), and syncUnreadBadgeStateForPanel(panelId) accordingly.
- Around line 8514-8520: The function markPanelRead currently returns early if
neither clearManualUnreadState nor clearRestoredUnreadIndicatorState report
changes, which skips calling syncUnreadBadgeStateForPanel and leaves
showsNotificationBadge stale when the notification store was the only source;
always invoke syncUnreadBadgeStateForPanel(panelId) after notifying
AppDelegate.shared?.notificationStore?.markRead to ensure the tab badge is
resynced. Update markPanelRead (referencing markPanelRead, panels,
AppDelegate.shared?.notificationStore?.markRead, clearManualUnreadState,
clearRestoredUnreadIndicatorState, syncUnreadBadgeStateForPanel) so that
syncUnreadBadgeStateForPanel(panelId) is executed unconditionally (or at least
whenever markRead was called), then you can keep or remove the existing
early-return logic around clearManual/clearRestored as appropriate.
In `@Sources/WorkspaceContentView.swift`:
- Around line 234-235: The callsite passes a boolean combining
workspace.manualUnreadPanelIds and workspace.restoredUnreadPanelIds into the
parameter named isManuallyUnread, which is misleading; rename the parameter in
Workspace.shouldShowUnreadIndicator(_:, isManuallyUnread:) to a more general
name such as isPanelUnread or hasPanelUnreadIndicator and update the function
signature and all call sites (including this call in WorkspaceContentView where
you currently pass workspace.manualUnreadPanelIds.contains(panel.id) ||
workspace.restoredUnreadPanelIds.contains(panel.id)) to use the new parameter
name; also update any related documentation/comments and usages inside
Workspace.shouldShowUnreadIndicator to reflect the broader meaning.
---
Outside diff comments:
In `@Sources/WorkspaceContentView.swift`:
- Around line 301-309: The view is missing an onChange handler for
workspace.restoredUnreadPanelIds so updates to the restored-unread set won't
call syncBonsplitNotificationBadges(); add an .onChange(of:
workspace.restoredUnreadPanelIds) { _, _ in syncBonsplitNotificationBadges() }
entry alongside the existing .onChange(of: workspace.manualUnreadPanelIds),
.onChange(of: isWorkspaceManuallyUnread), and .onChange(of:
workspaceManualUnreadPanelId) so changes to workspace.restoredUnreadPanelIds
trigger the same badge-sync logic.
- Around line 340-375: The badge sync misses panels marked as restored unread;
in syncBonsplitNotificationBadges compute isManuallyUnread by checking both
workspace.manualUnreadPanelIds and workspace.restoredUnreadPanelIds (e.g.,
replace manualUnread.contains($0) with a check that tests membership in either
set or precompute a combined set), then pass that result to
Workspace.shouldShowUnreadIndicator so restored unread panels get their tab
badges updated consistently with the main content.
---
Duplicate comments:
In `@cmuxTests/WorkspaceManualUnreadTests.swift`:
- Around line 449-453: resetUnreadState currently clears only panel
manual-unread and restored-unread; extend it to also remove focused-read and
workspace-level manual-unread state by invoking the corresponding store cleanup
APIs that mirror the setters used in the test (i.e., call the store API that
clears focused read indicators after store.setFocusedReadIndicator and call the
store API that clears workspace/manual unread after store.markUnread); keep the
existing workspace.clearManualUnread(panelId:) and
workspace.clearRestoredUnreadIndicator(panelId:) calls and add these two clear
calls inside resetUnreadState so each test starts from a fully clean unread
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: 61f48d31-7466-4a12-b616-cbfad151dbb6
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace+DetachedSurfaceTransfer.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/WorkspaceManualUnreadTests.swiftcmuxTests/WorkspaceUnitTests.swift
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 `@Sources/Workspace.swift`:
- Around line 216-225: The snapshot currently sets hasUnreadIndicator from the
manual-unread check (using
AppDelegate.shared?.notificationStore?.hasManualUnread(forTabId: id)) and the
restore path later treats hasUnreadIndicator as a manual-unread fallback; change
this so hasUnreadIndicator is populated from the actual visible-unread source
(e.g. a notificationStore visible/unread API or the workspace's visible-unread
state) instead of isWorkspaceManuallyUnread, and update the
restore/deserialization logic (the code that maps decoded hasUnreadIndicator
back into isManuallyUnread) so it does not coerce a visible unread into a manual
unread; update SessionWorkspaceSnapshot construction and the corresponding
decode/restore use of isManuallyUnread and hasUnreadIndicator to keep those two
orthogonal.
🪄 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: 0fe8fc60-53d0-40a3-80de-2f3d07ebcc74
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/WorkspaceManualUnreadTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4f86902. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
216-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep workspace visible-unread separate from manual unread.
Line 218 folds
isWorkspaceManuallyUnreadintohasUnreadIndicator, but Lines 323-329 only restore the notification-store restored flag when manual unread is false. That means “manual unread + visible unread” serializes the same as “manual unread only”, so after restore clearing the manual flag drops the remaining workspace unread indicator.Suggested fix
let notificationStore = AppDelegate.shared?.notificationStore let isWorkspaceManuallyUnread = notificationStore?.hasManualUnread(forTabId: id) ?? false -let hasWorkspaceUnreadIndicator = isWorkspaceManuallyUnread || - (notificationStore?.hasUnreadNotification(forTabId: id, surfaceId: nil) ?? false) || +let hasWorkspaceUnreadIndicator = + (notificationStore?.hasUnreadNotification(forTabId: id, surfaceId: nil) ?? false) || (notificationStore?.hasRestoredUnreadIndicator(forTabId: id) ?? false)let isWorkspaceManuallyUnread = snapshot.isManuallyUnread == true restoreWorkspaceManualUnread(isWorkspaceManuallyUnread) -if !isWorkspaceManuallyUnread { - if snapshot.hasUnreadIndicator == true { - AppDelegate.shared?.notificationStore?.restoreUnreadIndicator(forTabId: id) - } else { - AppDelegate.shared?.notificationStore?.clearRestoredUnreadIndicator(forTabId: id) - } +if snapshot.hasUnreadIndicator == true { + AppDelegate.shared?.notificationStore?.restoreUnreadIndicator(forTabId: id) +} else { + AppDelegate.shared?.notificationStore?.clearRestoredUnreadIndicator(forTabId: id) }Also applies to: 321-329
🤖 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/Workspace.swift` around lines 216 - 220, The code currently folds isWorkspaceManuallyUnread into hasWorkspaceUnreadIndicator causing manual unread and visible (restored) unread to serialize identically; update the logic so the two states are stored and restored separately: keep isWorkspaceManuallyUnread as its own boolean and compute hasWorkspaceVisibleUnreadIndicator using notificationStore?.hasUnreadNotification(forTabId: id, surfaceId: nil) ?? false and notificationStore?.hasRestoredUnreadIndicator(forTabId: id) ?? false (excluding the manual flag), and change the restore/clear path that uses notificationStore?.clearRestored... or similar so it only clears the restored-unread flag when appropriate and does not drop the visible unread when isWorkspaceManuallyUnread is true; ensure serialization persists both flags distinctly and deserialization/restoration logic restores both isWorkspaceManuallyUnread and the restored-visible flag independently.
🤖 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/TerminalNotificationStore.swift`:
- Line 714: restoredUnreadWorkspaceIds is not being cleared by the existing
clear/read flows so unread badges can remain; update the clear paths
(markAllRead, clearAll, and clearNotifications(forTabId:)) to remove any
affected workspace IDs from restoredUnreadWorkspaceIds (e.g. clear the set or
remove specific workspace UUIDs) whenever you mark notifications read or clear
notifications, and ensure unreadCount(forTabId:) logic still treats
restoredUnreadWorkspaceIds consistently after mutation.
In `@Sources/Workspace.swift`:
- Around line 8533-8537: The current clearManualUnread(panelId:) only clears
local manual/restored state but leaves the notification-store unread flag
intact, so user-facing “Mark as Read” must not call it; update the caller(s)
that implement the `.markAsRead` action to call markPanelRead(_:) instead of
clearManualUnread(panelId:), and keep clearManualUnread(panelId:) for internal
state-only clearing tied to clearManualUnreadState and
clearRestoredUnreadIndicatorState with the existing
syncUnreadBadgeStateForPanel(panelId) behavior.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 216-220: The code currently folds isWorkspaceManuallyUnread into
hasWorkspaceUnreadIndicator causing manual unread and visible (restored) unread
to serialize identically; update the logic so the two states are stored and
restored separately: keep isWorkspaceManuallyUnread as its own boolean and
compute hasWorkspaceVisibleUnreadIndicator using
notificationStore?.hasUnreadNotification(forTabId: id, surfaceId: nil) ?? false
and notificationStore?.hasRestoredUnreadIndicator(forTabId: id) ?? false
(excluding the manual flag), and change the restore/clear path that uses
notificationStore?.clearRestored... or similar so it only clears the
restored-unread flag when appropriate and does not drop the visible unread when
isWorkspaceManuallyUnread is true; ensure serialization persists both flags
distinctly and deserialization/restoration logic restores both
isWorkspaceManuallyUnread and the restored-visible flag independently.
🪄 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: ad5647a2-5714-4c13-85df-dc5917906148
📒 Files selected for processing (4)
Sources/TabManager.swiftSources/TerminalNotificationStore.swiftSources/Workspace.swiftcmuxTests/WorkspaceManualUnreadTests.swift
…-indicators-session-restoration
This branch was successfully deployed
1 participant
Summary:
Red failure:
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination platform=macOS,arch=arm64 -derivedDataPath /tmp/cmux-unreadrestore-red -only-testing:cmuxTests/WorkspaceManualUnreadTests testfailed on the new restore assertions before the fix.WorkspaceManualUnreadTests.testSessionRestorePreservesFocusedReadIndicator,WorkspaceManualUnreadTests.testSessionRestorePreservesNotificationUnreadIndicator, and autosave fingerprint coverage.Green pass:
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination platform=macOS,arch=arm64 -derivedDataPath /tmp/cmux-unreadrestore-green -only-testing:cmuxTests/WorkspaceManualUnreadTests/testSessionRestorePreservesNotificationUnreadIndicator -only-testing:cmuxTests/WorkspaceManualUnreadTests/testSessionRestorePreservesFocusedReadIndicator -only-testing:cmuxTests/WorkspaceManualUnreadTests/testSessionAutosaveFingerprintChangesWhenUnreadIndicatorsChange testpassed../scripts/reload.sh --tag unreadrestore.Additional check:
WorkspaceManualUnreadTestsstill hastestWorkspaceManualUnreadBadgeMovesWhenFocusChangesfailing locally. The same existing test failed in the red run before the implementation change.Cloud proof:
cgWindowNotFoundand only wallpaper display captures were available, so no cursor before/after video is claimed for this PR.Note
Medium Risk
Touches session snapshot/restore and notification badge logic across workspaces/panels; mistakes could cause unread badges to stick or clear incorrectly, but changes are localized and covered by new unit tests.
Overview
Unread indicator state is now persisted and restored across session restore for both panels and workspaces, distinct from manual-unread, by adding optional
hasUnreadIndicator(and workspaceisManuallyUnread) to session snapshots and replaying that state onto new IDs.Introduces restored unread tracking (
restoredUnreadPanelIdsonWorkspace,restoredUnreadWorkspaceIdsonTerminalNotificationStore) and updates UI/badge/command-palette logic, read/clear flows, terminal-interaction dismissal, and detach/attach transfer to treat manual + restored unread consistently.Session autosave fingerprinting now includes unread-indicator inputs (manual/restored + visible indicators) so unread-only changes trigger saves, with expanded tests covering restore behavior and clearing semantics.
Reviewed by Cursor Bugbot for commit fe770c0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Preserves workspace and panel unread indicators across session restore, separate from manual unread. Autosave now fingerprints unread-only changes, and badges/command palette update live.
Bug Fixes
hasUnreadIndicator(panel/workspace) and workspaceisManuallyUnread; on restore, set workspace manual unread or a restored workspace indicator, and set per‑panel restored indicators without marking panels manually unread.markRead,markAllRead), clear (clearNotifications,clearAll), detach/attach, focus changes, and panel “Mark as Read” all clear restored/manual state as appropriate; Bonsplit badges, overlays, and command palette reflect restored state; unread counts include restored workspace indicators.Migration
Written for commit fe770c0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests