Skip to content

refactor(extension): unify Options wiring and settings normalization - #417

Merged
steipete merged 1 commit into
mainfrom
codex/options-settings-ownership
Sep 5, 2026
Merged

refactor(extension): unify Options wiring and settings normalization#417
steipete merged 1 commit into
mainfrom
codex/options-settings-ownership

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

One owner for Options wiring and settings normalization

Options previously unpacked its DOM collection and reconstructed large overlapping projections, then maintained nine copies of checkbox-controller wiring. It now passes the existing collection to narrow consumers and declares checkbox IDs, labels, and state keys once. Viewer-specific mappings and runtime-radio permission gates remain explicit.

Settings load and save share normalization without sharing the wrong responsibilities: legacy migrations remain load-only, managed policy is applied only to effective settings, unknown stored fields survive, and policy metadata is never persisted. The form and checkbox runtime use the same state type.

This removes 237 production lines and adds regression coverage, for a net reduction of 175 lines. There are no public API, dependency, or intended behavior changes.

Proof

  • Full project gate: 3,088 tests pass (43 skipped), 94.30% line coverage and 85.22% branch coverage.
  • All 49 focused Options/settings unit tests pass, including migration/policy/unknown-field round trips.
  • Supported Chromium suite: 3 native-transport checks and 113 HTTP-build checks pass (7 expected skips). The new browser test toggles every checkbox and verifies no other preference changes.
  • Extension builds and independent Codex review pass; no actionable P0–P2 findings.
  • The new checkbox test initially compared raw storage with effective defaults. Its baseline now comes from the rendered controls; the completed suite passes without production workarounds.

Before and after

Synthetic General-tab captures are byte-identical. The same inspected image therefore represents both before and after; only the build label was fixed to synthetic text for comparison.

Options before and after (identical)

Hosted CI passes on the exact PR head: Node 24 gate, Chromium E2E, and Firefox smoke, with security checks green.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 12:25 AM ET / 04:25 UTC.

ClawSweeper review

What this changes

Simplifies the Chrome extension’s Options wiring and shares settings normalization while retaining existing preferences, migrations, and managed-policy handling.

Merge readiness

Ready for maintainer review

Keep open as a useful maintenance refactor with no actionable introduced defects found. The consolidation is absent from current main and v0.21.11, and this owner-authored PR is protected from cleanup closure.

Priority: P3
Reviewed head: 770751f32eef448a9443f4a15fad9712d60c4d58

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving refactor with relevant regression coverage and no actionable correctness or security findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The ordinary contributor-proof gate does not apply to this OWNER-authored refactor, and no material authority change triggers additional proof; the reported Chromium test exercises Options controls through browser storage.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The ordinary contributor-proof gate does not apply to this OWNER-authored refactor, and no material authority change triggers additional proof; the reported Chromium test exercises Options controls through browser storage.
Evidence reviewed 8 items Repository policy: Read the full root AGENTS.md and CONTRIBUTING.md; no nested AGENTS.md or maintainer-note files were found in the affected trees. Applied the supported Chromium validation guidance.
Normalization preserves existing responsibilities: The complete introduced settings diff retains accepted values and fallbacks. Legacy migrations remain in loadSettings, policy is applied afterward, and saveSettings removes policy metadata before writing; no stored schema or default changes were introduced.
Control wiring and permission gates: All nine declarative checkbox entries match the former IDs, labels, roots, and state keys. The automation callback and both runtime-radio permission gates remain intact; Options consumers access the same elements through their existing narrow interfaces.
Findings None None.
Security None None.

How this fits together

The extension’s Options page edits preferences stored in browser storage. Loading normalizes those preferences and applies administrator policy before they control summaries, slides, and other extension behavior.

flowchart LR
  A[Stored preferences] --> B[Load and legacy migration]
  B --> C[Shared normalization]
  D[Options controls] --> C
  C -->|Save| E[Remove policy metadata]
  E --> A
  C -->|Load| F[Apply administrator policy]
  F --> G[Effective settings]
  G --> D
  G --> H[Extension behavior]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +219/-456 (net -237); tests +61/-0 The refactor reduces duplicated production wiring and normalization while adding focused regression coverage.

Technical review

Best possible solution:

Preserve the existing settings contract with one normalization implementation and explicit boundaries for legacy migration, persistence, and administrator policy.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this refactor does not report a behavioral bug, and source review found no introduced failure requiring reproduction.

Is this the best way to solve the issue?

Yes: reusing the existing element collection and sharing equivalent normalization removes duplication without introducing another settings interface or migration path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a940f4849aaf.

Labels

Label changes:

  • add P3: This is internal maintenance with no intended user-facing behavior change or demonstrated urgent regression.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The ordinary contributor-proof gate does not apply to this OWNER-authored refactor, and no material authority change triggers additional proof; the reported Chromium test exercises Options controls through browser storage.

Label justifications:

  • P3: This is internal maintenance with no intended user-facing behavior change or demonstrated urgent regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The ordinary contributor-proof gate does not apply to this OWNER-authored refactor, and no material authority change triggers additional proof; the reported Chromium test exercises Options controls through browser storage.

Evidence

What I checked:

  • Repository policy: Read the full root AGENTS.md and CONTRIBUTING.md; no nested AGENTS.md or maintainer-note files were found in the affected trees. Applied the supported Chromium validation guidance. (AGENTS.md:29, 770751f32eef)
  • Normalization preserves existing responsibilities: The complete introduced settings diff retains accepted values and fallbacks. Legacy migrations remain in loadSettings, policy is applied afterward, and saveSettings removes policy metadata before writing; no stored schema or default changes were introduced. (apps/chrome-extension/src/lib/settings.ts:380, 770751f32eef)
  • Control wiring and permission gates: All nine declarative checkbox entries match the former IDs, labels, roots, and state keys. The automation callback and both runtime-radio permission gates remain intact; Options consumers access the same elements through their existing narrow interfaces. (apps/chrome-extension/src/entrypoints/options/boolean-settings.ts:162, 770751f32eef)
  • Fresh settings and upgrade regression coverage: Existing coverage checks empty-storage defaults; the added round-trip test checks legacy runtime and CLI fields, unknown-field preservation, and exclusion of policy metadata. The new browser test independently toggles all nine checkboxes and reads their persisted values. (tests/chrome.settings.test.ts:18, 770751f32eef)
  • Current main still has separate normalization: The pinned main source retains separate load/save normalization, confirming that this consolidation remains distinct work. (apps/chrome-extension/src/lib/settings.ts:429, a940f4849aaf)
  • Latest release comparison: The v0.21.11 tag resolves to the supplied release SHA and also retains separate load/save normalization. (apps/chrome-extension/src/lib/settings.ts:512, 1dcbff752de7)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 49805c2 into main Sep 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant