Keep agent completions unread until their pane is viewed - #782
Conversation
onevtail
left a comment
There was a problem hiding this comment.
The main fix works: an inactive window’s selected/focused pane now remains unread, and the focused tests and visible CI pass. However, there is a blocking actor-reentrancy issue in detectAgentState.
previous is captured before awaiting resolveRetainedSession. While that call is suspended, markAgentSeen can update the current state to seen = true. If session metadata changes so that next != previous, the final writeback can derive resolvedSeen from the stale pre-await snapshot and overwrite the acknowledgement with seen = false, causing Done to reappear after the user has already viewed it. A probe exercising this production branch fails on the exact PR head.
Please re-read and merge the current surfaceAgentStates[surfaceID] after session resolution so that acknowledgement for the same completion is monotonic, while still allowing a genuinely new working/blocked → idle completion to become unread. This should have a behavior-level regression test using a controllable resolver gate to cover acknowledgement during suspension followed by changed session metadata. With that coverage in place, resolvedSeen can remain private rather than being exposed for direct unit testing.
onevtail - an assistant to @onevcat
|
The shared completion-read fix and the session-resolution race reported in this review are now addressed in #781, with controlled-suspension regression coverage and local validation. We will use #781 as the merge vehicle to avoid merging the same implementation twice. Thank you @SunChJ for the original work in #778 and this resubmission. #781 now links back here and credits your contribution. Thanks also to @onevtail for the race report. |
|
Thanks for the follow-up — glad the session-resolution race is now covered with regression tests, and using #781 as the merge vehicle makes sense to avoid landing the same implementation twice. Appreciate the credit for the race report; happy to give #781 another look before it merges if that helps.
|
Resubmission
Replaces #778 after the source fork became unavailable. The original implementation commits are unchanged.
Summary
Fixes #777.
isViewedSurfacewhen resolving agent read state, evaluated after asynchronous session resolution rather than before it.Selected does not mean viewed: inactive, hidden, unknown-window, and Canvas-managed surfaces retain unread completions. This deliberately reuses the existing conservative Canvas policy rather than changing Canvas visibility semantics. Explicit notification “Mark as Read” behavior is unchanged.
Original validation (from the previous PR)
make build-apppassed with zero warnings/errors.make checkpassed its full-tree formatting lint, SwiftLint, and 146 script tests. Its changed-file formatter hit the existing macOS Bashmapfileincompatibility, so both changed Swift files were also explicitly formatted withxcrun swift-format.git diff --checkpassed.Resubmission checks
No new source changes. The validation above is historical; this branch’s build and Swift tests were not rerun for resubmission.