[codex] Implement method picker MVC#247
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe method picker was rebuilt as shared components with a composed dialog, Storybook/docs were updated to match, params editing now tracks positional-only arguments and warning-level validation, regex output and text refresh behavior changed, and build/version configuration now uses shared resolution helpers. ChangesMethod picker component split
Params validation and positional-only metadata
Regex schema rendering and refresh
Build metadata and site-config wiring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-view.js`:
- Around line 36-50: Validate the docsUrl value in method-help-display-view.js
before rendering the link in the method picker UI: the MethodHelpDisplayView
logic currently escapes HTML but still passes unsafe schemes into the anchor
href. Update the docsUrl handling in the render path so only trusted http/https
(and any explicitly allowed relative URLs) are rendered by the docs link branch,
and otherwise fall back to no link; use the existing selected.command,
model.docsUrl, and render path that builds the <a> element as the place to apply
the scheme check.
In
`@packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-utils.js`:
- Around line 67-79: normalizeReturnExampleValue currently calls JSON.stringify
for arrays and objects in method-picker-dialog-utils, which can throw on
circular or non-serializable values and break rendering. Update
normalizeReturnExampleValue to use a safe serialization fallback around the
JSON.stringify paths, returning a stable placeholder or stringified fallback
when serialization fails. Apply the same protection to the other
normalizeReturnExampleValue usages referenced in this module so help rendering
cannot crash on unsafe return values.
- Around line 59-64: The example normalization in toExampleList is dropping
valid falsy values because it uses String(value || '') and String(entry || '').
Update the logic so the array and single-value paths preserve 0 and false while
still trimming and filtering out only empty results; use the toExampleList
helper as the place to fix this behavior.
In
`@packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-view.js`:
- Around line 132-148: The dialog’s key handling in method-picker-dialog-view.js
only covers Escape, search, and Enter, so Tab navigation can escape the modal
despite role="dialog" and aria-modal="true". Update onKeyDown to intercept Tab
and Shift+Tab, query the dialog’s focusable elements from this.root, and cycle
focus between the first and last interactive controls. Keep the existing Escape
and search behavior intact, and ensure event.preventDefault() is called only
when wrapping focus.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0b4a1d4-fe72-4de8-a5af-b6c75de78195
📒 Files selected for processing (21)
apps/web/src/stories/method-picker-dialog.stories.jsdocs/frontend-component-architecture.mddocs/frontend-component-migration-plan.mddocs/frontend-legacy-ui-elimination-plan.mdpackages/core-ui/js/gui_components/shared/method-picker-dialog/index.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-controller.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-view.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-controller.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-view.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-list.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-controller.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-view.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-controller.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-utils.jspackages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-view.jspackages/core-ui/js/gui_components/shared/test-data/ui/method-picker-modal.jspackages/core-ui/src/tests/utils/method-picker-dialog-components.test.jspackages/core-ui/src/tests/utils/method-picker-dialog-controller.test.jspackages/core-ui/src/tests/utils/method-picker-modal.test.js
Greptile SummaryThis PR moves the method picker into a component-backed MVC dialog. The main changes are:
Confidence Score: 5/5The changes appear safe to merge with no code issues identified. The implementation is well covered across focused component, controller, modal, Storybook, and browser test paths, with compatibility behavior preserved for existing callers.
What T-Rex did
Reviews (18): Last reviewed commit: "Fix mobile app overflow" | Re-trigger Greptile |
|
closes #250 |
|
closes #252 |
|
closes #249 |
|
closes #251 |
|
clsoes #248 |
|
closes #246 |
|
closes #236 |
Summary
{ sourceType, command } | nullresultValidation
Summary by CodeRabbit
regex(...)form.helpers.arrayElementarray-argument validation messaging.