[codex] Implement method picker MVC#247
Merged
Merged
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 refactors the method picker modal from a monolithic imperative function into a layered
Confidence Score: 5/5Safe to merge — the refactor is well-decomposed, the compatibility shim preserves the existing public API, and all validation gates (Jest, Storybook, browser tests) are reported passing. The method-picker MVC is clean and thoroughly tested at controller, component, and story levels. The regex detection overhaul has integration tests covering the new JS-literal, flag-error, and comma-disambiguation paths. The two flagged items are both non-blocking: the Space key gap is a minor accessibility omission in the listbox keyboard model, and the broadened packages/core/js/data_generation/inline-schema-rule.js — the new Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant openMethodPickerModal
participant createMethodPickerDialog
participant MethodPickerDialogController
participant MethodPickerDialogView
participant Subcomponents
Caller->>openMethodPickerModal: "openMethodPickerModal({ options, title, ... })"
openMethodPickerModal->>openMethodPickerModal: lockDocumentScroll()
openMethodPickerModal->>createMethodPickerDialog: "createMethodPickerDialog({ root, props, services, callbacks })"
createMethodPickerDialog->>MethodPickerDialogController: "new MethodPickerDialogController({ props, services })"
createMethodPickerDialog->>MethodPickerDialogView: "new MethodPickerDialogView({ root, controller, callbacks })"
MethodPickerDialogView->>Subcomponents: createMethodNavigator + createMethodList + createMethodHelpDisplay
Subcomponents-->>MethodPickerDialogView: mounted
Note over Caller,Subcomponents: User interacts (search/tab/select/apply)
Subcomponents->>MethodPickerDialogView: onSelectCommand / onSearchTermChange / onTabChange
MethodPickerDialogView->>MethodPickerDialogController: setSearchTerm / setActiveTab / selectCommand
MethodPickerDialogController-->>MethodPickerDialogView: updated state
MethodPickerDialogView->>Subcomponents: update(props)
Note over Caller,Subcomponents: User clicks Apply
MethodPickerDialogView->>MethodPickerDialogController: applySelection()
MethodPickerDialogController-->>MethodPickerDialogView: "{ sourceType, command }"
MethodPickerDialogView->>openMethodPickerModal: callbacks.onApply(selection)
openMethodPickerModal->>openMethodPickerModal: close(selection): destroy + restoreScroll + restoreFocus
openMethodPickerModal-->>Caller: "Promise resolves { sourceType, command }"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant openMethodPickerModal
participant createMethodPickerDialog
participant MethodPickerDialogController
participant MethodPickerDialogView
participant Subcomponents
Caller->>openMethodPickerModal: "openMethodPickerModal({ options, title, ... })"
openMethodPickerModal->>openMethodPickerModal: lockDocumentScroll()
openMethodPickerModal->>createMethodPickerDialog: "createMethodPickerDialog({ root, props, services, callbacks })"
createMethodPickerDialog->>MethodPickerDialogController: "new MethodPickerDialogController({ props, services })"
createMethodPickerDialog->>MethodPickerDialogView: "new MethodPickerDialogView({ root, controller, callbacks })"
MethodPickerDialogView->>Subcomponents: createMethodNavigator + createMethodList + createMethodHelpDisplay
Subcomponents-->>MethodPickerDialogView: mounted
Note over Caller,Subcomponents: User interacts (search/tab/select/apply)
Subcomponents->>MethodPickerDialogView: onSelectCommand / onSearchTermChange / onTabChange
MethodPickerDialogView->>MethodPickerDialogController: setSearchTerm / setActiveTab / selectCommand
MethodPickerDialogController-->>MethodPickerDialogView: updated state
MethodPickerDialogView->>Subcomponents: update(props)
Note over Caller,Subcomponents: User clicks Apply
MethodPickerDialogView->>MethodPickerDialogController: applySelection()
MethodPickerDialogController-->>MethodPickerDialogView: "{ sourceType, command }"
MethodPickerDialogView->>openMethodPickerModal: callbacks.onApply(selection)
openMethodPickerModal->>openMethodPickerModal: close(selection): destroy + restoreScroll + restoreFocus
openMethodPickerModal-->>Caller: "Promise resolves { sourceType, command }"
Reviews (20): Last reviewed commit: "Fix method picker and params editor ARIA..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This was referenced Jun 25, 2026
Closed
This was referenced Jun 27, 2026
Owner
Author
|
closes #250 |
Owner
Author
|
closes #252 |
Owner
Author
|
closes #249 |
Owner
Author
|
closes #251 |
Owner
Author
|
clsoes #248 |
Owner
Author
|
closes #246 |
Owner
Author
|
closes #236 |
This was referenced Jun 28, 2026
Owner
Author
|
closes #255 |
Owner
Author
|
closes #256 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{ sourceType, command } | nullresultValidation
Summary by CodeRabbit
regex(...)form.helpers.arrayElementarray-argument validation messaging.