Fix generator schema row tab order#293
Conversation
|
Warning Review limit reached
Next review available in: 4 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 the row action buttons to the end of the DOM template and adding explicit CSS grid column placement for all row controls. Focus-out validation is deferred to the next event-loop tick via ChangesSchema Row Tab Order Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 order in schema rows by moving the row-action buttons (
Confidence Score: 5/5Safe to merge — the changes are well-scoped, covered by new unit and browser tests, and the approach (DOM reorder + CSS grid + deferred setTimeout(0)) is a standard, low-risk pattern for this class of focus problem. All three layers of the fix (DOM order, CSS grid placement, deferred validation) have direct test coverage at the unit, DOM/component, and Playwright levels. The timer management reuses the existing clearSemanticValidationTimer mechanism, so cleanup on destroy is already handled. No existing behaviour was removed — the change is additive except for the DOM reorder, which is fully compensated by the explicit grid positions. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant DOM as Schema Row DOM
participant Controller
participant Timer as setTimeout(0)
participant Renderer
Note over DOM: Actions div is last in DOM<br/>(CSS grid puts it visually first)
User->>DOM: Tab through fields (name → sourceType → helpLink → value → drag…)
User->>DOM: focusout fires on a field (name/command/params/value)
DOM->>Controller: handleFocusOut(event)
Controller->>Timer: "scheduleFocusSettledSemanticValidationForRow(rowId, delay=0)"
Note over Timer: clears any prior debounce timer<br/>schedules validation after focus settles
Timer-->>Controller: applySemanticValidationForRow(rowId)
Controller->>Controller: captureActiveFieldState(document)
Note over Controller: captures fieldName OR actionName<br/>(all action buttons now captured)
Controller->>Renderer: renderRows()
Renderer->>DOM: innerHTML rewritten
Controller->>DOM: restoreActiveFieldState(document, state)
Note over DOM: focus returned to correct<br/>field or action button
%%{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"}}}%%
sequenceDiagram
participant User
participant DOM as Schema Row DOM
participant Controller
participant Timer as setTimeout(0)
participant Renderer
Note over DOM: Actions div is last in DOM<br/>(CSS grid puts it visually first)
User->>DOM: Tab through fields (name → sourceType → helpLink → value → drag…)
User->>DOM: focusout fires on a field (name/command/params/value)
DOM->>Controller: handleFocusOut(event)
Controller->>Timer: "scheduleFocusSettledSemanticValidationForRow(rowId, delay=0)"
Note over Timer: clears any prior debounce timer<br/>schedules validation after focus settles
Timer-->>Controller: applySemanticValidationForRow(rowId)
Controller->>Controller: captureActiveFieldState(document)
Note over Controller: captures fieldName OR actionName<br/>(all action buttons now captured)
Controller->>Renderer: renderRows()
Renderer->>DOM: innerHTML rewritten
Controller->>DOM: restoreActiveFieldState(document, state)
Note over DOM: focus returned to correct<br/>field or action button
Reviews (3): Last reviewed commit: "Address PR 293 review 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 |
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
Bug Fixes
Tests