test(web): lock in transcript search null/empty behavior (#908) - #1235
Conversation
Issue #908 marks "Null and empty search behavior is preserved" as an accepted criterion, but nothing enforced it. The perf work landed via merged PR #972 (ded0ecc) and is correct; however both components carrying it had zero test coverage: - InteractiveTranscript.tsx - 0 tests - TranscriptViewer.tsx - 0 tests Both ship in production via dashboard/panels.tsx. The refactor *added* the null guards (`seg.text ? ... : false`, `part ? ... : ''`) because nullish text was a live risk - so the guards were load-bearing from day one, yet a future refactor could drop them and every check would stay green. This is not a competing implementation. Behavior is byte-for-byte preserved; the logic is only relocated so it can be tested. Approach: extract the pure search logic into src/lib/transcript-search.ts and test it in vitest's `node` environment. apps/web/vitest.config.ts deliberately avoids jsdom ("Add a jsdom project later if/when component rendering tests land"), so introducing jsdom/RTL would be an unasked-for architectural change. Precedent for the extraction already exists: src/lib/timestamp.ts is shared pure logic imported by these same two components. Rejected alternative: replicating the predicate inside the test. That validates a copy rather than the shipped code - a vacuous test. Covered invariants (16 tests): - empty / null / undefined query returns ALL segments, never zero - nullish seg.text is a non-match, never a thrown TypeError - case-insensitive matching in both directions - speaker + search compose as AND - regex metacharacters are escaped ("a.b" must not match "axb") - invalid raw patterns ( "(", "[" ) do not throw - highlight regex omits /g so .test() cannot desync via lastIndex - query is lowercased exactly ONCE per pass, not once per segment That last one asserts the actual performance property PR #972 shipped, so a regression to per-segment normalization now fails the suite instead of silently costing N allocations per keystroke. Non-vacuous by negative control - each guard reverted individually in the shipped module: NC-1 drop `seg.text ?` guard -> 1 failed / 15 passed NC-2 drop empty-query guard -> 2 failed / 14 passed NC-3 drop regex escaping -> 2 failed / 14 passed NC-4 add /g flag -> 1 failed / 15 passed NC-5 normalize inside the loop -> 1 failed / 15 passed NC-6 drop null short-circuit -> 1 failed / 15 passed restore -> 16 passed A/B against a stashed baseline (apps/web, ambient AI_GATEWAY_API_KEY unset): baseline 44 files / 245 passed / 0 failed with changes 45 files / 261 passed / 0 failed Zero new failures. tsc --noEmit and eslint both clean. Refs #908, #972 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
Summary
Closes the single open gap on #908 by adding the regression coverage its
acceptance criteria claim, without touching the optimization itself.
This is not a competing implementation. #908 explicitly forbids one. The
perf work already landed on
mainvia merged PR #972 (ded0eccf3) and iscorrect — I verified both call sites hoist properly. Behavior here is
byte-for-byte preserved; the logic is only relocated so it can be tested.
The finding
#908 lists this as satisfied:
It is preserved — but nothing enforced it:
InteractiveTranscript.tsxdashboard/panels.tsxTranscriptViewer.tsxdashboard/panels.tsxThe refactor added the null guards (
seg.text ? … : false,part ? … : '')precisely because nullish text was a live risk — so they were load-bearing from
day one. A future refactor could drop them and every check would stay green.
Approach, and the constraint
apps/web/vitest.config.tssetsenvironment: 'node'deliberately:Introducing jsdom/RTL would be an architectural decision that isn't mine to make
unilaterally, and rendering tests aren't needed to lock in this invariant.
So: extract the pure predicate into
src/lib/transcript-search.tsand test it inthe node env. Precedent already exists —
src/lib/timestamp.tsis shared purelogic imported by these exact two components.
Rejected alternative: replicating the predicate inside the test. That
validates a copy rather than the shipped code — a vacuous test.
InteractiveTranscriptre-exportsTranscriptSegment, sopanels.tsxneeded nochanges.
Covered invariants (16 tests)
null/undefinedquery returns all segments, never zeroseg.textis a non-match, never a thrownTypeError"a.b"must not match"axb"(,[) do not throw/gso.test()cannot desync vialastIndexThat last one asserts the actual performance property #972 shipped. A regression
to per-segment normalization now fails the suite instead of silently costing N
allocations per keystroke.
Non-vacuous by negative control
Each guard reverted individually in the shipped module, not in the test:
seg.text ?guard/gflagEvery mutation is caught. No test passes for the wrong reason.
Verification
A/B against a stashed baseline (ambient
AI_GATEWAY_API_KEYunset for hermeticity):Zero new failures.
tsc --noEmitclean,eslintclean.Refs #908, #972