From ede5b701bb867c563663b7ca1166eb8abda64663 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 20 Feb 2026 23:07:52 -0800 Subject: [PATCH] Fix Mark Tab as Unread no-op on focused tab (#249) * Fix manual unread clear race on focused tab * Add mark-as-read tab action and show ring for manual unread * Flash then clear manual unread on tab focus --- CLI/cmux.swift | 2 +- Sources/TerminalController.swift | 6 +- Sources/Workspace.swift | 69 +++++++++++- Sources/WorkspaceContentView.swift | 5 +- cmuxTests/WorkspaceManualUnreadTests.swift | 108 +++++++++++++++++++ tests_v2/test_tab_workspace_action_naming.py | 4 + vendor/bonsplit | 2 +- 7 files changed, 189 insertions(+), 7 deletions(-) create mode 100644 cmuxTests/WorkspaceManualUnreadTests.swift diff --git a/CLI/cmux.swift b/CLI/cmux.swift index 1873e352..88cab6e8 100644 --- a/CLI/cmux.swift +++ b/CLI/cmux.swift @@ -3031,7 +3031,7 @@ struct CMUXCLI { new-terminal-right | new-browser-right reload | duplicate pin | unpin - mark-unread + mark-read | mark-unread Flags: --action Action name (required if not positional) diff --git a/Sources/TerminalController.swift b/Sources/TerminalController.swift index d8724264..3193df82 100644 --- a/Sources/TerminalController.swift +++ b/Sources/TerminalController.swift @@ -2046,7 +2046,7 @@ class TerminalController { "close_left", "close_right", "close_others", "new_terminal_right", "new_browser_right", "reload", "duplicate", - "pin", "unpin", "mark_unread" + "pin", "unpin", "mark_read", "mark_unread" ] var result: V2CallResult = .err(code: "invalid_params", message: "Unknown tab action", data: [ @@ -2160,6 +2160,10 @@ class TerminalController { workspace.setPanelPinned(panelId: surfaceId, pinned: false) finish(["pinned": false]) + case "mark_read", "mark_as_read": + workspace.markPanelRead(surfaceId) + finish() + case "mark_unread", "mark_as_unread": workspace.markPanelUnread(surfaceId) finish() diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index d5613417..54b8b203 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -103,6 +103,9 @@ final class Workspace: Identifiable, ObservableObject { @Published private(set) var panelCustomTitles: [UUID: String] = [:] @Published private(set) var pinnedPanelIds: Set = [] @Published private(set) var manualUnreadPanelIds: Set = [] + private var manualUnreadMarkedAt: [UUID: Date] = [:] + nonisolated private static let manualUnreadFocusGraceInterval: TimeInterval = 0.2 + nonisolated private static let manualUnreadClearDelayAfterFocusFlash: TimeInterval = 0.2 @Published var statusEntries: [String: SidebarStatusEntry] = [:] @Published var logEntries: [SidebarLogEntry] = [] @Published var progress: SidebarProgressState? @@ -388,7 +391,10 @@ final class Workspace: Identifiable, ObservableObject { private func syncUnreadBadgeStateForPanel(_ panelId: UUID) { guard let tabId = surfaceIdFromPanelId(panelId) else { return } - let shouldShowUnread = manualUnreadPanelIds.contains(panelId) || hasUnreadNotification(panelId: panelId) + let shouldShowUnread = Self.shouldShowUnreadIndicator( + hasUnreadNotification: hasUnreadNotification(panelId: panelId), + isManuallyUnread: manualUnreadPanelIds.contains(panelId) + ) if let existing = bonsplitController.tab(tabId), existing.showsNotificationBadge == shouldShowUnread { return } @@ -481,14 +487,45 @@ final class Workspace: Identifiable, ObservableObject { func markPanelUnread(_ panelId: UUID) { guard panels[panelId] != nil else { return } guard manualUnreadPanelIds.insert(panelId).inserted else { return } + manualUnreadMarkedAt[panelId] = Date() syncUnreadBadgeStateForPanel(panelId) } + func markPanelRead(_ panelId: UUID) { + guard panels[panelId] != nil else { return } + AppDelegate.shared?.notificationStore?.markRead(forTabId: id, surfaceId: panelId) + clearManualUnread(panelId: panelId) + } + func clearManualUnread(panelId: UUID) { - guard manualUnreadPanelIds.remove(panelId) != nil else { return } + let didRemoveUnread = manualUnreadPanelIds.remove(panelId) != nil + manualUnreadMarkedAt.removeValue(forKey: panelId) + guard didRemoveUnread else { return } syncUnreadBadgeStateForPanel(panelId) } + static func shouldClearManualUnread( + previousFocusedPanelId: UUID?, + nextFocusedPanelId: UUID, + isManuallyUnread: Bool, + markedAt: Date?, + now: Date = Date(), + sameTabGraceInterval: TimeInterval = manualUnreadFocusGraceInterval + ) -> Bool { + guard isManuallyUnread else { return false } + + if let previousFocusedPanelId, previousFocusedPanelId != nextFocusedPanelId { + return true + } + + guard let markedAt else { return true } + return now.timeIntervalSince(markedAt) >= sameTabGraceInterval + } + + static func shouldShowUnreadIndicator(hasUnreadNotification: Bool, isManuallyUnread: Bool) -> Bool { + hasUnreadNotification || isManuallyUnread + } + // MARK: - Title Management var hasCustomTitle: Bool { @@ -571,6 +608,7 @@ final class Workspace: Identifiable, ObservableObject { panelCustomTitles = panelCustomTitles.filter { validSurfaceIds.contains($0.key) } pinnedPanelIds = pinnedPanelIds.filter { validSurfaceIds.contains($0) } manualUnreadPanelIds = manualUnreadPanelIds.filter { validSurfaceIds.contains($0) } + manualUnreadMarkedAt = manualUnreadMarkedAt.filter { validSurfaceIds.contains($0.key) } surfaceListeningPorts = surfaceListeningPorts.filter { validSurfaceIds.contains($0.key) } surfaceTTYNames = surfaceTTYNames.filter { validSurfaceIds.contains($0.key) } recomputeListeningPorts() @@ -1157,8 +1195,10 @@ final class Workspace: Identifiable, ObservableObject { } if detached.manuallyUnread { manualUnreadPanelIds.insert(detached.panelId) + manualUnreadMarkedAt[detached.panelId] = .distantPast } else { manualUnreadPanelIds.remove(detached.panelId) + manualUnreadMarkedAt.removeValue(forKey: detached.panelId) } guard let newTabId = bonsplitController.createTab( @@ -1178,6 +1218,7 @@ final class Workspace: Identifiable, ObservableObject { panelCustomTitles.removeValue(forKey: detached.panelId) pinnedPanelIds.remove(detached.panelId) manualUnreadPanelIds.remove(detached.panelId) + manualUnreadMarkedAt.removeValue(forKey: detached.panelId) panelSubscriptions.removeValue(forKey: detached.panelId) return nil } @@ -1611,6 +1652,7 @@ extension Workspace: BonsplitDelegate { } private func applyTabSelectionNow(tabId: TabID, inPane pane: PaneID) { + let previousFocusedPanelId = focusedPanelId if bonsplitController.allPaneIds.contains(pane) { if bonsplitController.focusedPaneId != pane { bonsplitController.focusPane(pane) @@ -1650,7 +1692,24 @@ extension Workspace: BonsplitDelegate { } panel.focus() - clearManualUnread(panelId: panelId) + let isManuallyUnread = manualUnreadPanelIds.contains(panelId) + let markedAt = manualUnreadMarkedAt[panelId] + if Self.shouldClearManualUnread( + previousFocusedPanelId: previousFocusedPanelId, + nextFocusedPanelId: panelId, + isManuallyUnread: isManuallyUnread, + markedAt: markedAt + ) { + triggerFocusFlash(panelId: panelId) + let clearDelay = Self.manualUnreadClearDelayAfterFocusFlash + if clearDelay <= 0 { + clearManualUnread(panelId: panelId) + } else { + DispatchQueue.main.asyncAfter(deadline: .now() + clearDelay) { [weak self] in + self?.clearManualUnread(panelId: panelId) + } + } + } // Converge AppKit first responder with bonsplit's selected tab in the focused pane. // Without this, keyboard input can remain on a different terminal than the blue tab indicator. @@ -1803,6 +1862,7 @@ extension Workspace: BonsplitDelegate { panelCustomTitles.removeValue(forKey: panelId) pinnedPanelIds.remove(panelId) manualUnreadPanelIds.remove(panelId) + manualUnreadMarkedAt.removeValue(forKey: panelId) panelSubscriptions.removeValue(forKey: panelId) surfaceTTYNames.removeValue(forKey: panelId) PortScanner.shared.unregisterPanel(workspaceId: id, panelId: panelId) @@ -2118,6 +2178,9 @@ extension Workspace: BonsplitDelegate { guard let panelId = panelIdFromSurfaceId(tab.id) else { return } let shouldPin = !pinnedPanelIds.contains(panelId) setPanelPinned(panelId: panelId, pinned: shouldPin) + case .markAsRead: + guard let panelId = panelIdFromSurfaceId(tab.id) else { return } + markPanelRead(panelId) case .markAsUnread: guard let panelId = panelIdFromSurfaceId(tab.id) else { return } markPanelUnread(panelId) diff --git a/Sources/WorkspaceContentView.swift b/Sources/WorkspaceContentView.swift index ec34dd1b..defce523 100644 --- a/Sources/WorkspaceContentView.swift +++ b/Sources/WorkspaceContentView.swift @@ -42,7 +42,10 @@ struct WorkspaceContentView: View { let isFocused = isWorkspaceInputActive && workspace.focusedPanelId == panel.id let isSelectedInPane = workspace.bonsplitController.selectedTab(inPane: paneId)?.id == tab.id let isVisibleInUI = isWorkspaceVisible && isSelectedInPane - let hasUnreadNotification = notificationStore.hasUnreadNotification(forTabId: workspace.id, surfaceId: panel.id) + let hasUnreadNotification = Workspace.shouldShowUnreadIndicator( + hasUnreadNotification: notificationStore.hasUnreadNotification(forTabId: workspace.id, surfaceId: panel.id), + isManuallyUnread: workspace.manualUnreadPanelIds.contains(panel.id) + ) PanelContentView( panel: panel, isFocused: isFocused, diff --git a/cmuxTests/WorkspaceManualUnreadTests.swift b/cmuxTests/WorkspaceManualUnreadTests.swift new file mode 100644 index 00000000..d5464d73 --- /dev/null +++ b/cmuxTests/WorkspaceManualUnreadTests.swift @@ -0,0 +1,108 @@ +import XCTest + +#if canImport(cmux_DEV) +@testable import cmux_DEV +#elseif canImport(cmux) +@testable import cmux +#endif + +final class WorkspaceManualUnreadTests: XCTestCase { + func testShouldClearManualUnreadWhenFocusMovesToDifferentPanel() { + let previousFocusedPanelId = UUID() + let nextFocusedPanelId = UUID() + + XCTAssertTrue( + Workspace.shouldClearManualUnread( + previousFocusedPanelId: previousFocusedPanelId, + nextFocusedPanelId: nextFocusedPanelId, + isManuallyUnread: true, + markedAt: Date() + ) + ) + } + + func testShouldNotClearManualUnreadWhenFocusStaysOnSamePanelWithinGrace() { + let panelId = UUID() + let now = Date() + + XCTAssertFalse( + Workspace.shouldClearManualUnread( + previousFocusedPanelId: panelId, + nextFocusedPanelId: panelId, + isManuallyUnread: true, + markedAt: now.addingTimeInterval(-0.05), + now: now, + sameTabGraceInterval: 0.2 + ) + ) + } + + func testShouldClearManualUnreadWhenFocusStaysOnSamePanelAfterGrace() { + let panelId = UUID() + let now = Date() + + XCTAssertTrue( + Workspace.shouldClearManualUnread( + previousFocusedPanelId: panelId, + nextFocusedPanelId: panelId, + isManuallyUnread: true, + markedAt: now.addingTimeInterval(-0.25), + now: now, + sameTabGraceInterval: 0.2 + ) + ) + } + + func testShouldNotClearManualUnreadWhenNotManuallyUnread() { + XCTAssertFalse( + Workspace.shouldClearManualUnread( + previousFocusedPanelId: UUID(), + nextFocusedPanelId: UUID(), + isManuallyUnread: false, + markedAt: Date() + ) + ) + } + + func testShouldNotClearManualUnreadWhenNoPreviousFocusAndWithinGrace() { + let now = Date() + + XCTAssertFalse( + Workspace.shouldClearManualUnread( + previousFocusedPanelId: nil, + nextFocusedPanelId: UUID(), + isManuallyUnread: true, + markedAt: now.addingTimeInterval(-0.05), + now: now, + sameTabGraceInterval: 0.2 + ) + ) + } + + func testShouldShowUnreadIndicatorWhenNotificationIsUnread() { + XCTAssertTrue( + Workspace.shouldShowUnreadIndicator( + hasUnreadNotification: true, + isManuallyUnread: false + ) + ) + } + + func testShouldShowUnreadIndicatorWhenManualUnreadIsSet() { + XCTAssertTrue( + Workspace.shouldShowUnreadIndicator( + hasUnreadNotification: false, + isManuallyUnread: true + ) + ) + } + + func testShouldHideUnreadIndicatorWhenNeitherNotificationNorManualUnreadExists() { + XCTAssertFalse( + Workspace.shouldShowUnreadIndicator( + hasUnreadNotification: false, + isManuallyUnread: false + ) + ) + } +} diff --git a/tests_v2/test_tab_workspace_action_naming.py b/tests_v2/test_tab_workspace_action_naming.py index 6b3f4805..c792b92e 100644 --- a/tests_v2/test_tab_workspace_action_naming.py +++ b/tests_v2/test_tab_workspace_action_naming.py @@ -146,6 +146,10 @@ def main() -> int: by_tab_only = c._call("tab.action", {"tab_id": tab_ref, "action": "mark_unread"}) or {} _must(str(by_tab_only.get("tab_ref") or "").startswith("tab:"), f"Expected tab_ref in tab_id-only result: {by_tab_only}") _must(str(by_tab_only.get("workspace_id") or "") == ws_id, f"tab_id-only action should resolve target workspace: {by_tab_only}") + + mark_read = c._call("tab.action", {"tab_id": tab_ref, "action": "mark_read"}) or {} + _must(str(mark_read.get("tab_ref") or "").startswith("tab:"), f"Expected tab_ref in mark_read result: {mark_read}") + _must(str(mark_read.get("workspace_id") or "") == ws_id, f"mark_read should resolve target workspace: {mark_read}") finally: if ws_other: try: diff --git a/vendor/bonsplit b/vendor/bonsplit index cf929c88..198736e4 160000 --- a/vendor/bonsplit +++ b/vendor/bonsplit @@ -1 +1 @@ -Subproject commit cf929c887af79ea8b881e39da5b8c4ee1d6b9009 +Subproject commit 198736e4e2db10931c263eb221b2592fc86e80e7