fix(chat): keep pull-request state in sync across sidebar and detail panel - #443
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/store/dashboardSlice.ts:151 -- False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound fix for a real desync, but the shape is a two-cache coherence protocol fragile enough to need its own runtime loop-breaker. Watch
Suggestions
[DESIGN-REVIEWED] 8c35ba9 |
d4c4260 to
f649bc1
Compare
Opus 5 Review — ✅ no blocking findingsReviewed No findings. Verdict recorded via the action's structured output for commit False positive or not applicable? A repository writer can comment: |
f649bc1 to
5a450fe
Compare
|
Windows shard failure: inherited from a stale base, fixed by rebase.
Nothing in this PR touches Rebased onto current Local gates on the rebased tree: pytest 17,165 passed, isort / flake8 / mypy (473 files) clean, |
Review round — disposition (head
|
Review round — disposition (head
|
Arbiter — ✅ no blocking findingsArbiter found no unresolved long-term items that require action before merging Second-order review for Review detailsI've read both files. The sub-threshold findings are: two GPT 5.6 line-level items (a frontend stale-field assignment and a function-local import), and the design reviewer's CONCERNS (a two-cache coherence protocol needing a runtime loop-breaker, eight coordinated module-globals, and undocumented GitLab CI semantic changes). Claude found nothing. Judging each against the narrow one-way-door / concrete-harm bar:
None of these lock in an expensive-to-reverse contract or trigger a security/data-loss/availability regression that this diff introduces. Arbiter-Verdict: PASS No sub-threshold finding meets the long-term-impact bar. Considered but not escalated
Suggested follow-ups (open as issues — non-blocking)
[ARBITER-REVIEWED] 8c35ba9 False positive or not applicable? A repository writer can comment: For a broader accepted-risk deferral, apply |
5a450fe to
6bb8210
Compare
Review round — disposition (head
|
6bb8210 to
38fdf99
Compare
CI note — two shard failures are unrelated flakes, re-run (head
|
38fdf99 to
9053d50
Compare
Review round — disposition (head
|
9053d50 to
81b33ce
Compare
Rebase + conflict resolution (head
|
81b33ce to
8419fa6
Compare
Review round — disposition (head
|
8419fa6 to
6f671f0
Compare
Review round — disposition (head
|
6f671f0 to
e23f0d9
Compare
|
Round: rebase to unblock Claude AI Review (no code change). Every mechanical check and GPT/Design were green; the only red was Diagnosis: the branch had drifted 39 commits behind Rebased onto current Gates on the rebased tree, all green: pytest 17,378 passed / 0 failed, isort, flake8, mypy (474 files) clean, |
…panel
The sidebar chips and the Changes-strip detail panel read two independent
caches that nothing invalidated on an agent turn, so they could render
different lifecycles for the same PR indefinitely:
- the full-payload cache (30s TTL) and the chip cache (60s TTL) never
cross-populated, so each could be "fresh" and still disagree;
- the chip sweep is TTL-paced and admits at most CHECK_STATUS_PENDING_MAX
URLs per round, so with more PR-linked slots than that cap a given chip
lagged by minutes;
- the detail query is staleTime: Infinity with no refetch triggers, so
after mount it only ever updated on a manual Refresh.
Unify the caches: a completed full fetch projects state/draft/checks onto
the chip cache (status_from_full_payload / record_full_payload_status),
and a chip refresh that observes a CHANGED status drops the now-stale
full payload so the panel cannot serve a lifecycle the chip has passed.
An unchanged status leaves the payload alone.
Refresh at turn boundaries: on the idle transition, DashboardState.
refresh_slot_source_status re-reads the finishing slot's chip URLs via
request_check_refresh_now, which bypasses the chip TTL. Gated on an owner
websocket being open (status is credential-backed and nobody else can
render it, so a headless gateway spawns no provider work), scoped to the
one slot, floored to one forced read per URL per 10s, and best-effort so
it can never break turn completion.
Push instead of poll: a changed status emits the owner-only source_status
WS event {url, origin, ci?, state?}. origin="chip" means the lightweight
path knows something the payload does not, so the client patches its
status batch AND invalidates the detail queries; origin="detail" means a
full fetch produced it, so the client patches chips only -- otherwise a
detail fetch's own echo would refetch the query that emitted it. The
client additionally invalidates the mounted pull-request queries on
chat_done for the active slot, since lifecycle/CI deltas do not cover
review comments or mergeability. Polling remains the safety net.
Tests: 8 backend cases for the projection, both write-through directions,
TTL bypass + force floor, sink dedup/failure isolation and ledger
bounding; 5 for per-slot scoping, owner gating, failure swallowing and
owner-only delta delivery; 14 frontend for delta parsing/merging and the
WS wiring.
e23f0d9 to
8c35ba9
Compare
|
Conflict resolved by rebase — the hook moved with main's refactor. Branch is single-author, so rebase (not merge). It was 14 commits behind and The collision was structural, not textual: main extracted the idle-transition finalization block ( Resolution: took main's refactored call site verbatim, and re-homed Checked the other two Pushed as Gates on the resolved tree: pytest 17,924 passed, isort / flake8 / mypy (481 files) clean, |
…panel (kirodotdev#443) The sidebar chips and the Changes-strip detail panel read two independent caches that nothing invalidated on an agent turn, so they could render different lifecycles for the same PR indefinitely: - the full-payload cache (30s TTL) and the chip cache (60s TTL) never cross-populated, so each could be "fresh" and still disagree; - the chip sweep is TTL-paced and admits at most CHECK_STATUS_PENDING_MAX URLs per round, so with more PR-linked slots than that cap a given chip lagged by minutes; - the detail query is staleTime: Infinity with no refetch triggers, so after mount it only ever updated on a manual Refresh. Unify the caches: a completed full fetch projects state/draft/checks onto the chip cache (status_from_full_payload / record_full_payload_status), and a chip refresh that observes a CHANGED status drops the now-stale full payload so the panel cannot serve a lifecycle the chip has passed. An unchanged status leaves the payload alone. Refresh at turn boundaries: on the idle transition, DashboardState. refresh_slot_source_status re-reads the finishing slot's chip URLs via request_check_refresh_now, which bypasses the chip TTL. Gated on an owner websocket being open (status is credential-backed and nobody else can render it, so a headless gateway spawns no provider work), scoped to the one slot, floored to one forced read per URL per 10s, and best-effort so it can never break turn completion. Push instead of poll: a changed status emits the owner-only source_status WS event {url, origin, ci?, state?}. origin="chip" means the lightweight path knows something the payload does not, so the client patches its status batch AND invalidates the detail queries; origin="detail" means a full fetch produced it, so the client patches chips only -- otherwise a detail fetch's own echo would refetch the query that emitted it. The client additionally invalidates the mounted pull-request queries on chat_done for the active slot, since lifecycle/CI deltas do not cover review comments or mergeability. Polling remains the safety net. Tests: 8 backend cases for the projection, both write-through directions, TTL bypass + force floor, sink dedup/failure isolation and ledger bounding; 5 for per-slot scoping, owner gating, failure swallowing and owner-only delta delivery; 14 frontend for delta parsing/merging and the WS wiring. Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
Problem
The sidebar session chips and the Changes-strip detail panel read two independent caches that nothing invalidated on an agent turn, so they could render different lifecycles for the same PR indefinitely:
_CACHE, 30s TTL) and the chip cache (_check_cache, 60s TTL) never cross-populated, so each could be individually "fresh" and still disagree._refresh_check_loop) is TTL-paced and admits at mostCHECK_STATUS_PENDING_MAX(16) URLs per round, backing the rest off a full TTL — so with more PR-linked slots than that cap, a given chip lagged by ⌈N/16⌉ minutes.staleTime: InfinitywithrefetchOnWindowFocus/refetchOnReconnectoff and no interval, so after mount it only ever updated when the user hit Refresh.Net effect: a PR that merged, went red, or gained review comments mid-session stayed visibly wrong on at least one of the two surfaces.
Change
1. Unified cache (both directions). A completed full fetch projects
state/draft/checksonto the chip cache (status_from_full_payload→record_full_payload_status), so the sidebar cannot render an older lifecycle than the panel it was just fetched for. Conversely, a chip refresh that observes a changed status drops the now-stale full payload, so the panel's next read can't serve a lifecycle the chip has already passed. An unchanged status leaves the payload alone (so this doesn't defeat the 30s cache).2. Turn-boundary refresh. On the idle transition in
_run_chat,DashboardState.refresh_slot_source_statusre-reads that slot's serialized chip URLs throughrequest_check_refresh_now, which bypasses the chip TTL. An agent turn that ran `gh pr create`, pushed a revision, or drove a review round is the moment the remote state most likely moved. Bounded by:_CHECK_FORCE_MIN_INTERVAL_SECS(10s); URLs inside the floor fall back to plain TTL pacing rather than being dropped;_CHECK_CONCURRENCYsemaphore still apply, so a forced round can never outgrow a paced one;3. Status deltas push instead of polling. When a URL's cached
{ci, state}changes, the owner-onlysource_statusWS event carries{url, origin, ci?, state?}(DashboardState.push_source_status, registered once as a delta sink at app wiring;_send_ws_ownersmeans it never reaches non-owner or app-token clients).The client invalidates the mounted pull-request queries (
['pull-request-source', url]/['pull-request-checks', url]) for every changed delta, regardless oforigin, and patches its status batch. This is required for cross-window convergence: adetail-origin delta is produced by one window's full HTTP fetch, so only that window received the fresh payload — other owner windows (whose detail queries arestaleTime: Infinity) must still invalidate to converge. This does not loop, becauserecord_full_payload_statusruns only in the uncached fetch path: the initiating window's redundant refetch hits the warm 30s payload cache and emits no new delta.origin("chip"vs"detail") is therefore diagnostic only — it records which path produced the delta and is retained on the wire for future requester-aware routing, but no client behavior branches on it today.origin: "chip"— a lightweight chip refresh learned something the full payload didn't know (and the gateway already dropped that payload).origin: "detail"— a full detail fetch's write-through produced the change.The client also invalidates the mounted pull-request queries on
chat_donefor the active slot, because lifecycle/CI deltas don't cover review comments or mergeability and the detail query would otherwise never refetch after mount. Only mounted queries refetch; the rest are just marked stale. Polling remains the safety net for a missed event.Files
src/kiro_crew/dashboard/handlers/source_providers.pyforce=/request_check_refresh_now, delta sink registry, force-ledger boundingsrc/kiro_crew/dashboard/state.pysource_link_urls_for_slot,push_source_status,refresh_slot_source_statussrc/kiro_crew/dashboard/chat_runner.pychat_done(not the mid-turn/compactflush)src/kiro_crew/dashboard/server.pywebsite/src/utils/pullRequestStatusDelta.tsparseStatusDelta(validates the wire payload as untrusted) +applyStatusDeltawebsite/src/hooks/useWebSocket.tssource_statuscase + turn-boundary invalidationdocs/system-specs/modules/learn-cron-dashboard.mdTests (27 new)
test_source_providers.py) — projection vocabulary across GitHub/GitLab spellings and failure dominance; full fetch → chip write-through withdetailorigin; chip change → full-payload drop withchiporigin; unchanged status preserves the payload; TTL bypass then force-floor suppression; refresh resumes after the floor; sink dedup + one broken sink not starving others; force ledger bounded by_trim_check_cache.test_dashboard_state_ws.py) — per-slot URL scoping and cap; owner-gated force refresh with the broadcast callback; no-op without an owner window; failure swallowed; delta serialized only to owner sockets.refreshing, records unseen URLs; WS patches the batch and invalidates the detail queries on every changed delta regardless of origin (chipanddetail) so other owner windows converge, ignores malformed deltas, refetches on active-slotchat_donebut not for a background slot.Verification
All local gates green: pytest 17,161 passed, isort / flake8 / mypy (473 files) clean,
tsc -bclean, vitest 4,493 passed across 389 files.4 backend failures are pre-existing host-environment issues, not from this change — the 3
test_dashboard_origin.py::TestParseDashboardUrlMalformedcases fail identically when run against unmodifiedorigin/mainsource (a local config default port leaks intoparse_dashboard_url), andtest_skills.py::test_flat_copy_untouched_when_nested_missingis the known locally-modified-skills-dir failure.Screenshots
Captured by
website/scripts/capture-pr-state-sync.mjs— it drives the real built SPA (website/dist) in Playwright with every/api/**call and the/api/wssocket answered from fixtures. No gateway, no dashboard token, no provider calls: only the network is stubbed, so the chips, the strip, and the delta handler are the production code paths. The cache states are staged deliberately (a naturally-caught desync isn't reproducible on demand) — stating that here rather than implying these were caught in the wild.The bug — one PR, two answers. Sidebar chip #443 still shows open + CI green while the detail panel beside it already knows the PR is Merged. Both caches were inside their own TTL; they simply never told each other.
After the write-through. The full fetch projects onto the chip cache, so the chip carries the same merge glyph the panel shows — and CI correctly drops off the chip once merged.
Delta pushes instead of polling. With the page already rendered and no poll due, one
source_statusframe for #409 (origin: "chip") is pushed into the websocket. The strip tab goes from open + checks-running to merged and the CI glyph disappears — previously this waited out a poll interval.Light theme parity for the synced state:
Full frame for context (dark, synced):