RFC: Unit Tests

[harshithmohan]

Unit Tests RFC

Summary

Shoko-WebUI has zero unit test infrastructure today: no runner, no *.test.* files, no test script in package.json, and no CI coverage gate. This RFC proposes adding a minimal, focused test layer aimed exclusively at regression protection for the modules that carry real, low-visibility logic. Tests exist to stop these complex things from breaking — never for coverage metrics; no coverage tooling or threshold will ever be introduced:

• Runner: Vitest (native Vite fit), node environment for pure-function tests, happy-dom only for Phase 3 effect tests.
• In scope: src/core/utilities/filterTree.ts (filter-tree parse/build round-trip), src/core/utilities/auto-match-logic.ts + auto-match-regexes.ts (the filename-parsing engine and its regex rule set, including Crc32Regex), and — as Phase 3 — the effect orchestration of useLinkWorkflow, useReleaseInfoForm, LinkFilesWithProviders.tsx, and Renamer.tsx.
• Explicitly out of scope: every other module — domain helpers, time/parsing utils, CleanDescription regexes, and the deprecated LinkFilesTab.tsx. These are either trivial one-liners or coupled to the Redux/axios/queryClient graph, and add maintenance cost without proportionate risk coverage. If they ever justify testing, they can be added incrementally under the same harness.

Motivation

Two modules concentrate the highest-risk, lowest-visibility logic in the codebase:

1. src/core/utilities/filterTree.ts is the round-trip boundary between the server's FilterCondition shape (used by Collection.tsx, the FilterSidebar, and saved presets) and the editable GroupNode/LeafNode tree held in Redux state. It is genuinely non-trivial: recursive parse/build, Not negation folding, same-expression sibling merging, unsupported-node preservation, and root normalization. A subtle break silently corrupts saved presets or the live collection query.

2. src/core/utilities/auto-match-logic.ts + auto-match-regexes.ts is a full filename-parsing engine (detectShow) driving a 13-rule regex set (PathMatchRuleSet) with named capture groups and a transform/validate post-processing pipeline (defaultTransform): drive-letter detection, episode-range parsing (E1-12), reversed-range swapping, season/version/crc32 extraction, show-name/release-group inheritance across parent/grandparent directories, and a null-vs-false transform control-flow contract. findMostCommonShowName (frequency counting + longest-common-prefix fallback) is the second substantive function. A subtle break here silently mislinks files to the wrong episodes/shows.

The neighboring filter.ts (buildFilter) is a 7-line recursion that semantically duplicates buildGroupChain inside filterTree.ts — it fails our own triviality filter and is out of scope.

3. Effect orchestration in useLinkWorkflow, useReleaseInfoForm, LinkFilesWithProviders.tsx, and Renamer.tsx. These components host subtle useEffect/useEffectEvent logic whose correctness depends on when effects fire, not just what their pure helpers return: dependency arrays that deliberately omit values, one-time init guards (initialClear, settingsRevision), debounced side-effects, and in-flight state-machine bookkeeping. A refactor that silently changes a dependency array or trigger condition is exactly the class of regression that pnpm lint/tscheck cannot catch.

Today a refactor to any of these has no safety net — pnpm lint (dprint + oxlint + stylelint) and pnpm tscheck catch syntax/types only, not behavior. Regressions are found by manual clicking.

Design

1. Tooling (minimal footprint)

• Vitest as runner (vitest). No @vitest/coverage-v8. Phases 1–2 run in a pure node environment with zero mocking (both in-scope modules import only types, plus lodash every/reduce in auto-match-logic.ts).
• Phase 3 harness (deferred): finalize when Phase 3 is started — see §5. Expected to need @testing-library/react (renderHook + act), happy-dom, vi.useFakeTimers, and boundary mocks for queryClient / Redux store / toast.
• Config: vitest.config.ts (or a test block in vite.config.mjs) with a single resolve.alias entry for @/ → src/. Default environment: 'node'.
• Scripts:
  • test → vitest run
  • test:watch → vitest
• CI: add a pnpm test step to the existing Lint-PR.yml workflow (decided — see Open questions: no new workflow, no extra install step since pnpm/setup already runs pnpm install when package.json is present). No coverage tooling or threshold — tests are regression guards only, never about coverage.
• Lint/format: test files are linted/typed by the same chain (see §4 follow-ups).

2. In scope, precisely

A. filterTree.ts. Tests assert returned values for representative inputs, including edge cases already observed in the wild:

• getWidgetKind — tag / multiPair / multi (Parameter === 'Number') / boolean selection.
• createLeafNode / createEmptyGroupNode — shape per widget kind (assert structurally, ignore id).
• parseFilterTree:
  • undefined → null
  • single atomic condition → wrapped as sole child of an implicit top-level And
  • Or-rooted preset → wrapped as sole child of an implicit And
  • And-rooted → returned as-is
  • Not(And(...)) → group negate toggle (rather than an explicit Not wrapper)
  • Not(atomic) → negation absorbed per widget kind (boolean value inversion; multi/multiPair negate flag; tag isExcluded)
  • unknown expression, or an expression with Left/Right slots → unsupported node, raw condition preserved byte-for-byte
  • same-expression sibling folding via mergeGroupChildren (HasTag(a) And HasTag(b) → single tag leaf with two tags)
  • single-child collapse (group with one child → the child)
  • scalar List<string> parameters (e.g. VideoExtensions) — leaf Parameter unwrapping
• buildFilterTree:
  • null / empty-children group → undefined
  • parseFilterTree → buildFilterTree round-trip equals the input for representative presets
  • negated group → Not(Left: ...)
  • multi match operator (And vs Or) → correct chain nesting shape
• findNodeById / findGroupById / removeNodeById — root vs nested lookup/removal, missing-id behavior.

B. auto-match-logic.ts + auto-match-regexes.ts. Tests drive detectShow / findMostCommonShowName through representative filename inputs, covering every rule in PathMatchRuleSet and every helper regex in defaultTransform:

• Crc32Regex (exported, tested directly) — 8-hex-digit capture with surrounding non-alphanumeric boundaries; match / no-match / case-insensitivity; embedded in a filename via defaultTransform (crc32 field extraction).
• Rules (one or more filenames each):
  • anti-timestamp — timestamp-formatted filename → invalidated (detectShow returns null)
  • raws-1 / raws-2 / raws-3 — space/dot/dash-separated raw release naming; resolution + release-group extraction
  • trash-anime — Show (Year) - S01E01 - 01 - EpisodeName [hash] shape; isSpecial (S00) handling
  • trailing-native-title — CR-style releases with a native title trailing after the codec
  • default / fallback — the general-case rules; isMovie / isMovie2 (gekijouban, ... the movie), part I/II, year capture, multi-episode ranges, version suffix
  • foreign-1 — Show - 1 「Title」 (sub)
  • brackets-1 / brackets-2 / brackets-3 — [Group][Show][Year][Episode] bracket shapes
  • reversed-1 — 05 - Show Name
• defaultTransform post-processing:
  • episode-range parsing + reversed-range swap (E1-12 → 12-1)
  • non-integer episode → Special (episodeStart/episodeEnd → 0)
  • detectEpisodeType precedence (isSpecial → isThemeSong → isOther → isTrailer → Episode)
  • theme-song (OP/ED/NC), trailer (CM/PV/trailer), extra (menu/web preview) detection
  • S\d+ season extraction, roman-numeral season (IV), year → (Year) parenthesization, TV/ - TV/ TV suffix normalization, S2 fold-in when no year
  • underscore/dot → space conversion, // hack, TrimShowNameRegex / ReStitchRegex movie-name re-stitching
  • movie numbering correction (episodeStart/episodeEnd → 1)
• DriveLetterRegex — drives the path-segment clearing in detectShow (trailing-newline / segment behavior).
• findMostCommonShowName — most-frequent name; all-once → longest-common-prefix via findSharedShowName; prefix fallback to first name; empty/null inputs.

C. Effect orchestration (Phase 3). Tests render a hook/component with renderHook and drive its dependencies with act to assert when effects fire — run counts, trigger conditions, dependency-array correctness, ordering, and the resulting dispatch/setLinks payloads. Allowed assertions are behavioral (not UI): toHaveBeenCalledTimes/toHaveBeenCalledWith on mocked dispatch, waitFor state convergence, and explicit "did not re-fire on X change" negative assertions. No DOM text/structure queries, no render-output assertions.

• useLinkWorkflow (src/hooks/utilities/useLinkWorkflow.ts) — the searching/submitting/fetching state machine: links iteration dispatch, in-flight Set dedup (no double-fire for an already-in-flight link), transition payloads on .then/.catch, and unmount cleanup clearing the in-flight sets.
• useReleaseInfoForm (src/hooks/utilities/useReleaseInfoForm.ts) — initForm fires exactly when show flips true (not on every render); isBulk-gated hasDifferent derivation (allSame per field); initialSeriesName via findMostCommonShowName.
• LinkFilesWithProviders.tsx — initializeLinks gates on isSettingsLoaded (settingsRevision > 0) and runs createLinksFromFiles once before setInitialized(true); the links.length === 0 && initialized → navigate guard; and handleSaveReleaseInfo's auto-match / range-fill episode assignment (specials counter, AUTO_MATCH_EPISODE_ID / RANGE_FILL_EPISODE_ID paths).
• Renamer.tsx — the debounced-config effect: clearResults fires exactly once per debounced config change and zero times on initial mount (the initialClear skip); the preset-change effect: fires on relocationPresetsQuery.data/isSuccess/settings changes but not when only the preset Name changes. (Confirmed — see Open questions: changeSelectedPresetEvent is a stable useEffectEvent deliberately omitted from deps, so testing when the effect fires on its real dependencies is the intended target.)

3. Explicitly out of scope (never required)

Per the RFC brief, reviewers should reject scope creep into:

• UI assertions — element position/layout, disabled/enabled state, text matching, snapshots, visual regression. This includes Phase 3: tests assert dispatched payloads and run counts, never rendered output or DOM text.
• E2E flows (Playwright/Cypress).
• Any module other than the files listed above — including filter.ts (buildFilter), util.ts time/parsing helpers, anidbUtils.ts link builders, getEpisodePrefix.ts, getEd2kLink.ts, releaseInfoHelpers.ts, releaseManagementHelpers.ts, CleanDescription.tsx (its BBCode/cruft-stripping regexes are deliberately excluded and remain untested), and the deprecated LinkFilesTab.tsx (parseLinks, autoFill, makeLinks).
• UI-interaction flows even within the in-scope components (button clicks, keybindings, drag/nav) — effects are tested by driving their dependencies, not by simulating user input.

4. Conventions

• Location: test files live separate from src, in a top-level tests/ tree mirroring src/ paths (tests/core/utilities/filterTree.test.ts, tests/core/utilities/auto-match-logic.test.ts, tests/hooks/utilities/useLinkWorkflow.test.ts, tests/pages/utilities/Renamer.test.ts). No colocated *.test.* files, no __tests__ dirs inside src.
• Naming: describe('<fn>', ...) / it('...', ...); one file per module under test.
• Phases 1–2: no network, no mocks — both in-scope modules import only types (and lodash), so tests hit nothing external.
• Phase 3 mocks at the boundary only: mock queryClient / @/core/store / toast; never mock the module under test. If effect logic can't be tested without mocking its own internals, that's a signal the effect is over-coupled and should be simplified or its derived data extracted.
• Determinism: assert structurally (ignore id fields) to avoid flaky assertions-by-id (decided — see Open questions). Do not stub crypto.randomUUID: that couples tests to generateNodeId's implementation rather than its contract and breaks silently if the mechanism changes. Use vi.useFakeTimers() for debounced effects.
• AGENTS.md guardrails apply: @/ alias imports, arrow functions, type over interface.
• Follow-up config changes required for tests/ to be treated as first-class code:
  • tsconfig: tsconfig.json currently has "include": ["./src/"] and "rootDir": "./src/". Add a dedicated test tsconfig (or extend include and drop rootDir) with the @/ alias so pnpm tscheck type-checks tests/.
  • oxlint: .oxlintrc.json has "ignorePatterns": ["*", "!src/", "!src/**"]. Extend to include !tests/ and !tests/** so pnpm lint lints test files.
  • dprint: .dprint.json includes is "src/**". Extend to tests/**.
  • lint-staged: lint-staged.config.js globs (*.{ts,tsx}) are path-agnostic and will match tests/** automatically once the oxlint/dprint scopes above are fixed.

5. Rollout (phased, low-risk)

• Phase 0 — infra only: add vitest devDep, vitest.config.ts + @/ alias, test scripts, CI step, and the tsconfig/oxlint/dprint scope extensions. Zero behavior change. Verifiable by pnpm test passing on an empty suite plus one smoke test (e.g. parseFilterTree(undefined, []) → null).
• Phase 1 — filterTree.ts: cover the round-trip, folding, negation, and navigation cases listed in §2A. This is the primary success criterion.
• Phase 2 — auto-match-logic.ts + auto-match-regexes.ts: cover the per-rule filename parsing, defaultTransform post-processing, and findMostCommonShowName cases in §2B. Needed before any refactor of the auto-match engine.
• Phase 3 — effect orchestration (optional follow-up): cover the run-count/trigger-condition/ordering cases in §2C for useLinkWorkflow, useReleaseInfoForm, LinkFilesWithProviders.tsx, and Renamer.tsx (in that order — cheapest mock surface first). Start with useLinkWorkflow to prove the harness before expanding; this is where over-mocking risk lives, so stop and retro after the first two. Tooling deferred: the exact harness (@testing-library/react + happy-dom + vi.useFakeTimers, with queryClient/store/toast mocked at the boundary) is deferred until Phases 1–2 prove out; finalize it when Phase 3 is actually started.
• Phase 4 — Dynamic UI generation (placeholder, nothing to do now): the Dynamic UI is currently being re-written server-side, and the schema the client receives (FormSchemaType / $ref resolution / x-uiDefinition) will change completely. Testing the current resolveSchemaRefs / Dynamic* renderer would pin a contract that is about to be discarded. Scope and targets will be defined once the new server schema lands.

Open questions (resolved per hidden4003's reply)

1. CI placement: add a step to Lint-PR.yml vs. a new Test-PR.yml workflow? — Decided: extend Lint-PR.yml. pnpm/setup@v1 already runs pnpm install when package.json is present, so no extra install step is needed; this reuses the same trigger and setup.
2. Determinism: stub crypto.randomUUID vs. assert-structurally-ignoring-id? — Decided: structural assertion, as proposed. Stubbing couples tests to generateNodeId's implementation instead of its contract and breaks silently if the mechanism ever changes.
3. Phase 3 React Compiler interaction: useEffectEvent callbacks (e.g. changeSelectedPresetEvent) are stable by design and omitted from dependency arrays — confirmed that testing effect behavior (not effect re-runs caused by handler identity) is the intended assertion target. Renamer.tsx's preset-change effect deliberately omits changeSelectedPresetEvent from its deps (with an existing oxlint-disable comment), so there is no reactive identity to test.

Alternatives considered

• No tests / manual QA only (status quo): rejected — filter-tree and auto-match logic already carry documented past defects and have no safety net; manual QA doesn't scale.
• Broader unit suite (util, domain helpers, CleanDescription regexes, buildFilter): rejected as over-broad — most candidates are trivial one-liners or coupled to Redux/axios, adding maintenance cost without proportionate risk.
• LinkFilesTab.tsx (parseLinks/autoFill/makeLinks): excluded — the file is deprecated and superseded by LinkFilesWithProviders.tsx. No tests will be written for it.
• Dynamic UI (resolveSchemaRefs, Dynamic* components) right now: not testable — the schema contract is being re-written server-side and will change completely. Revisit as Phase 4 once the new contract lands.
• Jest + RTL full UI suite / E2E (Playwright): rejected — heavier setup, and the brief explicitly excludes UI assertions; E2E needs a live server + seeded data and is a separate conversation.

[hidden4003]

Reply to RFC: Unit Tests open questions

1. CI placement: Lint-PR.yml already installs dependencies implicitly — pnpm/setup@v1 runs pnpm install by default when package.json is present, so no extra install step is needed. Given that, adding a pnpm test step to Lint-PR.yml is simpler than a new workflow, since it reuses the same trigger and setup.
Verdict: Extend Lint-PR.yml, not a new workflow.

2. Determinism: Assert structurally and ignore id fields rather than stubbing crypto.randomUUID — stubbing couples tests to generateNodeId's implementation instead of its contract. This also breaks silently if the ID generation mechanism ever changes.
Verdict: Structural assertion, as proposed.

3. Phase 3 useEffectEvent: Confirmed correct — Renamer.tsx's preset-change effect deliberately omits changeSelectedPresetEvent from its deps (with an existing oxlint-disable comment), so there's no reactive identity to test. Testing when the effect fires based on its real dependencies is the right and only meaningful target.
Verdict: RFC's assertion target is correct as-is.