Fix auto defect regressions#285
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds validation for ChangesValidation, schema editor, and grid filter updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: f647e46b97
ℹ️ 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".
| return !getStaticSchemaRowValidationIssues(row, rowIndex).some((issue) => | ||
| SCHEMA_UI_BLOCKING_ROW_ERROR_CODES.has(issue?.code) | ||
| ); |
There was a problem hiding this comment.
When text schema contains a known domain command with malformed syntax (for example Status\ndatatype.enum(values="active,pending)), parseSchemaTextToRows still returns a domain row with no static row issues, so this new branch treats it as editable in Schema UI. The subsequent applySemanticValidationForAllRows() rebuilds the enum rule and can throw on the unbalanced quote instead of leaving the editor in text mode with the schema error; the existing invalid-enum text-mode flow regresses because this check only blocks static command errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 85fe188. I tightened the row-mode recovery gate so every compiler validation error must map to an editable faker/domain schema row before the schema-level error can be cleared. Malformed or unanchored compiler errors now keep the editor in text mode, with a regression test covering the unmatched-error path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core-ui/src/tests/grid/tabulator-duplicate-column-copy.test.js (1)
189-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise an actual active filter in these regression tests.
Both tests assert
refreshFilteris called, but neither seeds a global filter nor verifies the active/visible rows after mutation. Add one assertion path that callsextension.filterText(...)or stubssetFilter/active rows so this catches regressions to the#270behavior, not just the method call.Also applies to: 211-240
🤖 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/grid/tabulator-duplicate-column-copy.test.js` around lines 189 - 209, The regression test in GridExtensionTabulator only checks that refreshFilter is invoked, but it does not simulate an actual active filter or validate visible rows after data replacement. Update the test around setGridFromGenericDataTable to first apply a real global filter via extension.filterText(...) or stub Tabulator’s setFilter/getFilteredRows behavior, then assert the active rows/filtered result after mutation so the `#270` behavior is covered. Apply the same pattern to the related duplicate-column copy test setup so the assertion verifies filter preservation, not just the refreshFilter call.
🤖 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/schema/shared-schema-editor-controller.js`:
- Around line 461-480: `canEditCompilerValidationErrorsInSchemaRows()` is too
permissive because it only checks that some invalid rows are editable, not that
every `compiler_validation_error` is mapped to an editable row. Update this
helper in `shared-schema-editor-controller.js` to verify all compiler errors
resolve to rows that are editable (using `getInvalidSchemaRowIndexes`,
`normaliseSourceType`, and `getStaticSchemaRowValidationIssues`), and return
false if any compiler error is unmatched or non-editable. Then ensure the caller
path that clears the global schema error before switching to row mode only runs
when this stricter check passes.
In
`@packages/core/js/keywords/domain/autoincrement/sequence-keyword-definition.js`:
- Around line 6-10: The step validation in sequence-keyword-definition.js is
only checking Object.is(argsByName.step, 0), so -0 can slip through as a valid
value. Update the validation in the sequence keyword definition logic to use a
numeric zero check that rejects both 0 and -0, while keeping the existing
non-zero integer error handling in place.
---
Nitpick comments:
In `@packages/core-ui/src/tests/grid/tabulator-duplicate-column-copy.test.js`:
- Around line 189-209: The regression test in GridExtensionTabulator only checks
that refreshFilter is invoked, but it does not simulate an actual active filter
or validate visible rows after data replacement. Update the test around
setGridFromGenericDataTable to first apply a real global filter via
extension.filterText(...) or stub Tabulator’s setFilter/getFilteredRows
behavior, then assert the active rows/filtered result after mutation so the `#270`
behavior is covered. Apply the same pattern to the related duplicate-column copy
test setup so the assertion verifies filter preservation, not just the
refreshFilter call.
🪄 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: 38905639-c2a3-420f-bd7b-ec429e0c16a4
📒 Files selected for processing (10)
apps/web/src/tests/browser/app/functional/test-data/text-schema-grid-sync.spec.jsapps/web/src/tests/browser/generator/functional/schema-edit.spec.jspackages/core-ui/js/gui_components/data-grid-editor/tabulator/gridExtension-tabulator.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/src/tests/grid/tabulator-duplicate-column-copy.test.jspackages/core-ui/src/tests/shared/shared-schema-editor-controller.test.jspackages/core/js/keywords/domain/autoincrement/sequence-keyword-definition.jspackages/core/src/tests/core-api/generateFromTextSpec.test.jspackages/core/src/tests/data_generation/keywords/domain/autoincrement/sequence-exec.test.jspackages/core/src/tests/data_generation/unit/domain/domainKeywords.test.js
Greptile SummaryThis PR fixes several schema editing and generated-data regressions. The main changes are:
Confidence Score: 5/5The changes are narrowly scoped to validation, schema editor state transitions, and grid filter preservation, with matching unit and browser coverage. No code issues were identified in the changed paths, and the updated tests cover the regressions described by the PR.
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "Address PR review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the remaining top-level review notes in 85fe188:
|
Summary
Fixes the auto defect batch by tightening invalid schema argument handling, preserving active grid filters after generated-data amendments, and allowing known commands with invalid params to move from text schema back into Schema UI with row-level validation.
Changes
autoIncrement.sequencesostep=0and negativezeropaddingfail at schema validation instead of producingERRORrows.Validation
pnpm run verify:uipnpm run verify:localCloses #253
Closes #268
Closes #269
Closes #270
Summary by CodeRabbit
autoIncrement.sequenceargument validation by rejectingstep = 0and negativezeropadding, with clearer compiler-style error messages.