Implement structured command usage examples and validators#231
Open
eviltester wants to merge 4 commits into
Open
Implement structured command usage examples and validators#231eviltester wants to merge 4 commits into
eviltester wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR standardizes command help metadata around structured ChangesCommand-help rollout
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
This PR standardizes command help metadata by making structured usageExamples (plus explicit validators) the source of truth across docs, UI help surfaces, and contract tests, aligning with the requirements in #226.
Changes:
- Introduces/consumes structured
usageExamples(withfunctionCall,sampleReturnValue,description) and explicit validator functions as the primary help contract. - Updates domain docs generation to derive return examples from
usageExamplesand improves sampling for literal-union types when generating example invocations. - Refreshes/extends tests and fixtures to validate parsing, example execution, and updated behavior (including some increased timeouts).
Reviewed changes
Copilot reviewed 99 out of 100 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/cross-surface-option-parity.test.js | Increases MCP server request timeout for cross-surface parity integration tests. |
| scripts/generate-domain-docs.mjs | Switches docs generation to pull return examples from usageExamples, improves sample arg generation for literal unions, and separates docsUrl vs fakerDocsUrl output. |
| packages/core/src/tests/data_generation/unit/domain/domain-result-assertions.test-helper.js | Tightens expected httpMethod outputs to match updated return type/validator contract. |
| packages/core/src/tests/data_generation/unit/domain/domain-param-invocation-coverage.test-helper.js | Enhances sample value generation for literal unions and adds keyword-specific sampling for network args. |
| packages/core/src/tests/data_generation/unit/domain/domain-keyword-params-usage.test.js | Updates param sampling to better honor literal unions and keyword-specific argument values. |
| packages/core/src/tests/data_generation/unit/domain/domain-internet-exec.test.js | Aligns internet.httpMethod execution assertions with updated allowed methods. |
| packages/core/src/tests/data_generation/unit/domain/domain-image-exec.test.js | Removes execution coverage for image.urlLoremFlickr (no longer present in definitions). |
| packages/core/src/tests/data_generation/unit/domain/domain-doc-param-examples.test.js | Reworks doc example tests to validate structured usage examples parse and only use documented args. |
| packages/core/src/tests/data_generation/unit/domain/domain-doc-generator-output.test.js | Makes doc output assertions more robust using regex matching for inline JSON code formatting. |
| packages/core/src/tests/command-help/command-definition-usage-examples.test.js | Adds contract tests to enforce “every command has at least one usage example (and validator where required)” across definition modules. |
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge; the two findings are code-quality observations that do not affect runtime behaviour. The dead branch in packages/core/js/domain/domain-keywords.js — dead branch and duplicated helpers are worth cleaning up before this pattern spreads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Keyword Definition\n usageExamples / validator / returnType] -->|buildDomainKeywordCatalog| B[normalizeUsageExamples\ncommand-help-contract.js]
B --> C[normalizeDomainUsageExamplesToNamedForm\ndomain-keywords.js]
C --> D[Catalog Entry\nnormalized named-arg examples]
D --> E[getDomainKeywordHelpByAlias\nDomainCommandHelp]
E --> F[getDomainCommandHelp\ncore-ui metadata]
F --> G[UI / Help Surface]
H[Faker Helper Definitions\nFAKER_HELPER_KEYWORD_DEFINITIONS] -->|normalizeFakerHelperKeywordHelp| B
I[validateCommandHelpValue\ncommand-help-contract.js] -->|validator fn| J[command-help-validators.js\n~50 named validators]
J --> K{value + context}
K -->|createPredicateValidator| L[format checks\nLuhn / IBAN / UUID / etc]
K -->|createRegexValidator| M[regex patterns\nBIC / MIME / semver / etc]
K -->|context-aware| N[enum / counterstring\nliteral / account]
%%{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[Keyword Definition\n usageExamples / validator / returnType] -->|buildDomainKeywordCatalog| B[normalizeUsageExamples\ncommand-help-contract.js]
B --> C[normalizeDomainUsageExamplesToNamedForm\ndomain-keywords.js]
C --> D[Catalog Entry\nnormalized named-arg examples]
D --> E[getDomainKeywordHelpByAlias\nDomainCommandHelp]
E --> F[getDomainCommandHelp\ncore-ui metadata]
F --> G[UI / Help Surface]
H[Faker Helper Definitions\nFAKER_HELPER_KEYWORD_DEFINITIONS] -->|normalizeFakerHelperKeywordHelp| B
I[validateCommandHelpValue\ncommand-help-contract.js] -->|validator fn| J[command-help-validators.js\n~50 named validators]
J --> K{value + context}
K -->|createPredicateValidator| L[format checks\nLuhn / IBAN / UUID / etc]
K -->|createRegexValidator| M[regex patterns\nBIC / MIME / semver / etc]
K -->|context-aware| N[enum / counterstring\nliteral / account]
Reviews (1): Last reviewed commit: "Implement structured command examples an..." | 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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate-domain-docs.mjs (1)
264-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLiteral-union sampling is bypassed by earlier name defaults.
sampleValueForArgapplies hardcoded name fallbacks beforegetLiteralUnionValues(...). That can override declared literal unions (e.g.,version: 4|7) and generate non-contract values for examples.💡 Proposed fix
function sampleValueForArg(argSpec, keyword = '') { const name = String(argSpec?.name || '').trim(); const typeName = String(argSpec?.type || '').trim(); const examples = Array.isArray(argSpec?.examples) ? argSpec.examples.filter((value) => value !== undefined) : []; if (examples.length > 0) { return examples[0]; } + + const literalUnionValues = getLiteralUnionValues(typeName); + if (literalUnionValues.length > 0) { + return choosePreferredLiteralUnionValue(literalUnionValues, name, keyword); + } + if (name === 'firstName') return 'Alex'; if (name === 'lastName') return 'Taylor'; if (name === 'provider') return 'example.com'; if (name === 'aircraftType') return 'narrowbody'; if (name === 'countryCode') return 'GB'; @@ - if (name === 'version') return 'v4'; + if (name === 'version') return 4; @@ - const literalUnionValues = getLiteralUnionValues(typeName); - if (literalUnionValues.length > 0) { - return choosePreferredLiteralUnionValue(literalUnionValues, name, keyword); - }🤖 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 `@scripts/generate-domain-docs.mjs` around lines 264 - 299, The hardcoded name-based fallbacks in the sampleValueForArg function are being evaluated before the literal union value check, which causes the function to return incorrect values for parameters with declared literal union types like version: 4|7. Move the getLiteralUnionValues and choosePreferredLiteralUnionValue logic to execute at the beginning of the function (after extracting name, typeName, and examples) so that literal union types are evaluated first and have priority over the generic hardcoded name fallbacks.
🟡 Minor comments (10)
packages/core/js/domain/domain-keywords.js-69-72 (1)
69-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve named-argument types by argument name, not index.
In mixed named/positional examples, using
args[index]?.typeforargument.kind === 'named'can apply the wrong type serializer when named args are out of positional order.💡 Proposed fix
function normalizeDomainExampleFunctionCall(functionCall, keyword, args) { @@ + const argTypeByName = new Map(args.map((arg) => [arg?.name, arg?.type])); const namedArgs = parsed.arguments.map((argument, index) => { if (argument.kind === 'named') { - return `${argument.name}=${serializeDomainInvocationValue(argument.value, args[index]?.type)}`; + return `${argument.name}=${serializeDomainInvocationValue(argument.value, argTypeByName.get(argument.name))}`; } @@ });🤖 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/domain/domain-keywords.js` around lines 69 - 72, The code currently uses index-based lookup (args[index]?.type) to resolve the type for named arguments, which fails when named arguments are out of positional order. In the map function where argument.kind === 'named', instead of accessing args[index], locate the correct argument object in the args array by matching its name property to argument.name, then extract the type from that matched argument to pass to serializeDomainInvocationValue. This ensures named arguments are typed correctly regardless of their position relative to positional arguments.packages/core/js/command-help/command-help-contract.js-110-112 (1)
110-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNumeric literal return types are not coerced consistently.
createReturnValueValidatoraccepts numeric literal tokens (e.g.,0|1), butcoerceSampleReturnValueonly converts numeric strings whenintegerornumberis present. That leaves"1"as a string for numeric-literal return contracts and can fail validator checks unexpectedly.💡 Proposed fix
function coerceSampleReturnValue(value, returnType = '') { @@ if ((allowedTypes.includes('integer') || allowedTypes.includes('number')) && /^[+-]?\d+(?:\.\d+)?$/u.test(unquoted)) { return Number(unquoted); } + if ( + allowedTypes.some((typeToken) => /^[+-]?\d+(?:\.\d+)?$/.test(typeToken)) && + /^[+-]?\d+(?:\.\d+)?$/u.test(unquoted) + ) { + return Number(unquoted); + } if (allowedTypes.includes('date')) { return unquoted; }🤖 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/command-help/command-help-contract.js` around lines 110 - 112, The numeric coercion in the condition checking for 'integer' or 'number' does not handle numeric literal return types like 0|1. Expand the condition in the if statement to also check whether any of the allowedTypes are themselves numeric values (by testing if they can be parsed as numbers), in addition to the existing check for the 'integer' and 'number' type strings. This ensures that when allowedTypes contains numeric literals, the unquoted string value is properly converted to a Number for consistent validation behavior.packages/core/js/domain/domain-faker-lorem-keyword-definitions.js-210-213 (1)
210-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect mismatched lorem sample outputs in
usageExamples.Several samples are inconsistent with their function calls (e.g., unexpected
5separators andseparator="-"examples that don’t show hyphen-separated output). These examples are currently misleading.Also applies to: 224-229, 391-394, 403-406
🤖 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/domain/domain-faker-lorem-keyword-definitions.js` around lines 210 - 213, The `usageExamples` array contains mismatched sample return values that do not accurately reflect what the corresponding function calls would produce. Correct the `sampleReturnValue` fields to match their respective `functionCall` definitions. Specifically, for the `lorem.paragraphs` examples with `max=5` parameter, the sample output should contain 5 properly formatted paragraphs without extraneous numeric separators like "5". For any examples using a `separator` parameter (such as `separator="-"`), ensure the sample return value actually displays the specified separator between elements rather than showing default formatting. This applies to all mismatched examples in the usageExamples entries throughout the file at the specified locations.packages/core/js/domain/domain-faker-internet-keyword-definitions.js-390-414 (1)
390-414:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid embedding full JWT-shaped literals in usage sample values.
These token-like strings are already being flagged by static analysis and can create leak-scanner noise or CI gating friction. Prefer sanitized fixture values or an approved suppression mechanism for docs/examples.
🤖 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/domain/domain-faker-internet-keyword-definitions.js` around lines 390 - 414, The full JWT tokens in the sampleReturnValue fields of the usageExamples array for internet.jwt() are triggering security scanning tools. Replace each of the four complete JWT token strings (in the examples without parameters, with header parameter, with payload parameter, and with refDate parameter) with sanitized versions such as truncated tokens with ellipsis indicators, placeholder text like "eyJhbGciOiJSUzI1NiIs..." followed by "...truncated", or use an approved suppression mechanism to prevent leak-scanner false positives while maintaining the example clarity.Source: Linters/SAST tools
packages/core/js/domain/domain-faker-string-keyword-definitions.js-355-357 (1)
355-357:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse digit exclusions in the
string.numericexample.
excludeis documented as an array of digits, but this example uses letters, so it doesn’t demonstrate the parameter meaningfully.Suggested fix
- functionCall: 'string.numeric(exclude=["A","B","C"])', + functionCall: 'string.numeric(exclude=["1","2","3"])',🤖 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/domain/domain-faker-string-keyword-definitions.js` around lines 355 - 357, The functionCall property in the string.numeric example uses letters like "A", "B", "C" in the exclude array, but since string.numeric generates numeric strings, the exclude parameter should contain actual digits to demonstrate the feature meaningfully. Replace the exclude array with digit characters such as "0", "1", "2" or similar numeric digits to properly show how the exclude parameter filters out unwanted digits from the numeric output.packages/core/js/domain/domain-faker-system-keyword-definitions.js-175-177 (1)
175-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
system.fileExtMIME-type example has an inconsistent sample return.Line 175 uses
mimeType="image/png"but Line 176 shows7z, which does not match the constrained call intent. This should use a PNG-compatible extension sample.🤖 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/domain/domain-faker-system-keyword-definitions.js` around lines 175 - 177, The sampleReturnValue for the system.fileExt example is inconsistent with its functionCall definition. The functionCall specifies mimeType="image/png" but the sampleReturnValue is set to '7z', which is a compressed archive extension and does not correspond to PNG images. Change the sampleReturnValue from '7z' to 'png' to match the PNG MIME type specified in the functionCall parameter.docs-src/docs/040-test-data/domain/260-string.md-312-313 (1)
312-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
string.numericexclude example uses letters instead of digits.Line 312 shows
exclude=["A","B","C"], but this method documents digit exclusion. Use numeric characters in the example to match actual usage intent.🤖 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 `@docs-src/docs/040-test-data/domain/260-string.md` around lines 312 - 313, The string.numeric method exclude parameter example at the specified location uses letters (A, B, C) instead of numeric digits. Replace the exclude array values with actual digit characters (such as 0, 1, 2 or equivalent numeric strings) to correctly demonstrate the intended behavior of excluding specific digits from the generated numeric string.docs-src/docs/040-test-data/domain/200-lorem.md-175-175 (1)
175-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHyphenate compound modifiers in method descriptions.
Use
space-separated/hyphen-separatedin these lines for correct grammar.✏️ Proposed wording update
- Generates a space separated list of words ... + Generates a space-separated list of words ... - ... hyphen separated words. + ... hyphen-separated words. - Generates a space separated list of words. + Generates a space-separated list of words.Also applies to: 281-281, 394-394
🤖 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 `@docs-src/docs/040-test-data/domain/200-lorem.md` at line 175, The method descriptions in the Lorem markdown documentation use unhyphenated compound modifiers that should be hyphenated for correct grammar. In the description at line 175 and the similar instances also mentioned at lines 281 and 394, change "space separated" to "space-separated" and apply hyphenation to any other compound modifiers like "hyphen separated" to "hyphen-separated" to ensure proper formatting of compound adjectives in the documentation.Source: Linters/SAST tools
docs-src/docs/040-test-data/domain/200-lorem.md-153-170 (1)
153-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSeparator example outputs don’t match the shown invocation.
Line 153 and Line 259 use
separator="-", but Line 167 and Line 273 show5as the separator in return samples. Please align the sample return values (or invocation) so they represent the same example.💡 Proposed doc fix
- `... stultus.5Varius ... aperio.5Spoliatio ...` + `... stultus.-Varius ... aperio.-Spoliatio ...` - `... crux fugit.5Stillicidium ... utor.5Ago ...` + `... crux fugit.-Stillicidium ... utor.-Ago ...`Also applies to: 259-276
🤖 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 `@docs-src/docs/040-test-data/domain/200-lorem.md` around lines 153 - 170, The example return values for the lorem.paragraphs(separator="-") invocation are inconsistent. One of the sample outputs shows paragraphs separated by "5" instead of "-", which does not match the documented function call. Replace the example return value that contains "5" as the separator (the one showing three paragraphs joined with "5") with a correct example that uses "-" as the separator to match the lorem.paragraphs(separator="-") invocation shown above it.packages/core-ui/src/tests/interaction/support/generated-value-quality.js-19-30 (1)
19-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrimitive token list is missing
comma-separated list.This parser can treat
comma-separated listas a literal-union member, causing false negatives in row value checks for that return type.💡 Proposed fix
const PRIMITIVE_TYPE_TOKENS = new Set([ 'string', 'number', 'integer', 'boolean', 'array', 'object', 'date', 'regexp', 'unknown', 'bigint', + 'comma-separated list', ]);🤖 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/src/tests/interaction/support/generated-value-quality.js` around lines 19 - 30, The PRIMITIVE_TYPE_TOKENS Set in the generated-value-quality.js file is missing the 'comma-separated list' token, which causes it to be incorrectly treated as a literal-union member and leads to false negatives in row value checks. Add 'comma-separated list' as a new entry to the PRIMITIVE_TYPE_TOKENS Set definition to ensure the parser correctly recognizes it as a primitive type token.
🧹 Nitpick comments (2)
apps/web/src/stories/method-picker-dialog.stories.js (1)
47-61: 💤 Low valueConsider importing shared helpers from method-picker-modal.js.
The story file duplicates
getReturnExamplesandgetUsageFunctionCallslogic frommethod-picker-modal.js. The story'sgetReturnExamplesuses simpleString()coercion while the modal usesnormalizeReturnExampleValue(which handlesbigint,Date, arrays differently).For Storybook visual inspection this is likely fine, but importing the shared helpers would ensure parity and reduce maintenance burden.
🤖 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/stories/method-picker-dialog.stories.js` around lines 47 - 61, Remove the duplicate helper functions `getReturnExamples` and `getUsageFunctionCalls` from the story file and import them directly from method-picker-modal.js instead. This will ensure the story uses the same logic as the actual modal implementation (including proper normalization via normalizeReturnExampleValue) and eliminate code duplication.docs-src/docs/040-test-data/domain/120-finance.md (1)
112-137: 💤 Low valueMinor: Remove redundant phrasing in SWIFT and PIN descriptions.
LanguageTool flags two tautologies:
- Line 114: "SWIFT/BIC code" → BIC already stands for "code," making this redundant
- Line 401: "PIN number" → PIN already stands for "number," making this redundant
Consider simplifying to "SWIFT/BIC" and "PIN" respectively for conciseness.
🤖 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 `@docs-src/docs/040-test-data/domain/120-finance.md` around lines 112 - 137, Remove redundant phrasing in two locations within the documentation. In the finance.bic section, change "SWIFT/BIC code" to "SWIFT/BIC" in the description since BIC already stands for Bank Identifier Code. Additionally, locate the PIN description elsewhere in the documentation (referenced at line 401) and change "PIN number" to "PIN" since PIN already stands for Personal Identification Number. These changes will eliminate tautologies and improve conciseness of the documentation.Source: Linters/SAST tools
🤖 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/src/tests/interaction/app-test-data-focused-schema-sync.test.js`:
- Line 18: The jest.setTimeout(45000) call represents a 3x increase from the
original 15-second timeout, which may indicate underlying performance issues in
the test suite rather than being a legitimate requirement. Investigate the
specific tests in this suite to identify which ones are slow and causing the
timeout increase, then either optimize those slow tests to run faster and revert
the timeout to a more reasonable value, or provide clear documentation of why
the extended timeout is necessary if the tests cannot be optimized further.
In `@packages/core/js/command-help/command-help-contract.js`:
- Around line 121-123: In the `validateCommandHelpValue` function where you
check if validator is a RegExp instance, you need to reset the RegExp's
lastIndex property to 0 before calling test() to prevent nondeterministic
behavior with stateful regexes (those with global or sticky flags). Check if the
validator has the `g` or `y` flags, and if so, set validator.lastIndex = 0
before calling validator.test(String(value ?? '')), ensuring the regex starts
from the beginning on each validation call.
In `@packages/core/js/command-help/command-help-validators.js`:
- Around line 476-488: The getCounterStringRangeFromContext function currently
only reads bounds from context.args but should also check
context.fieldDefinition for rule-specific counterstring limits. Update the
function to first attempt to extract min and max bounds from
context.fieldDefinition if it exists, then fall back to reading from
context.args if fieldDefinition bounds are not available. This ensures that
validators receive the configured counterstring length limits when invoked with
the (value, context) signature as per coding guidelines.
In `@packages/core/js/domain/domain-custom-literal-keyword-definitions.js`:
- Around line 18-24: The usageExamples in the literal.value function calls are
using positional parameter syntax (literal.value("...")) but should use
named-parameter form for consistency with domain guidelines. Convert all
literal.value invocations in the usageExamples array to use named parameters
(for example, literal.value(text="Pending") and literal.value(text="")) instead
of positional arguments to maintain consistency across the domain documentation
and examples.
In `@packages/core/js/domain/domain-faker-color-keyword-definitions.js`:
- Around line 64-80: The usageExamples array for color.colorByCSSColorSpace() is
missing a combined example that demonstrates all optional parameters working
together. Add a fourth usage example object to the usageExamples array that
shows both the format and space parameters being used simultaneously (e.g.,
format="decimal" and space="display-p3"). This combined example should follow
the same structure as the existing examples with functionCall,
sampleReturnValue, and description properties.
- Around line 332-358: The usageExamples array in the color keyword definitions
for color.rgb currently contains five examples, each demonstrating only a single
parameter in isolation. To meet the PR objectives requirement `#226`, add at least
one additional example object to the usageExamples array that combines multiple
parameters together (such as format, casing, and includeAlpha used
simultaneously) to demonstrate how these parameters work together. This combined
example should follow the same structure as the existing examples with
functionCall, sampleReturnValue, and description properties.
In `@packages/core/js/domain/domain-faker-date-keyword-definitions.js`:
- Around line 163-194: The usageExamples for the date.birthdate function is
missing a min-only usage example. Add a new example object to the usageExamples
array that demonstrates using only the min parameter in isolation (similar to
how max is shown in isolation). Additionally, according to the comment, the same
coverage issue applies to other functions listed in the "Also applies to"
section: date.future, date.past, date.recent, date.soon, date.month, and
date.weekday. For each of these functions, ensure their usageExamples arrays
include at least one example that combines multiple optional parameters together
to demonstrate how they work when used simultaneously.
In `@packages/core/js/domain/domain-faker-finance-keyword-definitions.js`:
- Around line 91-122: The usageExamples array for the finance.amount method is
missing two required examples for issue `#226` compliance: an
all-parameters-together example that combines multiple parameters (autoFormat,
dec, max, min, and symbol) in a single call, and an isolated min parameter
example showing finance.amount with only the min parameter. Additionally, locate
the usageExamples arrays for finance.bitcoinAddress and finance.iban methods
(referenced in the comment as also applying to lines 210-226 and 473-489) and
add similar all-parameters-together examples to each of those methods as well to
ensure complete contract coverage.
In `@packages/core/js/domain/domain-faker-image-keyword-definitions.js`:
- Around line 106-122: The usageExamples array for image.url is missing an
example that demonstrates both height and width parameters used together. Add a
fourth usage example object to the usageExamples array that shows a function
call with both height and width parameters specified (e.g., 'image.url(height=X,
width=Y)'), include an appropriate sampleReturnValue URL that reflects both
dimensions being applied, and provide a clear description explaining that this
example shows image.url using both height and width parameters together.
In `@packages/core/js/domain/domain-faker-word-keyword-definitions.js`:
- Around line 31-33: The usageExamples objects contain inconsistent sample
return values that violate their own declared max parameter constraints. For all
instances where the functionCall specifies `max=5` (at the locations indicated:
31-33, 89-91, 147-149, 263-265, 321-323, 380-382, 438-440, 496-498), update the
sampleReturnValue field to contain a word that is 5 characters or fewer in
length. For example, replace 'inferior' (8 characters) with a shorter word like
'small' (5 characters) that satisfies the max constraint declared in the
corresponding functionCall.
In
`@packages/core/src/tests/data_generation/unit/domain/domain-doc-param-examples.test.js`:
- Around line 24-31: The test currently only validates that documented named
parameters are used, but does not enforce that all arguments must be in
named-parameter form. Add a check to ensure that all entries in parsed.arguments
have kind === 'named'. If any arguments have a different kind (such as
positional parameters), add them to the invalid array with an appropriate error
message indicating that positional invocation form is not allowed for non-helper
commands. This validation should be added alongside the existing check for
undocumented named parameters.
---
Outside diff comments:
In `@scripts/generate-domain-docs.mjs`:
- Around line 264-299: The hardcoded name-based fallbacks in the
sampleValueForArg function are being evaluated before the literal union value
check, which causes the function to return incorrect values for parameters with
declared literal union types like version: 4|7. Move the getLiteralUnionValues
and choosePreferredLiteralUnionValue logic to execute at the beginning of the
function (after extracting name, typeName, and examples) so that literal union
types are evaluated first and have priority over the generic hardcoded name
fallbacks.
---
Minor comments:
In `@docs-src/docs/040-test-data/domain/200-lorem.md`:
- Line 175: The method descriptions in the Lorem markdown documentation use
unhyphenated compound modifiers that should be hyphenated for correct grammar.
In the description at line 175 and the similar instances also mentioned at lines
281 and 394, change "space separated" to "space-separated" and apply hyphenation
to any other compound modifiers like "hyphen separated" to "hyphen-separated" to
ensure proper formatting of compound adjectives in the documentation.
- Around line 153-170: The example return values for the
lorem.paragraphs(separator="-") invocation are inconsistent. One of the sample
outputs shows paragraphs separated by "5" instead of "-", which does not match
the documented function call. Replace the example return value that contains "5"
as the separator (the one showing three paragraphs joined with "5") with a
correct example that uses "-" as the separator to match the
lorem.paragraphs(separator="-") invocation shown above it.
In `@docs-src/docs/040-test-data/domain/260-string.md`:
- Around line 312-313: The string.numeric method exclude parameter example at
the specified location uses letters (A, B, C) instead of numeric digits. Replace
the exclude array values with actual digit characters (such as 0, 1, 2 or
equivalent numeric strings) to correctly demonstrate the intended behavior of
excluding specific digits from the generated numeric string.
In `@packages/core-ui/src/tests/interaction/support/generated-value-quality.js`:
- Around line 19-30: The PRIMITIVE_TYPE_TOKENS Set in the
generated-value-quality.js file is missing the 'comma-separated list' token,
which causes it to be incorrectly treated as a literal-union member and leads to
false negatives in row value checks. Add 'comma-separated list' as a new entry
to the PRIMITIVE_TYPE_TOKENS Set definition to ensure the parser correctly
recognizes it as a primitive type token.
In `@packages/core/js/command-help/command-help-contract.js`:
- Around line 110-112: The numeric coercion in the condition checking for
'integer' or 'number' does not handle numeric literal return types like 0|1.
Expand the condition in the if statement to also check whether any of the
allowedTypes are themselves numeric values (by testing if they can be parsed as
numbers), in addition to the existing check for the 'integer' and 'number' type
strings. This ensures that when allowedTypes contains numeric literals, the
unquoted string value is properly converted to a Number for consistent
validation behavior.
In `@packages/core/js/domain/domain-faker-internet-keyword-definitions.js`:
- Around line 390-414: The full JWT tokens in the sampleReturnValue fields of
the usageExamples array for internet.jwt() are triggering security scanning
tools. Replace each of the four complete JWT token strings (in the examples
without parameters, with header parameter, with payload parameter, and with
refDate parameter) with sanitized versions such as truncated tokens with
ellipsis indicators, placeholder text like "eyJhbGciOiJSUzI1NiIs..." followed by
"...truncated", or use an approved suppression mechanism to prevent leak-scanner
false positives while maintaining the example clarity.
In `@packages/core/js/domain/domain-faker-lorem-keyword-definitions.js`:
- Around line 210-213: The `usageExamples` array contains mismatched sample
return values that do not accurately reflect what the corresponding function
calls would produce. Correct the `sampleReturnValue` fields to match their
respective `functionCall` definitions. Specifically, for the `lorem.paragraphs`
examples with `max=5` parameter, the sample output should contain 5 properly
formatted paragraphs without extraneous numeric separators like "5". For any
examples using a `separator` parameter (such as `separator="-"`), ensure the
sample return value actually displays the specified separator between elements
rather than showing default formatting. This applies to all mismatched examples
in the usageExamples entries throughout the file at the specified locations.
In `@packages/core/js/domain/domain-faker-string-keyword-definitions.js`:
- Around line 355-357: The functionCall property in the string.numeric example
uses letters like "A", "B", "C" in the exclude array, but since string.numeric
generates numeric strings, the exclude parameter should contain actual digits to
demonstrate the feature meaningfully. Replace the exclude array with digit
characters such as "0", "1", "2" or similar numeric digits to properly show how
the exclude parameter filters out unwanted digits from the numeric output.
In `@packages/core/js/domain/domain-faker-system-keyword-definitions.js`:
- Around line 175-177: The sampleReturnValue for the system.fileExt example is
inconsistent with its functionCall definition. The functionCall specifies
mimeType="image/png" but the sampleReturnValue is set to '7z', which is a
compressed archive extension and does not correspond to PNG images. Change the
sampleReturnValue from '7z' to 'png' to match the PNG MIME type specified in the
functionCall parameter.
In `@packages/core/js/domain/domain-keywords.js`:
- Around line 69-72: The code currently uses index-based lookup
(args[index]?.type) to resolve the type for named arguments, which fails when
named arguments are out of positional order. In the map function where
argument.kind === 'named', instead of accessing args[index], locate the correct
argument object in the args array by matching its name property to
argument.name, then extract the type from that matched argument to pass to
serializeDomainInvocationValue. This ensures named arguments are typed correctly
regardless of their position relative to positional arguments.
---
Nitpick comments:
In `@apps/web/src/stories/method-picker-dialog.stories.js`:
- Around line 47-61: Remove the duplicate helper functions `getReturnExamples`
and `getUsageFunctionCalls` from the story file and import them directly from
method-picker-modal.js instead. This will ensure the story uses the same logic
as the actual modal implementation (including proper normalization via
normalizeReturnExampleValue) and eliminate code duplication.
In `@docs-src/docs/040-test-data/domain/120-finance.md`:
- Around line 112-137: Remove redundant phrasing in two locations within the
documentation. In the finance.bic section, change "SWIFT/BIC code" to
"SWIFT/BIC" in the description since BIC already stands for Bank Identifier
Code. Additionally, locate the PIN description elsewhere in the documentation
(referenced at line 401) and change "PIN number" to "PIN" since PIN already
stands for Personal Identification Number. These changes will eliminate
tautologies and improve conciseness of the documentation.
🪄 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: 22f4f421-dab3-4cee-85ed-98759b4104fc
📒 Files selected for processing (100)
.storybook/main.jsAGENTS.mdapps/web/src/stories/method-picker-dialog.stories.jsapps/web/src/writer-schema-page.mjsapps/web/vite.config.mjsdocs-src/docs/040-test-data/domain/020-airline.mddocs-src/docs/040-test-data/domain/030-animal.mddocs-src/docs/040-test-data/domain/040-autoIncrement.mddocs-src/docs/040-test-data/domain/050-book.mddocs-src/docs/040-test-data/domain/060-color.mddocs-src/docs/040-test-data/domain/070-commerce.mddocs-src/docs/040-test-data/domain/080-company.mddocs-src/docs/040-test-data/domain/090-database.mddocs-src/docs/040-test-data/domain/100-datatype.mddocs-src/docs/040-test-data/domain/110-date.mddocs-src/docs/040-test-data/domain/120-finance.mddocs-src/docs/040-test-data/domain/130-food.mddocs-src/docs/040-test-data/domain/140-git.mddocs-src/docs/040-test-data/domain/150-hacker.mddocs-src/docs/040-test-data/domain/160-image.mddocs-src/docs/040-test-data/domain/170-internet.mddocs-src/docs/040-test-data/domain/180-literal.mddocs-src/docs/040-test-data/domain/190-location.mddocs-src/docs/040-test-data/domain/200-lorem.mddocs-src/docs/040-test-data/domain/210-music.mddocs-src/docs/040-test-data/domain/220-number.mddocs-src/docs/040-test-data/domain/230-person.mddocs-src/docs/040-test-data/domain/240-phone.mddocs-src/docs/040-test-data/domain/250-science.mddocs-src/docs/040-test-data/domain/260-string.mddocs-src/docs/040-test-data/domain/270-system.mddocs-src/docs/040-test-data/domain/280-vehicle.mddocs-src/docs/040-test-data/domain/290-word.mddocs/faker-10.4.0-comparison-report.mddocs/faker-options-param-report.mdpackages/core-ui/js/gui_components/shared/domain-command-help-metadata.jspackages/core-ui/js/gui_components/shared/test-data/help/help-model-builder.jspackages/core-ui/js/gui_components/shared/test-data/ui/method-picker-modal.jspackages/core-ui/src/tests/generator/data-generator-page.test.jspackages/core-ui/src/tests/grid/schema/test-data-schema-grid-engine-compat.test.jspackages/core-ui/src/tests/interaction/app-test-data-focused-schema-sync.test.jspackages/core-ui/src/tests/interaction/matrix/fixtures/schema-interaction-matrix-summary.mdpackages/core-ui/src/tests/interaction/matrix/fixtures/schema-interaction-matrix.jsonpackages/core-ui/src/tests/interaction/matrix/schema-interaction-scenario-builder.test.jspackages/core-ui/src/tests/interaction/matrix/support/schema-interaction-scenario-builder.jspackages/core-ui/src/tests/interaction/matrix/ui-scenario-parity-support.test.jspackages/core-ui/src/tests/interaction/support/generated-value-quality.jspackages/core-ui/src/tests/shared/help-model-builder.test.jspackages/core-ui/src/tests/utils/domain-command-help-metadata.test.jspackages/core-ui/src/tests/utils/faker-command-help-metadata.test.jspackages/core-ui/src/tests/utils/method-picker-modal.test.jspackages/core/js/command-help/command-help-contract.jspackages/core/js/command-help/command-help-validators.jspackages/core/js/domain/domain-custom-autoincrement-keyword-definitions.jspackages/core/js/domain/domain-custom-literal-keyword-definitions.jspackages/core/js/domain/domain-custom-string-keyword-definitions.jspackages/core/js/domain/domain-doc-markdown.jspackages/core/js/domain/domain-faker-airline-keyword-definitions.jspackages/core/js/domain/domain-faker-animal-keyword-definitions.jspackages/core/js/domain/domain-faker-book-keyword-definitions.jspackages/core/js/domain/domain-faker-color-keyword-definitions.jspackages/core/js/domain/domain-faker-commerce-keyword-definitions.jspackages/core/js/domain/domain-faker-company-keyword-definitions.jspackages/core/js/domain/domain-faker-database-keyword-definitions.jspackages/core/js/domain/domain-faker-datatype-keyword-definitions.jspackages/core/js/domain/domain-faker-date-keyword-definitions.jspackages/core/js/domain/domain-faker-finance-keyword-definitions.jspackages/core/js/domain/domain-faker-food-keyword-definitions.jspackages/core/js/domain/domain-faker-git-keyword-definitions.jspackages/core/js/domain/domain-faker-hacker-keyword-definitions.jspackages/core/js/domain/domain-faker-image-keyword-definitions.jspackages/core/js/domain/domain-faker-internet-keyword-definitions.jspackages/core/js/domain/domain-faker-location-keyword-definitions.jspackages/core/js/domain/domain-faker-lorem-keyword-definitions.jspackages/core/js/domain/domain-faker-music-keyword-definitions.jspackages/core/js/domain/domain-faker-number-keyword-definitions.jspackages/core/js/domain/domain-faker-person-keyword-definitions.jspackages/core/js/domain/domain-faker-phone-keyword-definitions.jspackages/core/js/domain/domain-faker-science-keyword-definitions.jspackages/core/js/domain/domain-faker-string-keyword-definitions.jspackages/core/js/domain/domain-faker-system-keyword-definitions.jspackages/core/js/domain/domain-faker-vehicle-keyword-definitions.jspackages/core/js/domain/domain-faker-word-keyword-definitions.jspackages/core/js/domain/domain-keywords.jspackages/core/js/faker/faker-commands.jspackages/core/js/faker/faker-helper-keyword-definitions.jspackages/core/package.jsonpackages/core/src/tests/command-help/command-definition-usage-examples.test.jspackages/core/src/tests/command-help/command-help-examples.test.jspackages/core/src/tests/command-help/command-help-validators.test.jspackages/core/src/tests/data_generation/unit/domain/domain-doc-generator-output.test.jspackages/core/src/tests/data_generation/unit/domain/domain-doc-param-examples.test.jspackages/core/src/tests/data_generation/unit/domain/domain-image-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domain-internet-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domain-keyword-params-usage.test.jspackages/core/src/tests/data_generation/unit/domain/domain-param-invocation-coverage.test-helper.jspackages/core/src/tests/data_generation/unit/domain/domain-result-assertions.test-helper.jspackages/core/src/tests/data_generation/unit/domain/domainKeywords.test.jsscripts/generate-domain-docs.mjstests/integration/cross-surface-option-parity.test.js
💤 Files with no reviewable changes (5)
- packages/core/js/faker/faker-commands.js
- packages/core/src/tests/data_generation/unit/domain/domain-image-exec.test.js
- docs/faker-options-param-report.md
- docs/faker-10.4.0-comparison-report.md
- packages/core-ui/src/tests/interaction/matrix/fixtures/schema-interaction-matrix-summary.md
|
Too many files changed for review. ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/tests/data_generation/unit/domain/internet-http-method.test.js (1)
1-37: ⚡ Quick winAdd test coverage for the error case.
The test suite thoroughly covers the helper functions but is missing coverage for
executeCustomInternetHttpMethodwhen all methods are excluded. The implementation throws an error atinternet-http-method.js:40-42with message "Invalid argument for excludes: no HTTP methods remain after exclusions."🧪 Proposed test to add
+ test('executeCustomInternetHttpMethod throws when all methods are excluded', () => { + expect(() => { + executeCustomInternetHttpMethod({ + args: [false, 'GET,HEAD,POST,PUT,DELETE,PATCH,OPTIONS,TRACE,CONNECT'], + }); + }).toThrow('Invalid argument for excludes: no HTTP methods remain after exclusions.'); + });🤖 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/src/tests/data_generation/unit/domain/internet-http-method.test.js` around lines 1 - 37, Add a new test case to the describe block that verifies getInternetHttpMethodPool throws an error when all available HTTP methods are excluded. The test should call getInternetHttpMethodPool with an excludes parameter that removes all methods from the available set (either exclude all items from ALL_HTTP_METHODS or exclude all items from COMMON_HTTP_METHODS when commonOnly is true) and expect the function to throw an error with the message "Invalid argument for excludes: no HTTP methods remain after exclusions."
🤖 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/src/tests/data_generation/unit/domain/internet-http-method.test.js`:
- Around line 1-37: Add a new test case to the describe block that verifies
getInternetHttpMethodPool throws an error when all available HTTP methods are
excluded. The test should call getInternetHttpMethodPool with an excludes
parameter that removes all methods from the available set (either exclude all
items from ALL_HTTP_METHODS or exclude all items from COMMON_HTTP_METHODS when
commonOnly is true) and expect the function to throw an error with the message
"Invalid argument for excludes: no HTTP methods remain after exclusions."
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4c75b15-1e1d-4c4d-b9a0-95b48c3b6ba2
📒 Files selected for processing (18)
apps/web/src/stories/shared-schema-definition.stories.jsjest.config.cjspackages/core/js/command-help/command-help-validators.jspackages/core/js/domain/domain-custom-internet-keyword-definitions.jspackages/core/js/domain/domain-faker-internet-keyword-definitions.jspackages/core/js/domain/domain-faker-string-keyword-definitions.jspackages/core/js/domain/domain-keyword-definitions.jspackages/core/js/domain/domain-keywords.jspackages/core/js/domain/internet-http-method.jspackages/core/src/tests/command-help/command-help-examples.test-support.jspackages/core/src/tests/command-help/command-help-examples.test.jspackages/core/src/tests/command-help/command-help-validators.test.jspackages/core/src/tests/command-help/seeded-all-commands-generate-and-validate-test.jspackages/core/src/tests/data_generation/unit/domain/domain-internet-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domain-result-assertions.test-helper.jspackages/core/src/tests/data_generation/unit/domain/domain-string-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domainKeywords.test.jspackages/core/src/tests/data_generation/unit/domain/internet-http-method.test.js
💤 Files with no reviewable changes (1)
- packages/core/js/domain/domain-faker-internet-keyword-definitions.js
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/tests/command-help/seeded-all-commands-generate-and-validate-test.js
- packages/core/js/domain/domain-keyword-definitions.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/js/domain/domain-faker-string-keyword-definitions.js
- packages/core/js/domain/domain-keywords.js
- packages/core/src/tests/data_generation/unit/domain/domainKeywords.test.js
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.
Closes #226
Summary
usageExamplesthe source of truth for keyword help metadataNotes
Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
image.urlLoremFlickrcommand from available commands and related documentation.Improvements