Skip to content

test(ui): give the answer-identity guards teeth - #2950

Merged
Astro-Han merged 1 commit into
mainfrom
test/ui-answer-identity-teeth
Aug 13, 2026
Merged

test(ui): give the answer-identity guards teeth#2950
Astro-Han merged 1 commit into
mainfrom
test/ui-answer-identity-teeth

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #2946. Three test-layer findings from an adversarial review of that PR; no product code changes.

The regression tests guarded only half the fix. Both identity assertions read container.querySelector('.maka-assistant-answer') — the segment-level article, which pins the key half of the fix. The merged AssistantAnswerBubble renders a descendant carrying .maka-chat-message-bubble-assistant, and that is the node a text Selection actually lives in. Splitting the bubble back into separate streaming and historical component types remounts it while the article stays identical, so every test stayed green. The narrowed e2e now deliberately selects from a settled answer, so it does not cover that transition either. Test 1 now retains and asserts the bubble node as well.

The steered-turn docstring overclaimed. It said the test catches "an identity that is constant across segments — the sentinel-vs-real-id collision". It cannot: React reconciles duplicate-key siblings of the same component type by position, so isSameNode still holds and regressing the key to the collision-prone form leaves all three tests green. Corrected to state what the test really pins — two segments exist and each keeps its element across a settle.

The test.fixme could go green with the bug fully present. It asserted .maka-quote-actions is visible, and that state is reachable from two worlds: the Selection survived, or the quote hook's 350ms read won the race against the stream close and left a stale bar standing over text whose highlight the browser had already erased — verbatim the outcome the comment above the test calls unacceptable. Instrumented runs reproduced the underlying bug 7/7 while the body passed 6/7. It now asserts the contract that is actually broken: wait for the stream to close, then require window.getSelection() to still be uncollapsed. With retries: 0, un-fixme-ing the old body would have yielded either a false "gap closed" or a ~1-in-6 CI flake.

Refs #2946

Verification

  • npm --workspace @maka/ui run test — 136 passing, 0 failing.
  • Teeth check for finding 1 (required, not just asserted): temporarily split the bubble into a distinct component type at the TurnTimelineEntry call site, rebuilt, re-ran — the new bubble assertion fails (the answer bubble survives the turn settling) while the pre-existing article assertion still passes, confirming the old test could not see the split. Reverted.
  • Teeth check for finding 3: temporarily flipped test.fixme to test, ran playwright test e2e/quote-selection.spec.ts --repeat-each=3 against a fresh build:with-deps — the streaming-drag test failed 3/3, every failure at the post-close poll (not the pre-close one), i.e. the Selection is present right after mouse.up() and gone once the stream closes. The pointer-capture test passed 3/3. Restored to test.fixme.
  • npm --workspace @maka/ui run typecheck, npm run format:check — clean.
  • Not run: repository-wide suites (left to CI). apps/desktop/e2e/slash-command-menu.spec.ts is a known pre-existing flake tracked as flaky e2e: slash-command-menu inline menu shows the 命令 group for a non-leading slash #2948 and was not exercised here.

Review focus

Whether the two teeth checks are the right falsification for each guard — a test that stays green under the regression it names is worse than no test.

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

Generated-by: Claude Code (Opus 5). Authored under human direction; the human contributor reviews the final diff and owns the merge decision.

The identity regression tests asserted only on `.maka-assistant-answer`, the
segment article whose key the fix stabilized. The merged AssistantAnswerBubble
— the single component type that keeps the inner bubble mounted, and the node a
Selection actually lives in — was unguarded: splitting it back into streaming
and historical component types left every test green. Pin the bubble node too;
verified red under that split.
Correct the steered-turn docstring, which claimed to catch a key collision it
cannot. React reconciles duplicate-key siblings of one component type by
position, so the collision-prone key leaves all three tests green. State what
the test really pins: two segments, each keeping its element across a settle.
Point the streaming-drag fixme at the contract that is broken. Asserting the
quote bar is visible passes from two different worlds — the Selection survived,
or the hook's 350ms read beat the stream close and left a stale bar over erased
text — so the body passed 6 of 7 runs with the bug fully present. Wait for the
close, then require the Selection to still be uncollapsed: fails 3 of 3 today,
and can only go green on a real upstream fix.
Generated-by: Claude Code
@Astro-Han

Copy link
Copy Markdown
ContributorAuthor

Fast-path rationale

Test-layer only, two files, no product code — none of the protected areas, no user-visible behavior, and the reworked test stays test.fixme so it never enters the CI gate. Reverting returns the tree to the post-#2946 state.

Tests are a fast-path candidate rather than a free pass, so each guard was falsified rather than assumed:

  • Split AssistantAnswerBubble back into two component types — the new bubble assertion fails, the pre-existing article assertion still passes. That gap is what this closes.
  • Un-fixme'd the streaming-drag test, --repeat-each=3 — 3/3 red, all at the post-close poll, so it separates "nothing was selected" from "the close destroyed the Selection".
  • Finding 2 is a docstring fix; the point is that the test cannot catch what it claimed.

All required checks green. The fast path waives independent review, not the contributor of record's own review of the final diff.

@Astro-Han
Astro-Han marked this pull request as ready for review August 13, 2026 09:39
@Astro-Han
Astro-Han merged commit db77473 into mainAug 13, 2026
10 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Astro-Han