Settings split pass 7: extract Import History markup (SettingsView 6.7k, -41% overall) - #153
Merged
Merged
Conversation
Import History was ~1,100 lines of state, effects and handlers living directly in SettingsView, interleaved with unrelated code: the AI debug-log fetchers and the whole backup/restore flow sat in the middle of it. Moving it out means lifting four separate ranges, not one. The dependency analysis is what made this safe to do wholesale. Of the ~50 component-scope bindings the section touches, exactly five cross the boundary — the import-queue provider, the router/searchParams pair used for deep links, and showToast with its translator. Those five become the hook's inputs; everything else was read only by Import History. `deleteTarget` is returned rather than kept private because the delete-confirm modal renders as a page-level overlay at the bottom of SettingsView rather than inside the section. Same feature, different place in the tree — worth returning one binding instead of relocating a modal and changing DOM order. The queued-row countdown ticker moved too. It lived a thousand lines away from the rest of the subsystem but only exists to re-render queue countdowns, so it went into the hook rather than re-exporting `setNowTick` to drive it from outside. As with the activity log, the JSX is untouched: hook fields are renamed back on destructure, and all eight diff hunks land above the first `return (` at line 3702. The 1,299-line markup move is the next pass. Two things worth noting for review: - The hook call sits just after `showToast` rather than where the state block used to be. `showToast` is a const arrow, so calling the hook at the old position was a TDZ error. Moving the call is the honest fix; a lazy `(msg) => showToast(msg)` wrapper would have hidden it. - The first cut returned all 51 bindings mechanically. Thirteen were dead at the call site, so they are private to the hook now. SettingsView 9,404 -> 8,215. Verified: typecheck, lint, 441 unit tests, full Playwright suite 45 passed including the settings structural net. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 2 of the Import History split. The 1,299-line section moves to ImportHistorySection, and the 27-line compact link card shown on the Settings landing page moves to ImportHistoryLinkCard. The section takes a single prop: the whole `useImportHistory` return value, as `ih`. The hook cannot be called inside the section, because the delete-confirm modal renders as a page-level overlay outside it and needs the same state — two call sites would mean two independent copies. One object prop beats threading thirty-one bindings through a signature. SettingsView keeps only the ten bindings its two modals actually use; everything else now flows through `ih`. Its `viewMode` gates stay put, per the section-level-gate rule in ./settings/README.md. Also moved: FILTER_META, STATUS_META, fmtQueueCountdown and pickSourceIcon into the section (nothing else referenced them), and fmtShortDate to ./settings/format.ts (four other call sites still need it). The two components deliberately share the `import-history` anchor id. They are mutually exclusive `viewMode` branches, so only one is ever in the document — worth stating because it reads like a duplicate-id bug. Verification. The before/after HTML diff needed real data to be worth anything: the canned fixture seeds apps but no imports, so the first run compared two empty states. The harness now creates a six-item import covering every status the UI branches on (imported / unmatched / error / queued / removed / skipped), expands the row so the item list and its per-item actions are in the tree, and freezes the queued-row countdown. Both surfaces then compare byte-identical at 4,205 chars with zero console errors. The only raw difference was the fixture's own generated row ids (`iti_…`), which change per seed run — normalised, not ignored. Also: typecheck, lint, 441 unit tests, full Playwright suite 45 passed, i18n parity, and all 17 translation namespaces in the new files resolved against locales/en.json. SettingsView 8,215 -> 6,711. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Pass 7 — Import History, stage 2 (markup)
SettingsView.tsx8,215 → 6,711 (−1,504). Completes the Import History split started in #152.What moved
ImportHistorySection— the 1,299-line review-and-retry surface.ImportHistoryLinkCard— the 27-line compact card on the Settings landing page.The section takes one prop: the whole
useImportHistoryreturn value asih. It can't call the hook itself — the delete-confirm modal renders as a page-level overlay outside the section and needs the same state, and two call sites would mean two independent copies of it. One object prop beats threading thirty-one bindings.SettingsView keeps only the ten bindings its two modals actually use. Its
viewModegates stay put, per the section-level-gate rule.Also moved:
FILTER_META,STATUS_META,fmtQueueCountdownandpickSourceIconinto the section (nothing else referenced them), andfmtShortDatetosettings/format.ts(four other call sites still need it).The two components deliberately share the
import-historyanchor id — they're mutually exclusiveviewModebranches, so only one is ever in the document. Worth stating explicitly because it reads like a duplicate-id bug.Verification — the first attempt proved nothing
The HTML diff needed real data to mean anything. The canned fixture seeds apps but no imports, so my first run compared two 877-char empty states and passed trivially. That would have been a green check that tested nothing.
The harness now:
Both surfaces then compare byte-identical at 4,205 chars, zero console errors on both sides.
One real difference showed up and was worth understanding rather than waving through: the fixture's own generated row ids (
iti_…) differ per seed run. Normalised, not ignored.Plus: typecheck, lint, 441 unit tests, full Playwright suite 45 passed, i18n parity, and all 17 translation namespaces in the new files resolved against
locales/en.json— the check that caught three wrong namespaces in #151.Phase 3 running total
main)−41% overall. Remaining sections are much smaller:
ai-summaries(804),reset(532),wayback-import(483),backup(354),sync-status(91).🤖 Generated with Claude Code