Skip to content

fix(tui): drop cached unread rows the server no longer lists - #175

Open
allenhutchison wants to merge 1 commit into
mainfrom
unread-prune-rows
Open

allenhutchison wants to merge 1 commit into
mainfrom
unread-prune-rows

Conversation

@allenhutchison

@allenhutchison allenhutchison commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Human overview

The TUI cached unread rows only ever grew: a topic missing from a later GET /api/v1/groups/unread answer kept its old dot. When the server answers, its rows are now the whole snapshot and unlisted topics are dropped. Per the decision on the issue, the rail shows dots only and unread_total is not displayed.

Closes #155

AI reviewer

Change:

  • RailUpdate.unread_answered (poller.py) is true only when the unread request came back. An empty unread otherwise means "not asked" (no room moved) or "failed", not "nothing unread".
  • ReadMarks.merge_unread(..., snapshot=False) (state.py): with snapshot=True, cached rows (and their counted_from) for topics not in the answer are deleted before the upsert; a snapshot without a total also clears unread_total rather than leaving a stale one. Default behavior is unchanged, so partial callers and existing tests are unaffected.
  • app._apply_groups passes snapshot=update.unread_answered.

Not touched: the display of unread_total (declined on the issue, #155); settle().

Relation to #174 (the 404 back-off): independent. Both edit RailPoller.step/RailUpdate, but this branch is cut from main and does not need that change. During a 404 back-off fetched is None, so unread_answered is false and no rows are pruned, which is the wanted result; the two PRs touch different lines, but whichever merges second may need a trivial rebase.

Assumption to check: the server lists every topic with unread messages (and may omit read topics). I could not confirm this from this repo (the route is mirrored from pepper with no schema). If the server ever caps or omits topics it could not evaluate, pruning would clear their dots.
Also: a row the client rejects as malformed (non-int count) is absent from the parsed list, so a snapshot would drop that topic's cached row instead of leaving it.

Verification:

  • New tests: snapshot drops unlisted rows and counted_from, a partial answer keeps them, snapshot clears a stale total, and unread_answered is true/false in the right polls.
  • uv run ruff format --check, ruff check, ty check src/pepper_cli/ clean; uv run pytest --cov --cov-report=json:coverage-summary.json: 2659 passed, 2 skipped.
  • Simplify: diff reviewed directly, nothing to change. code-review (medium): 4 findings, 1 fixed (stale total after a snapshot with none), 3 not fixed: the server-contract assumption and malformed-row case above, and the extra flag on RailUpdate, which I kept because unread=[] cannot say "not asked".

Checklist (repo-specific)

  • uv run ruff format --check and uv run ruff check clean (no new lint ignores)
  • uv run ty check src/pepper_cli/ clean
  • uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered
  • Server-contract changes: none; the response is read as before
  • No token or credential is logged, printed, or put in a fixture
  • README and the bundled SKILL.md: CHANGELOG updated; no command or README-visible change

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lk5Upqk2oCRiV3ZENbLyLL

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no new actionable issue was found.

Summary

Successful unread answers now replace the cached topic list instead of only adding rows.

  • The TUI rail drops unread dots the server no longer reports.

Acknowledged by allenhutchison: the server's answer is assumed to list every unread topic; malformed rows are dropped before pruning; and the extra RailUpdate flag is intentional because an empty list cannot distinguish “not asked” from “nothing unread.” These were not reported again.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Rail poll] --> B{Unread request answered?}
  B -->|No| C[Keep cached rows]
  B -->|Yes| D[Remove topics missing from answer]
  D --> E[Save returned rows and total]
  C --> F[Apply existing read rules]
  E --> F
Loading

Reviews (1) · Last reviewed commit: "fix(tui): drop cached unread rows the se..." · Reviewed by Greptile

merge_unread only upserted, so a topic absent from a later unread answer kept its old row.
When the server answered the unread request, its rows are now the whole snapshot and the rest
are dropped (and a snapshot without a total clears the cached total). A poll that did not
ask, or whose request failed, still keeps what is cached. The rail shows dots only; the
unread total is not displayed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lk5Upqk2oCRiV3ZENbLyLL
@vycari-review

vycari-review Bot commented Oct 29, 2026

Copy link
Copy Markdown

Vycari Review (shadow)

Shadow round 1 on 9d68355. 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 1 · P2 0 · confirmed fixed 0 · withdrawn 0

Judge's notes

I found two P1 problems where a bad answer from the server is treated as "nothing unread". First, a 200 answer with no usable topics list clears every cached unread dot. Second, a topic whose row has a malformed count is dropped from the parsed list, so the snapshot erases its dot. In both cases the user is told rooms are caught up when they still have unread messages.

P1 · VR-09d47854 · A 200 unread answer with no usable topics list clears every cached unread dot

src/pepper_cli/tui/state.py lines 183–187 · symbol ReadMarks.merge_unread · lens fail-open · status new

The snapshot prune treats any successful unread answer as the server's complete list. api_client.get_groups_unread (api_client.py:3339-3340) builds the rows with body.get('topics') or [], so a 200 body that has no 'topics' key, or a non-list value, parses to an empty list. poller.step then sets unread_answered=True (poller.py:129), and merge_unread(snapshot=True) deletes every cached row and counted_from entry (state.py:183-187) and clears unread_total (state.py:191-192). The client cannot tell 'nothing unread' from 'the body was not the expected shape'. Before this PR the same body left the cache untouched.

Evidence: poller.py:113-129: unread_answered=fetched is not None, and _fetch_unread returns any 200 body. api_client.py:3339-3349: body.get('topics') or [] and no check that the body has the expected shape; the response is untyped (MIRROR comment, no snapshot schema). state.py:183-192: snapshot deletes rows not in the parsed list and clears unread_total. I looked for a degraded or shape guard on the unread answer in poller.py, app.py _apply_groups and api_client.py and found none. The PR body's own assumption ('the server lists every topic with unread messages') is unverified in this repo, so the trigger is not proven, but the harm when it happens is wrong dots on every room.

Also reported here: Malformed unread rows erase existing unread dots: Snapshot pruning treats absence from the parsed rows as absence from the server response. However, get_groups_unread silently discards rows with malformed counts. A topic explicitly present in the response can therefore lose its cached unread state without any successful count or read establishing that it is read.

Evidence: api_client.py:1300-1304 rejects non-integer counts; api_client.py:3339-3348 silently filters rejected rows while returning GroupsUnread successfully. This behavior is explicitly exercised by tests/test_api_client_groups.py:820-852. poller.py:123-131 treats that return as complete and sets unread_answered=true; app.py:761-765 forwards it as snapshot=true. state.py:183-187 then deletes the rejected topic's cached row. This is distinct from an HTTP or transport failure, which preserves the cache, and requires no assumption that the server legitimately omits unread topics.

Failure scenario

The server answers GET /api/v1/groups/unread with 200 and a body without a 'topics' array (a different envelope, a wrapper or proxy body, or a degraded answer that omits rows). The rail's room signature moved, so the poll requests the unread route. The answer parses to no rows. Every topic in every room loses its dot in the sidebar and the terminal title's unread marker clears, although the server still lists unread messages. The dots come back only when the next room moves and a parseable answer arrives. The user sees 'all caught up' with unread messages present.

How this was verified (traced-callers)

Read api_client.py:3339-3350, which uses body.get('topics') or [] with no shape check. Read poller.py:113-129 and found no guard between a 200 answer and unread_answered=True. Reachable from: RailPoller.step called from the app rail poll.

Proving test

Seed ReadMarks (or a stub server) with a good unread answer listing topic t1 with unread_count 2. Then return HTTP 200 with body {"unread_total": 0} (no topics key) for the next /groups/unread request, with a moved room. After RailPoller.step and app._apply_groups, assert marks.is_unread(t1) is still True. On the PR head it is False, because merge_unread(snapshot=True) removes the row.

@allenhutchison

Copy link
Copy Markdown
Contributor Author

On the open question in the body (does the server list every unread topic?): yes. Groups' services/unread.py _unread (origin/main) returns one row per visible topic across every segment of the person's rooms. The query has no LIMIT and no count filter, so the response is a complete snapshot, and pruning unlisted rows is safe.

https://claude.ai/code/session_01Lk5Upqk2oCRiV3ZENbLyLL

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TUI: show unread_total, and drop cached unread rows the server no longer lists

1 participant