test(desktop): cover composer mention catalog context guard - #4894
Conversation
|
@Astro-Han CI is currently blocked at
The same mismatch is present on current Could you take a look at the budget mismatch? I've kept that unrelated change out of this PR. TYSM! |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head c926fb260aee2914013965a239688618489df758. I found no P0-P3 issues in this test-only change.
The new apps/desktop/src/main/__tests__/composer-mentions.test.ts exercises the real ComposerMentionsProvider and reads the exposed catalog from a layout effect before the provider passive effect runs. It therefore pins the production timing contract: switching sessions immediately hides both populated and empty stale catalogs, while a same-context refresh retains settled skills. Independent mutation probes confirmed the tests fail if the context-key guard is inverted or if same-context refreshes clear the catalog.
Validation passed on the exact branch: clean install, build, Desktop typecheck, focused tests (26/26), full Desktop tests (2230/2230), renderer architecture (101/101), Biome/format, ASF headers, diff check, and the branch-local E2E budget. A clean synthetic merge with current main also built and passed focused tests (26/26), full Desktop tests (2289/2289), and architecture checks.
The required hosted test check is still red because the current merge tree contains an unrelated E2E budget mismatch in session-workbar.spec.ts (6 tests versus a budget of 5). The synthetic merge also exposes unrelated current-main Storybook type errors around WorkbarLayoutState.rightCollapsed. Neither path is touched by this PR, so I am not filing a PR finding for them, but the required gate is not green and this is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
…4895) `check:e2e-budget` and the Storybook typecheck both failed on `main` after #4694 merged: that PR added a sixth test to `session-workbar.spec.ts` without updating `e2e-budget.json`, and replaced `WorkbarLayoutState.rightCollapsed` with `activeSessionId` + `collapsedBySession` while #4877 had just landed `stories/app-shell.stories.tsx` building the old shape. Its CI ran on a base that predated both, so neither showed until merge (reported on #4894). The budget entry now records 6 tests and names the Electron-owned mechanism (per-Session collapse persisted across a renderer reload). The story builds the new layout state and reads the collapse bit through `isSessionWorkbarCollapsed`, as the controller does. No behavior change; no migration. Refs #4694, #4761, #4877, #4894 Generated-by: Claude Code
Test the first committed catalog after a session switch, before passive effects can reset stale state. Cover populated and empty catalogs, and preserve settled skills during same-context refreshes. Reuse the existing node:test and linkedom setup without production changes. Fixes apache#4892 Generated-by: OpenAI Codex
c926fb2 to
da01731
Compare
|
@Astro-Han Thanks for fixing the main-branch blockers in #4895! I've rebased onto The patch is unchanged: one test file covering #4892, with no production changes. Could you take a look when you have a chance? tysm! |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at da01731 against main (90014db94), +195/−0, one test file, CI green.
This is the test #4892 asked for, in the tier it asked for: ComposerMentionsProvider mounted through the existing node:test + linkedom path under src/main/__tests__, observing the first committed context after a Session switch before passive effects can hide stale Skills, from both populated and empty catalogs, plus the same-context refresh keeping settled Skills visible. The mutation checks in the body (inverted comparison, removed guard, cleared Skills on refresh) are the three regressions the guard exists to catch, and each is caught by at least one test. No production code touched.
Thanks for the report on the budget breakage too; #4895 fixed it on main.
AI-assisted review: drafted with Maka; I read the three tests against composer-mentions.tsx myself.
Summary
Fixes #4892
Add three regression tests for
ComposerMentionsProviderusing the existingnode:test+linkedomsetup:Production code, dependencies, compiler configuration, and test locations are unchanged.
Verification
npm --workspace @maka/desktop run build:main— passed.node --test --test-concurrency=4 dist/main/__tests__/composer-mentions.test.jsfromapps/desktop— 3 passed; also passed in 20 consecutive fresh processes.agent-graph-panelandcomposer-directoriessuites — 23 passed.npm run lint,npm run format:check,npm run build, andnpm run typecheck— passed.npx knip --workspace apps/desktopandnpx knip --workspace packages/ui— passed.Not run: full-repository
npm test, Storybook browser smoke, or Electron GUI E2E.AI use
Tool(s) and scope:
deepseek-v4-pro, reasoning effortmax): independent read-only review and focused test execution.Checklist
Does this PR entail a change in behavior?