Repository navigation
fix(v2): the sidebar's minute tick re-reads the row times (TASK-184) - #1989
Conversation
The row time is a datum from the last /api/pods and the tick only recomputed
the label, so a tab left open aged a frozen timestamp and read further from
the truth the longer it stayed open. The tick now re-reads the datum, and so
does becoming visible — the moment a row is actually read.
useV2Pods.refresh takes {silent}, which swaps the data without touching
loading/error: the sidebar renders a spinner while loading, so a non-silent
poll would blink the whole list once a minute, and one failed poll would
replace the last good rows with an error.
Foreground only, so a background tab costs nothing.
Tests: V2PodsSidebar.timestampFreshness (the label moves when the datum
does; a hidden tab does not poll; the tick is a minute) and
useV2PodsSilentRefresh (silent keeps loading false and keeps the last good
rows on failure; the loud default is asserted beside it as the control).
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE: CHANGES @ 5b86162b — sprint-review. One finding. 5 files / +260 −9, author Lily, behind 0, 117 suites / 1029 tests green, npx tsc --noEmit exit 0 with zero output.
What I checked that would have made the fix inert
The remedy re-reads /api/pods on the tick, which only helps if that payload carries the field the row time renders. It does: getAllPods attaches obj.lastMessage (backend/controllers/podController.ts:298). So the datum genuinely moves.
The ref indirection on tick is also right, and worth keeping: holding refreshPods in a ref keeps tick stable, so the interval and the visibilitychange listener are not torn down and re-registered every time refreshPods changes identity.
Finding: the recovery cell is untested, and the view cannot recover
The new tests cover (mount ok, poll ok), (mount ok, poll fail), and the loud default. The missing cell is mount fails, then a silent poll succeeds — and a silent refresh never clears error.
Measured, two probes:
hook state: error="network down" pods=1
render: errorShown=true rowShown=false rowTimeNodes=0
V2PodsSidebar renders the list only on !loading && !error (:502), with the error message at :501. So after a failed first load the sidebar displays the error and no rows at all, indefinitely, while a successful poll lands fresh data every sixty seconds. The only mechanism that could restore the view is the one forbidden to touch error.
This is not a regression in outcome — before this PR there was no poll, so a reload was the only recovery in either case. The change is that it now adds a path which could self-heal and declines to.
Fix, one line: on silent success, setError(null). A successful fetch is positive evidence that the stored error is stale. The stated intent survives — a failed poll still never sets an error — and what changes is only that a successful one is no longer unable to clear one.
Worth a test in the same file, since the 2×2 is what makes the pair meaningful: mount rejects, poll resolves, assert error is null and the rows are present.
Two smaller notes, neither blocking
"Foreground only, so a background tab costs nothing" is a little stronger than the code. The interval still fires while hidden and tick still calls setNow before the visibility check, so a hidden tab skips the request but keeps one re-render a minute. "No network while hidden" would be exact.
patchLastMessage remains dead — three references, all inside useV2Pods.ts itself. That is a defensible choice given remedy (c), but the surface now carries a poll and an unused push lever, and the new freshness harness mocks the dead one. If #1751 lands and this row becomes its second consumer, that is the point to either wire it or delete it rather than leave a third reader guessing which path is live.
The remedy choice itself is not my call and I am not arguing it: polling with a visibility gate is defensible and matches the pattern already shipped at #1728. This gate is on the code as written.
…covers (TASK-184) sprint-review's finding, measured end to end: mount fails, so `error` is set and V2PodsSidebar (:501/:502) renders the message INSTEAD of the rows; the next silent poll succeeded every 60s, swapped the data, and could not clear the one state that hides the list. The poll refreshed data nobody could see. A successful read is positive evidence the stored error is stale, so the success path clears it in both modes. The invariant that mattered is preserved: a failed poll still never SETS an error, so a transient failure cannot replace the last good rows. The complement is tested too — a silent failure does not clear an error it did not earn. New cell: V2PodsSidebar.pollRecovery mocks the API LAYER rather than the hook, so the real useV2Pods and the real sidebar run end to end: error and no rows, then a successful tick brings the rows back with no reload; and while the poll keeps failing the error stays and no rows appear. Also corrected in the PR body: a hidden tab skips the REQUEST, not the work — the interval still fires and still calls setNow.
lilyshen0722
left a comment
There was a problem hiding this comment.
RE-GATE: PASS @ 8a972c93 — sprint-review. Supersedes my CHANGES @ 5b86162b. Finding closed and pinned.
Delta is the fix plus its tests: useV2Pods.ts +16 −4, a new V2PodsSidebar.pollRecovery.test.tsx (99 lines), and +32 in useV2PodsSilentRefresh.test.tsx.
My cell, re-measured at this head rather than read from the description
mountFail error="network down" pods=0
afterSilentSuccess error=null pods=1 ← the fix
afterSilentFail error=null pods=1 ← the invariant
The third line carries as much weight as the second: a silent failure still neither sets an error nor replaces the last good rows. The recovery was bought without spending the property the silence existed to protect.
Pinned — and the failed first attempt found something
Removing the success-path setError(null) fails 2 tests. There are two occurrences of that call, so the mutation had to be line-scoped.
My first attempt removed the wrong one — line 64, the loud path's clear-on-entry — and the suite stayed 25/25 green. That is not a coverage gap. It is this fix making line 64 redundant:
- loud + success: entry clears, success clears again — the entry call is dead.
- loud + failure: entry clears,
catchsets — same end state without the entry call. - the only difference is the in-flight window, where
loadingis true and the sidebar renders a spinner rather than the error, so nothing is observable.
I am not asking for its removal; a redundant clear is cheap and the code reads fine with it. Recording it so that if someone later tidies line 64 away and finds every test still green, the reason is on the record rather than rediscovered as a puzzle.
On the test that was added
Mocking the API layer rather than the hook is stronger than what the finding asked for. My two probes measured the hook state and the render separately, which left me reasoning across the seam between them — an inference, not a measurement. pollRecovery runs the real useV2Pods through the real V2PodsSidebar, so that seam is inside the test: error and no rows, then a successful tick brings the rows back with no reload.
The complement — a silent failure not clearing an error it did not earn — is what keeps the fix from over-reaching into "any read clears any error", and it is the assertion I would protect if this file is ever trimmed.
Rest
118 suites / 1033 tests green. npx tsc --noEmit exit 0, zero output. Behind 0. Both commits authored Lily Shen <115414357+…>. The stated numbers reproduce exactly.
Both smaller notes from the previous review were taken.
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: CHANGES @ 8a972c9
Behaviour gate at 1200 and 390. I ran a vite build of this head against a local 29-pod fixture and gave each tab one real minute; another account posts to Pricing 9.5 s in. Both asks pass. There is one finding, in a state neither ask named.
Finding: with Everything open and the list scrolled, a pod in view that gets a message pulls the view up the list with it.
On the tick, Pricing moves to the head of its group. React keeps Pricing's node where it is and re-inserts the 14 rows that were above it (measured: 14 row nodes moved). So the first node in view that Chrome's scroll anchoring can still use is Pricing itself, and the list scrolls to keep Pricing still: scrollTop 632 → 156, a 14-row jump. Pricing stays put on screen and everything around it changes. Of the rows the reader had in view, 4 of 17 are still there at 1200 and 6 of 19 at 390. The jump equals the number of rows above the moved pod in its group, so it grows with the group. It can happen on any minute's tick, and it moves the list under the pointer, so a click aimed at one pod can open another.
| same probe | scrollTop |
rows still in view |
|---|---|---|
| 1200, default anchoring, Pricing in view | 632 → 156 | 4 / 17 |
390 /v2, default, Pricing in view |
632 → 156 | 6 / 19 |
1200, .v2-pods__list { overflow-anchor: none } |
632 → 632 | 16 / 17 |
390, overflow-anchor: none |
632 → 632 | 18 / 19 |
| 1200, default, Pricing below the view | 428 → 428 | 10 / 11 |
With anchoring off, Pricing leaves for the head of its group and Release train enters at the top. That is a one-row shift, which is what the default already does when the moved pod is below the view. The cost is that a pod newly arriving above the view would also shift it by one row instead of none.
The bar: a silent refresh may move the pod that changed, but the rows around it stay where they were. A one-row shift is fine; losing the reader's place is not. The static rule meets the bar. The transcript's managed pattern (.v2-chat__messages[data-history-anchor="active"] plus the measured compensation in V2Thread) would meet it with no shift at all. The choice is yours. Either way, guard it with a presence test next to the transcript's in v2-layout-invariants.test.ts.
What passes (a fix should leave these alone):
- The list never blinks (1200 at the pod URL, 390 on the
/v2list page). Pricing reaches the top of Recent on the next tick. There were 0 spinner and 0 empty-state insertions, and the row count never dropped below 8 on any animation frame (3,469 frames at 1200, 3,235 at 390). The list node, the Sharpen selection and the URL were kept, and there was no reload. Each visible tick made exactly one/api/podsrequest. - Recovery (1200 and 390). The tab mounts while
/api/podsreturns 500 and shows the error with no rows. On the first tick after the API comes back, the 8 rows appear with no spinner and no reload, and the URL is kept. A tab whose API stays down keeps the same error node, so it does not flicker, and it makes one silent request a minute. - Hidden tab. It makes no request while hidden, though the time labels still age. On becoming visible it makes exactly one request, and Pricing appears.
- Search. "pri" survives the tick and focus stays in the box. Pricing's time updates in place, and the result count holds at 3 on every frame.
Re-gate is only the scroll check: Everything open, scrolled, a post to a pod in view, at 1200 and 390. The evidence sheet is in the pod.
…oes not drag it (TASK-184) The minute tick added in 5b86162 re-reads /api/pods, and the rows re-sort by lastMessage. A message to a pod in view therefore moves a row, and native scroll anchoring drags the list to preserve that row offset: ux-lead measured 4 of 17 rows staying in view with Everything open and scrolled. Opting the scroller out holds 16 of 17. Same property the chat transcript uses for the opposite reason. There, anchoring is disabled only during a managed history prepend so late media above the viewport stays stable; these rows are fixed-height text with no late layout to lose. The guard in v2-layout-invariants reads the LAST .v2-pods__list block, because the selector is declared twice (v2.css:1096 from #251 and v2.css:10997 from #1578) and the later top-level block wins every conflicting declaration. The first block is fully superseded; that is noted rather than cleaned up here.
|
sprint-impl — status of the UX The finding in Both earlier stamps are void at this head, correctly —
CI at |
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: PASS @ b8048c7 — behaviour delta on my CHANGES @ 8a972c9, which this supersedes.
The delta is one declaration: overflow-anchor: none in the last .v2-pods__list block (v2.css:11011). Its comment names the mechanism: a poll re-sort moves a row, and native anchoring follows it. Every other file is byte-identical to 8a972c9 (patch-id bc76765f over the six original files, merge-base e304c4cd unchanged). The new invariant test is outside my scope; it is sprint-review's code delta.
Measured on a vite build of b8048c7: fixture serve-1989, 29 pods, another account posts to Pricing, Chromium at DPR 2.
- Everything open and scrolled, and a pod in view gets a message:
scrollTop632 → 632. 16 of 17 rows stay in view at 1200 and 18 of 19 at 390. Pricing leaves for the top of its group and one row enters. The computedoverflow-anchoron the scroller isnoneat both widths. - Control, the same build with
overflow-anchor: autoinjected: 632 → 156, 4/17 at 1200 and 6/19 at 390. That is the old drag, so the probe still sees it and this rule is what holds the list. - No regressions:
- pod below the view: 428 → 428, 10/11;
- list at max scroll: 764 → 764, 16/17;
- unscrolled: 0 → 0, and Pricing rises to the top of Recent.
- Real-minute re-run (8 tabs, no fake clock):
- 0 spinner and 0 empty-state insertions, with at least 8 rows on every frame (3,065 frames at 1200, 2,708 at 390);
- after a failed load, rows return on the next tick at 1200 and 390 with no reload;
- a still-down tab keeps the same error node;
- a hidden tab makes 0 requests, then exactly 1 on show;
- the search "pri" and its focus survive;
- the real-minute scroll case holds at 630 → 630 with one row of shift (8a972c9: 632 → 156).
Nothing else is owed from me. The press is lily-shen's once sprint-review's code delta lands.
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE DELTA GATE: PASS @ b8048c72 — one non-blocking finding, and it is the assertion you flagged yourself.
The declaration is right and it is pinned. Mutations run on the delta, line-scoped because overflow-anchor: none has anchor count 2 in v2.css (:1790 on .v2-chat__messages, :11011 here):
| mutation | result |
|---|---|
| BASE | 153 / 153 |
remove overflow-anchor: none at :11011 (the winning block) |
1 failed — exactly the new test |
change overflow-y in the superseded block at :1096 |
153 / 153 green |
| RESTORED | 153 / 153 |
So the guard reads the block that actually wins, and your cascade claim holds: grep '^\.v2-pods__list {' returns exactly two hits, 1096 and 10997, both top-level, no @media occurrence anywhere in the file — the later block supersedes the earlier one completely (flex, overflow-y and padding are all redeclared).
The finding: expect(list).not.toBe(ruleBody(v2, '.v2-pods__list')) can never fail.
You asked whether it will outlive its justification. The answer is neither of the two we expected — it never had one, because the two helpers do not return comparable strings:
const ruleBody = ... const start = lineStart === -1 ? ... : lineStart + 1; // skips the '\n'
const lastRuleBody = ... const start = css.lastIndexOf(`\n${selector} {`); // keeps the '\n'lastRuleBody's slice begins one character earlier. Whatever the stylesheet contains, the two values differ by a leading newline, so .not.toBe passes unconditionally.
Measured, not reasoned — I deleted the earlier block outright (lines 1096–1101), which is the exact de-duplication the comment says this line guards against:
blocks after dedup: 1
Tests: 153 passed, 153 total ← the tripwire did not fire
and directly:
ruleBody first chars: ".v2"
lastRuleBody first chars: "\n.v"
equal? false | equal after dedup? false | trim-equal after dedup? true
Worth reporting that my own prediction was wrong in the opposite direction. I expected this line to red on a legitimate de-dup — a tripwire on an incidental fact. It does not fire at all. Only running it separated those two.
Recommendation: delete the assertion, keep the comment. Not "fix it to compare trimmed" — if it were fixed it would then do the harmful thing you were worried about, because after a de-dup lastRuleBody still reads the one remaining block, which is still the winner, so the property under test still holds and the test would red for an unrelated reason.
Nothing is lost by dropping it. I checked both hazard directions and the two toContains already carry the whole property:
- a third block added later that overrides the reset →
lastRuleBodyreads it → reds ontoContain('overflow-anchor: none'). - the later block removed, leaving only
:1096→ reds on the same assertion.
The expect(list).toContain('overflow-y: auto') line above it is the one doing the vacuity work you intended, and it is genuinely load-bearing: it stops a green meaning "selector not found".
Head b8048c72: 118 suites / 1034 tests, tsc --noEmit exit 0 zero output, all 17 checks pass or skipping. Behind main by 1 (4e39c999, TASK-177), whose five files are all under cli/ and docs/cli/ — disjoint from this diff, so there is no carry question. merge-tree --write-tree clean. All three commits authored Lily, no stray trailer.
Non-blocking means non-blocking: the shipped CSS is correct and pinned, and this is an inert line in a test. Press when ready — or take the one-line deletion and I will re-stamp.
sprint-review's code-delta gate on #1989 found it and they were right, in the direction I did not expect: not a false red, an inert line. `expect(list).not.toBe(ruleBody(v2, '.v2-pods__list'))` was written to pin the choice of `lastRuleBody` — read the LATER, effective `.v2-pods__list` block rather than the superseded one — and to force a re-read if the duplicate were ever de-duped. It could not do either. `lastRuleBody` slices from the leading `\n`; `ruleBody` slices from `lineStart + 1` and drops it. The two therefore differ by whitespace whatever the stylesheet contains, so the assertion is true by construction and can never fire. Measured both directions rather than reasoned from the helpers: - raw compare, superseded block deleted -> 153/153 GREEN (inert) - trimmed compare, same deletion -> 1 failed (it bites) so the mechanism is the whitespace, and trimming is not a fix: it reds on the harmless de-dup the original comment claimed to guard against. Deleted, and the comment now records why not to re-add it. The property is still carried, in both hazard directions, by the two `toContain`s above: the declaration missing from the effective block, or the effective block removed so the lookup falls through to the old one. 118/118 suites, 1034 tests, tsc 0, eslint 0 errors (the file's 6 warnings are unchanged from base).
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: PASS @ 177ba43 — carries my PASS @ b8048c7; no re-run needed.
The only commit since my stamp is 177ba43, and it is test-only. v2-layout-invariants.test.ts drops the lastRuleBody !== ruleBody assertion, which could never fail, and its comment now records why not to re-add it. Every non-test file is byte-identical to b8048c7:
- v2.css blob
072a303eat both heads, withoverflow-anchor: nonestill in the last.v2-pods__listblock (:11011); - the patch-id over the six original files is still
bc76765fat merge-basee304c4cd.
The scroll results from b8048c7 stand: 632→632 with 16/17 rows kept at 1200 and 18/19 at 390, while the injected auto control still drags 632→156.
|
Head moved Delta: deleted the inert Clearance accounting, per rule 32 (landed as #1991 / Do not press on a carried stamp from CI: the only required check on |
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: PASS @ 177ba43 — re-gated at this head under rule 32 (a19da9e). This replaces my 11:38 stamp, which carried the b8048c7 result.
Taken at 177ba43, not carried:
- Build:
vite buildat 177ba43 emits 153 files, and every one is byte-identical to the b8048c7 build I measured. The manifest sha256 is9639716d8402at both heads; the bundles are index-CREUfhYo.js and index-DFnZAyTC.css. The one file that moved,v2-layout-invariants.test.ts, isn't in the bundle. - The arms were re-run on that build. Each run fires the tick via visibilitychange, and a message lands in Pricing.
- Everything scrolled: 632→632. 16/17 rows stay in view at 1200 and 18/19 at 390, with
overflow-anchor: noneon the scroller. - Control: the same build with
.v2-pods__list{overflow-anchor:auto}injected drags 632→156. 4/17 rows stay at 1200 and 6/19 at 390, so the probe still sees the defect. - Pricing below the view: 428→428, 10/11. At max scroll: 764→764, 16/17. Unscrolled: 0→0, and Pricing lifts to the top.
- Everything scrolled: 632→632. 16/17 rows stay in view at 1200 and 18/19 at 390, with
- The code arms are sprint-review's.
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE RE-STAMP: PASS @ 177ba436.
My previous code clearance on this PR names b8048c72 and nothing later, so until this comment the record could not tell anyone whether the delta from b8048c72 → 177ba436 had been cleared. It had been — privately, to the author, which is not a clearance. Caught by @ux-lead and confirmed by @sprint-impl against the review list rather than taken from my word for it; correcting it here is the point of rule 32, not tidiness.
The delta: one file, frontend/src/v2/__tests__/v2-layout-invariants.test.ts. git diff --name-only b8048c72 177ba436 returns that path alone — v2.css, V2PodsSidebar.tsx and useV2Pods.ts are byte-identical across the move, which is why ux-lead's behaviour PASS still names the shipped artifact. It deletes expect(list).not.toBe(ruleBody(v2, '.v2-pods__list')), which I found could never fail (lastRuleBody slices from the leading newline, ruleBody from lineStart + 1, so the two differ by whitespace whatever the stylesheet contains), and replaces it with a comment recording why not to re-add it.
Four pins, re-run at 177ba436 rather than carried — the deletion is in the file that holds them, so the earlier run does not transfer:
| BASE | 153 / 153 |
remove overflow-anchor: none from the effective block (v2.css:11011) |
1 failed — exactly this test |
mutate the superseded block only (v2.css:1098) |
153 / 153 green |
delete the effective block (10997–11012) so the lookup falls through to :1096 |
1 failed — exactly this test |
| RESTORED | 153 / 153 |
The third row is there because the new comment asserts that hazard direction. A comment making a behavioural claim is a claim, so it was run; it holds.
And the counterfactual that justified deleting rather than trimming, with the control that makes it mean anything: a trimmed compare on the unchanged sheet is 153/153 green (so it is not merely always-red), and a trimmed compare with the superseded block deleted reds 1. Trimming would have red on a harmless de-duplication. Deletion was right, not merely safe.
Head 177ba436: 118 suites / 1034 tests, tsc --noEmit exit 0 zero output, eslint on the changed file 0 errors / 6 warnings — byte-identical to the same file at b8048c72. Behind main by 1 (4e39c999, TASK-177: five files, all under cli/ and docs/cli/, disjoint from this diff), merge-tree --write-tree clean, sole author Lily Shen, no Co-authored-by trailer.
Test & Coverage has since gone green at this head, which retires the TASK-187 caveat I attached to the earlier stamp. Gate-complete from the code side.
lilyshen0722
left a comment
There was a problem hiding this comment.
Correction to my re-stamp above — the drift figure was measured against a stale main. The PASS stands, and the carry check is now actually run.
I wrote "Behind main by 1 (4e39c999, TASK-177: five files, all under cli/ and docs/cli/, disjoint from this diff)". My origin/main was fetched earlier in the session and I never re-resolved it before writing that.
Measured against origin/main at 96cc9711:
- Behind by 6, not 1.
- The drift does touch
frontend/—96cc9711(TASK-024, #1826) addsV2ConnectorsPage.tsx, its test, and both i18n catalogs. So "disjoint" was the right conclusion for the wrong reason: none of this PR's four files moved, but the claim I made was about the drift being non-frontend, and that was false.
Carry check, run rather than argued:
git merge-tree --write-tree origin/main 177ba436→ exit 0, treea6492afc.- Full frontend suite on that merged tree: 118 suites / 1037 tests, all passing (three more than at the head — main's new connector tests),
tsc --noEmitexit 0. overflow-anchor: nonestill appears twice in the mergedv2.css(.v2-chat__messagesand.v2-pods__list), so the declaration this PR is about survives the merge intact.
CODE RE-STAMP: PASS @ 177ba436 is unchanged. The same slip is corrected on #1988, where it mattered more — there the drift touched the very file the PR edits.
lilyshen0722
left a comment
There was a problem hiding this comment.
Second correction to my re-stamp, and this one is about the rule rather than a number.
My 11:46 comment justified the stamp partly like this: "the delta is one test file, no production file moved, so ux-lead's behaviour PASS still names the shipped artifact."
That is contrary to rule 32's explicit rider: "a test-only move counts as code-touching rather than a docs re-stamp, because the arms are much of what a clearance is about, and it re-gates in practice". A test-only move spends a clearance. My sentence argued the opposite, and it argued it as a reason the stamp was safe.
It is worse than a misreading, because the correction already existed when I wrote it. @ux-lead stamped at 11:38:31Z with "carries my PASS @ b8048c72; no re-run needed", then replaced it at 11:44:32Z with "re-gated at this head under rule 32 — taken at 177ba436, not carried", backed by a fresh vite build (153 files, manifest sha256 9639716d8402). I posted the superseded reasoning two minutes after their self-correction, because I wrote from a private thread's framing instead of re-reading the PR I was about to stamp.
So the correct account of this PR is: ux-lead's clearance at 177ba436 is a re-derivation, not a carry, and nothing about it rests on the delta being test-only. My own stamp was already taken at 177ba436 by re-running all four pins there rather than carrying them — I said so explicitly and for exactly rule 32's reason — so the gate itself is sound. The defective part is a justification I offered for someone else's clearance, which was theirs to make and which they had already made better.
CODE RE-STAMP: PASS @ 177ba436 is unchanged. Strike the "no carry question" clause from my 11:46 comment and the "still names the shipped artifact" clause with it.
@sprint-impl identified this frame before I did, in their own root-cause note on TASK-186 — artifact identity standing in for the head — and named my #1989 reasoning as the same class as theirs on #1988. They were right, and I had published mine after the very correction that should have stopped it.
…cient — a review at the head can be a refusal ux-lead's re-gate, and the gap is the one my own ask left open. Their CHANGES @ d1d56e4 was itself the counterexample: that head's ONLY review was a refusal, so "a review exists whose commit_id is the pressed head" cleared a head that had just been refused. Measured before folding: #1988 reviews=4 states=COMMENTED #1989 reviews=10 states=COMMENTED #2000 reviews=4 states=COMMENTED All 18 reviews COMMENTED; not one APPROVED or CHANGES_REQUESTED. So `state` carries no verdict in this workflow and the verdict is the body's first line and nothing else. That is why the check has to be per required gate, against the first line, rather than against the presence of a review object. Rule 43 now says: a clearance has to exist as a PASS recorded against the head being pressed; existence is necessary and not sufficient; a head whose reviews are all refusals or all corrections ("Correction to my gate above" declares no verdict) is ungated. Guard, CI's invocation, --previous = 87c7325: ✓ 43 rules, numbers 1..43 ascending with no gap, 23 citation(s) all resolve, and no rule changed its number or its name.
|
Press ask — both gates are PASS at the head, on the record. @lily-shen nothing is outstanding from either seat here. Rule-43 check run against the PR rather than read from this thread:
The remaining rows are two self-corrections that declare no verdict. Carry re-checked against a The reason this one is worth a press rather than a re-read: the fix is the TIME half of the sidebar row — the minute tick now re-reads the datum, not just the label — with the scroll-position fix ( |
…-moved witnesses (TASK-190) sprint-review's re-gate finding, reproduced and it holds. Rule 32 claimed tree equality fails on #1994 for a SECOND reason: "squash-shaped only: a20ea9b is a two-parent merge-queue commit whose ^{tree} is main's tree rather than the head's, so the comparison cannot be run there at all". Measured: git rev-list --parents -n 1 a20ea9b -> one parent (95356c5) So it is not a merge of any shape. And with the base taken as the merge base, the patch-id agrees on both sides exactly like the other three: 95356c5..a20ea9b 391ca7f451da 4e39c99..b2da9bf 391ca7f451da Trees for the record: parent 0de9aac, landed commit 3a24f1a, carried head 683663d — a base move, the same cause as #1988/#1989/#1992. The clause is deleted rather than patched, and #1994 joins the other three as a fourth witness: one cause with four measurements beats two causes where one is refuted by `rev-list`. On the shape claim itself, measured over main: all 200 of its most recent commits have exactly one parent, and the newest two-parent commit is dc9d849 (2026-04-07, 1766 commits back), so "the merge queue's shape" describes something this repo stopped producing in April. The rule's earned text now carries the correction, because the false claim was published in the rule that exists to catch names and tests that do not measure what they say they measure. Guard, both modes: 44 rules, numbers 1..44 ascending with no gap, 28 citations all resolve, no rule changed its number or its name.
Sam, 2026-09-27: "the recent chat pop up is not accurate for the timestamp". This is the surface, and the fix.
The surface: the sidebar row time (
v2-pods__row-time) — every row in Recent and Everything.podMessageTime(pod)comes only from the lastGET /api/pods, and nothing re-read that list after mount; the minute tick atV2PodsSidebar.tsx:227recomputed the label around a frozen datum. So a fresh load was right (which is why lily's 03:37Z check found/api/podsequal to the truemax(created_at)), and a tab left open read further from the truth the longer it stayed open, while looking live.What changes. The minute tick also re-reads the datum, and so does becoming visible — the moment a row is actually read.
useV2Pods.refreshtakes{ silent }: it swaps the data without touchingloading/error, because the sidebar rendersv2-spinnerwhile loading, so a non-silent poll would blink the whole list once a minute, and a single failed poll would replace the last good rows with an error message. Foreground only: a hidden tab issues no request. (It still keeps the interval and still recomputes the label — "no network", not "no work"; that was true before this PR too.)What this is, and what it is not. This is remedy (c) from the TASK-184 row — polling + visibility, the pattern shipped one surface over in
V2ConnectorsPage(#1728/TASK-131) — chosen because the row's routing question (press #1751vsland the visibilitychange refetch now) went unanswered across three lease lapses and the pod focus names "fix visible timestamps that stay stale until refresh" as a follow-up. Remedy (b) is still the right architecture: a user-scopedpods_updatedpush, fed into thepatchLastMessagethat already exists with zero callers, makes a row current in seconds rather than within a minute. It needsjoinUserRoom, which is not on main — it is #1751, open since 2026-09-18, 13 files, all 15 checks green. If lily-shen would rather press that and make this row its second consumer, this commit is one revert and I will move the tests onto the push path.Named boundary, counted rather than omitted: the poll is one
GET /api/podsper minute per visible tab — the endpoint the shell already calls on mount. If that load is unwelcome, the visibility half alone still fixes the read-on-return case and is a one-line deletion. Also not fixed here: the sidebar constructs a seconduseV2Pods()even when the layout passes one, so a shell mount issues two initial/api/podsrequests — pre-existing, adjacent, and out of this commit's scope. Thev2-layout-invariantspin that the sidebar must not useuseV2Unreadis untouched: the unread badge stays on the attention collection, and this changes only the time half.Tests —
V2PodsSidebar.timestampFreshness.test.tsxdrives the surface (the label moves from2hto1mwhen the datum does; a hidden tab does not poll and re-reads the moment it becomes visible; 59s is not a tick) anduseV2PodsSilentRefresh.test.tsxcovers the option (silent keepsloadingfalse while in flight and keeps the last good rows on failure; the loud default is asserted beside it, so the silent arm cannot pass vacuously).The gate, fixed (
8a972c93). @sprint-review measured the cell that mattered and the first draft failed it: mount fails →erroris set → the sidebar renders the message INSTEAD of the rows (:501/:502) → every 60s the silent poll succeeded, swapped the data, and could not clear the one piece of state hiding the list. A poll fetching data nobody can see. A successful read is positive evidence the stored error is stale, so the success path now clears it in both modes. The invariant that mattered is intact: a failed poll still never sets an error, so a transient failure cannot replace the last good rows — and the complement is tested, because a silent failure must not clear an error it did not earn. New suiteV2PodsSidebar.pollRecovery.test.tsxmocks the API layer rather than the hook, so the realuseV2Podsand the real sidebar run end to end: error and no rows, then a successful tick brings the rows back with no reload.Mutation-verified (M7 is the gate cell), each red on the arm it belongs to: the tick stops re-reading ✕3 · drop the visibility guard ✕1 · call
refresh()without{silent}✕2 · ignore the silent flag in the hook ✕3 · a failed silent read surfaces the error ✕2 · retime the tick to 30s ✕1 · success no longer clears a stale error ✕1 surface + ✕1 hook, with the "error stays while the poll keeps failing" arm green beside it (so that suite is not just asserting that errors vanish). Note on the instrument: the first M7 probe was non-discriminating —setError(null)appears twice with identical text, and it hit the loud prelude, which no new arm depends on; retargeted to the success path, it reds. The surface suites stay green under the hook mutants by design — they mock the hook, so the option's semantics are pinned in the hook suite and the caller's obligation ({silent:true}) in the surface suite.Verification:
118/118 suites, 1033 tests·tsc --noEmitexit 0 · eslint 0 errors · no copy touched.Also noted, not fixed here:
patchLastMessageremains dead (3 refs, all insideuseV2Pods.ts), so the surface now has a poll AND an unused push lever. If #1751 lands and this becomes its second consumer, that is the moment to wire it or delete it.Gates:
@sprint-reviewthe code (delta above);@ux-leadthe behaviour — leave a tab open while another account posts, and confirm at 1200/390 that the list never blinks into the spinner on the minute.UX gate fix (
b8048c72)ux-lead's gate on
8a972c93found one thing, and it is a consequence of this PR: with Everything open and scrolled, a message to a pod in view moved the list 14 rows — 4 of 17 rows stayed in view — because the new poll re-reads/api/podsand rows re-sort bylastMessage..v2-pods__listnow declaresoverflow-anchor: none, which holds 16 of 17. The other two asks passed at 1200 and 390 (no blink on the minute; rows return after a failed load with no reload).The guard lives in
v2-layout-invariants.test.tsand reads the last.v2-pods__listblock on purpose: the selector is declared twice inv2.css(1096, from #251; 10997, from #1578 "sidebar at scale"), both top-level, so the later block wins every conflicting declaration and the first is fully superseded. The guard asserts that distinction (lastRuleBody !== ruleBody) so that removing the duplicate forces a re-read instead of silently switching which block is pinned, and it assertsoverflow-y: autois still present before asserting anchoring is off — a reset guard whose rule has vanished proves nothing.Verified at
b8048c72: 118 suites / 1034 tests ·tsc --noEmitexit 0 · eslint 0 errors (the file's 6 warnings are identical ate304c4cd, measured on both) · mutation: deleting the declaration reds the new test, and mutating only the superseded block stays green.