Settings split pass 11: extract the five confirmation modals (SettingsView 3.9k, -66%) - #158
Merged
Conversation
Stage 1 of the last section. The AI Summaries card's state machine — provider/base-URL/model/key form state, the connection test, model discovery against a custom endpoint, the sample-policy dry run, the three per-phase request timeouts, and the debounced blob auto-save — moves to lib/use-ai-settings.ts. That is ~480 lines spread across six regions of SettingsView. The JSX is untouched: hook fields are renamed back on destructure, so all nine diff hunks land above the first `return (` at line 3037. The 791-line markup move is the next pass. Inputs are five: showToast, three translators used by the auto-save toast labels, the task-center handle, and loadSettings. Nothing else crossed the boundary. Three more candidates the identifier sweep suggested — `base`, `schedule`, `toast` — turned out to be matches inside comments. Several bindings leak deliberately and are returned rather than kept private: `aiProvider` (the timeouts panel disables its inputs when AI is off), `saveAiSettings` + `debugLogging` (the Developer Options debug toggle round-trips through this same blob), and the three `aiTimeout*` values with their auto-savers. `hydrate()` is the seam for the shared settings loader: it reads every settings key in one request, so it hands the AI slice over in a single call rather than this hook mounting a duplicate fetch. Four types move to ./settings/types.ts — StoredAiSettings and AiSamplePolicyResult, which both sides now need. The hook call sits below `showToast` and `loadSettings` rather than where the state block was, for the same TDZ reason as use-import-history: both are const arrows. SettingsView 5,643 -> 5,204. Verified with tsc, biome, 441 unit tests and the full Playwright suite (45 passed). Note for anyone verifying locally: `pnpm install` currently fails on this repo with ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION for ultracite@7.10.0, which arrived via the Renovate bump in #154 and is newer than pnpm's release-age cutoff. It is a local policy, not a CI one (#154 merged green), and it resolves itself as the package ages. Until then, `--config.minimumReleaseAge=0` on install and `--config.verifyDepsBeforeRun=false` on run get you moving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 2, and the last section. The 791-line AI Summaries card moves to AiSummariesSection, which takes a single prop: the whole `useAiSettings` return value as `ai`. The hook cannot be called inside the section — Developer Options needs the same blob for its debug-logging toggle and the timeouts panel reads `aiProvider`, so two call sites would mean two independent copies of the state. One object prop beats thirty-five. Its two sub-flags (`provider_selector`, `summarize_on_import`) are resolved in the section; SettingsView keeps the section-level `flag.settings.ai.enabled` gate, per the rule in ./settings/README.md. `hydrate()` now actually exists. The previous commit's docstring described it as the seam for the shared settings loader, but it was never implemented — the loader still called eight setters directly, which is why they were leaking out of the hook. It is implemented here and the loader calls it, so the destructure at the call site drops from 57 bindings to 14. Also removed: two params I threaded into the hook out of habit (`showToast`, `tToast`) that nothing in the moved code ever used, and nine translator declarations left stranded in SettingsView. Verified with the DOM diff, and this one needed a configured provider to be worth running: the card's `disabled` state is a fraction of its markup, so comparing that would have proved almost nothing — the same trap as the empty import list in the previous pass. With a custom provider, base URL and model set, both sides render identical at 7,491 chars with zero console errors. Also: tsc, biome, 441 unit tests, full Playwright suite 45 passed, and all 14 translation namespaces resolved against locales/en.json. SettingsView 5,204 -> 4,322. Every section is now extracted: 11,460 -> 4,322 across the phase, a 62% reduction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With every section out of SettingsView, the remaining JSX was the page shell plus five dialogs. Those move to RestoreBackupModal, DeleteImportModal, WaybackRemoveModal, RemoveItemModal and ResetAppModal — 453 lines, 5 to 13 props each. They stay rendered from SettingsView rather than moving into the sections they belong to, because they are page-level overlays: relocating them into a section would change their position in the tree and therefore their stacking context. Keeping the render site and moving only the markup is the change that is provably behaviour-neutral. Two types that were declared inside the component move out so both sides can name them: BackupRestorePreview to ./settings/types.ts, and the staged "remove from Apps" target as a named PendingItemRemoval export on use-import-history (it had been an inline object literal in the useState generic). Verified. Modals render nothing until opened, so a closed-modal diff would have proved nothing — the harness drives the UI to open each one before capturing. Reset App and Wayback Remove both open and compare identical at 1,465 chars, zero console errors. One namespace was wrong: I wrote settings.modals.wayback_remove for the wayback dialog when the real one is settings.wayback.remove_modal — it sits under the wayback namespace, not with the other modals. Caught by resolving every key against locales/en.json, which is the third time that check has caught a namespace typecheck could not. Also: tsc, biome, 441 unit tests, full Playwright suite 45 passed. SettingsView 4,322 -> 3,908. 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 11 — the five confirmation modals
SettingsView.tsx4,322 → 3,908 (−414).What moved
With every section out, the remaining JSX was the page shell plus five dialogs:
RestoreBackupModalDeleteImportModalResetAppModalRemoveItemModalWaybackRemoveModalThey stay rendered from SettingsView rather than moving into the sections they logically belong to. They're page-level overlays — relocating them into a section would change their position in the tree and therefore their stacking context. Moving only the markup, and keeping the render site, is the change that's provably behaviour-neutral.
Two types declared inside the component move out so both sides can name them:
BackupRestorePreviewtosettings/types.ts, and the staged remove-from-Apps target as a namedPendingItemRemovalexport onuse-import-history(it had been an inline object literal in theuseStategeneric).Verification
Modals render nothing until opened, so a closed-modal diff would have proved nothing — the same empty-state trap as #153 and #157. The harness drives the UI to open each dialog before capturing. Reset App and Wayback Remove both open and compare identical at 1,465 chars, zero console errors.
One namespace was wrong. I wrote
settings.modals.wayback_remove; the real one issettings.wayback.remove_modal— it lives under the wayback namespace, not with the other modals. Caught by resolving every key againstlocales/en.json. That's the third time this check has caught a namespacetsccouldn't, which is why it's in the README as a required step.Plus tsc, biome, 441 unit tests, full Playwright 45 passed.
Phase 3 running total
main)−66%. What's left is genuinely the page: the settings loader, the sidebar/hash plumbing, and the section composition — no more extractable units of this kind.
Next in Phase 3 is a different sort of work: the route split from
SECTION_GROUPSwith hash redirects (which also fixes the deep-link scroll race), theOnboardWizardsplit, CSS out of the 32k-lineglobals.css, and the 224-key flag audit.🤖 Generated with Claude Code