Add enum picker editing for schema params#305
Open
eviltester wants to merge 9 commits into
Open
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:
📝 WalkthroughWalkthroughThis PR introduces explicit enum metadata across keyword definitions, adds enum-aware parameter editing with select controls, preserves explicit empty enum values while rejecting accidental empty CSV entries, and expands domain validation, schema integration, and UI test coverage. ChangesEnum metadata and keyword contracts
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class enum parameter metadata and editing support across the domain keyword catalog and the schema params editor, so commands that define explicit enum choices render as dropdown pickers (with correct serialization/validation and end-to-end test coverage).
Changes:
- Refactors many domain keyword param schemas to use
type: "enum"+enumValuesinstead of pipe-delimited unions. - Updates domain/faker help normalization + keyword arg validation to preserve and validate enum metadata.
- Enhances the params editor modal UI to render enum params as
<select>controls (including unset + empty-string handling) and adds Jest/Storybook/Playwright coverage.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/tests/data_generation/unit/domain/domainKeywords.test.js | Updates test arg sampling to respect enumValues. |
| packages/core/src/tests/data_generation/unit/domain/domain-keyword-params-usage.test.js | Updates keyword param sampling/execution tests for enum specs. |
| packages/core/src/tests/data_generation/keywords/domain/autoincrement/timestamp-exec.test.js | Updates expected validation error output for enum timestamp units. |
| packages/core/js/keywords/domain/word/verb-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/sample-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/preposition-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/noun-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/interjection-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/conjunction-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/adverb-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/word/adjective-keyword-definition.js | Converts strategy to explicit enum metadata. |
| packages/core/js/keywords/domain/string/uuid-keyword-definition.js | Converts UUID version param to enum metadata. |
| packages/core/js/keywords/domain/string/hexadecimal-keyword-definition.js | Converts casing param to enum metadata. |
| packages/core/js/keywords/domain/string/alphanumeric-keyword-definition.js | Converts casing param to enum metadata. |
| packages/core/js/keywords/domain/string/alpha-keyword-definition.js | Converts casing param to enum metadata. |
| packages/core/js/keywords/domain/phone/number-keyword-definition.js | Converts phone style param to enum metadata. |
| packages/core/js/keywords/domain/person/prefix-keyword-definition.js | Converts sex param to enum metadata. |
| packages/core/js/keywords/domain/person/middle-name-keyword-definition.js | Converts sex param to enum metadata. |
| packages/core/js/keywords/domain/person/last-name-keyword-definition.js | Converts sex param to enum metadata. |
| packages/core/js/keywords/domain/person/first-name-keyword-definition.js | Converts sex param to enum metadata. |
| packages/core/js/keywords/domain/lorem/word-keyword-definition.js | Converts strategy param to enum metadata. |
| packages/core/js/keywords/domain/location/country-code-keyword-definition.js | Converts variant param to enum metadata. |
| packages/core/js/keywords/domain/internet/url-keyword-definition.js | Converts protocol param to enum metadata. |
| packages/core/js/keywords/domain/internet/mac-keyword-definition.js | Converts separator param to enum metadata (incl empty string choice). |
| packages/core/js/keywords/domain/internet/ipv4-keyword-definition.js | Converts network param to enum metadata with explicit array choices. |
| packages/core/js/keywords/domain/finance/bitcoin-address-keyword-definition.js | Converts type/network params to enum metadata. |
| packages/core/js/keywords/domain/date/birthdate-keyword-definition.js | Converts mode param to enum metadata. |
| packages/core/js/keywords/domain/commerce/isbn-keyword-definition.js | Converts variant param to enum metadata. |
| packages/core/js/keywords/domain/color/rgb-keyword-definition.js | Converts casing/format params to enum metadata. |
| packages/core/js/keywords/domain/color/lch-keyword-definition.js | Converts format param to enum metadata. |
| packages/core/js/keywords/domain/color/lab-keyword-definition.js | Converts format param to enum metadata. |
| packages/core/js/keywords/domain/color/hwb-keyword-definition.js | Converts format param to enum metadata. |
| packages/core/js/keywords/domain/color/hsl-keyword-definition.js | Converts format param to enum metadata. |
| packages/core/js/keywords/domain/color/color-by-csscolor-space-keyword-definition.js | Converts format/space params to enum metadata. |
| packages/core/js/keywords/domain/color/cmyk-keyword-definition.js | Converts format param to enum metadata. |
| packages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.js | Defines canonical plural timestamp units and exposes them as enum choices. |
| packages/core/js/keywords/domain/airline/seat-keyword-definition.js | Converts aircraftType param to enum metadata. |
| packages/core/js/faker/faker-helper-keyword-definitions.js | Preserves enum/choice metadata through faker helper help normalization/mapping. |
| packages/core/js/domain/domain-keywords.js | Adds enum-aware type tokenization/validation + preserves allowed/choice metadata on args. |
| packages/core-ui/src/tests/utils/params-editor-modal.test.js | Adds unit tests for enum-choice resolution, parsing, rendering, and serialization. |
| packages/core-ui/src/tests/utils/faker-command-help-metadata.test.js | Updates expectations for enum-typed params in generated help metadata. |
| packages/core-ui/src/tests/shared/shared-schema-editor-controller.test.js | Adds controller-level test for applying enum picker params back into schema rows. |
| packages/core-ui/src/tests/shared/help-model-builder.test.js | Verifies enum metadata appears in schema help models (timestamp units). |
| packages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.js | Implements enum choice inference/resolution + enum <select> editor + serialization rules. |
| packages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.css | Styles enum selects consistently with other inputs and adds focus-visible handling. |
| packages/core-ui/js/gui_components/shared/test-data/help/help-model-builder.js | Normalizes enum/choice arrays into help param models. |
| apps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.js | Updates schema editor abstraction focus behavior and adds helper to edit enum params via dialog. |
| apps/web/src/tests/browser/shared/abstractions/components/params-editor-dialog.component.js | Adds Playwright abstraction for enum selects in the params editor dialog. |
| apps/web/src/tests/browser/generator/functional/schema-edit.spec.js | Adds Playwright functional coverage for selecting enum params via guided dialog. |
| apps/web/src/stories/shared-schema-definition.stories.js | Adds Storybook story demonstrating enum dropdown param editing flow. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9568f40b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds enum-aware dropdown editing to the params editor: when a schema param carries explicit
Confidence Score: 5/5Safe to merge. The enum picker is self-contained in the UI layer, the domain validator and enumParser changes are surgical, and all three layers are covered by new Jest, Storybook, and Playwright tests. The core round-trip (parse → render as select → read back → serialize) is correctly handled for string values, numeric values, and the tricky empty-string case, with explicit test assertions at every step. No existing validation contracts are broken — isTypeMatch and formatExpectedType are strictly additive for the new type:enum shape, and old pipe-union strings still work through the unchanged code path. The only practical rough edge is the singular timestamp alias display regression, which is benign because the singular and plural forms generate identical output. params-editor-modal.js is the most complex file in the diff; the allowedValues hint rendering for enums with empty-string members warrants a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Param metadata\n(keyword definition)"] -->|"enumValues / allowedValues / choices"| B["resolveEnumChoices()"]
B -->|"choices found"| C["mode = 'enum'\nenumChoices = [...]"]
B -->|"no explicit choices"| D["inferEnumChoicesFromType()\n(pipe-union heuristic)"]
D -->|"all literal tokens"| C
D -->|"broad union / typed tokens"| E["mode = 'auto'\ntext input"]
C --> F["renderEnumValueEditor()\n<select> with data-enum-value attrs"]
E --> G["renderValueEditor()\n<input type=text> or boolean radio"]
F -->|"user changes select"| H["readRenderedEntryState()\ndata-enum-value → value + isSet=true\nUnset option → value='' + isSet=false"]
G -->|"user types"| I["readRenderedEntryState()\nvalue + isSet via trim()"]
H --> J["buildParamsTextFromEditorEntries()"]
I --> J
J -->|"mode=enum, numeric value"| K["raw: version=7"]
J -->|"mode=enum, string value"| L["quoted: variant=\"alpha-3\""]
J -->|"mode=enum, empty string + isSet=true"| M["quoted: separator=\"\""]
J -->|"isSet=false + optional"| N["omit param"]
%%{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"}}}%%
flowchart TD
A["Param metadata\n(keyword definition)"] -->|"enumValues / allowedValues / choices"| B["resolveEnumChoices()"]
B -->|"choices found"| C["mode = 'enum'\nenumChoices = [...]"]
B -->|"no explicit choices"| D["inferEnumChoicesFromType()\n(pipe-union heuristic)"]
D -->|"all literal tokens"| C
D -->|"broad union / typed tokens"| E["mode = 'auto'\ntext input"]
C --> F["renderEnumValueEditor()\n<select> with data-enum-value attrs"]
E --> G["renderValueEditor()\n<input type=text> or boolean radio"]
F -->|"user changes select"| H["readRenderedEntryState()\ndata-enum-value → value + isSet=true\nUnset option → value='' + isSet=false"]
G -->|"user types"| I["readRenderedEntryState()\nvalue + isSet via trim()"]
H --> J["buildParamsTextFromEditorEntries()"]
I --> J
J -->|"mode=enum, numeric value"| K["raw: version=7"]
J -->|"mode=enum, string value"| L["quoted: variant=\"alpha-3\""]
J -->|"mode=enum, empty string + isSet=true"| M["quoted: separator=\"\""]
J -->|"isSet=false + optional"| N["omit param"]
Reviews (8): Last reviewed commit: "Fix Storybook test runner browser API po..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.js (1)
189-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the near-duplicate params-dialog helpers.
editRowParamsWithDialogandeditRowEnumParamsWithDialogdiffer only in whichparamsEditorsetter is invoked per entry. A single helper taking a setter callback would avoid future drift between the two.♻️ Proposed consolidation
- async editRowParamsWithDialog(index, valuesByName) { - await this.ensureSchemaMode(); - await this.dismissOpenHelpTooltips(); - await this.row(index).locator('[data-action="edit-params"]').click(); - for (const [name, value] of Object.entries(valuesByName || {})) { - await this.paramsEditor.setValue(name, value); - } - await this.paramsEditor.apply(); - } - - async editRowEnumParamsWithDialog(index, valuesByName) { - await this.ensureSchemaMode(); - await this.dismissOpenHelpTooltips(); - await this.row(index).locator('[data-action="edit-params"]').click(); - for (const [name, value] of Object.entries(valuesByName || {})) { - await this.paramsEditor.selectEnumValue(name, value); - } - await this.paramsEditor.apply(); - } + async editRowParamsWithDialog(index, valuesByName, { setter = 'setValue' } = {}) { + await this.ensureSchemaMode(); + await this.dismissOpenHelpTooltips(); + await this.row(index).locator('[data-action="edit-params"]').click(); + for (const [name, value] of Object.entries(valuesByName || {})) { + await this.paramsEditor[setter](name, value); + } + await this.paramsEditor.apply(); + }🤖 Prompt for 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. In `@apps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.js` around lines 189 - 208, The two params-dialog helpers are near-duplicates, so consolidate them into a single shared flow in schema-editor.component.js. Refactor editRowParamsWithDialog and editRowEnumParamsWithDialog to use one internal helper that handles ensureSchemaMode, dismissOpenHelpTooltips, row(...).locator('[data-action="edit-params"]').click(), iteration over valuesByName, and paramsEditor.apply(), while accepting a setter callback to choose between paramsEditor.setValue and paramsEditor.selectEnumValue. Keep the existing public method names as thin wrappers if needed.packages/core/js/keywords/domain/word/adverb-keyword-definition.js (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting
WORD_SELECTION_STRATEGY_TYPEinto a shared constant.This same array is now duplicated verbatim across adverb, conjunction, noun, preposition, sample, and verb keyword definitions (and likely adjective/interjection). A single source of truth would prevent future drift if the allowed strategies ever change.
♻️ Suggested consolidation
+// packages/core/js/keywords/domain/word/word-selection-strategy.js +export const WORD_SELECTION_STRATEGY_TYPE = ['fail', 'closest', 'shortest', 'longest', 'any-length'];Then import it in each
word/*-keyword-definition.jsfile instead of redeclaring.🤖 Prompt for 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. In `@packages/core/js/keywords/domain/word/adverb-keyword-definition.js` at line 3, Extract the duplicated WORD_SELECTION_STRATEGY_TYPE array into a shared constant and import it from each word keyword definition instead of redeclaring it. Update adverbKeywordDefinition and the other word/*-keyword-definition.js modules that currently define the same array so they all reference the shared source of truth, keeping the existing symbol name available where needed.
🤖 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/test-data/ui/params-editor-modal.js`:
- Around line 412-414: The boolean option handling in readRenderedEntryState and
hasEntryValue is treating the always-checked Unset radio as a filled value,
which makes optional booleans look set and affects lastFilledIndex/syncPreview.
Update the boolean branch so isSet is derived from the selected radio’s
non-empty value rather than Boolean(checkedBooleanOption), and keep
hasEntryValue aligned with that behavior. Add a regression test for an optional
boolean left Unset to verify it produces no param/empty preview and does not
shift later required entries.
---
Nitpick comments:
In
`@apps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.js`:
- Around line 189-208: The two params-dialog helpers are near-duplicates, so
consolidate them into a single shared flow in schema-editor.component.js.
Refactor editRowParamsWithDialog and editRowEnumParamsWithDialog to use one
internal helper that handles ensureSchemaMode, dismissOpenHelpTooltips,
row(...).locator('[data-action="edit-params"]').click(), iteration over
valuesByName, and paramsEditor.apply(), while accepting a setter callback to
choose between paramsEditor.setValue and paramsEditor.selectEnumValue. Keep the
existing public method names as thin wrappers if needed.
In `@packages/core/js/keywords/domain/word/adverb-keyword-definition.js`:
- Line 3: Extract the duplicated WORD_SELECTION_STRATEGY_TYPE array into a
shared constant and import it from each word keyword definition instead of
redeclaring it. Update adverbKeywordDefinition and the other
word/*-keyword-definition.js modules that currently define the same array so
they all reference the shared source of truth, keeping the existing symbol name
available where needed.
🪄 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: 677345a8-da2c-4b80-9bfe-b3eebd9f0be0
📒 Files selected for processing (50)
apps/web/src/stories/shared-schema-definition.stories.jsapps/web/src/tests/browser/generator/functional/schema-edit.spec.jsapps/web/src/tests/browser/shared/abstractions/components/params-editor-dialog.component.jsapps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.jspackages/core-ui/js/gui_components/shared/test-data/help/help-model-builder.jspackages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.csspackages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.jspackages/core-ui/src/tests/shared/help-model-builder.test.jspackages/core-ui/src/tests/shared/shared-schema-editor-controller.test.jspackages/core-ui/src/tests/utils/faker-command-help-metadata.test.jspackages/core-ui/src/tests/utils/params-editor-modal.test.jspackages/core/js/domain/domain-keywords.jspackages/core/js/faker/faker-helper-keyword-definitions.jspackages/core/js/keywords/domain/airline/seat-keyword-definition.jspackages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.jspackages/core/js/keywords/domain/color/cmyk-keyword-definition.jspackages/core/js/keywords/domain/color/color-by-csscolor-space-keyword-definition.jspackages/core/js/keywords/domain/color/hsl-keyword-definition.jspackages/core/js/keywords/domain/color/hwb-keyword-definition.jspackages/core/js/keywords/domain/color/lab-keyword-definition.jspackages/core/js/keywords/domain/color/lch-keyword-definition.jspackages/core/js/keywords/domain/color/rgb-keyword-definition.jspackages/core/js/keywords/domain/commerce/isbn-keyword-definition.jspackages/core/js/keywords/domain/date/birthdate-keyword-definition.jspackages/core/js/keywords/domain/finance/bitcoin-address-keyword-definition.jspackages/core/js/keywords/domain/internet/ipv4-keyword-definition.jspackages/core/js/keywords/domain/internet/mac-keyword-definition.jspackages/core/js/keywords/domain/internet/url-keyword-definition.jspackages/core/js/keywords/domain/location/country-code-keyword-definition.jspackages/core/js/keywords/domain/lorem/word-keyword-definition.jspackages/core/js/keywords/domain/person/first-name-keyword-definition.jspackages/core/js/keywords/domain/person/last-name-keyword-definition.jspackages/core/js/keywords/domain/person/middle-name-keyword-definition.jspackages/core/js/keywords/domain/person/prefix-keyword-definition.jspackages/core/js/keywords/domain/phone/number-keyword-definition.jspackages/core/js/keywords/domain/string/alpha-keyword-definition.jspackages/core/js/keywords/domain/string/alphanumeric-keyword-definition.jspackages/core/js/keywords/domain/string/hexadecimal-keyword-definition.jspackages/core/js/keywords/domain/string/uuid-keyword-definition.jspackages/core/js/keywords/domain/word/adjective-keyword-definition.jspackages/core/js/keywords/domain/word/adverb-keyword-definition.jspackages/core/js/keywords/domain/word/conjunction-keyword-definition.jspackages/core/js/keywords/domain/word/interjection-keyword-definition.jspackages/core/js/keywords/domain/word/noun-keyword-definition.jspackages/core/js/keywords/domain/word/preposition-keyword-definition.jspackages/core/js/keywords/domain/word/sample-keyword-definition.jspackages/core/js/keywords/domain/word/verb-keyword-definition.jspackages/core/src/tests/data_generation/keywords/domain/autoincrement/timestamp-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domain-keyword-params-usage.test.jspackages/core/src/tests/data_generation/unit/domain/domainKeywords.test.js
This was referenced Jul 2, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/js/data_generation/utils/enumParser.js (1)
106-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "unquoted empty field" check across three call sites.
The predicate
field.value.length === 0 && !field.quoted(or its negation via.every) is reimplemented independently inisImplicitCsvEnumRuleSpec(Line 114), the implicit-CSV branch ofparseEnumRuleSpec(Line 184), andparseCsvEnumValues(Line 418). Extracting a shared helper (e.g.hasUnquotedEmptyField(fields)) would keep these three call sites in sync as the empty-value rules evolve.♻️ Suggested consolidation
+ static hasUnquotedEmptyField(fields) { + return fields.some((field) => field.value.length === 0 && !field.quoted); + } + static isImplicitCsvEnumRuleSpec(ruleSpec) { ... try { const fields = this.parseCsvLiteralFields(spec); - return fields.length >= 2 && fields.every((field) => field.value.length > 0 || field.quoted); + return fields.length >= 2 && !this.hasUnquotedEmptyField(fields); } catch { return false; } }Also applies to: 180-192, 398-422
🤖 Prompt for 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. In `@packages/core/js/data_generation/utils/enumParser.js` around lines 106 - 118, The empty unquoted CSV field check is duplicated across isImplicitCsvEnumRuleSpec, the implicit-CSV branch in parseEnumRuleSpec, and parseCsvEnumValues. Extract a shared helper for this rule (for example, a predicate over the parsed fields) and update all three call sites to use it so the empty-value behavior stays consistent in EnumParser.
🤖 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.
Nitpick comments:
In `@packages/core/js/data_generation/utils/enumParser.js`:
- Around line 106-118: The empty unquoted CSV field check is duplicated across
isImplicitCsvEnumRuleSpec, the implicit-CSV branch in parseEnumRuleSpec, and
parseCsvEnumValues. Extract a shared helper for this rule (for example, a
predicate over the parsed fields) and update all three call sites to use it so
the empty-value behavior stays consistent in EnumParser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 935bfd21-88a7-41b9-b055-80dc74475207
📒 Files selected for processing (14)
apps/web/src/tests/browser/generator/functional/schema-edit.spec.jspackages/core-ui/js/gui_components/shared/schema-row-rule-mapper.jspackages/core-ui/src/tests/generator/schema-row-rule-mapper.test.jspackages/core-ui/src/tests/shared/shared-schema-definition-view.test.jspackages/core/js/data_generation/enum/enumTestDataRuleValidator.jspackages/core/js/data_generation/utils/enumParser.jspackages/core/js/keywords/domain/datatype/datatype-enum.jspackages/core/js/keywords/domain/datatype/enum-keyword-definition.jspackages/core/src/tests/core-api/generateFromTextSpec.test.jspackages/core/src/tests/data_generation/enum-compiler-integration.test.jspackages/core/src/tests/data_generation/enum-surface-parity.test.jspackages/core/src/tests/data_generation/keywords/domain/datatype/enum-keyword-definition.test.jspackages/core/src/tests/data_generation/unit/enum/enumParser.test.jspackages/core/src/tests/data_generation/unit/enum/enumTestDataRuleValidator.test.js
💤 Files with no reviewable changes (1)
- packages/core/js/data_generation/enum/enumTestDataRuleValidator.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/js/data_generation/domain/domainTestDataRuleValidator.js (1)
83-84: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider skipping execution when faker isolation is unavailable.
createIsolatedFakersilently returns the originalfakerInstancewhenrawDefinitionsis missing. For faker-type delegates, this means the delegate executes against the caller's live faker, potentially advancing its internal RNG state. The test at line 109-119 indomain-test-data-rule-validator.test.jsverifies no-state-advancement, but only because the standard@faker-js/fakerinstance hasrawDefinitions. A non-standard or mock faker withoutrawDefinitionswould silently mutate.Returning
nullhere and havingcreateValidationExecutionContexttreatnullas "skip execution" would be safer.🤖 Prompt for 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. In `@packages/core/js/data_generation/domain/domainTestDataRuleValidator.js` around lines 83 - 84, Update createIsolatedFaker to return null when rawDefinitions is missing or invalid instead of the original fakerInstance, and update createValidationExecutionContext to treat a null isolated faker as a skipped execution for faker-type delegates. Ensure callers handle this skip without invoking the delegate or mutating the caller’s faker state.packages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.js (1)
34-49: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
argsValidatorcauses redundant delegate execution during full validation.
validateAutoIncrementTimestampArgscallsexecuteCustomAutoIncrementTimestampto validate args. WhenvalidateDomainKeywordExecutionsubsequently callsexecuteDomainKeyword, the internalvalidateDomainKeywordArgscall re-invokes thisargsValidator, and then the custom delegate itself is called a third time. ForautoIncrement.timestamp, this meansexecuteCustomAutoIncrementTimestampruns 3 times per validation pass.The delegate is lightweight, so the cost is low. But for keywords with heavier custom delegates, this pattern could become expensive. Consider whether the
argsValidatorshould be skipped when execution validation is already planned, or whetherexecuteDomainKeywordshould skip re-validating args when called from the validation path.🤖 Prompt for 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. In `@packages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.js` around lines 34 - 49, Remove the redundant delegate execution during full validation: coordinate validateDomainKeywordExecution and executeDomainKeyword so argsValidator is not invoked again when execution validation has already run, while preserving standalone argument validation. Use the existing validateAutoIncrementTimestampArgs, validateDomainKeywordArgs, validateDomainKeywordExecution, and executeDomainKeyword symbols to thread an explicit validation-state/skip option through the call path.
🤖 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/test-data/ui/params-editor-modal.css`:
- Line 326: Replace the deprecated clip declaration in the relevant
accessibility-hidden style rule with clip-path: inset(50%), preserving the
rule’s existing behavior and other declarations.
---
Nitpick comments:
In `@packages/core/js/data_generation/domain/domainTestDataRuleValidator.js`:
- Around line 83-84: Update createIsolatedFaker to return null when
rawDefinitions is missing or invalid instead of the original fakerInstance, and
update createValidationExecutionContext to treat a null isolated faker as a
skipped execution for faker-type delegates. Ensure callers handle this skip
without invoking the delegate or mutating the caller’s faker state.
In
`@packages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.js`:
- Around line 34-49: Remove the redundant delegate execution during full
validation: coordinate validateDomainKeywordExecution and executeDomainKeyword
so argsValidator is not invoked again when execution validation has already run,
while preserving standalone argument validation. Use the existing
validateAutoIncrementTimestampArgs, validateDomainKeywordArgs,
validateDomainKeywordExecution, and executeDomainKeyword symbols to thread an
explicit validation-state/skip option through the call path.
🪄 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: 61c68f08-28d4-42d6-9b4f-d9266c2be937
📒 Files selected for processing (11)
.gitignorepackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.csspackages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.jspackages/core-ui/src/tests/shared/schema-row-validation.test.jspackages/core-ui/src/tests/shared/shared-schema-editor-controller.test.jspackages/core-ui/src/tests/utils/params-editor-modal.test.jspackages/core/js/data_generation/domain/domainTestDataRuleValidator.jspackages/core/js/keywords/domain/autoincrement/timestamp-keyword-definition.jspackages/core/src/tests/data_generation/keywords/domain/autoincrement/timestamp-keyword-definition.test.jspackages/core/src/tests/data_generation/unit/domain/domain-test-data-rule-validator.test.js
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/tests/data_generation/keywords/domain/autoincrement/timestamp-keyword-definition.test.js
- packages/core-ui/src/tests/shared/shared-schema-editor-controller.test.js
- packages/core-ui/src/tests/utils/params-editor-modal.test.js
| width: 1px; | ||
| height: 1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace deprecated clip property with clip-path.
Stylelint flags clip: rect(0, 0, 0, 0) as a deprecated property. The modern equivalent is clip-path: inset(50%), which is well-supported across current browsers.
🔧 Proposed fix
.params-editor-table thead {
position: absolute;
width: 1px;
height: 1px;
overflow: hidden;
- clip: rect(0, 0, 0, 0);
+ clip-path: inset(50%);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| clip: rect(0, 0, 0, 0); | |
| clip-path: inset(50%); |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 326-326: Deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for 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.
In
`@packages/core-ui/js/gui_components/shared/test-data/ui/params-editor-modal.css`
at line 326, Replace the deprecated clip declaration in the relevant
accessibility-hidden style rule with clip-path: inset(50%), preserving the
rule’s existing behavior and other declarations.
Source: Linters/SAST tools
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
type: "enum"withenumValuesarrays, including canonical plural timestamp unitsValidation
pnpm run verify:uipnpm run test:browserpnpm run test:storybookpnpm run build-storybookpnpm run verify:localpnpm run format:checkandpnpm run testverbosepnpm run verify:localSummary by CodeRabbit