Repository navigation
Conversation
|
@duroey is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughThis PR captures prompt-submit terminal scrollbar offsets, records them per (tab,surface), persists them with notifications and session snapshots, and replays the scroll position when opening notifications (deferring until Ghostty scrollbar state is ready); it also adds settings-section visibility measurement and sidebar sync. ChangesNotification Open Anchor Feature
Settings visible-section resolver & sync
Sequence Diagram(s)sequenceDiagram
participant Prompt as WorkspacePromptSubmit
participant Store as TerminalNotificationStore
participant App as AppDelegate
participant Panel as TerminalPanel
participant View as GhosttySurfaceScrollView
Prompt->>Store: recordPromptSubmitOpenAnchor(tabId,surfaceId,anchor)
Store-->>App: promptSubmitOpenAnchor(panelId/surfaceId)?
App->>Panel: openNotification(..., openAnchor)
Panel->>View: scrollToNotificationOpenAnchor(anchor)
View->>View: queue or emit binding scroll_to_row:N when scrollbar ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 adds two behaviors: (1) the Settings sidebar now syncs its selection to the section visible in the scroll view as the user scrolls, and (2) clicking a terminal notification jumps to the start of the agent turn that produced it rather than the live bottom of the surface.
Confidence Score: 4/5The notification anchor machinery and anchor store lifecycle are solid; the one issue worth addressing before merge is the settings sidebar sync guard in SettingsWindowScene.swift. The Task.yield() used to re-enable sidebar sync after proxy.scrollTo will almost always fire before SwiftUI has propagated updated geometry frames, causing the sidebar to snap back to the wrong section immediately after any programmatic navigation (e.g., search-result click or window restore). All other changed paths — anchor capture, queuing, persistence, clear/rebind lifecycle, and surface_id resolution — look correct and are well-covered by tests. Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swift — the Task.yield() / visibleSectionSyncSuppressed interaction in scrollToNavigationTarget. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TerminalController
participant TabManager
participant TerminalNotificationStore
participant GhosttySurfaceScrollView
participant AppDelegate
participant UNNotificationCenter
User->>TerminalController: workspace.prompt_submit (surface_id?)
TerminalController->>TabManager: handlePromptSubmit(workspaceId, surfaceId)
TabManager->>GhosttySurfaceScrollView: notificationOpenAnchor()
GhosttySurfaceScrollView-->>TabManager: TerminalNotificationOpenAnchor(scrollbar.total)
TabManager->>TerminalNotificationStore: recordPromptSubmitOpenAnchor(anchor, tabId, panelId)
Note over TerminalNotificationStore: Agent runs…
TerminalNotificationStore->>TerminalNotificationStore: addNotification → reads anchor → attaches to TerminalNotification
TerminalNotificationStore->>UNNotificationCenter: deliver with userInfo (includes scrollbar_offset)
User->>AppDelegate: userNotificationCenter didReceive (clicks notification)
AppDelegate->>AppDelegate: openNotification(tabId, surfaceId, openAnchor)
AppDelegate->>GhosttySurfaceScrollView: scrollToNotificationOpenAnchor(anchor)
alt scrollbar available
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: performBindingAction("scroll_to_row:N")
else scrollbar nil
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: queue pendingNotificationOpenAnchor
Note over GhosttySurfaceScrollView: ghosttyDidUpdateScrollbar fires → applyPendingNotificationOpenAnchorIfPossible
end
Reviews (9): Last reviewed commit: "fix(settings): replace visibility namesp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !didScroll { | ||
| DispatchQueue.main.async { [weak terminalPanel] in | ||
| _ = terminalPanel?.scrollToNotificationOpenAnchor(openAnchor) | ||
| } | ||
| } |
There was a problem hiding this comment.
When scrollToNotificationOpenAnchor returns false (either because isAgentHibernated is true or Ghostty's scrollbar is still nil), a one-shot DispatchQueue.main.async re-try is enqueued hoping the state will have advanced by the next run-loop cycle. There is no real signal from the owning subsystem — if the scrollbar is still nil on the next tick (e.g., Ghostty hasn't delivered its first scroll-state update after focus), the anchor scroll is silently dropped. Similarly, a hibernated agent will not un-hibernate in one run-loop, so the retry for that case also no-ops silently. This matches the blocked pattern in the cmux-swift-blocking-runtime rule (delayed dispatch used as synchronization) and the cmux-swift-architectural-rethink rule (timing repair that leaves bad state reachable). The fix should subscribe to a real readiness signal — e.g., observe the scrollbar property becoming non-nil and deliver the scroll then — rather than hoping a deferred queue hop is sufficient.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| } | ||
| } | ||
| let message = messageKeys.lazy.compactMap { self.v2RawString(params, $0) }.first | ||
| let surfaceId = v2UUID(params, "surface_id") ?? v2UUID(params, "tab_id") |
There was a problem hiding this comment.
tab_id fallback makes surfaceId always non-nil, silently bypassing the "all panels" anchor path
v2UUID(params, "tab_id") will succeed for every valid prompt_submit request (the workspace UUID is always present). This means surfaceId is never nil when it reaches handlePromptSubmit, so recordPromptSubmitOpenAnchors can never take the surfaceId == nil branch that records anchors for all panels in the workspace. When an agent sends prompt_submit with tab_id but no surface_id, the workspace UUID is forwarded as the surfaceId; workspace.panels[workspace_uuid] and panelIdFromSurfaceId(TabID(uuid: workspace_uuid)) both return nil, leaving panelIds = [] and recording no anchor at all. The notification will fire but clicking it won't scroll to the prompt start. To restore the intended fallback, drop the ?? v2UUID(params, "tab_id") so that a missing surface_id properly propagates as nil and triggers the "record for all panels" path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
Sources/TerminalController.swift (1)
4789-4804:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not use
tab_idas a fallback forsurfaceId.On Line 4789,
v2UUID(params, "tab_id")can be a workspace/tab UUID, not a surface UUID. Passing that intohandlePromptSubmit(... surfaceId:)on Line 4803 can mis-scope (or skip) anchor capture and break reopen-to-turn-start for callers that omitsurface_id.Suggested fix
- let surfaceId = v2UUID(params, "surface_id") ?? v2UUID(params, "tab_id") + let surfaceId = v2UUID(params, "surface_id")Based on learnings, keep tab/surface identifiers from a consistent origin and avoid panel/surface route fallback patterns that introduce active-context bias or wrong-ID routing.
🤖 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 4789 - 4804, The code currently falls back to v2UUID(params, "tab_id") when computing surfaceId which can be a workspace/tab UUID and mis-scope anchor capture; change the surfaceId calculation to use only v2UUID(params, "surface_id") (no fallback to "tab_id") and let handlePromptSubmit receive nil when no surface_id is present so anchor/surface routing remains correct; update any related logic that assumed a non-nil surfaceId and add a comment by surfaceId, v2UUID, and handlePromptSubmit to explain why "tab_id" must never be used as a surface fallback.Source: Learnings
🤖 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 16245-16250: Add a one-line comment above the
DispatchQueue.main.async retry that explains the rationale: note that
scrollToNotificationOpenAnchor can return false if
GhosttyTerminalView.surfaceView.scrollbar/state hasn't been delivered yet
because scrollbar updates are flushed to the main queue asynchronously, so
deferring to the next runloop gives Ghostty time to populate scrollbar
state/view geometry before retrying the scroll (reference terminalPanel and
scrollToNotificationOpenAnchor).
In `@Sources/GhosttyTerminalView.swift`:
- Around line 15277-15280: The code sets userScrolledAwayFromBottom,
allowExplicitScrollbarSync and lastSentRow before calling
surfaceView.performBindingAction("scroll_to_row:\(row)"), which can fail and
leave state stale; change the flow to first call performBindingAction(...) and
only if it succeeds update userScrolledAwayFromBottom,
allowExplicitScrollbarSync and lastSentRow (or roll back on failure). Locate the
call to surfaceView.performBindingAction("scroll_to_row:\(row)") and ensure you
inspect its return/throw value (handle Bool/Result/try) and move the state
mutations (userScrolledAwayFromBottom, allowExplicitScrollbarSync, lastSentRow)
to the success path, preserving existing behavior on failure.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 824-833: The struct TerminalNotificationOpenAnchor has an explicit
memberwise init(scrollbarOffset:) which Swift synthesizes (triggering a
SwiftLint redundancy warning); remove the redundant initializer and move the
existing initializer documentation to the scrollbarOffset property comment so
the documentation is preserved (keep the struct declaration and conformances
Codable/Hashable/Sendable intact and ensure the scrollbarOffset property's
docstring describes the "Absolute top-row offset reported by Ghostty at prompt
submit time").
- Line 926: promptSubmitOpenAnchors is never pruned and should be cleaned up
when surfaces/workspaces close; inside clearNotifications(forTabId:) iterate
promptSubmitOpenAnchors and remove any entries whose key.tabId matches the
provided tabId, and inside clearNotifications(forTabId:surfaceId:) remove
entries whose key.tabId and key.surfaceId match the provided tabId and
surfaceId; update both methods (the ones at clearNotifications(forTabId:) and
clearNotifications(forTabId:surfaceId:)) to perform this removal in the same
style as lastNotificationDateByCooldownKey cleanup so stale anchors do not
accumulate.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 4789-4804: The code currently falls back to v2UUID(params,
"tab_id") when computing surfaceId which can be a workspace/tab UUID and
mis-scope anchor capture; change the surfaceId calculation to use only
v2UUID(params, "surface_id") (no fallback to "tab_id") and let
handlePromptSubmit receive nil when no surface_id is present so anchor/surface
routing remains correct; update any related logic that assumed a non-nil
surfaceId and add a comment by surfaceId, v2UUID, and handlePromptSubmit to
explain why "tab_id" must never be used as a surface fallback.
🪄 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: b013fef5-ca9e-446d-bc15-afc09de04f36
📒 Files selected for processing (11)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/SessionPersistence.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftSources/WorkspacePromptSubmit.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/TerminalNotificationSocketActionTests.swiftcmuxTests/WorkspacePromptSubmitTests.swift
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 (3)
Sources/TerminalNotificationStore.swift (3)
1848-1872:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
promptSubmitOpenAnchorswhen restoring session notifications.
restoreSessionNotificationsreplaces notifications for a tab but does not clearpromptSubmitOpenAnchorsentries for that tab. Stale runtime anchors from the previous session will accumulate, potentially causing incorrect scroll positions if the same surface IDs are reused.🧹 Proposed fix
func restoreSessionNotifications(_ restoredNotifications: [TerminalNotification], forTabId tabId: UUID) { TerminalMutationBus.shared.discardPendingNotifications(forTabId: tabId) let removedIds = notifications .filter { $0.tabId == tabId } .map { $0.id.uuidString } + promptSubmitOpenAnchors = promptSubmitOpenAnchors.filter { entry in + entry.key.tabId != tabId + } var usedNotificationIds = Set(notifications.filter { $0.tabId != tabId }.map(\.id)) let restoredForTab = restoredNotifications .filter { $0.tabId == tabId }🤖 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 1848 - 1872, restoreSessionNotifications replaces notifications for a tab but doesn't clear stale promptSubmitOpenAnchors entries, so update restoreSessionNotifications to also remove any promptSubmitOpenAnchors entries associated with the given tabId (e.g. clear or filter promptSubmitOpenAnchors for keys/values tied to tabId) before or after clearing focused read indicator; reference the promptSubmitOpenAnchors storage and the restoreSessionNotifications method so the anchors for the previous session don't persist when notifications are restored.
1905-1923:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
promptSubmitOpenAnchorswhen clearing all notifications.
clearAll()removes all notifications and workspace unread state but does not clearpromptSubmitOpenAnchors. This leaves stale anchor entries that could be incorrectly attached to new notifications if the sametabId/surfaceIdis reused (e.g., after workspace close and recreation).🛠️ Proposed fix
func clearAll(discardQueuedNotifications: Bool = true) { if discardQueuedNotifications { TerminalMutationBus.shared.discardPendingNotifications() } guard !notifications.isEmpty || !focusedReadIndicatorByTabId.isEmpty || !manualUnreadWorkspaceIds.isEmpty || !panelDerivedUnreadWorkspaceIds.isEmpty || !restoredUnreadWorkspaceIds.isEmpty else { return } let tabIdsToClearPanelUnread = panelDerivedUnreadWorkspaceIds.union(notifications.map(\.tabId)) let ids = notifications.map { $0.id.uuidString } replaceNotificationsForClear([]) + promptSubmitOpenAnchors.removeAll() clearWorkspaceManualUnread() clearAllWorkspacePanelUnread(forTabIds: tabIdsToClearPanelUnread)🤖 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 1905 - 1923, In clearAll() inside TerminalNotificationStore, also clear the promptSubmitOpenAnchors state to avoid stale anchors being reused; locate the clearAll method and add a step to remove or reset promptSubmitOpenAnchors (e.g., call whatever clearPromptSubmitOpenAnchors() or promptSubmitOpenAnchors.removeAll()) alongside the other clear* calls (before publishing CmuxEventBus.shared.publishNotificationCleared and removing notifications from UNUserNotificationCenter) so anchors are fully reset when notifications and workspace unread state are cleared.
1961-1996:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebind
promptSubmitOpenAnchorsentry when moving surface notifications.
rebindSurfaceNotificationsmoves notifications and the focused-read indicator fromsourceTabIdtodestinationTabId, but it does not move thepromptSubmitOpenAnchorsentry. After a surface is detached from one workspace and reattached to another, future notifications on that surface will not include the saved anchor (unless a new prompt-submit happens), breaking the "return to prompt start" feature.🔧 Proposed fix
func rebindSurfaceNotifications(fromTabId sourceTabId: UUID, toTabId destinationTabId: UUID, surfaceId: UUID) { guard sourceTabId != destinationTabId else { return } TerminalMutationBus.shared.discardPendingNotifications(forTabId: sourceTabId, surfaceId: surfaceId) var didMoveNotification = false let updated = notifications.map { notification -> TerminalNotification in guard notification.matches(tabId: sourceTabId, surfaceId: surfaceId) else { return notification } didMoveNotification = true return TerminalNotification( id: notification.id, tabId: destinationTabId, surfaceId: notification.surfaceId, panelId: notification.panelId, title: notification.title, subtitle: notification.subtitle, body: notification.body, createdAt: notification.createdAt, isRead: notification.isRead, paneFlash: notification.paneFlash, clickAction: notification.clickAction, openAnchor: notification.openAnchor ) } if didMoveNotification { notifications = updated } + if let anchor = promptSubmitOpenAnchors[TabSurfaceKey(tabId: sourceTabId, surfaceId: surfaceId)] { + promptSubmitOpenAnchors.removeValue(forKey: TabSurfaceKey(tabId: sourceTabId, surfaceId: surfaceId)) + promptSubmitOpenAnchors[TabSurfaceKey(tabId: destinationTabId, surfaceId: surfaceId)] = anchor + } + if focusedReadIndicatorByTabId[sourceTabId] == surfaceId { focusedReadIndicatorByTabId.removeValue(forKey: sourceTabId) if focusedReadIndicatorByTabId[destinationTabId] == nil { focusedReadIndicatorByTabId[destinationTabId] = surfaceId } } }🤖 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 1961 - 1996, rebindSurfaceNotifications currently moves notifications and focusedReadIndicatorByTabId but forgets to rebind promptSubmitOpenAnchors; update rebindSurfaceNotifications to check promptSubmitOpenAnchors for an entry for sourceTabId that equals surfaceId, remove it from sourceTabId, and set it on destinationTabId if destination has no entry (mirror the focusedReadIndicatorByTabId logic). Refer to rebindSurfaceNotifications, promptSubmitOpenAnchors, sourceTabId, destinationTabId, and surfaceId when making the change.
🤖 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.
Outside diff comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 1848-1872: restoreSessionNotifications replaces notifications for
a tab but doesn't clear stale promptSubmitOpenAnchors entries, so update
restoreSessionNotifications to also remove any promptSubmitOpenAnchors entries
associated with the given tabId (e.g. clear or filter promptSubmitOpenAnchors
for keys/values tied to tabId) before or after clearing focused read indicator;
reference the promptSubmitOpenAnchors storage and the
restoreSessionNotifications method so the anchors for the previous session don't
persist when notifications are restored.
- Around line 1905-1923: In clearAll() inside TerminalNotificationStore, also
clear the promptSubmitOpenAnchors state to avoid stale anchors being reused;
locate the clearAll method and add a step to remove or reset
promptSubmitOpenAnchors (e.g., call whatever clearPromptSubmitOpenAnchors() or
promptSubmitOpenAnchors.removeAll()) alongside the other clear* calls (before
publishing CmuxEventBus.shared.publishNotificationCleared and removing
notifications from UNUserNotificationCenter) so anchors are fully reset when
notifications and workspace unread state are cleared.
- Around line 1961-1996: rebindSurfaceNotifications currently moves
notifications and focusedReadIndicatorByTabId but forgets to rebind
promptSubmitOpenAnchors; update rebindSurfaceNotifications to check
promptSubmitOpenAnchors for an entry for sourceTabId that equals surfaceId,
remove it from sourceTabId, and set it on destinationTabId if destination has no
entry (mirror the focusedReadIndicatorByTabId logic). Refer to
rebindSurfaceNotifications, promptSubmitOpenAnchors, sourceTabId,
destinationTabId, and surfaceId when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f479b8e-94f0-4ada-91b2-aa28b8e85485
📒 Files selected for processing (5)
Sources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/WorkspacePromptSubmitTests.swift
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
duroey
changed the title
|
@codex review |
@duroey I can't start this review because your workspace has reached its free monthly review limit. Reviews resume at the start of your next billing cycle. Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb3dc5f9c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let resolvedOpenAnchor = openAnchor ?? notificationId.flatMap { id in | ||
| notificationStore?.notifications.first(where: { $0.id == id })?.openAnchor | ||
| } |
There was a problem hiding this comment.
When a notification policy sets effects.record=false while leaving desktop=true, applyNotification computes an openAnchor but deliberately skips recordNotification, and scheduleUserNotification only puts IDs in the delivered notification payload. On a Notification Center click, handleNotificationResponse calls openNotification without an anchor, so this store lookup cannot find the unrecorded notification and the click still opens at the live bottom instead of the turn start. Serialize the anchor into the UN notification userInfo or fall back to the prompt-submit anchor for the tab/surface.
Useful? React with 👍 / 👎.
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 (2)
Sources/TerminalNotificationStore.swift (2)
1235-1282: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract prompt-submit anchor lifecycle out of this store.
TerminalNotificationStore.swiftis already far beyond the file-size limit, and this PR adds more lifecycle/state logic into it. Please move anchor record/lookup/prune behavior into a focused type (and file) to keep this class maintainable.As per coding guidelines: “Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility.”
🤖 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 1235 - 1282, The prompt-submit anchor lifecycle logic (recordPromptSubmitOpenAnchor, promptSubmitOpenAnchor, removePromptSubmitOpenAnchors and the promptSubmitOpenAnchors storage using TabSurfaceKey/TerminalNotificationOpenAnchor) should be extracted into a focused type and file: create a new AnchorStore (or PromptSubmitAnchorStore) that encapsulates the dictionary and the three methods (record, lookup, prune) with the same signatures/semantics, update TerminalNotificationStore to hold an instance of that new type and forward calls to it, and move unit tests or add new ones for the new type; ensure TabSurfaceKey and TerminalNotificationOpenAnchor remain usable (move or import them if needed) and update any callers to use the new store instance instead of the in-file dictionary and methods.Source: Coding guidelines
1925-1943:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
clearAllleaves stale prompt-submit anchors behind.Global clear does not clear
promptSubmitOpenAnchors, and the guard can early-return when only anchors remain. That lets old anchors leak into future notifications and reopen at the wrong turn position.🛠️ Proposed fix
func clearAll(discardQueuedNotifications: Bool = true) { if discardQueuedNotifications { TerminalMutationBus.shared.discardPendingNotifications() } guard !notifications.isEmpty || !focusedReadIndicatorByTabId.isEmpty || !manualUnreadWorkspaceIds.isEmpty || !panelDerivedUnreadWorkspaceIds.isEmpty || - !restoredUnreadWorkspaceIds.isEmpty else { return } + !restoredUnreadWorkspaceIds.isEmpty || + !promptSubmitOpenAnchors.isEmpty else { return } let tabIdsToClearPanelUnread = panelDerivedUnreadWorkspaceIds.union(notifications.map(\.tabId)) let ids = notifications.map { $0.id.uuidString } replaceNotificationsForClear([]) + promptSubmitOpenAnchors.removeAll() clearWorkspaceManualUnread() clearAllWorkspacePanelUnread(forTabIds: tabIdsToClearPanelUnread) clearPanelDerivedWorkspaceUnread() clearWorkspaceRestoredUnread()🤖 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 1925 - 1943, The clearAll method currently can early-return while leaving promptSubmitOpenAnchors populated; modify clearAll (the function named clearAll) to clear promptSubmitOpenAnchors before the guard early-return (or include promptSubmitOpenAnchors.empty in the guard check) so anchors never persist across a global clear; specifically ensure promptSubmitOpenAnchors is emptied (e.g. call the existing clear/close routine for promptSubmitOpenAnchors or remove all entries) before or as part of the replaceNotificationsForClear/clearWorkspace* sequence (refer to replaceNotificationsForClear, clearWorkspaceManualUnread, clearAllWorkspacePanelUnread, clearPanelDerivedWorkspaceUnread, clearWorkspaceRestoredUnread and focusedReadIndicatorByTabId) so no stale anchors remain after a global clear.
🤖 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
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsVisibleSectionResolver.swift`:
- Around line 4-45: This file declares multiple top-level major types; split
each major type into its own file so the package follows “one major type per
file”: create separate files containing
SettingsSectionVisibilityCoordinateSpace, SettingsSectionFramePreferenceKey,
SettingsSectionVisibilityMarker, and keep SettingsVisibleSectionResolver in its
original file; move the corresponding type declarations into those new files
(preserve their exact names, visibility, and code), update the original file to
remove the moved declarations and import/compile in the same module, and ensure
any references to SettingsSectionVisibilityCoordinateSpace.name,
SettingsSectionFramePreferenceKey, SettingsSectionVisibilityMarker, and the View
extension settingsSectionVisibility(_:) remain unchanged and compile.
---
Outside diff comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 1235-1282: The prompt-submit anchor lifecycle logic
(recordPromptSubmitOpenAnchor, promptSubmitOpenAnchor,
removePromptSubmitOpenAnchors and the promptSubmitOpenAnchors storage using
TabSurfaceKey/TerminalNotificationOpenAnchor) should be extracted into a focused
type and file: create a new AnchorStore (or PromptSubmitAnchorStore) that
encapsulates the dictionary and the three methods (record, lookup, prune) with
the same signatures/semantics, update TerminalNotificationStore to hold an
instance of that new type and forward calls to it, and move unit tests or add
new ones for the new type; ensure TabSurfaceKey and
TerminalNotificationOpenAnchor remain usable (move or import them if needed) and
update any callers to use the new store instance instead of the in-file
dictionary and methods.
- Around line 1925-1943: The clearAll method currently can early-return while
leaving promptSubmitOpenAnchors populated; modify clearAll (the function named
clearAll) to clear promptSubmitOpenAnchors before the guard early-return (or
include promptSubmitOpenAnchors.empty in the guard check) so anchors never
persist across a global clear; specifically ensure promptSubmitOpenAnchors is
emptied (e.g. call the existing clear/close routine for promptSubmitOpenAnchors
or remove all entries) before or as part of the
replaceNotificationsForClear/clearWorkspace* sequence (refer to
replaceNotificationsForClear, clearWorkspaceManualUnread,
clearAllWorkspacePanelUnread, clearPanelDerivedWorkspaceUnread,
clearWorkspaceRestoredUnread and focusedReadIndicatorByTabId) so no stale
anchors remain after a global clear.
🪄 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: b4416d34-1292-48d6-8115-97b07e9f9c79
📒 Files selected for processing (12)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSidebarEntryRow.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsVisibleSectionResolver.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BrowserSection.swiftPackages/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsVisibleSectionResolverTests.swiftSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/TerminalNotificationStore.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/TerminalNotificationSocketActionTests.swiftcmuxTests/WorkspacePromptSubmitTests.swift
|
Addressed the latest review feedback in Changes included:
Validation:
Current PR checks: Socket passes, CodeRabbit/Greptile are still processing the new commit, Cubic is skipped due review limit, and Vercel still requires Manaflow team deploy authorization. |
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
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionVisibilityCoordinateSpace.swift`:
- Around line 1-2: The enum SettingsSectionVisibilityCoordinateSpace is being
used purely as a namespace for the static constant name; replace the no-case
enum with a concrete non-enum type (e.g., a struct) to comply with the “no
namespace-enums” rule: locate the SettingsSectionVisibilityCoordinateSpace
declaration and change it to a struct (or another appropriate type) while
preserving the static let name = "SettingsSectionVisibilityCoordinateSpace"
member and any access level.
🪄 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: d41fff17-658e-487c-952c-02597304ae60
📒 Files selected for processing (9)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionFramePreferenceKey.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionVisibilityCoordinateSpace.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionVisibilityMarker.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsVisibleSectionResolver.swiftSources/AppDelegate.swiftSources/PromptSubmitOpenAnchorStore.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspacePromptSubmitTests.swift
💤 Files with no reviewable changes (1)
- Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsVisibleSectionResolver.swift
teamleaderleo
added
area: settings
This branch has not been deployed
2 participants
Summary
Testing
git diff --checkcd Packages/CmuxSettingsUI && swift test(22 tests passed)CMUX_SKIP_ZIG_BUILD=1 xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/GhosttySurfaceOverlayTests/testNotificationOpenAnchorCapturesTurnStartInsteadOfViewportTop -only-testing:cmuxTests/TerminalNotificationSocketActionTests/testPromptSubmitWithoutSurfaceIdRecordsAnchorForFocusedTerminalNotificationCMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag notification-turn-anchorFull non-skip Zig validation was attempted with verified Zig 0.15.2 at
/Users/duroy/.local/opt/zig/0.15.2/zig, but this localmacOS 26.5.1 / Xcode 26.5environment fails before cmux tests run while linking Zig'szig buildbuild-runner (undefined symbol: __availability_version_check,_dispatch_queue_create,_abort, etc.). A minimal/tmpZig build project reproduces the same linker failure, so this appears to be a local Zig 0.15.2 + Xcode 26.x SDK compatibility issue rather than a cmux code failure.Demo Video
CMUX_SKIP_ZIG_BUILD=1.Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
New Features
Tests