test: cover the search row and version-history dialog#118
Merged
Conversation
Adds component-level testing. The suite was entirely logic-level — nothing
rendered a component — so the search-row behaviours had no coverage at all,
despite one of them being a regression caught by hand two PRs ago.
Infrastructure is opt-in per file: node stays the default environment so the
existing suites keep running DOM-free, and component tests declare
`@vitest-environment jsdom` in a docblock. The shared setup file guards
everything on `document` being present, so the node suites can load it
harmlessly.
Every test was mutation-checked — the implementation was reverted to the
broken form and the suite confirmed to fail:
- condition-triggered search open (`searchActive && !searchOpen`) fails
`closes with an active query and stays closed`, the regression that
made the row impossible to dismiss
- dropping the `/` render adjustment fails the shortcut test
- dropping `key={deckId}` fails the deck-switch test
That last one only fails after tightening the test. The first version
waited for the new deck's rows, which tolerated the stale window the key
exists to close; it now asserts against the previous deck's rows while the
new read is still in flight.
The same exercise corrected a comment: removing the `open &&` guard broke
nothing, because DialogContent already unmounts its children on close.
That unmount is what resets the state per open — the guard is redundant and
the comment now says so rather than claiming credit for the reset.
The lockfile in the previous commit was written by npm 11, which hoists vitest's nested @esbuild/* platform packages to the top level and drops the nested entries. CI resolves the tree with the npm 10 that ships with Node 22 and expects them where they were, so `npm ci` failed on every platform binary before a single test ran. package.json pins packageManager to npm@10.9.8 for exactly this reason. Regenerated with that version: the lockfile diff is now purely additive (755 insertions, 0 deletions) and `npm ci` reproduces the tree cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #117. The suite was entirely logic-level — nothing rendered a component — so the search-row behaviours had no coverage at all, despite one of them being a regression I caught by hand while writing #117. This adds the missing layer.
279 → 292 tests.
Infrastructure
Opt-in per file, so nothing existing changes:
environment: "node"stays the default; the 279 existing tests keep running DOM-free// @vitest-environment jsdomin a docblocktest/setup-dom.tsguards everything ondocumentexisting, so the node suites load it harmlesslytest/jest-dom.d.tsregisters the matcher types, since the runtime import is dynamic and invisible totscNew devDeps:
@testing-library/react,@testing-library/user-event,@testing-library/jest-dom,jsdom.Every test was mutation-checked
A green test proves nothing until it's seen to fail, so I reverted the implementation to each broken form and confirmed the suite catches it:
searchActive && !searchOpen(condition- not edge-triggered)closes with an active query and stays closed/render adjustmentopens and focuses the input when the / signal arriveskey={deckId}never shows the previous deck's rows after switching decksThe first is the #117 regression: it re-opens the row on the next render, making it impossible to dismiss.
The third only fails after tightening the test. My first version waited for the new deck's rows to appear, which happily tolerated the stale window that the
keyexists to close — the effect refetches on thedeckIdchange regardless. It now holds deck B's read open and asserts that deck A's rows are already gone.A comment this exercise corrected
Removing the
open &&guard from the dialog broke nothing — becauseDialogContentalready unmounts its children on close, and that is what resets the state per open. The guard is belt-and-braces. My #117 comment implied it was the reset mechanism, so it's been rewritten to describe what actually happens and to explain what thekeyseparately covers.That's the only non-test source change in this PR.
Checks
eslint --max-warnings=0clean · 292/292 tests ·tsc --noEmit·next build. Node suites are unaffected — full run is ~4s.