fix(checks): keep cached check details on screen instead of re-spinning by nplez1 · Pull Request #198 · nplez1/orca

nplez1 · 2026-10-28T03:38:35Z

What

Fixes #196. Expanding a check — a Jenkins build in particular — replaced the rows it already had with
"Loading check details…", and a poll that landed while a detail was open deleted its cache entry
before refetching, so the spinner came back on its own. Jenkins made it worse: details had no renderer
cache at all, and one expand is up to three main-process GETs.

loading now means refreshing. A payload stays on screen, a background read swaps in the new one,
and the spinner belongs only to a build nothing has read yet.

Changed behaviour

beginGitHubChecksTabDetails carries the payload it already has (or one the shared cache holds)
instead of nulling details. The spinner renders only when there is no payload; with one, a
"Refreshing check details…" status line sits in the meta row.
resetGitHubChecksTabForSource keeps detailsByCheckKey. A list refresh replaces every row
object, and clearing there re-read every open detail on the next render — the "spinner comes back by
itself" half of the report. Keys are the build's identity, so they survive a poll; a context change
(a push moves the head SHA, and the context key with it) still drops them.
The sidebar no longer deletes an entry whose status/conclusion moved. checkDetailsNeedRefresh
decides instead: re-read when the row moved since the read, or when a still-running build outlives
the checks TTL (the constant getPRChecksCacheTtl already used, now exported) — both behind the rows
already on screen. A finished build is never re-read, so re-expanding one is a pure cache hit.
A failed refresh keeps the rows it had and reports itself beside them rather than blanking the
pane (settleGitHubChecksTabDetailsKeepingShown).
New components/github/check-details-cache.ts — one bounded, module-level cache keyed by build
identity, so the PR page, the item dialog and the right sidebar share a payload instead of each
keeping a map that dies with the mount. This is the "add the Jenkins equivalent beside it so the two
surfaces share one store" option the issue offered.
One identity for all three keys. The cache and both surfaces' row keys now share
checkDetailsBuildIdentity: checkRunId → gitlabJobId → url → workflow-run:name → name. Ids lead
because a check run's url is its details_url, which an app can point several runs at; the
workflow-run branch carries the job name because a gh pr checks fallback row has only the run's id,
which every job in the run shares, while its payload is filtered to one job by name. One function
rather than three copies — [Bug]: Checks panel keeps a stale run after a push — workspace chip updates first; only a panel switch refreshes #102's root cause was reader/writer key drift.
useDelayedFlag defers the refreshing label ~200ms, per the styleguide's in-flight rule and
matching the full-details tab, so a fast read never flashes a label beside rows that are correct.

CheckDetailsLoadState gained readAt/readFor (the row state at the read) in both of its shapes; the
sidebar's old errorAt folded into them.

Not done

A settled build still costs one background read the first time a surface re-expands it: only the
local entry short-circuits the predicate, and a fresh mount has none. The rows render immediately
either way, so the read is invisible apart from the refresh label.
The checks Refresh button refreshes the list; it does not force a fresh detail read. An open
detail is re-read when its row moves or, for a still-running build, when the 60s TTL lapses.

Review nits, left deliberately

An id-keyed identity can collide across hosts (a gitlab.com job and a self-hosted job with the same
numeric id): the panel can briefly show the other's payload until its own read lands. Self-correcting
and capped, and closing it means parsing a host into every id key.
A failed refresh labels the kept payload with the new row state, spending the "row moved" trigger;
the payload is re-read on the next row change, the TTL, or Retry. The alternative polls a failing
endpoint once per row move.
A successful refresh that returns no inline details blanks the rows it was showing — the server's
answer is authoritative.
A retry after a failed first read shows no spinner by design: the error banner stays with its
"Retrying…" button, which already spins while loading.

Verification

pnpm test on components/right-sidebar, components/pull-request-page, components/github-item-dialog,
components/github — 3,210 tests passed (1 skipped), 0 failed.
New check-details-cache.test.ts (build identity: two runs sharing one details url, two jobs of one
run with and without urls; cache sharing; predicate matrix — row move, TTL, finished build, failed
read pinned until the row moves), new use-delayed-flag.test.tsx, github-checks-tab-state.test.ts
reworked to the new contract plus coverage for a failed refresh keeping its rows.
Regression tests for the reported repro: a Jenkins row re-expanded after collapsing renders its
stages again with no spinner and no second load (jenkins-check-row-details.test.tsx); a poll that
moves a row while its detail is open keeps the rows and refreshes behind them, and a failed refresh
keeps them too (checks-list-expanded-details.test.tsx).
pnpm tc:web clean; pnpm run check:code-quality:changed clean. Gate 1 is not required for this
diff — every changed path is under src/renderer, no global inputs.
Not exercised: a live Jenkins server (none reachable from here) and an Electron run of the built app.
Rendered behaviour is covered by the component tests above. The PR page and item dialog have no
component harness in this repo, so their share is covered at the state-helper level (begin / settle)
rather than end to end.

Review

Reviewed by Opus 5.5, high: R1 (1 blocking fixed), round 2 (1 blocking fixed), round 3 clean on R4

Round 2 found the key-order collision between a check run's id and its shared details_url; R4 applies
its prescribed ordering verbatim, pinned by the two identity tests above. Each round received only the
branch's own diff.

Expanding a check blanked the pane and showed "Loading check details…" every time, and a poll that landed while a detail was open deleted its cache entry before refetching, so the spinner came back on its own. Jenkins made it worse: it has no renderer cache at all, and one expand is up to three main-process GETs. A payload now stays on screen while a refresh runs, and `loading` means "refreshing": - `beginGitHubChecksTabDetails` carries the payload it already has (or one the shared cache holds) instead of nulling it; the spinner is rendered only when there is no payload yet, beside a subtle "Refreshing check details…" line when there is. - `resetGitHubChecksTabForSource` keeps its detail entries: a list refresh replacing row objects no longer re-reads every open detail. A context change (a push moves the head SHA) still drops them. - The sidebar stops deleting an entry whose status/conclusion moved. The shared `checkDetailsNeedRefresh` predicate decides instead: re-read when the row moved since the read, or when a still-running build outlives the checks TTL; a finished build is never re-read. - A failed refresh keeps the rows it had and reports itself beside them. - New `components/github/check-details-cache.ts` holds one bounded cache keyed by build identity, so the PR page, the item dialog and the sidebar share a payload rather than keeping three component-local maps. Refs #196

STYLEGUIDE's in-flight rule defers visible loading ~200ms; the full-details tab already does this. The inline panes showed "Refreshing check details…" immediately, so a local read that finished inside 200ms flashed a label beside rows that were already correct. Shared `useDelayedFlag` hook, used by the three inline surfaces. Refs #196

Review finding: a `gh pr checks` fallback row carries no checkRunId, only a workflow-run id that every job in that run shares, while the payload is filtered to one job by name — so two jobs of one run shared a cache key and could show each other's build. The url is per job, and it carries the host and repo, so it now leads the identity; a workflow-run key keeps the name. The same ordering bug was in both surfaces' row keys, so the shared helper now backs all three. Also from review: the failed-refresh merge moves into `settleGitHubChecksTabDetailsKeepingShown` (one tested helper instead of two hand-rolled ones), and the cache comment says what it actually saves. Refs #196

Delta-round finding: a check run's url is its `details_url`, and a third-party app can point several runs at one page — the Jenkins Checks plugin gives every run it publishes the build's url — so url-first made two runs, and two rows, share a key. Checks are back to id-first (check run, GitLab job), then url for the rows that carry no id at all, then the run id with the job name. Both cases are pinned: two runs sharing one details url, and two jobs of one workflow run with and without urls. Refs #196

) * fix(review): give the collapsed-thread bubble its gutter slot back The add-note "+" overlay and a collapsed PR review thread's bubble both claim the same gutter slot, so the "+" was drawn on top of the bubble and swallowed the click. Hovering a commented line therefore offered "add a note" and hid "there is a conversation here". The thread decorator now reports the lines it draws as bubbles and the note decorator's overlay skips them: one slot, one affordance, and the conversation wins. The Add Review Note chord still reads the editor selection, so a note can still be left on a threaded line from the keyboard — displaced, not dropped. Two fork-owned files split instead of growing past the max-lines budget: the line-set derivation and the overlay's own install effect move into their own modules. Closes #190. * fix(review): skip the "+" on threaded lines in the plain editor too Review round 1 on 68d29feb3e found the fix incomplete and the overlay fragile: - MonacoEditor's markdown annotations mount the same add-note decorator on the editor that carries the thread bubbles, so a markdown file with a collapsed thread still showed the "+" over the bubble. The thread decorator now runs first there and hands its occupied lines down. - Handing the occupied set in as an install dependency rebuilt the overlay whenever any thread anywhere was toggled or PR comments were refetched. That reinstall drops a gutter drag in flight and re-parks the button under a stationary pointer, so the running overlay is told through its handle instead, and the set is value-keyed like the commentable one. - A parked line that stops being ours (its thread collapsed) now hides the "+" instead of drawing it back over the bubble on the next scroll. - The test asserts the bubble decoration is really on the line, and that a thread opening does not rebuild the running button. * fix(review): hide a parked "+" whose line gained a thread, and pin the rest Delta re-check of d0e5714983 left three small gaps and two test holes: - the scroll handler skipped the "+" on a line that stopped being ours instead of hiding it, leaving a button parked at the wrong pixels for the wrong line; it now hides and clears the hovered line. - the occupied-line setter could hide the "+" mid-drag, where that button is the only sign the gesture is live; it stands down there. - the line-sets comment claimed a new set rebuilds the overlay, which the previous commit stopped doing. - the test now also covers a thread collapsing under a parked "+", the markdown editor's forwarding of the occupied lines, and the fake keeps one decoration record per collection so a clear by one collection cannot erase another's. * chore(lint): record the two flaky-test-pattern reds #198 merged The base commit 941acb7 lands two `check:flaky-test-pattern-ratchet` findings in its own test files, so the lint gate on any branch cut from it fails on work that branch never touched. Neither file is in this branch's diff, and both are static ast-grep findings rather than a bad run. Recorded with one narrow `match` per file, so a new finding in either still fails the gate, plus the evidence line LOCAL-PATCHES.md requires. The fix belongs to the checks-panel work: fake timers or a fixed timestamp.

The release gate on the merge found 140 failures across 25 test files. Three were merge damage in product code: - `updater-prerelease-feed.ts`: upstream's new `fetchLatestStableReleaseTag` hardcoded `stablyai/orca` in both the `releases/latest` URL and the redirect regex, so a fork build's stable-fallback probe never matched the fork's own redirect and every background check died in preflight. Retargeted to `nplez1/orca` with the rest of the file. - `use-checks-panel-ai-acknowledgement.tsx`: the resolution dropped the latest-consumer refresh ref both parents had, so a head-churn rerender bound the post-launch refresh to a stale `fetchComments` and the forced refresh was silently dropped. - `ai-vault-session-refresh.ts`: upstream declares the throttle test hook in this module; the fork's throttle lives in its own file, so the name is re-exported from where upstream's suites import it. The rest were expectations that predate the code they assert — the fork's own PRs (#134 refresh-on-refusal, #169 reply-only comment ack, #170 gh exec options, #198 keep-cached-check-details, #175 window binding) changed behaviour with `pr.yml` disabled and no unit lane backstopping them, so `main` was red before this merge. Each expectation now asserts the fork's contract; no test was deleted, skipped, or widened. Four reference repairs follow modules the merge moved (`filesystem-auth`, `filesystem-search-file-paths`), two fixtures gained fields upstream added, and two suites needed the WSL gate mocked so they stop spawning a real `wsl.exe` per case. `ws-inbound-message-queue.test.ts` is a GC-timing flake, not a fix: it passes 18/18 in isolation and the gate's isolation re-run reproduces it as green. Verified per file with `pnpm test <path>` — all 25 green — plus `pnpm tc:web` clean and the identity sweep still 0 fixable.

…s it moved LOCAL-PATCHES.md § Sync log gains the entry (three inline, the 2026-10-21 one rotated verbatim into local/sync/sync-log-archive.md), the convergence ledger gains the 2026-10-28 section with every question this sync settled, and BRANCHES.md records the new base. The lint baselines moved with the code: thirteen upstream-owned files over the max-lines budget are exempted by ownership, the focus-read and workspace-path baselines were pruned, `worktree-initial-terminal-close-records.test.ts` goes to three wall-clock reads for its restored TTL case, and `local/lint-known-red.json` follows upstream's step rename (`check:flaky-test-pattern-ratchet` folded into `lint:ast-grep-gates`), keeping only the two #198 foreign reds. Two fixes ride along, both found by the gate: - `check-flaky-test-pattern-ratchet.mjs` shells out to `git diff` with no `maxBuffer`, so a ~20k-path merge aborts with ENOBUFS and reads as a lint failure. It now uses the 256 MB ceiling `ast-grep-scan.mjs` already had. - Three jenkins suites carry a deliberate `// ast-grep-ignore: test-no-in-test-import-actual` with its reason: they redirect the real homedir. A duplicate import in `updater.publishing-window-feed.test.ts` is merged. Verified: gate-lint PASS (39 green, 1 failing = 2 known red, 0 new), oxfmt clean, fork-process-docs test green.

#247) * fix(lint): drive the two #198 flaky-pattern reds with fake timers `lint:ast-grep-gates` carried a known-red entry for each of two files PR #198 merged: four `Date.now()` reads in `check-details-cache.test.ts` and a 250 ms real-time wait in `checks-list-expanded-details.test.tsx`. Both are the fork's own work, and `local/lint-known-red.json` had them down as `foreign-red`. Both are now fixed where the ratchet asked for them: - the cache suite pins the clock with `vi.useFakeTimers()` + `vi.setSystemTime()`, which is what the TTL cases needed — `checkDetailsNeedRefresh` compares a payload's `readAt` against the wall clock, so they were really asking how fast the machine ran them; - the checks-list suite drives `useDelayedFlag`'s 200 ms deferral with `vi.advanceTimersByTimeAsync(250)` instead of sleeping 250 real milliseconds to catch a label a fast machine never shows. The claim recorded alongside the entries — that each `match` naming the rule and the path means "a new finding in either file still fails the gate" — was false: the ratchet prints one line per rule and file, and the regex is anchored on the rule and the path alone, so a fifth `Date.now()` in the same file was masked. The section in LOCAL-PATCHES.md now says so. * fix(test): route the two Bun-missed SQLite suites to Node `findSqliteRuntimeTests` classifies a test by the imports it names, and it cannot follow an indirect one. Two suites reach `node:sqlite` through modules it does not know — `tests/e2e/helpers/persisted-profile-state.unit.test.ts` imports the profile-state modules directly, and `src/main/ai-vault-search/session-search-indexer.test.ts` reaches the driver through the indexer, the schema and its fixture — so both ran in the Bun project. Bun's `node:sqlite` on macOS cannot open a WAL database read-only once `close()` has removed `-wal`/`-shm`, so they failed 2/3 and 1/42 there and pass 3/3 and 42/42 here. Both are now in `NODE_RUNTIME_INCLUDE`, with the reason, and `vitest-node-runtime-boundary.test.ts` pins the pair: nothing else would notice their removal, because the only other evidence was the known-red manifest. That also retires the two `Unable to read profile state database` entries. Their `match` string is what a genuine profile-state open regression throws, so while they were allowlisted the gate could not have seen one. * fix(unit-gate): catch a dead allowlist entry, and prune nine `local/gate-unit.mjs` derives staleness only from an entry that *matched a failure*, so an entry naming a test that no longer exists can never be reported: it sits in the manifest for ever, and only a 25-minute full run could have said anything about it — and that run derives nothing from an entry nothing matched. One such entry was there. `src/main/sqlite/sync-database-portability.test.ts > SQLite runtime contract rejects explicit undefined bindings before modifying rows` names a case upstream stablyai#34661 (`e5f1eef466`) deleted, because Node 26.10 binds an explicit `undefined` as NULL. Its recorded cause — "the bundled SQLite's own binding behaviour" — was wrong, and `LOCAL-PATCHES.md` now says what actually happened. So the title is now checked against its file, statically, at PR time. `titleExistsInSource` in `local/unit-gate-judgement.mjs` rebuilds a Vitest full name — the `describe` chain and the case title joined by single spaces — from the file's own title literals, treating an `it.each` substitution as a wildcard, and `tests/tools/unit-gate.test.mjs` asserts the real manifest has no dead entry. Nine entries are pruned with it, every one re-measured today: - the four topology cases run in the Node project already, since upstream stablyai#33533 (`a886dcd1fe9`) seated their fixture in the import scan; 3/3, 1/1 and 9/9 pass; - `profile-state-streaming-validation`'s rowid case was rebuilt by stablyai#34661 to use `CREATE TABLE` over `ALTER TABLE … ADD COLUMN`; 16/16 pass; - the two re-routed suites from the previous commit, and the now-deleted case. What remains is 8 entries, all `host-toolchain`; the `sqlite-binding` class is empty. Each prune carries its reason and the revision that closed it in `prunedSince`, which keeps `observedRun`'s arithmetic honest. * fix(unit-gate): close the review round on the guard and the timers Adversarial review of the branch (Opus, high, over 6d7cb78..HEAD) found nothing blocking and two should-fix gaps in the new guard, plus five MINOR and four NIT. Each is addressed here. Guard soundness — it reported live tests as dead in shapes this repo writes: - `$name` off an `it.each` table row is a substitution like `%s`; the comment claimed it and the pattern did not have it. `%$` too. - case tables are named `itRunsUnixShell.each(...)`, not `it.each(...)`, so the caller pattern now accepts any identifier starting with describe/it/test/suite. `src/main/cli/packaged-cli-assets.test.ts` is exactly that shape and would have reported its live title as dead. - `src/main/runtime/orca-runtime.test.ts` imports every case from `*.spec` modules, so its titles are in files the entry does not name. `source` is now the file plus those modules, via `importedCaseSpecifiers`. Guard precision — it called dead entries live: - `it.skip` / `it.todo` never run, so a title behind one can never be a known red. Both now read as absent. A title surviving only in a comment still counts as present, and the doc comment says so. Coverage for both directions (`tests/tools/unit-gate.test.mjs`, 29 → 31 tests): nested describes, `$name`, an aliased table, a generic `.each`, an apostrophe in a double-quoted title, a template-literal title, skip/todo, and a real spec-imported title that must be found through the follow and not without it. The deferred-label case also asserted only half of what its comment claims: it now proves "Refreshing check details…" is absent at 199 ms and present at 250 ms, which is the deferral `useDelayedFlag` exists for and costs milliseconds with the clock faked. Three LOCAL-PATCHES claims were wrong and are corrected: the prune split is seven re-routed and two upstream-rebuilt (not six and three), the rowid diagnosis was right about SQLite 3.54's `ADD COLUMN` refusal and wrong only about the cause being fork-local, and the `local(lint)` entry count is now zero. * fix(unit-gate): keep a literal %% in an each title a wildcard The delta re-check (Opus, high, over R1..R2) came back clean with one MINOR regression from that round's own fix: the widened substitution class dropped `%%`, which Vitest renders as a single `%`, so a title written `shows 50%% done` would have looked dead at `shows 50% done`. `%` is back in the class, and the test pins `%%` and `%$` so neither can be dropped again. The doc comment also claimed the comment-only shape was the *one* thing the check cannot tell apart. It is not: a title behind `(cond ? it : it.skip)('t')` is unresolved callee, not a skip, so it reads as present too. Both are stated.

nplez1 added 4 commits October 27, 2026 20:24

nplez1 merged commit 941acb7 into nplez1/main Oct 28, 2026

nplez1 deleted the nplez1/bug-jenkins-check-details-show-a-spinner-on-ever branch October 28, 2026 03:40

This was referenced Oct 28, 2026

fix(review): give the collapsed-thread bubble its gutter slot back #202

Merged

feat(checks): rerun Jenkins builds from the Checks surface #203

Merged

nplez1 mentioned this pull request Oct 28, 2026

fix(jira): stop a lane drop blanking the board while it revalidates #206

Merged

This was referenced Oct 29, 2026

Audit 2026-10-29 · Allowlist rot, and one untested security refusal #224

Closed

fix(test): clear the allowlist rot and the two false red claims (#224) #247

Merged

Navigation Menu

Repository navigation

fix(checks): keep cached check details on screen instead of re-spinning - #198

fix(checks): keep cached check details on screen instead of re-spinning#198
nplez1 merged 4 commits into
nplez1/mainfrom
nplez1/bug-jenkins-check-details-show-a-spinner-on-ever

nplez1 commented Oct 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nplez1 commented Oct 28, 2026

What

Changed behaviour

Not done

Review nits, left deliberately

Verification

Review

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant