Skip to content

[codex] Implement method picker MVC#247

Open
eviltester wants to merge 18 commits into
masterfrom
codex/230-method-picker-mvc
Open

[codex] Implement method picker MVC#247
eviltester wants to merge 18 commits into
masterfrom
codex/230-method-picker-mvc

Conversation

@eviltester

@eviltester eviltester commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • refactor the method picker modal into a component-backed dialog with controller/view/factory layers
  • add real Method Navigator, Method List, and Method Help Display subcomponents plus shared pure helpers
  • keep openMethodPickerModal as a thin compatibility service preserving the existing { sourceType, command } | null result
  • update Storybook coverage, focused Jest coverage, and frontend migration/architecture docs

Validation

  • pnpm run test:ui:jest
  • pnpm run test:storybook
  • pnpm run build-storybook
  • pnpm run test:browser
  • pnpm run verify:ui
  • pnpm run verify:local
  • commit hook also reran formatting and the full Jest suite successfully

Summary by CodeRabbit

  • New Features
    • Storybook method-picker stories now use the real component-driven dialog experience (search, tabs, help/details).
    • Added a shared method-picker dialog component family powering navigator, list, help/details, and apply/cancel behavior.
  • Bug Fixes
    • Improved method selection/apply enablement, cancel/backdrop/close flows, focus restoration, and keyboard navigation.
    • Help/details rendering is safer (sanitized docs links) and more resilient (handles falsy/circular return-example data).
  • Documentation
    • Updated regex schema docs to show the explicit regex(...) form.
    • Refined component vs compatibility boundaries for the method picker.
  • Tests
    • Expanded method-picker dialog/controller/modal and keyboard/focus/warning coverage.
  • Chores
    • Updated build/version metadata plumbing and strengthened faker helpers.arrayElement array-argument validation messaging.

Copilot AI review requested due to automatic review settings June 25, 2026 15:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

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

Changes

Method picker component split

Layer / File(s) Summary
Shared helpers and state
packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-utils.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-controller.js, packages/core-ui/src/tests/utils/method-picker-dialog-controller.test.js
The method picker utility module adds recent-entry storage, filtering, tab-building, selection, parsing, and HTML rendering helpers, and the dialog controller tests cover tab setup, filtering, selection, and recent persistence.
Subcomponent views and factories
packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-controller.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-view.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-controller.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-view.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-controller.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-view.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator.js, packages/core-ui/src/tests/utils/method-picker-dialog-components.test.js
The navigator, list, and help display views render their DOM, forward callbacks, expose lifecycle helpers, and are covered by component-level tests.
Dialog composition and wrapper
packages/core-ui/js/gui_components/shared/method-picker-dialog/index.js, packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-view.js, packages/core-ui/js/gui_components/shared/test-data/ui/method-picker-modal.js, packages/core-ui/src/tests/utils/method-picker-modal.test.js
The composed dialog controller and view wire the subcomponents into the overlay dialog, and the compatibility modal now mounts that dialog, restores focus, and handles apply and cancel flows.
Storybook and documentation
apps/web/src/stories/method-picker-dialog.stories.js, docs/frontend-component-architecture.md, docs/frontend-component-migration-plan.md, docs/frontend-legacy-ui-elimination-plan.md
Storybook now uses the component-backed method picker pieces and updated controls, and the frontend architecture and migration docs are revised for the new dialog split.

Params validation and positional-only metadata

Layer / File(s) Summary
Keyword metadata and validation tests
packages/core/js/keywords/faker/helpers/array-element-keyword-definition.js, packages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.js, packages/core-ui/src/tests/shared/shared-schema-editor-controller.test.js, packages/core-ui/src/tests/utils/faker-command-help-metadata.test.js
The faker helper metadata marks the array parameter as positional-only, and the schema editor and help-metadata tests assert the positional-only shape.
Params editor warning flow
packages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.css, packages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.js, packages/core-ui/src/tests/utils/params-editor-modal.test.js
The params editor parses positional-only flags, separates warning-level validation from errors, renders a warning area, and the modal tests cover the warning state and apply flow.

Regex schema rendering and refresh

Layer / File(s) Summary
Regex rule output
packages/core-ui/js/gui_components/shared/schema-row-rule-mapper.js, packages/core-ui/src/tests/generator/data-generator-page.runtime.test.js, packages/core-ui/src/tests/generator/data-generator-page.text-mode.test.js, packages/core-ui/src/tests/generator/generator-schema-rule-helpers.test.js, packages/core-ui/src/tests/generator/generator-schema-runtime-service.test.js, packages/core-ui/src/tests/generator/schema-row-rule-mapper.test.js, docs-src/docs/040-test-data/018-Schema-Definition.md
Regex rows now serialize as explicit regex(...) rules, and the generator tests and schema docs assert the wrapped format.
Schema text refresh
packages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.js, packages/core-ui/js/gui_components/app/data-population-panel/index.js, packages/core-ui/js/gui_components/generator/runtime/generator-schema-sync.js, packages/core-ui/src/tests/shared/shared-schema-editor-controller.test.js
The schema editor controller now normalizes validation issues with severity, refreshes text from rows through a helper, and the text-mode sync callers pass the new refresh flag.

Build metadata and site-config wiring

Layer / File(s) Summary
Build metadata helpers
packages/core-ui/js/build-metadata/build-metadata.js, packages/core-ui/src/tests/shared/build-metadata.test.js
The build-metadata module formats and normalizes build versions, reads injected version globals, and exposes the resolved version through getBuildMetadata().
Instructions footer version
packages/core-ui/js/gui_components/shared/instructions/app-page-instructions.js, packages/core-ui/src/tests/shared/instructions-view.test.js
The app-page instructions props are now factory-generated from build metadata, and the shared instructions view tests render the version from injected build metadata.
Storybook and testenv config
.storybook/main.js, apps/web/vite.config.mjs, scripts/create-testenv.mjs, apps/web/src/tests/jest/build-version-config.test.js, apps/web/src/tests/jest/testenv-site-config-override.test.js
Storybook, the web Vite config, and the testenv helper now resolve site-config overrides and build versions through shared helpers, and the Jest tests assert the injected alias and define values.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90+ minutes

Possibly related issues

Possibly related PRs

Poem

🐰 I hopped through tabs and shiny panes,
and split the carrots into brains.
One dialog blooms with navigator light,
while warning signs now glow just right.
Hop-hop—apply, and off I zoom!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: refactoring the method picker into an MVC-based implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/230-method-picker-mvc

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9579270 and 21aef64.

📒 Files selected for processing (21)
  • apps/web/src/stories/method-picker-dialog.stories.js
  • docs/frontend-component-architecture.md
  • docs/frontend-component-migration-plan.md
  • docs/frontend-legacy-ui-elimination-plan.md
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/index.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-controller.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display-view.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-help-display.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-controller.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list-view.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-list.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-controller.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator-view.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-navigator.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-controller.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-utils.js
  • packages/core-ui/js/gui_components/shared/method-picker-dialog/method-picker-dialog-view.js
  • packages/core-ui/js/gui_components/shared/test-data/ui/method-picker-modal.js
  • packages/core-ui/src/tests/utils/method-picker-dialog-components.test.js
  • packages/core-ui/src/tests/utils/method-picker-dialog-controller.test.js
  • packages/core-ui/src/tests/utils/method-picker-modal.test.js

@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR moves the method picker into a component-backed MVC dialog. The main changes are:

  • New navigator, method list, help display, controller, view, and shared helper modules for the picker.
  • A compatibility openMethodPickerModal service that mounts the new dialog and preserves the existing selection result shape.
  • Updated schema parsing and row serialization behavior for explicit literal, regex, enum, faker, and domain forms.
  • Expanded Storybook, Jest, and Playwright coverage for picker interaction, focus handling, invalid-schema recovery, and related UI behavior.
  • Documentation and styling updates for the frontend component migration.

Confidence Score: 5/5

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

T-Rex T-Rex Logs

What T-Rex did

  • Observed the method picker dialog behavior in Storybook across the base and head variants, as shown by the before and after videos and corroborating logs.
  • Saved the attempted before-run command output for the method-picker-compatibility test to trex-artifacts/method-picker-compatibility-01-before.log because no after artifact could be produced due to missing test runner dependencies.
  • Compared the regex schema explicit form runs; both before and after logs show ok: true and generated rows such as FDK-8359 and EHA-6856, with five matches: true, and saved the repro script.
  • Observed base validator handling: the base accepted invalid non-array validator and runner cases as HTTP 200 OK, while the head rejects with HTTP 400 Bad Request and a helpful message; valid array cases remain HTTP 200 OK.
  • Resolved the build metadata flow: the head now exposes version v20261224.2359 and 7 focused Jest tests pass, with the resolver exposed for Storybook builds.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (18): Last reviewed commit: "Fix mobile app overflow" | Re-trigger Greptile

This was referenced Jun 25, 2026
@eviltester

Copy link
Copy Markdown
Owner Author

closes #250

@eviltester

Copy link
Copy Markdown
Owner Author

closes #252

@eviltester

Copy link
Copy Markdown
Owner Author

closes #249

@eviltester

Copy link
Copy Markdown
Owner Author

closes #251

@eviltester

Copy link
Copy Markdown
Owner Author

clsoes #248

@eviltester

Copy link
Copy Markdown
Owner Author

closes #246

@eviltester

Copy link
Copy Markdown
Owner Author

closes #236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diffray-review-failed diffray review status: failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants