Fix generator schema row tab order#293
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughFixes keyboard tab order in schema editor rows by moving row actions after the validation message, adding explicit grid placement for row controls, deferring blur-time semantic validation, and updating related test harnesses and coverage. The PR also updates two release blog posts and broadens one gitignore log pattern. ChangesSchema Row Tab Order Fix
Blog Release Posts
Gitignore Log Pattern Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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: aee9eebf16
ℹ️ 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".
| .shared-schema-row-actions { | ||
| grid-column: 1; | ||
| grid-row: 1; |
There was a problem hiding this comment.
At widths 481–980px, the later @media (max-width: 980px) rules only change grid-column for the row actions and name input and do not reset grid-row. Because this new base rule pins the actions to row 1 while the new name-input rule also pins the textbox to row 1, the action buttons and column-name field overlap on tablet-width layouts; add responsive grid-row overrides while keeping the desktop left-action column.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in aa7450b1.
The tablet layout now explicitly places the shared schema row actions on grid row 1 and the column-name input on grid row 2 inside the max-width: 980px rules, so the desktop left-action-column placement does not carry an overlapping row assignment into tablet widths.
Verified with the targeted generator schema-edit Playwright spec, pnpm run verify:ui, pnpm run verify:local, and the pre-push verification hook.
| element.focus(); | ||
| element.value = ''; | ||
| fireEvent.input(element, { target: { value: '' } }); | ||
| fireEvent.change(element, { target: { value: '' } }); |
There was a problem hiding this comment.
AGENTS.md's Browser Test Interaction Rules require UI/browser abstractions to use user-like interactions only and avoid synthetic event dispatch or direct DOM hooks; this helper now focuses the element, mutates .value, and dispatches input/change directly for every clear. Tests using this focused app harness can therefore pass without exercising the real click/clear/type path, which is especially risky for this focusout/tab-order fix; keep the generic path on userEvent and limit direct dispatch to unavoidable jsdom-only exceptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in aa7450b1.
The focused app harness now uses user-event driven interactions for the generic helper path: user.click, user.clear, user.type/user.paste, user.selectOptions, and user.tab. The only special handling is still through user-event: number inputs replace the selected existing value in one type action so the row-count control does not clamp an intermediate empty value before typing.
Verified with the focused app harness Jest suites, the targeted generator schema-edit Playwright spec, pnpm run verify:ui, pnpm run verify:local, and the pre-push verification hook.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core-ui/src/tests/shared/shared-schema-editor-ui.test.js (1)
92-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a command-row tab-order case.
This only exercises the value-row branch.
renderSharedSchemaRowsbuilds command rows through a different path (pick-command, help link, params, edit-params), and the acceptance criteria here explicitly include command parameters. A companion assertion for a faker/domain row would close the main remaining regression gap.🤖 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/shared/shared-schema-editor-ui.test.js` around lines 92 - 111, Add a command-row tab-order test to cover the non-value-row path in `renderSharedSchemaRows`. The current case only verifies the editable value-row branch, so add a companion assertion for a command row that exercises `pick-command`, the help link, params, and edit-params controls. Keep the focus-order check in the same style as the existing `orders focusable row controls from editable fields to row actions` test, using the row query and focusable element collection to validate the expected tab sequence.
🤖 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/src/tests/shared/shared-schema-editor-ui.test.js`:
- Around line 92-111: Add a command-row tab-order test to cover the
non-value-row path in `renderSharedSchemaRows`. The current case only verifies
the editable value-row branch, so add a companion assertion for a command row
that exercises `pick-command`, the help link, params, and edit-params controls.
Keep the focus-order check in the same style as the existing `orders focusable
row controls from editable fields to row actions` test, using the row query and
focusable element collection to validate the expected tab sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7845862a-9232-4ebb-8d3c-15e343309dea
📒 Files selected for processing (11)
.gitignoreapps/web/src/tests/browser/generator/functional/schema-edit.spec.jsapps/web/styles.csspackages/core-ui/js/gui_components/shared/test-data/schema/schema-focus-state.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-ui.jspackages/core-ui/src/tests/interaction/support/focused-app-test-data-harness.jspackages/core-ui/src/tests/interaction/support/focused-generator-harness.jspackages/core-ui/src/tests/shared/schema-focus-state.test.jspackages/core-ui/src/tests/shared/shared-schema-editor-controller.test.jspackages/core-ui/src/tests/shared/shared-schema-editor-ui.test.js
Greptile SummaryThis PR fixes keyboard Tab navigation order in schema row editing by moving the row-action buttons (
Confidence Score: 5/5Safe to merge — changes are tightly scoped to DOM ordering, CSS grid placement, and a well-understood 0 ms timer deferral; all three layers are covered by new unit, component, and Playwright tests. The DOM reordering is straightforward: actions div moves to the end of the template while CSS grid keeps the visual layout identical. The deferred-validation approach (setTimeout 0) is a standard event-loop technique with no side effects on the validation logic itself. Focus capture/restore was already present for pick-command; widening it to all action buttons is a minimal, well-tested extension. The test harness migration from fireEvent to userEvent aligns with the project's interaction-test guidelines and reduces brittleness. All changed paths have direct test coverage added in this PR. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["name input\n(grid-col 2, DOM pos 1)"] -->|Tab| B["sourceType select\n(grid-col 3, DOM pos 2)"]
B -->|Tab| C["faker-doc-link anchor\n(grid-col 4-5, DOM pos 3)"]
C -->|Tab| D["value / params input\n(grid-col 5-6, DOM pos 4)"]
D -->|Tab| E["row actions div\n(grid-col 1 visual, DOM pos LAST)"]
E --> F["drag button"]
E --> G["add button"]
E --> H["remove / up / down buttons"]
subgraph FocusOut Validation
D -->|focusout| I["scheduleFocusSettledSemanticValidationForRow"]
I -->|"setTimeout(fn, 0)"| J["applySemanticValidationForRow"]
J --> K["captureActiveFieldState\n(captures any data-action button)"]
J --> L["renderRows re-render"]
L --> M["restoreActiveFieldState\n(restores action button focus)"]
end
%%{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["name input\n(grid-col 2, DOM pos 1)"] -->|Tab| B["sourceType select\n(grid-col 3, DOM pos 2)"]
B -->|Tab| C["faker-doc-link anchor\n(grid-col 4-5, DOM pos 3)"]
C -->|Tab| D["value / params input\n(grid-col 5-6, DOM pos 4)"]
D -->|Tab| E["row actions div\n(grid-col 1 visual, DOM pos LAST)"]
E --> F["drag button"]
E --> G["add button"]
E --> H["remove / up / down buttons"]
subgraph FocusOut Validation
D -->|focusout| I["scheduleFocusSettledSemanticValidationForRow"]
I -->|"setTimeout(fn, 0)"| J["applySemanticValidationForRow"]
J --> K["captureActiveFieldState\n(captures any data-action button)"]
J --> L["renderRows re-render"]
L --> M["restoreActiveFieldState\n(restores action button focus)"]
end
Reviews (6): Last reviewed commit: "Address PR 293 docs and harness feedback" | Re-trigger Greptile |
eviltester
left a comment
eviltester
left a comment
There was a problem hiding this comment.
Code Review for PR #293\n\nI've completed the review of this PR. Key observations and suggested fixes follow.
Code Review Feedback for PR #2931. Potential Bug: Focus State Capture Logic RegressionFile: The original condition explicitly excluded if (!rowId || (!fieldName && actionName !== 'pick-command')) {The new condition captures all action buttons including if (!rowId || (!fieldName && !actionName)) {This changes behavior - Suggestion: Revert to the explicit exclusion or add a comment explaining why 2. Incomplete Test Coverage for Focus Restore on Action ButtonsFile: The new test only covers the
Suggestion: Add tests for other action types or parameterize the existing test. 3. Deferred Validation Timer Cleanup on DestroyFile: The test verifies cleanup on Suggestion: Confirm 4. CSS Grid Ordering - Missing Explicit Column for Drag HandleFile: The CSS explicitly assigns Suggestion: Add explicit 5. Test Harness Async Handling Could Be FlakyFile: Uses Suggestion: Replace with |
|
Addressed the review feedback in
Validation passed locally with focused Jest coverage, |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core-ui/src/tests/interaction/support/focused-app-test-data-harness.js (1)
87-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winApply the same focus-settle wait after select tabbing.
setInputValuewaits for the edited control to lose focus afteruser.tab(), butsetSelectValuereturns immediately. Since select changes can swap the row editor before the next harness action, reuse the same wait here.Proposed change
async function setSelectValue(element, value) { if (!element) { throw new Error('Target select element was not found in focused app test-data harness'); } await user.selectOptions(element, value); await user.tab(); + await waitForFocusToLeave(element); }🤖 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/focused-app-test-data-harness.js` around lines 87 - 92, `setSelectValue` in the focused-app test-data harness should mirror `setInputValue` by waiting for focus to settle after `user.tab()`, because select changes can trigger editor swaps before the next action. Update `setSelectValue` to reuse the same post-tab focus-loss wait logic used by `setInputValue`, keeping the existing select interaction flow in sync with the input helper.
🤖 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/blog/2026-06-30-increments-and-helpers.md`:
- Around line 1-5: The blog post frontmatter is missing the opening delimiter,
so the metadata in the file is not being parsed. Update the frontmatter in the
markdown file by adding the required leading delimiter before the existing
`slug`, `authors`, `tags`, and `date` fields and keep the closing delimiter
as-is so Docusaurus recognizes the post metadata correctly.
- Around line 46-52: The example output for the zero-padding section is
inconsistent with the stated zeropadding=3 setting. Update the generated values
in the “Generated values” block so they all use three digits, and verify the
example rows in the blog content match the behavior described by the
increments/helpers explanation.
---
Nitpick comments:
In
`@packages/core-ui/src/tests/interaction/support/focused-app-test-data-harness.js`:
- Around line 87-92: `setSelectValue` in the focused-app test-data harness
should mirror `setInputValue` by waiting for focus to settle after `user.tab()`,
because select changes can trigger editor swaps before the next action. Update
`setSelectValue` to reuse the same post-tab focus-loss wait logic used by
`setInputValue`, keeping the existing select interaction flow in sync with the
input helper.
🪄 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: a0d3842c-42df-4e9f-8f3f-2cd1508ee0fe
📒 Files selected for processing (5)
apps/web/styles.cssdocs-src/blog/2026-06-12-combinatorial-grid-workflows.mddocs-src/blog/2026-06-30-increments-and-helpers.mdpackages/core-ui/src/tests/interaction/support/focused-app-test-data-harness.jspackages/core-ui/src/tests/shared/schema-focus-state.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/styles.css
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
…-schema-tab-order # Conflicts: # .gitignore
Summary
Fixes the generator schema row keyboard tab order so primary editing controls are reached before row action buttons.
.codex-dev-web*.log.Fixed Issues
Closes #271
Closes #291
Validation
pnpm run verify:uipnpm run verify:localSummary by CodeRabbit