Conversation
- render simple menu labels as static text instead of Base UI group labels - prevent SystemMenu from crashing the Electron shell on open - keep advanced dropdown group label exports unchanged for grouped menus
- replace the canvas shell wrapper with full-height sizing so it cannot collapse inside the absolute workspace shell - add a renderer regression test that guards the canvas host height contract
|
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 (3)
📝 WalkthroughWalkthroughThis PR adds a comprehensive unit test for CanvasView, updates the component's wrapper className for full-height rendering, and simplifies MenuLabel rendering by replacing a styled group label component with a plain div. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 unit tests (beta)
Comment |
Greptile SummaryThis PR makes three targeted stability fixes to the canvas-first Electron shell: it corrects the canvas container's height class ( Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Electron Renderer Shell"] -->|"renders"| B["CanvasView\n(h-full container)"]
B --> C["Title overlay\n(absolute, z-20)"]
B --> D["Empty-state hint\n(absolute, pointer-events-none)"]
B --> E["Inner wrapper\n(h-full)"]
E --> F["Canvas component\n(fills inner wrapper)"]
G["Menu trigger"] --> H["BaseMenu.Root / Popup"]
H --> I["MenuItem(s)"]
H --> J["MenuLabel\n(plain div — no BaseMenu.GroupLabel)"]
H --> K["MenuSeparator\n(BaseMenu.Separator)"]
Last reviewed commit: 49c8459 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| render(<CanvasView docId="canvas-1" />) | ||
|
|
||
| const canvas = screen.getByTestId('mock-canvas') | ||
| const canvasHost = canvas.parentElement?.parentElement |
There was a problem hiding this comment.
Brittle DOM traversal for host element lookup
The test navigates up the DOM via canvas.parentElement?.parentElement to find the canvas host container. This couples the test to the exact depth of nesting between the mock-canvas element and the target div. If an additional wrapper element is ever introduced inside <div className="h-full"> (the inner wrapper at line 247 of CanvasView.tsx), this traversal silently resolves to the wrong element and the assertion either gives a false positive or a confusing failure.
A more resilient approach is to add a data-testid to the host div in CanvasView.tsx and query it directly:
// CanvasView.tsx line 230
<div data-testid="canvas-host" className="relative h-full overflow-hidden">// CanvasView.test.tsx
const canvasHost = screen.getByTestId('canvas-host')
expect(canvasHost.className).toContain('h-full')This makes the test resilient to structural changes while still locking the h-full behaviour.
| export function MenuLabel({ children }: { children: React.ReactNode }) { | ||
| return ( | ||
| <BaseMenu.GroupLabel className="px-2 py-1.5 text-xs font-semibold text-foreground-muted"> | ||
| {children} | ||
| </BaseMenu.GroupLabel> | ||
| ) | ||
| return <div className="px-2 py-1.5 text-xs font-semibold text-foreground-muted">{children}</div> | ||
| } |
There was a problem hiding this comment.
Accessibility regression: ARIA group label semantics removed
Replacing BaseMenu.GroupLabel with a plain <div> fixes the crash (the Base UI primitive requires a BaseMenu.Group ancestor to supply its context), but it drops the semantic ARIA association. BaseMenu.GroupLabel renders with role="group" labelling semantics so that screen readers can announce the label when entering the group. The plain <div> has no implicit ARIA role, so assistive technology users will no longer hear the label when navigating through the menu.
If the intention is to avoid the crash entirely, the minimal fix that preserves semantics is to wrap any MenuLabel usage in a BaseMenu.Group:
// at the call site
<BaseMenu.Group>
<MenuLabel>Section heading</MenuLabel>
<MenuItem …>…</MenuItem>
</BaseMenu.Group>Alternatively, if the MenuLabel component must be usable without a group parent, emit a <div role="presentation"> or <div role="group" aria-label={…}> to maintain at least partial semantics. The current plain <div> is silent to screen readers.
1 participant
Summary
Validation
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes
Refactor