Add unsafe faker browser setting#292
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a browser Generation settings toggle for unsafe faker expressions, threads that setting through generation and schema validation paths, and updates MCP, REST, OpenAPI, stories, docs, and tests to expose and verify the new opt-in. ChangesUnsafe Faker Expressions Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 4b4ff30569
ℹ️ 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".
| }), | ||
| storedSchemasEnabled: true, | ||
| storedSchemasProps: {}, | ||
| unsafeFakerExpressions: true, |
There was a problem hiding this comment.
In the production app mount this starts the new allow risky faker control enabled, so browser generation opts into expression-style helper callbacks before the user has explicitly trusted the schema. That contradicts the newly added docs for REST/MCP/CLI/Web UI that describe risky helpers as opt-in for trusted schemas, and it changes the app surface from safe-by-default to executing callback-shaped schema text by default. Initialize this to false and require the user to enable it from the cog.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in the follow-up commits.
The browser default now matches the API/CLI/MCP behavior: unsafe faker is false unless the user explicitly enables it in Test Data Settings. The old true fallback is gone from the app/controller path, and the current PR head uses the unsafeFakerExpressions naming with the default off.
Nothing is intentionally left unimplemented for this thread.
| schemaSource, | ||
| fakerInstance: faker, | ||
| RandExpClass: RandExp, | ||
| unsafeFakerExpressions: getUnsafeFakerExpressions({ validationOptions, schemaState }) === true, |
There was a problem hiding this comment.
This passes the setting only when creating the generation session, but the app reaches this line only after getSchemaState() succeeds; the real schema editor still parses text/semantic validation through shared-schema-editor-controller.js without unsafeFakerExpressions, and those unsafe-faker diagnostics are returned as validation errors. In the Web UI, a user who enables the cog and enters the documented helpers.mustache("Hello {{name}}", { name: () => "Ada" }) path is rejected during schema validation before this opt-in is used. Thread the setting into text sync/semantic validation or stop treating that diagnostic as blocking when enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in the follow-up commits.
The unsafe faker setting is now applied before blocking schema validation. It is threaded through the schema parse/semantic validation path and the generation session path, so callback-style helpers such as the documented mustache/multiple variants are allowed only when the UI setting is enabled. Regression coverage was added around the shared schema validation and population/generation behavior.
Nothing is intentionally left unimplemented for this thread.
Greptile SummaryThis PR introduces an opt-in "allow unsafe faker expressions" toggle for browser-based data generation, exposing a generation-settings cog in the population toolbar and panel. The same opt-in is threaded consistently through the REST API, MCP contract, CLI, schema validation, and the schema editing session — all defaulting to
Confidence Score: 5/5Safe to merge; the opt-in defaults to false across every entry point and the propagation chain is well-tested. The unsafe-faker flag is gated by === true everywhere it is initialized or read, so it is off by default for all surfaces. The MCP contract correctly inverts safeFakerRules when the opt-in is set. Schema validation, session creation, and the generator constructor all receive the flag through explicit injection rather than global state. Test coverage spans the full lifecycle — checkbox interaction, session creation, schema validation filtering, and MCP/API contract. population-actions-view.js — rebuildTemplate() does not reset the generationSettingsOpen controller state; re-enabling unsafeFakerExpressionsVisible after the dialog was open would render it open immediately. Important Files Changed
Reviews (4): Last reviewed commit: "Improve unsafe faker UI guidance" | 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/js/mcp/anywaydata-mcp-contract.js (1)
357-376: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the actual validation mode in
unsafe_faker_ruleerrors.Opting into
unsafeFakerExpressionsmakescreateGenerationSessionrun withsafeFakerRules: false, but the later error remap still hard-codes{ mode: 'safe' }. That now returns the wrong MCP error details for forbidden-rule failures in non-safe mode. Please forward the session/result diagnostics mode instead of always reporting"safe".🤖 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/mcp/anywaydata-mcp-contract.js` around lines 357 - 376, The validation error remap in the MCP contract is hard-coding the diagnostics mode to safe even when unsafeFakerExpressions is enabled. Update the result handling around createGenerationSession and the unsafe_faker_rule error mapping in anywaydata-mcp-contract.js so it forwards the actual session/result diagnostics mode instead of always using safe, ensuring forbidden-rule failures report the correct mode.
🤖 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 `@docs-src/docs/040-test-data/faker/010-helpers.md`:
- Around line 45-64: The safe/risky guidance in the helpers docs needs to match
the later `helpers.uniqueArray` example. Update the `Risky Helper Variants`
section and the later `helpers.uniqueArray` example so they agree: either
rewrite the example to use the literal-array safe form, or explicitly mark it as
requiring the risky opt-in. Make sure the wording around `helpers.uniqueArray`
is consistent with the rest of the table and the trust-based guidance.
In `@docs-src/docs/070-interfaces-and-deployment/030-rest-api.md`:
- Line 157: The REST docs note for unsafe faker helpers only mentions
/v1/generate and /v1/generate/fromschema, but it now needs to include
/v1/generate/amend as well. Update the prose in the REST API documentation near
the unsafeFakerExpressions guidance to mention that the same opt-in can be
passed in the JSON body for the amend endpoint, matching the contract exposed by
apps/api/src/openapi.js and keeping the guidance aligned with the published REST
surface.
In
`@packages/core-ui/js/gui_components/app/population-actions/population-actions-view.js`:
- Around line 49-52: `unsafeFakerExpressionsVisible` is only reflected during
the initial template render, so later state changes do not add or remove the
cog/dialog. Update `createPopulationActionsComponent.update()` to re-render the
parts controlled by `renderGenerationSettings()` when
`state.unsafeFakerExpressionsVisible` changes, using the existing
`view`/`PopulationActionsView` methods so the generation-settings UI stays in
sync after mount.
In
`@packages/core-ui/js/gui_components/app/test-data-grid/controller/test-data-grid-controller.js`:
- Line 190: Browser generation is still defaulting to risky Faker processing,
which keeps the UI in the wrong mode. Update the test-data-grid controller’s
getUnsafeFakerExpressions fallback so it does not return true when panel state
is unavailable, and change the dataPopulationPanel initialization that currently
seeds unsafeFakerExpressions to true so browser flows start disabled until the
user explicitly enables it. Keep the behavior aligned between
getUnsafeFakerExpressions and the panel setup in test-data-grid-controller.
In
`@packages/core-ui/js/gui_components/app/test-data-grid/generation/test-data-generation-service.js`:
- Line 65: The browser grid flow is still enabling unsafe faker helpers by
default via TestDataGenerationService.getUnsafeFakerExpressions and propagating
that into generatorOptions/session engine. Change the default to
opt-in/disabled-by-default in the service, and make sure the controller’s
fallback in TestDataGridController does not re-enable it with ?? true so the
browser grid path only turns this on when explicitly requested.
In
`@packages/core-ui/js/gui_components/app/test-data-population-toolbar/test-data-population-toolbar-controller.js`:
- Line 13: The browser risky Faker flag is currently enabled by default when the
prop is omitted, which conflicts with the intended default-off behavior. Update
the default initialization in test-data-population-toolbar-controller and the
matching default in population-actions-controller so unsafeFakerExpressions only
becomes true when the prop is explicitly true, then adjust the new toolbar test
expectation to assert the default remains disabled.
In
`@packages/core-ui/js/gui_components/generator/generation/generator-schema-generation-service.js`:
- Line 26: The generator page service is defaulting getUnsafeFakerExpressions to
true, which makes unsafe helpers opt-out instead of opt-in. Update
generator-schema-generation-service so the default for getUnsafeFakerExpressions
is false in the relevant defaults used by generator configuration and
session-backed generation, and ensure any fallback/constructor wiring in the
generator service path preserves that false default unless a caller explicitly
overrides it.
In
`@packages/core-ui/js/gui_components/shared/test-data/generation/ui-generation-session-service.js`:
- Line 137: The shared session helper is defaulting unsafe faker expressions on,
which breaks the intended opt-in behavior. Update
createUiGenerationSessionService and the default getUnsafeFakerExpressions
fallback in ui-generation-session-service so the service starts in safe mode
unless a caller explicitly injects an unsafe-allowing implementation. Keep the
existing symbol names consistent and ensure any consumers relying on the default
path no longer enable risky helpers implicitly.
---
Outside diff comments:
In `@packages/core/js/mcp/anywaydata-mcp-contract.js`:
- Around line 357-376: The validation error remap in the MCP contract is
hard-coding the diagnostics mode to safe even when unsafeFakerExpressions is
enabled. Update the result handling around createGenerationSession and the
unsafe_faker_rule error mapping in anywaydata-mcp-contract.js so it forwards the
actual session/result diagnostics mode instead of always using safe, ensuring
forbidden-rule failures report the correct mode.
🪄 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: 57c09e4c-49b0-4752-a84a-362d5be8e48e
📒 Files selected for processing (32)
.gitignoreapps/api/src/openapi.jsapps/mcp/src/mcp.test.jsapps/web/src/stories/population-actions.stories.jsapps/web/src/stories/test-data-embedded-panel.stories.jsapps/web/src/stories/test-data-population-toolbar.stories.jsapps/web/styles.cssdocs-src/docs/040-test-data/faker/010-helpers.mddocs-src/docs/070-interfaces-and-deployment/030-rest-api.mddocs-src/docs/070-interfaces-and-deployment/040-mcp.mddocs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.mdpackages/core-ui/js/gui_components/app/data-population-panel/data-population-panel-controller.jspackages/core-ui/js/gui_components/app/data-population-panel/data-population-panel-view.jspackages/core-ui/js/gui_components/app/data-population-panel/index.jspackages/core-ui/js/gui_components/app/population-actions/index.jspackages/core-ui/js/gui_components/app/population-actions/population-actions-controller.jspackages/core-ui/js/gui_components/app/population-actions/population-actions-view.jspackages/core-ui/js/gui_components/app/test-data-grid/controller/test-data-grid-controller.jspackages/core-ui/js/gui_components/app/test-data-grid/generation/test-data-generation-service.jspackages/core-ui/js/gui_components/app/test-data-population-toolbar/index.jspackages/core-ui/js/gui_components/app/test-data-population-toolbar/test-data-population-toolbar-controller.jspackages/core-ui/js/gui_components/app/test-data-population-toolbar/test-data-population-toolbar-view.jspackages/core-ui/js/gui_components/generator/generation/generator-schema-generation-service.jspackages/core-ui/js/gui_components/shared/test-data/generation/generation-controller.jspackages/core-ui/js/gui_components/shared/test-data/generation/ui-generation-session-service.jspackages/core-ui/src/tests/app/data-population-panel.test.jspackages/core-ui/src/tests/app/population-actions.test.jspackages/core-ui/src/tests/app/test-data-population-toolbar.test.jspackages/core-ui/src/tests/grid/generation/test-data-generation-service.test.jspackages/core-ui/src/tests/shared/ui-generation-session-service.test.jspackages/core/js/mcp/anywaydata-mcp-contract.jspackages/core/src/tests/mcp/anywaydata-mcp-contract.test.js
eviltester
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core-ui/js/gui_components/app/population-actions/population-actions-view.js (1)
66-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAdd keyboard dismissal/focus handling for the settings dialog.
The popover uses
role="dialog"but is only dismissable via the close button or an outside click; there is noEscapehandler and focus is not moved into the dialog on open. Keyboard/screen-reader users get a dialog role without the expected interaction contract. Consider adding anEscapekeydown handler that callscloseGenerationSettings()and moving focus to the dialog (or first control) when it opens.🤖 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/app/population-actions/population-actions-view.js` around lines 66 - 72, The generation settings popover in population-actions-view.js has a dialog role but lacks keyboard dismissal and focus management. Update the dialog behavior around the generation settings markup and the closeGenerationSettings/open flow to add an Escape keydown handler that closes the dialog, and move focus into the dialog or its first interactive control when generationSettingsOpen becomes true. Use the existing closeGenerationSettings() and the dialog element identified by data-role="generation-settings-dialog" so the interaction matches the dialog semantics.
🤖 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-ui/js/gui_components/app/population-actions/population-actions-view.js`:
- Around line 66-72: The generation settings popover in
population-actions-view.js has a dialog role but lacks keyboard dismissal and
focus management. Update the dialog behavior around the generation settings
markup and the closeGenerationSettings/open flow to add an Escape keydown
handler that closes the dialog, and move focus into the dialog or its first
interactive control when generationSettingsOpen becomes true. Use the existing
closeGenerationSettings() and the dialog element identified by
data-role="generation-settings-dialog" so the interaction matches the dialog
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5b626d4-7bf6-4df0-94b1-6c3c8daef88f
📒 Files selected for processing (31)
apps/web/src/stories/population-actions.stories.jsapps/web/src/stories/test-data-embedded-panel.stories.jsapps/web/src/stories/test-data-population-toolbar.stories.jsdocs-src/docs/040-test-data/faker/010-helpers.mddocs-src/docs/070-interfaces-and-deployment/030-rest-api.mddocs-src/docs/070-interfaces-and-deployment/040-mcp.mddocs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.mdpackages/core-ui/js/gui_components/app/data-population-panel/data-population-panel-controller.jspackages/core-ui/js/gui_components/app/data-population-panel/data-population-panel-view.jspackages/core-ui/js/gui_components/app/population-actions/population-actions-controller.jspackages/core-ui/js/gui_components/app/population-actions/population-actions-view.jspackages/core-ui/js/gui_components/app/test-data-grid/controller/test-data-grid-controller.jspackages/core-ui/js/gui_components/app/test-data-grid/generation/test-data-generation-service.jspackages/core-ui/js/gui_components/app/test-data-population-toolbar/test-data-population-toolbar-controller.jspackages/core-ui/js/gui_components/generator/generation/generator-schema-generation-service.jspackages/core-ui/js/gui_components/generator/runtime/create-generator-page-defaults.jspackages/core-ui/js/gui_components/generator/runtime/generator-schema-rule-helpers.jspackages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-controller.jspackages/core-ui/js/gui_components/shared/test-data/generation/ui-generation-session-service.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-controller.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-editor-core.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-row-validation.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-runtime.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/src/tests/app/data-population-panel.test.jspackages/core-ui/src/tests/app/population-actions.test.jspackages/core-ui/src/tests/app/test-data-population-toolbar.test.jspackages/core-ui/src/tests/shared/schema-controller.test.jspackages/core-ui/src/tests/shared/schema-editor-core.test.jspackages/core-ui/src/tests/shared/schema-row-validation.test.jspackages/core-ui/src/tests/shared/ui-generation-session-service.test.js
✅ Files skipped from review due to trivial changes (4)
- packages/core-ui/src/tests/shared/schema-editor-core.test.js
- docs-src/docs/070-interfaces-and-deployment/040-mcp.md
- docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md
- docs-src/docs/070-interfaces-and-deployment/030-rest-api.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core-ui/js/gui_components/generator/generation/generator-schema-generation-service.js
- docs-src/docs/040-test-data/faker/010-helpers.md
- packages/core-ui/js/gui_components/app/population-actions/population-actions-controller.js
- packages/core-ui/js/gui_components/app/data-population-panel/data-population-panel-view.js
|
Implementation status for the PR review comments and follow-up testing: Implemented:
Not implemented:
Verification already run for the current PR head: focused Jest coverage, |
Summary
allow risky fakeroption for browser generation flowsCloses #245
Verification
pnpm --dir docs-src run buildhttp://127.0.0.1:4173/app.htmlpnpm run verify:uipnpm run verify:localSummary by CodeRabbit