Skip to content

refactor: extract dashboard display panel#168

Closed
ndycode wants to merge 2 commits intorefactor/pr2-behavior-settings-splitfrom
refactor/pr2-dashboard-display-split
Closed

refactor: extract dashboard display panel#168
ndycode wants to merge 2 commits intorefactor/pr2-behavior-settings-splitfrom
refactor/pr2-dashboard-display-split

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 21, 2026

Summary

  • extract the account-list display settings flow from settings-hub.ts into a dedicated panel module
  • keep the current dashboard settings behavior intact while continuing the settings-hub split in small reviewable slices

What Changed

  • added lib/codex-manager/dashboard-display-panel.ts containing the dashboard display prompt flow and its local action model
  • updated lib/codex-manager/settings-hub.ts so promptDashboardDisplaySettings(...) delegates to the extracted panel module
  • preserved preview, sort-mode, layout-mode, reset, save, and cancel behavior by passing the existing 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 17c3d6a to restore the inline dashboard display panel implementation in settings-hub.ts

Additional Notes

  • this continues the settings-hub split in the same isolated worktree after the theme and behavior panel extractions

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

clean extraction of the dashboard display panel from settings-hub.ts into lib/codex-manager/dashboard-display-panel.ts, continuing the same isolated worktree split pattern used for the theme and behavior panels. the delegation in settings-hub.ts is a thin wrapper that threads all local helpers and constants through the DashboardDisplayPanelDeps interface — no behavioral changes.

  • promptDashboardDisplayPanel is a faithful port; all toggle, reset, cycle-sort, cycle-layout, and cancel paths preserved exactly
  • types (DashboardDisplaySettingKey, DashboardDisplaySettingOption, DashboardConfigAction) correctly moved to the new module and re-imported as needed in settings-hub.ts
  • import type { UI_COPY } is correct — the actual value is injected through deps; the panel only needs the type for the interface definition
  • no windows filesystem or token safety risks introduced; no concurrency concerns (panel is a pure interactive ui loop with no shared mutable state)
  • new test file covers the six main flows; onInput and onCursorChange callback branches are untested — worth a follow-up to stay above the 80% coverage threshold
  • lib/AGENTS.md and AGENTS.md still reference settings-hub.ts as the sole file under codex-manager/ and cite the old 2100-line count — minor doc drift worth updating in a follow-up or regenerating the knowledge base

Confidence Score: 5/5

  • safe to merge — pure structural refactor with no behavioral changes and passing ci
  • behavior is identical to the pre-extraction implementation; the only open item is test coverage for onInput/onCursorChange callbacks, which is a non-blocking follow-up rather than a production risk
  • test/dashboard-display-panel.test.ts — onInput and onCursorChange branches uncovered

Important Files Changed

Filename Overview
lib/codex-manager/dashboard-display-panel.ts new panel module faithfully extracted from settings-hub.ts; dependency injection pattern consistent with prior theme/behavior panel splits; types correctly exported; no behavioral differences from original
lib/codex-manager/settings-hub.ts delegation wrapper is minimal and correct; all helpers and constants threaded through deps; DashboardConfigAction/DashboardDisplaySettingKey/DashboardDisplaySettingOption types moved to panel and re-imported as needed
test/dashboard-display-panel.test.ts covers main flows (toggle, reset, cycle-sort, cycle-layout, cancel, non-TTY guard) but onInput/onCursorChange callback branches are not tested; 80% threshold may be at risk for this file

Sequence Diagram

sequenceDiagram
    participant caller as settings-hub.ts
    participant hub as promptDashboardDisplaySettings()
    participant panel as promptDashboardDisplayPanel()
    participant select as ui/select.ts

    caller->>hub: promptDashboardDisplaySettings(initial)
    hub->>panel: promptDashboardDisplayPanel(initial, deps{helpers, constants, UI_COPY})
    panel->>panel: cloneDashboardSettings(initial) → draft
    loop while true
        panel->>select: select(items, {onCursorChange, onInput, ...})
        select-->>panel: DashboardConfigAction | null
        alt cancel / null
            panel-->>hub: null
        else save
            panel-->>hub: draft
        else reset
            panel->>panel: applyDashboardDefaultsForKeys(draft, ACCOUNT_LIST_PANEL_KEYS)
        else cycle-sort-mode
            panel->>panel: nextMode = ready-first | manual → update draft
        else cycle-layout-mode
            panel->>panel: nextLayout = compact-details | expanded-rows → update draft
        else toggle
            panel->>panel: draft[key] = !draft[key]
        end
    end
    hub-->>caller: DashboardDisplaySettings | null
Loading

Comments Outside Diff (1)

  1. lib/codex-manager/settings-hub.ts, line 38-55 (link)

    P1 Duplicate DashboardDisplaySettingKey type will silently diverge

    settings-hub.ts still declares its own local DashboardDisplaySettingKey (lines 38-49) that is byte-for-byte identical to the one exported from dashboard-display-panel.ts. because both are structural string-union types, typescript won't raise an error if one grows a new key and the other doesn't — the mismatched type will silently pass structural checks and only blow up at runtime when the panel receives a key it doesn't know how to toggle.

    the hub's local type is still consumed by DashboardDisplaySettingOption (line 52) and PreviewFocusKey (line 151). both should import from the panel module to guarantee they stay in sync:

    import type {
        DashboardDisplaySettingKey,
        DashboardDisplaySettingOption,
    } from "./dashboard-display-panel.js";

    then remove the local type DashboardDisplaySettingKey and interface DashboardDisplaySettingOption declarations.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: lib/codex-manager/settings-hub.ts
    Line: 38-55
    
    Comment:
    **Duplicate `DashboardDisplaySettingKey` type will silently diverge**
    
    `settings-hub.ts` still declares its own local `DashboardDisplaySettingKey` (lines 38-49) that is byte-for-byte identical to the one exported from `dashboard-display-panel.ts`. because both are structural string-union types, typescript won't raise an error if one grows a new key and the other doesn't — the mismatched type will silently pass structural checks and only blow up at runtime when the panel receives a key it doesn't know how to toggle.
    
    the hub's local type is still consumed by `DashboardDisplaySettingOption` (line 52) and `PreviewFocusKey` (line 151). both should import from the panel module to guarantee they stay in sync:
    
    ```ts
    import type {
        DashboardDisplaySettingKey,
        DashboardDisplaySettingOption,
    } from "./dashboard-display-panel.js";
    ```
    
    then remove the local `type DashboardDisplaySettingKey` and `interface DashboardDisplaySettingOption` declarations.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Codex

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/dashboard-display-panel.test.ts
Line: 130-140

Comment:
**missing coverage: `onInput` and `onCursorChange` callbacks**

the test mocks `select` at the resolved-value level so none of the `onInput` keyboard shortcuts (`q`, `s`, `r`, `m`, `l`, numeric keys) or `onCursorChange` preview-update logic are exercised. these paths live entirely inside `promptDashboardDisplayPanel` and aren't covered by `codex-manager-cli.test.ts` either (that suite tests the delegation, not the callbacks).

worth adding at least one test that captures the callbacks from the `select` call arguments and invokes them directly — e.g. confirming that `onInput("q")` returns `{ type: "cancel" }` and that `onInput("12")` (for option 12) falls through to `undefined` given only 3 options in the fixture list. the project targets 80% coverage so these uncovered branches are a gap.

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

Reviews (2): Last reviewed commit: "test: cover dashboard display panel extr..." | Re-trigger Greptile

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 21, 2026

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 756274c4-bc80-4779-bfee-ad60513d2377

📥 Commits

Reviewing files that changed from the base of the PR and between 66d07c6 and 5f0405e.

📒 Files selected for processing (3)
  • lib/codex-manager/dashboard-display-panel.ts
  • lib/codex-manager/settings-hub.ts
  • test/dashboard-display-panel.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr2-dashboard-display-split
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr2-dashboard-display-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.

@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.

@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant