Skip to content

Fix workspace unread dismissal on explicit tab focus - #1001

Closed
lawrencecchen wants to merge 2 commits into
mainfrom
task-focus-tab-dismiss-regression
Closed

lawrencecchen wants to merge 2 commits into
mainfrom
task-focus-tab-dismiss-regression

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Mar 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add a regression test for selecting a workspace with a workspace-level unread notification (surfaceId == nil)
  • mark workspace-level unread notifications read when the user explicitly focuses that workspace and there is no focused-panel unread to clear
  • restore AppDelegate.shared and AppFocusState.overrideIsFocused after the regression test so the suite stays order-independent

Testing

  • red: xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /Users/lawrencechen/Library/Developer/Xcode/DerivedData/cmux-task-focus-tab-dismiss-regression-red test -only-testing:cmuxTests/NotificationDockBadgeTests/testSelectingWorkspaceMarksSurfaceLessNotificationRead (fails on commit 12543dbbe^)
  • green: xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /Users/lawrencechen/Library/Developer/Xcode/DerivedData/cmux-task-focus-tab-dismiss-regression-green test -only-testing:cmuxTests/NotificationDockBadgeTests/testSelectingWorkspaceMarksSurfaceLessNotificationRead
  • review: ./scripts/codex-review.sh /Users/lawrencechen/fun/cmuxterm-hq/worktrees/task-focus-tab-dismiss-regression2 (0 findings)

Regression Source


Summary by cubic

Fixes a regression where workspace-level unreads (surfaceId = nil) were not dismissed when explicitly focusing a workspace tab. Adds a regression test to lock this behavior and resets global state to keep tests order-independent.

Written for commit 740762d. Summary will update on new commits.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved notification read status logic for focused panels, enhancing how unread states are managed and ensuring notifications clear correctly
    • Fixed notification flash behavior to trigger reliably when navigating workspaces
  • Tests

    • Added test coverage to verify notifications are properly marked as read when selecting workspaces

@vercel

vercel Bot commented Mar 6, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Mar 6, 2026 6:20am

@coderabbitai

coderabbitai Bot commented Mar 6, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5aee8cd3-3de8-424b-9737-e2ac6b3afe09

📥 Commits

Reviewing files that changed from the base of the PR and between 46e810f and 740762d.

📒 Files selected for processing (2)
  • Sources/TabManager.swift
  • cmuxTests/CmuxWebViewKeyEquivalentTests.swift

📝 Walkthrough

Walkthrough

Modified TabManager to conditionally mark panel-level unread notifications as read only when unread exists, then proceed to tab-level checks. Removed guard blocking flash triggers when unread notifications are absent. Added test case validating that selecting a workspace marks surface-less notifications as read.

Changes

Cohort / File(s) Summary
Notification Handling Logic
Sources/TabManager.swift
Broadened conditional logic in markFocusedPanelReadIfActive to check and mark panel unread notifications only when present, then fall through to tab-level checks. Removed guard in focus-flash path that blocked flashing when target panel lacked unread notifications.
Notification Test Coverage
cmuxTests/CmuxWebViewKeyEquivalentTests.swift
Added testSelectingWorkspaceMarksSurfaceLessNotificationRead test case to verify that selecting a workspace marks surface-less (nil surfaceId) notifications as read and clears unread state for the corresponding tab.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #971: Directly modifies TabManager.swift's notification handling logic, including focus behavior and mark-read operations on panel and tab levels.

Poem

🐰 A notification shifts its silent call,
Flash now dances free, with panel or without,
Surface-less whispers marked in workspace's thrall,
Conditional logic clears the cloudy doubt,
Test cases bloom to prove the flow is true! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: fixing workspace unread dismissal behavior when explicitly focusing a tab.
Description check ✅ Passed The description covers the required template sections (Summary, Testing, Checklist) with comprehensive technical details about the change, regression source, and testing verification commands.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch task-focus-tab-dismiss-regression

Comment @coderabbitai help to get the list of available commands and usage tips.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@greptile-apps

greptile-apps Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a regression where workspace-level unread notifications (surfaceId == nil) were not being dismissed when a user explicitly focused that workspace tab. It also adds a regression test to prevent recurrence and restores global state (AppDelegate/AppFocusState) via defer to keep the test suite order-independent.

Changes:

  • markFocusedPanelReadIfActive now falls through to a workspace-level markRead call when the focused panel has no unread notification, correctly handling the surfaceId: nil case
  • A new regression test testSelectingWorkspaceMarksSurfaceLessNotificationRead verifies the fix end-to-end, properly using defer to restore AppDelegate.shared and AppFocusState.overrideIsFocused after cleanup

Confidence Score: 5/5

  • The core workspace-level unread dismissal fix is sound and well-tested with proper state cleanup.
  • The PR correctly implements workspace-level unread notification dismissal in markFocusedPanelReadIfActive by falling through when the focused panel has no unread notification. The new regression test properly verifies the fix with correct defer-based teardown to maintain test suite order-independence. The change is minimal, focused, and addresses the specific regression without introducing functional issues.
  • No files require special attention

Last reviewed commit: 740762d

Comment on lines +7461 to 7462
}
func testNotificationIndexesUpdateAfterReadAndClearMutations() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a blank line between test methods for consistency with surrounding tests:

Suggested change
}
func testNotificationIndexesUpdateAfterReadAndClearMutations() {
}
func testNotificationIndexesUpdateAfterReadAndClearMutations() {

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!

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview — 740762d1 Deployed Mar 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants