Skip to content

fix(desktop): keep Workbar visibility until the first session list lands - #5951

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:fix/session-workbar-reload-flake
Oct 3, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:fix/session-workbar-reload-flake

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

e2e/session-workbar.spec.ts:179 ("Terminal survives navigation and reload…") is flaky on main. It always fails at line 224: after page.reload(), the owner Session's right Workbar comes back collapsed, so the terminal region never appears. This is a product bug, not a harness problem. A reload can drop every other Session's per-Session Workbar visibility.

The cause is a race at startup. After a reload, a sessions:changed event from a Session whose turn is still finishing (the test does not wait for the replacement Session's turn to end) is read by the patch drain. catalog.commitPatch commits it before bootstrapSessions()'s sessions.list() does. commitPatch moves the catalog revision off 0, and selectAuthoritativeSessionIds read any revision > 0 as a membership observation. It therefore published a one-row set. useWorkbarLayoutState dispatched retain-sessions with it. No Session is active yet at that point, so every other Session's collapsedBySession entry was dropped, and the right-visibility effect persisted the loss to maka-session-workbar-collapsed-v2. The rate went from 9/40 at 8ad836c to 19/40 at 255ae23 (Fisher p = 0.034) but the root cause is older. Row patches have been able to land before the list since #5532. The range 8ad836c..255ae23 changes nothing on the catalog, Workbar layout, main, preload or runtime path, so #5934/#5935 only moved the timing.

The fix: the catalog now records listed, meaning a list read has committed. selectAuthoritativeSessionIds keys on it instead of revision. So does the commitSessions no-op check. Without that second change, a first list that only repeats rows patches already admitted would return early and never mark the catalog as listed. The E2E test is unchanged.

Refs #5936 (where the flake was raised)

Root cause evidence

A diagnostic copy of the test, not committed, hooked localStorage.setItem and sessions.subscribeChanges through context.addInitScript. It ran 12 times on 8ad836c and 12 times on 255ae23:

first sidebar render after reload collapsed-v2 rewritten to the replacement Session only
7 failing runs 1 row (the patched replacement), full list 50–70 ms later 7/7
17 passing runs 2 rows (the list) 0/17

Verification

All Electron counts below were run on one machine, alternating builds in ABBA order, 8 rounds × --repeat-each=5 per build. The command was npx playwright test --config e2e/playwright.config.ts e2e/session-workbar.spec.ts:179. Every failure was at line 224.

build failed
8ad836c (before #5934/#5935) 9/40
255ae23 (main after #5935) 19/40 (p = 0.034 vs 8ad836c)
255ae23 20/40
255ae23 + this fix 0/40 (p = 7.8e-8)
229e1b4 (main after #5937) 13/40
229e1b4 + this fix 0/40 (p = 7.6e-5)
  • Two node-tier tests in workbar-model.test.ts. Each was mutation-checked: restoring revision > 0 in the selector fails the first test, and restoring it in the commitSessions no-op check fails the second. Each mutation fails only its own test.
  • npm --workspace @maka/desktop run test:dist: 3208/3208 on 229e1b4
  • typecheck (preload/main/renderer/storybook), npm run lint, check-renderer-architecture --base upstream/main --strict-base, check:app-shell-hooks, check:e2e-budget (34 tests, unchanged), check:asf-headers: all pass.
  • Full Electron suite on 229e1b4 + this fix: 34/34

Review focus

sessionCount in AppShell (authoritativeSessionIds?.size ?? 0) now stays 0 until the first list commits, even if a row patch lands first. Before, it briefly read 1. It feeds isOnboardingLoading, showOnboardingHero and getOnboardingActivationCandidate. The list normally lands tens of milliseconds later, and an unhydrated catalog is exactly the state these readers already handle.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Claude Opus 5.5) measured the flake, wrote the diagnostic, wrote the fix and tests, and ran the verification. The commit carries Generated-by: Claude Opus 5.5.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

After a reload, a sessions:changed event for a Session whose turn is
still finishing can be read and committed through commitPatch before
bootstrapSessions' sessions.list() commits. commitPatch moves the
catalog revision off 0, and selectAuthoritativeSessionIds read any
revision above 0 as a membership observation, so it published a one-row
set. useWorkbarLayoutState then dispatched retain-sessions, which
dropped every other Session's right-Workbar visibility (no Session is
active yet at that point) and persisted the loss. The Electron test
session-workbar.spec.ts:179 caught it as a collapsed Workbar at line 224.

The catalog now records whether a list read has committed. The selector
and the commitSessions no-op check key on that flag instead of the
revision, so a first list that only repeats rows patches already
admitted still counts as the observation. Row patches can precede the
list since apache#5532.

Generated-by: Claude Opus 5.5
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Oct 3, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6a272c3ed4bb6b306e109936a8c9a8b1e06ee24f (2 files, +46/−5, one commit).

No P0–P3 findings.

The bug, and why the fix is at the right level. The "have we had an authoritative observation yet" test was state.revision > 0, but a row patch also advances revision while only vouching for its own row. So after a targeted patch and before any list read, selectAuthoritativeSessionIds would hand membership readers a set derived from a partial catalog — which is what let Workbar visibility settle before the first session list landed. The fix replaces the proxy with an explicit listed flag that only a list read sets:

  • The publish guard no longer swallows the first list: sameRows && removedIds === current.removedIds && current.listed — so the first list always publishes, even an empty one, and even one that happens to match the rows patches had already admitted.
  • selectAuthoritativeSessionIds becomes state.listed ? new Set(...) : undefined, with the comment stating the reason precisely: neither the initial empty catalog nor rows admitted by targeted patches before the first list can prove that persisted sessions were deleted.

That is the same shape as the rest of this series — replace a signal that merely correlates with the truth by one that is the truth — and the flag is monotonic (set to true in the list commit, false only in the initial state), which is correct: an authoritative list, once observed, stays authoritative.

Two checks beyond the diff. The old proxy has no remaining users anywhere in the desktop or package sources (the surviving revision > 0 hits are an unrelated CLI test and sqlite schema constraints), so the retirement is clean rather than half-done. And the new tests cover the edge that the old guard would have swallowed: "treats a first list that matches the patched rows as authoritative", alongside one covering the other-sessions case.

Gate on this head: test and label are completed/success; mergeable is true, and the base is current main. No Grok involvement.

What I could not judge

  • I did not run the suite; the above is read from the diff, the two new tests' names, and the proxy check.
  • No desktop run, so the visibility timing itself is verified at the state-contract level rather than observed.

I did not approve, request changes, or merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at @Astro-Han's explicit request: no blocking findings in our automated reviews of this head, and CI is green.

@Astro-Han
Astro-Han merged commit b088a56 into apache:main Oct 3, 2026
2 checks passed
chihumyum added a commit to chihumyum/maka that referenced this pull request Oct 3, 2026
…composer-read-side

apache#5952 (R2 M5 closure) and this branch both cut the AppShell hook gate
and rewrote retained-root rows:
- Hook gate: both removals apply. `useEffect` (apache#5952) and
  `useAppShellTurnPresentation` (this branch) are gone: 19 hooks / 24
  call sites, matching the merged tree.
- Retained-root table: apache#5952's rewritten rows are the base. This
  branch's removed hooks drop out, and its three cross-region rows
  replace apache#5952's M3 rows. Those rows no longer list the transcript's
  model labels and model picker, which apache#5952 removed from ChatView. The
  Session-workspace row names the Composer edits instead of the ref,
  and the bootstrap row drops the pending-ref cleanup this branch
  retired. No row is scheduled for removal.
- Exports table: apache#5952's two "stays" rows (OverlaysConsumer,
  ManualDiagnosticReportConsumer) and this branch's three are kept.
- Ledger: main's copy minus this branch's retired legacy paths and
  emptied ownership entry, regenerated.

Generated-by: Claude Opus 5.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants