refactor: extract experimental settings entry wrapper#297
refactor: extract experimental settings entry wrapper#297ndycode wants to merge 2 commits intorefactor/pr2-backend-category-entry-wrapperfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughintroduces a new wrapper entry point Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes the wrapper pattern is straightforward delegation with no complex logic, but parameter density in 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/experimental-settings-entry.ts`:
- Around line 3-86: The wrapper function promptExperimentalSettingsEntry
currently erases precise types (hotkeyMapper, copy, and the inner callback)
forcing callers to use as never; fix by preserving the original generic/compound
type: either import and re-export the exact prompt params/type from
experimental-settings-prompt (the promptExperimentalSettingsMenu signature and
its generic action type) and use that imported type for the inner callback and
outer params, or make promptExperimentalSettingsEntry generic over the same
action/type parameter(s) so type information flows through (and extract the
duplicated 14-property callback shape into a shared type alias used by both
promptExperimentalSettingsMenu and promptExperimentalSettingsEntry to avoid
duplication). Ensure references to promptExperimentalSettingsMenu,
promptExperimentalSettingsEntry, hotkeyMapper, copy and the duplicated inner
callback shape are updated to use the shared/imported type so call sites no
longer need as never casts.
In `@test/experimental-settings-entry.test.ts`:
- Around line 39-40: The test currently only checks that
promptExperimentalSettingsMenu was called but not that the forwarded args
contain the expected initial config; update the assertion for
promptExperimentalSettingsMenu (the mock) to use toHaveBeenCalledWith and assert
it was called with an object containing initialConfig: { fetchTimeoutMs: 2000 }
(e.g. toHaveBeenCalledWith(expect.objectContaining({ initialConfig: {
fetchTimeoutMs: 2000 } }))) so the test verifies the delegation actually
forwards the expected value while keeping the existing result assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2eadd62a-879b-4b9b-a7b6-b2e30605e713
📒 Files selected for processing (3)
lib/codex-manager/experimental-settings-entry.tslib/codex-manager/settings-hub.tstest/experimental-settings-entry.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.tslib/codex-manager/experimental-settings-entry.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/experimental-settings-entry.test.ts
🔇 Additional comments (3)
lib/codex-manager/experimental-settings-entry.ts (1)
87-114: delegation logic looks correct.the pass-through is straightforward and preserves the return type. no side effects or logging concerns.
lib/codex-manager/settings-hub.ts (2)
67-67: import looks good.correctly imports the new entry wrapper alongside the existing
promptExperimentalSettingsMenuimport at line 68.
667-669: refactor looks correct, butas nevercasts highlight the type mismatch.the delegation to
promptExperimentalSettingsEntrypreserves behavior. theas nevercasts at lines 669, 673-676, 684-685 are required because the wrapper's type declarations erased the generics frompromptExperimentalSettingsMenu.if you address the type safety suggestion in
lib/codex-manager/experimental-settings-entry.ts:3-86, these casts can be removed.
Summary
promptExperimentalSettingsentry wrapper out ofsettings-hubon top of the latest settings leafValidation
npm run typechecknpm run lint -- lib/codex-manager/experimental-settings-entry.ts lib/codex-manager/settings-hub.ts test/experimental-settings-entry.test.tsnpm run test -- test/experimental-settings-entry.test.tsnote: 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
promptExperimentalSettingsEntrywrapper out ofsettings-hubinto its own module, slimming the facade and making the entry point independently testable. it also tightens types acrossexperimental-settings-prompt.ts— replacing allas nevercasts with concreteoc-chatgpt-orchestratorandstoragetypes and exportingExperimentalSettingsCopy/ExperimentalSettingsPromptDepsas named types that other modules can reference directly.lib/codex-manager/experimental-settings-entry.ts— new thin wrapper; delegates all deps topromptExperimentalSettingsMenuwith no added logiclib/codex-manager/experimental-settings-prompt.ts— allTAction/TPlan/TAppliedfree type params collapsed into concrete types;ExperimentalSettingsPromptDeps<TTargetState>extracted and exported; minor indentation regression introduced in the inline hotkey callback (~line 320)lib/codex-manager/settings-hub.ts— call site updated;as nevercasts fully removed;getTargetDestinationnow correctly defaultsdestinationtonullinstead ofundefinedtest/experimental-settings-entry.test.ts— focused passthrough test verifies all deps are forwarded; only the non-null return path is covered (null path missing)Confidence Score: 5/5
as nevercasts are gone, types are tighter, and the extracted module has a focused test. the two remaining items (indentation nit, missing null test case) are non-blocking p2s that don't affect runtime behavior or coverage thresholds in a meaningful way.Important Files Changed
promptExperimentalSettingsMenu; types are sound, no logic of its ownas never, concrete oc-chatgpt types),ExperimentalSettingsPromptDepsexported as named type — one indentation inconsistency introduced around the inline hotkey callback at line 320promptExperimentalSettingsEntry; allas nevercasts replaced with properly-typed lambdas; no behavioral changeSequence Diagram
sequenceDiagram participant SH as settings-hub participant EE as experimental-settings-entry participant EP as experimental-settings-prompt SH->>EE: promptExperimentalSettingsEntry({ initialConfig, promptExperimentalSettingsMenu, ...deps }) EE->>EP: promptExperimentalSettingsMenu({ initialConfig, ...deps }) EP-->>EE: PluginConfig | null EE-->>SH: PluginConfig | nullPrompt To Fix All With AI
Last reviewed commit: "Preserve experimenta..."