Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded two integration tests to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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/src/app/__tests__/widgetRenderer.integration.test.ts (1)
1131-1176: Consider adding reverse traversal (Shift+Tab) in this focus-path test.You already verify forward traversal and ESC return; adding one reverse-hop assertion would tighten keyboard focus coverage for tabs content/bar transitions.
♻️ Suggested addition
renderer.routeEngineEvent(keyEvent(3 /* TAB */)); assert.equal(renderer.getFocusedId(), "general-save"); + renderer.routeEngineEvent(keyEvent(3 /* TAB */, ZR_MOD_SHIFT)); + assert.equal(renderer.getFocusedId(), generalTrigger); + + renderer.routeEngineEvent(keyEvent(3 /* TAB */)); + assert.equal(renderer.getFocusedId(), "general-save"); + renderer.routeEngineEvent(keyEvent(1 /* ESC */)); assert.equal(renderer.getFocusedId(), generalTrigger);As per coding guidelines
**/*.test.{ts,tsx}: “Test focus traversal by asserting focused ID movement across Tab/Shift+Tab navigation.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/widgetRenderer.integration.test.ts` around lines 1131 - 1176, Add a reverse traversal assertion to the "Escape from tabs content returns focus to the tab bar" test: after you move focus to "general-save" (using renderer.routeEngineEvent(keyEvent(3 /* TAB */)) and verifying renderer.getFocusedId() === "general-save"), dispatch a Shift+Tab key event via renderer.routeEngineEvent (use the same keyEvent helper but with the shift modifier as used elsewhere in tests) and assert renderer.getFocusedId() === getTabsTriggerId("settings-tabs", 0, "general") (the generalTrigger variable), keeping the existing forward-tab and ESC assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/app/__tests__/widgetRenderer.integration.test.ts`:
- Around line 1131-1176: Add a reverse traversal assertion to the "Escape from
tabs content returns focus to the tab bar" test: after you move focus to
"general-save" (using renderer.routeEngineEvent(keyEvent(3 /* TAB */)) and
verifying renderer.getFocusedId() === "general-save"), dispatch a Shift+Tab key
event via renderer.routeEngineEvent (use the same keyEvent helper but with the
shift modifier as used elsewhere in tests) and assert renderer.getFocusedId()
=== getTabsTriggerId("settings-tabs", 0, "general") (the generalTrigger
variable), keeping the existing forward-tab and ESC assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ca82c46-2bc4-4a29-a356-e0283520783a
📒 Files selected for processing (1)
packages/core/src/app/__tests__/widgetRenderer.integration.test.ts
RtlZeroMemory
force-pushed
the
feature/testing-tabs-behavior
branch
3 times, most recently
from
964a121 to
78079e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/app/__tests__/widgetRenderer.integration.test.ts (1)
1411-1457: Add aShift+Tabreverse-traversal assertion for tabs focus flow.This test already proves
ESCrecovery well. Adding one explicitShift+Tabcheck (content -> trigger) would complete the focus traversal contract for this path.Suggested test refinement
renderer.routeEngineEvent(keyEvent(3 /* TAB */)); assert.equal(renderer.getFocusedId(), "general-save"); + renderer.routeEngineEvent(keyEvent(3 /* TAB */, ZR_MOD_SHIFT)); + assert.equal(renderer.getFocusedId(), generalTrigger); + + renderer.routeEngineEvent(keyEvent(3 /* TAB */)); + assert.equal(renderer.getFocusedId(), "general-save"); + renderer.routeEngineEvent(keyEvent(1 /* ESC */)); assert.equal(renderer.getFocusedId(), generalTrigger);Based on learnings: "Test focus traversal by asserting focused ID movement across Tab/Shift+Tab navigation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/widgetRenderer.integration.test.ts` around lines 1411 - 1457, Extend the "Escape from tabs content returns focus to the tab bar" test by adding a reverse-traversal assertion using Shift+Tab: after focusing the content button ("general-save") (currently achieved by calling renderer.routeEngineEvent(keyEvent(3 /* TAB */)) twice), simulate Shift+Tab (use renderer.routeEngineEvent with the keyEvent representing Shift+Tab) and assert that renderer.getFocusedId() returns the tab trigger id (generalTrigger from getTabsTriggerId("settings-tabs", 0, "general")). This complements the existing ESC check and verifies content -> trigger focus movement on reverse traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/app/__tests__/widgetRenderer.integration.test.ts`:
- Around line 1411-1457: Extend the "Escape from tabs content returns focus to
the tab bar" test by adding a reverse-traversal assertion using Shift+Tab: after
focusing the content button ("general-save") (currently achieved by calling
renderer.routeEngineEvent(keyEvent(3 /* TAB */)) twice), simulate Shift+Tab (use
renderer.routeEngineEvent with the keyEvent representing Shift+Tab) and assert
that renderer.getFocusedId() returns the tab trigger id (generalTrigger from
getTabsTriggerId("settings-tabs", 0, "general")). This complements the existing
ESC check and verifies content -> trigger focus movement on reverse traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09bb8f24-c8ec-42b5-87e7-e2dd2612a271
📒 Files selected for processing (1)
packages/core/src/app/__tests__/widgetRenderer.integration.test.ts
RtlZeroMemory
force-pushed
the
feature/testing-tabs-behavior
branch
from
78079e2 to
de44b24
Compare
1 participant
Summary
Escapereturning focus from tab content to the tab barFamilies Covered
Tests Added, Rewritten, Removed
packages/core/src/app/__tests__/widgetRenderer.integration.test.tsfor arrow-key tab switching with visible active-content updatesEscapereturning focus from tab content to the tab barImplementation Bugs Fixed
Commands Run
./node_modules/.bin/tsc -b packages/core/tsconfig.json --pretty falsenode --test packages/core/dist/app/__tests__/widgetRenderer.integration.test.js packages/core/dist/widgets/__tests__/tabs.test.js packages/core/dist/router/__tests__/helpers.test.js packages/core/dist/__tests__/integration/integration.file-manager.test.jsUnresolved Areas Left Explicit
accordion,breadcrumb, andpaginationremains separateui.tabssliceDependency Note
Summary by CodeRabbit