testing: cover breadcrumb behavior#377
Merged
RtlZeroMemory merged 2 commits intomainfrom Apr 15, 2026
Merged
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 (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBreadcrumbs no longer render inside a top-level focusZone; they now output a frozen Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Breadcrumb Builder
participant Renderer as Navigation Renderer
participant Drawlist as Drawlist
participant Focus as Focus/Keyboard System
participant App as App (onPress)
Builder->>Renderer: emit `breadcrumb` vnode (frozen `row` with children)
Renderer->>Renderer: detect `breadcrumb` kind
Renderer->>Renderer: iterate children, clone with navigation DS props when needed
Renderer->>Drawlist: apply overrides and render items
Focus->>Drawlist: TAB / Shift+TAB moves focus among rendered items
Focus->>App: ENTER on focused clickable item
App->>App: invoke `onPress` handler (once)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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/widgets/breadcrumb.ts (1)
25-27: Consider whethergetBreadcrumbZoneIdneeds to remain exported.The function has dedicated test coverage for deterministic encoding behavior, suggesting it was a deliberate API contract. However, if it's no longer required by external consumers (since the breadcrumb widget auto-generates IDs internally), consider marking it
@deprecatedor removing it along with its tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/breadcrumb.ts` around lines 25 - 27, The exported helper getBreadcrumbZoneId appears to be internal-only now; decide whether to remove or mark deprecated: search for external usages of getBreadcrumbZoneId (references/imports outside packages/core), if none remove the exported function and delete its dedicated tests and any exports from index/barrel files; otherwise add a JSDoc `@deprecated` tag to getBreadcrumbZoneId (update its comment) and keep tests but add a note in the test describing deprecation, ensuring callers still pass compilation and exports remain intact. Ensure you update any type exports or re-exports (barrel files) to match the chosen approach.
🤖 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/widgets/breadcrumb.ts`:
- Around line 25-27: The exported helper getBreadcrumbZoneId appears to be
internal-only now; decide whether to remove or mark deprecated: search for
external usages of getBreadcrumbZoneId (references/imports outside
packages/core), if none remove the exported function and delete its dedicated
tests and any exports from index/barrel files; otherwise add a JSDoc `@deprecated`
tag to getBreadcrumbZoneId (update its comment) and keep tests but add a note in
the test describing deprecation, ensuring callers still pass compilation and
exports remain intact. Ensure you update any type exports or re-exports (barrel
files) to match the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a733375d-b010-407b-8382-bd4f221e89e8
📒 Files selected for processing (4)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/renderToDrawlist/widgets/navigation.tspackages/core/src/widgets/__tests__/breadcrumb.test.tspackages/core/src/widgets/breadcrumb.ts
RtlZeroMemory
force-pushed
the
feature/testing-breadcrumb-behavior
branch
2 times, most recently
from
d048cde to
392e2b4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/widgets/__tests__/breadcrumb.test.ts (1)
93-96: Rename test to match what it actually exercises.The test title references
createBreadcrumbVNode, but the body callsbuildBreadcrumbChildren. Renaming avoids confusion during failures.Suggested rename
- test("createBreadcrumbVNode applies custom separator", () => { + test("buildBreadcrumbChildren applies custom separator", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/breadcrumb.test.ts` around lines 93 - 96, The test title is misleading: it says createBreadcrumbVNode but the body calls buildBreadcrumbChildren; rename the test to match the exercised function by changing the description string in the test declaration to something like "buildBreadcrumbChildren applies custom separator" so the test name reflects the function under test (look for the test(...) call containing buildBreadcrumbChildren and update its first argument).
🤖 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/widgets/__tests__/breadcrumb.test.ts`:
- Around line 93-96: The test title is misleading: it says createBreadcrumbVNode
but the body calls buildBreadcrumbChildren; rename the test to match the
exercised function by changing the description string in the test declaration to
something like "buildBreadcrumbChildren applies custom separator" so the test
name reflects the function under test (look for the test(...) call containing
buildBreadcrumbChildren and update its first argument).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8911e5bd-866f-4814-afc1-aa20ace88c44
📒 Files selected for processing (4)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/renderToDrawlist/widgets/navigation.tspackages/core/src/widgets/__tests__/breadcrumb.test.tspackages/core/src/widgets/breadcrumb.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/renderer/renderToDrawlist/widgets/navigation.ts
- packages/core/src/app/tests/widgetRenderer.integration.test.ts
RtlZeroMemory
force-pushed
the
feature/testing-breadcrumb-behavior
branch
from
392e2b4 to
bd6975a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/widgets/__tests__/breadcrumb.test.ts`:
- Around line 94-96: The test currently early-returns if the row shape is wrong,
allowing false-positive passes; update the test around buildBreadcrumbChildren
and the row variable to assert row.kind === "row" (e.g., using
expect(row?.kind).toBe("row")) before the guard so the test fails fast with
clear contract evidence, then keep the existing guard only as a
TypeScript/Narrowing aid; ensure the assertion references the row variable from
buildBreadcrumbChildren and preserves subsequent checks of row.props /
row.children.
🪄 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
Run ID: 0858940f-6dae-4a01-b978-66b4c5c49a29
📒 Files selected for processing (4)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/renderToDrawlist/widgets/navigation.tspackages/core/src/widgets/__tests__/breadcrumb.test.tspackages/core/src/widgets/breadcrumb.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/widgets/breadcrumb.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/app/tests/widgetRenderer.integration.test.ts
RtlZeroMemory
force-pushed
the
feature/testing-breadcrumb-behavior
branch
from
bd6975a to
4388d4a
Compare
RtlZeroMemory
force-pushed
the
feature/testing-breadcrumb-behavior
branch
from
4388d4a to
b5025eb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
breadcrumbkeyboard focus movement and activation at renderer-integration fidelityTabandShift+Tabmove across clickable crumbs as documentedTests added, rewritten, removed
packages/core/src/app/__tests__/widgetRenderer.integration.test.tsforTab,Shift+Tab, andEnteron clickable breadcrumb itemspackages/core/src/widgets/__tests__/breadcrumb.test.tsto keep deterministic clickable-item and separator coverage without pinningfocusZone/fragmentwrapper shapeImplementation bugs fixed because valid tests exposed them
packages/core/src/widgets/breadcrumb.tsno longer wraps clickable crumbs in afocusZone; the previous structure caused repeatedTabto cycle by zone traversal instead of moving crumb-by-crumbpackages/core/src/renderer/renderToDrawlist/widgets/navigation.tsnow applies breadcrumb design-system overrides directly to child buttons after the wrapper removalTest 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__/breadcrumb.test.js packages/core/dist/router/__tests__/helpers.test.js packages/core/dist/widgets/__tests__/widgetRenderSmoke.test.jsRemaining explicit gaps
getBreadcrumbZoneId(...)remains exported even though breadcrumb no longer uses a runtime focus-zone wrapperDependency note
Summary by CodeRabbit
Tests
Refactor