Skip to content

refactor: extract theme settings panel#166

Closed
ndycode wants to merge 1 commit intorefactor/pr1-route-fix-directfrom
refactor/pr2-theme-settings-split
Closed

refactor: extract theme settings panel#166
ndycode wants to merge 1 commit intorefactor/pr1-route-fix-directfrom
refactor/pr2-theme-settings-split

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 21, 2026

Summary

  • extract the theme-settings interaction flow from settings-hub.ts into a dedicated panel module
  • keep the existing settings behavior and CLI coverage intact while starting the settings-hub split in small slices

What Changed

  • added lib/codex-manager/theme-settings-panel.ts containing the theme panel prompt flow and its local action model
  • updated lib/codex-manager/settings-hub.ts so promptThemeSettings(...) delegates to the extracted panel module
  • preserved existing preview, reset, save, and cancel behavior by passing the current helpers and constants into the new module

Validation

  • npm run test -- test/codex-manager-cli.test.ts
  • npm run lint
  • npm run typecheck
  • npm run build

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert 51aab76 to restore the inline theme panel implementation in settings-hub.ts

Additional Notes

  • this begins the settings-hub split after the manager-command train, while staying in the same isolated worktree and stacked review flow

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this PR extracts the theme-settings interaction loop from settings-hub.ts into a new lib/codex-manager/theme-settings-panel.ts module, keeping the hub as a thin delegation shim. it's a clean, low-risk slice of the ongoing settings-hub split.

  • ThemeConfigAction union type and the full while(true) prompt loop are moved to the new file; the hub's promptThemeSettings now delegates via promptThemeSettingsPanel(initial, deps) with all required helpers injected
  • the dependency-injection interface (ThemeSettingsPanelDeps) makes the new module straightforwardly testable in isolation
  • no filesystem i/o or token handling is introduced — no windows path or token safety risk in this diff
  • the existing settings-hub-utils.test.ts cancel-baseline test still exercises the flow through the delegation path, but the four action branches (set-palette, set-accent, reset, save) in the new standalone module have no direct vitest coverage — a dedicated test/codex-manager/theme-settings-panel.test.ts should be added

Confidence Score: 4/5

  • safe to merge; logic is a faithful port with no behavioral changes, only missing direct unit tests for the new module
  • the refactor is mechanical — no logic was altered, only relocated behind a deps-injection boundary. the hub-level test still catches cancel/baseline regressions. score docked one point because the new module's action branches lack direct vitest coverage, which is a gap for future regressions.
  • lib/codex-manager/theme-settings-panel.ts — needs a dedicated test file covering set-palette, set-accent, reset, and save branches

Important Files Changed

Filename Overview
lib/codex-manager/theme-settings-panel.ts new module correctly extracts the theme panel loop with injected deps; logic is a faithful port of the original; missing dedicated vitest coverage for the four action branches
lib/codex-manager/settings-hub.ts slim delegation shim — promptThemeSettings now forwards to the panel module; no logic changes, ThemeConfigAction type moved to the new file, import order adjusted

Sequence Diagram

sequenceDiagram
    participant Caller
    participant settings-hub.ts
    participant theme-settings-panel.ts
    participant select (UI)

    Caller->>settings-hub.ts: promptThemeSettings(initial)
    settings-hub.ts->>theme-settings-panel.ts: promptThemeSettingsPanel(initial, deps)
    theme-settings-panel.ts->>theme-settings-panel.ts: cloneDashboardSettings(initial) → baseline, draft
    loop while awaiting user action
        theme-settings-panel.ts->>select (UI): select(items, options)
        select (UI)-->>theme-settings-panel.ts: ThemeConfigAction
        alt cancel / null
            theme-settings-panel.ts->>theme-settings-panel.ts: applyUiThemeFromDashboardSettings(baseline)
            theme-settings-panel.ts-->>settings-hub.ts: null
        else save
            theme-settings-panel.ts-->>settings-hub.ts: draft
        else reset
            theme-settings-panel.ts->>theme-settings-panel.ts: applyDashboardDefaultsForKeys(draft, THEME_PANEL_KEYS)
        else set-palette / set-accent
            theme-settings-panel.ts->>theme-settings-panel.ts: applyUiThemeFromDashboardSettings(draft)
        end
    end
    settings-hub.ts-->>Caller: DashboardDisplaySettings | null
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/codex-manager/theme-settings-panel.ts
Line: 35-166

Comment:
**no vitest coverage for the extracted module**

`promptThemeSettingsPanel` is now an independent, dependency-injected module — exactly the kind of thing that's easiest to unit-test directly. but there's no `test/codex-manager/theme-settings-panel.test.ts` (or equivalent) added in this PR.

the existing `settings-hub-utils.test.ts` line 656 covers the cancel→baseline-restore path through the hub delegation, so the regression test still passes. however, the new module's own paths — `set-palette`, `set-accent`, `reset`, and `save` — have no direct coverage. if someone later changes `promptThemeSettingsPanel`'s internals, the hub-level test may not catch the regression.

since deps are injected, a minimal test can stub everything without touching the real TTY or `select`:
```ts
// test/codex-manager/theme-settings-panel.test.ts
it("returns draft on save", async () => {
  vi.mocked(selectMock).mockResolvedValueOnce({ type: "save" });
  const result = await promptThemeSettingsPanel(initial, mockDeps);
  expect(result).toEqual(initial);
});
```
please add coverage for the four action branches before merging.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: extract th..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 21, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: aa39a339-db19-4839-b386-4f29df4e4976

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr2-theme-settings-split
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr2-theme-settings-split

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode added the passed label Mar 21, 2026
@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant