Settings split pass 8: extract the four remaining small sections (SettingsView 5.6k, -51%) - #155
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>
Sync Status, Backup & Restore, Wayback Import and Reset App move out
together — 957 lines of JSX. They are grouped into one pass because each
is small and independent, and splitting four ~10-prop components across
four PRs would be ceremony rather than review value.
AI Summaries is deliberately not here. Its 37 bindings want the same
hook treatment Import History got, which is a pass of its own.
StartOverButton moves into ResetSection (its only caller, and its
danger-zone sibling) and fmtRelative into SyncStatusSection (likewise).
The types the sections need — BackupRestoreStage, WaybackRunStatus,
WaybackProgress, WaybackLastRun, BackupSnapshot{Settings,Row,sPayload} —
move to ./settings/types.ts.
Every prop signature is typed from the actual `useState` declaration
rather than inferred from the JSX. Guessing produced 60 type errors on
the first pass: `restoreError` is `string` not `string | null`,
`waybackRunStatus` is non-nullable, `waybackControlBusy` is a union not a
boolean, `setBackupSnapshotSettings` needs the Dispatch form because the
JSX calls it with an updater, and `runBulkWaybackImport` takes an options
object. Worth stating because every one of those typechecks fine as a
plain `boolean`/`string` until the JSX actually uses it.
Two props I initially threaded were phantoms: `schedule` on SyncStatus
(the JSX reads `status.schedule`, and my analysis matched the word
"scheduled" in a comment) and `applyBackupSnapshotPayload` on Backup
(referenced only inside a comment). Both dropped.
Verified. Rendered all four sections before and after with their flags
forced on, and diffed the normalised HTML: identical at 7,355 chars,
zero console errors. Two harness artefacts showed up and were normalised
rather than ignored — the fixture's generated row ids, and the per-side
data directory that the backup card legitimately prints.
Also: typecheck, lint, 441 unit tests, full Playwright suite 45 passed,
and all 17 translation namespaces across the four files resolved against
locales/en.json.
SettingsView 6,711 -> 5,643.
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 8 — the four remaining small sections
SettingsView.tsx6,711 → 5,643 (−1,068).What moved
SyncStatusSectionBackupSectionWaybackImportSectionResetSectionGrouped into one pass because each is small and independent — four PRs for four ~10-prop components would be ceremony rather than review value.
AI Summaries is deliberately not here. Its 37 bindings want the same hook treatment Import History got, which is a pass of its own.
StartOverButtonmoves intoResetSection(its only caller, and its danger-zone sibling) andfmtRelativeintoSyncStatusSection(likewise). Seven types move tosettings/types.ts.Types come from the declarations, not from reading the JSX
Every prop signature is typed from the actual
useStatedeclaration. Guessing produced 60 type errors on the first attempt:restoreErrorisstring, notstring | nullwaybackRunStatusis non-nullablewaybackControlBusyisnull | "pause" | "resume" | "cancel" | "force", not a booleansetBackupSnapshotSettingsneeds theDispatch<SetStateAction<…>>form — the JSX calls it with an updaterrunBulkWaybackImporttakes an options objectWorth stating because each of those typechecks fine as a plain
boolean/stringright up until the JSX uses it.Two props I initially threaded turned out to be phantoms, both from the same over-eager identifier matching that has bitten this refactor before:
scheduleon SyncStatus (the JSX readsstatus.schedule; my sweep matched the word "scheduled" in a comment) andapplyBackupSnapshotPayloadon Backup (appears only inside a comment). Both dropped, and I verified against the original bodies rather than just deleting what the linter flagged.Verification
Rendered all four sections before and after with their flags forced on, and diffed the normalised HTML: identical at 7,355 chars, zero console errors both sides. All four confirmed present with real content first — the pass-7 lesson about empty states passing trivially.
Two harness artefacts surfaced and were normalised rather than ignored: the fixture's generated row ids, and the per-side data directory that the backup card legitimately prints in its snapshot path.
Plus typecheck, lint, 441 unit tests, full Playwright 45 passed, and all 17 translation namespaces resolved against
locales/en.json.Phase 3 running total
main)−51%. One section left:
ai-summaries(791 lines).🤖 Generated with Claude Code