Repository navigation
fix(tui): ignore a queued PinChanged once the thread is gone - #146
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QNDeh9nWVhiZkwR7qyrAZT
Vycari Review (shadow)Shadow round 1 on 486c9d2. This comment is non-gating and needs no response: it is collected to measure agreement with the current reviewer. The round's verdict is the Vycari Review (shadow) check run. Open: P0 0 · P1 0 · P2 0 · confirmed fixed 0 · withdrawn 0 Judge's notes Incomplete: 1 stage call(s) failed (hunter:general). No candidate findings were raised. No open findings this round. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human overview
Fixes a flaky TUI test (
test_nothing_is_marked_read_while_the_thread_is_scrolled_up) that failed intermittently withNoMatches: 'ThreadView'. The cause was in app code, not the test: a queued scroll-position message was handled during teardown, after the thread widget was gone. The handler now ignores it.Closes #139
AI reviewer
Root cause. The failing traceback (windows run 40552203583) is not in the test body: it is raised from
run_test.__aexit__(raise self._exception), viaon_thread_view_pin_changed->_note_viewed->_scrolled_up->self.thread->query_one(ThreadView). The test ends right after ascroll_end/open_conversation, which can leave aThreadView.PinChangedmessage queued; Textual dispatches it while the screen is being torn down, so the query finds noThreadView. The issue's guess (test racing the screen mount) is not what happens: the test already waits onbody_textbefore touching the thread, and the body is reached viaapp.threadonly after that.Change.
src/pepper_cli/tui/app.py:on_thread_view_pin_changedreturns early whenself.query(ThreadView)is empty (query, unlikequery_one, does not raise). Nothing to read or mark with no thread, and_run("publish")/refresh_chromehave nothing new to do at teardown.tests/test_tui_live.py: new deterministic regression testtest_a_pin_change_delivered_after_the_thread_is_gone_is_ignored(opens a topic, removes the thread, deliversPinChanged). The flaky test itself is untouched, so its assertions are unchanged.Verification. New test fails without the
app.pychange (NoMatches) and passes with it. Could not reproduce the original flake locally (30 runs unloaded, 30 under 8 busy-loop CPU hogs: 0 failures before the fix), so the regression test is the proof. After: target file x50 with-p no:randomly: 0 failures.uv run ruff format --check,ruff check,ty check src/pepper_cli/: clean.uv run pytest --cov --cov-report=json:coverage-summary.json: 2485 passed, 2 skipped.-n autonot run (xdist is not a dev dependency here).Where to look. The guard sits on the one handler that reads
self.threadfrom a message that can outlive the widget; other handlers that callself.threadfrom queued messages were not audited. Not refactored:until/_quietwait helpers are already shared viatests/tui_support.py.Checklist (repo-specific)
uv run ruff format --checkanduv run ruff checkclean (no new lint ignores)uv run ty check src/pepper_cli/cleanuv run pytestpasses; coverage floor in.claude/maintainerd.jsonnot loweredSKILL.mdupdated if commands or output changed: N/A, no command change🤖 Generated with Claude Code
https://claude.ai/code/session_01QNDeh9nWVhiZkwR7qyrAZT


Confidence Score: 5/5
The PR appears safe to merge.Summary
Prevents a queued
ThreadView.PinChangedmessage from crashing the app after the thread widget is removed.Reviews (1) · Last reviewed commit: "fix(tui): ignore a queued PinChanged onc..." · Reviewed by Greptile