Skip to content

fix(electron): stabilize canvas-first shell #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
crs48 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(electron): stabilize canvas-first shell #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90133c9
fix(ui): avoid invalid base menu labels
crs48 Mar 8, 2026
49c8459
fix(electron): restore canvas workspace height
crs48 Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions apps/electron/src/renderer/components/CanvasView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* @vitest-environment jsdom
*/

import { render, screen } from '@testing-library/react'
import React from 'react'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { CanvasView } from './CanvasView'

const mockUseIdentity = vi.fn()
const mockUseNode = vi.fn()

vi.mock('@xnetjs/canvas', async () => {
const ReactModule = await import('react')

return {
Canvas: ReactModule.forwardRef(function MockCanvas() {
return <div data-testid="mock-canvas">canvas</div>
}),
createNode: vi.fn(() => ({
id: 'node-1',
type: 'card',
position: { x: 0, y: 0, width: 100, height: 100 },
properties: {}
}))
}
})

vi.mock('@xnetjs/data', () => ({
CanvasSchema: {
schema: {
'@id': 'xnet://schema/canvas'
}
}
}))

vi.mock('@xnetjs/react', () => ({
useIdentity: (...args: unknown[]) => mockUseIdentity(...args),
useNode: (...args: unknown[]) => mockUseNode(...args)
}))

describe('CanvasView', () => {
beforeEach(() => {
const nodesMap = {
size: 0,
observe: vi.fn(),
unobserve: vi.fn()
}

mockUseIdentity.mockReturnValue({ did: 'did:key:test' })
mockUseNode.mockReturnValue({
data: { title: 'Workspace Canvas' },
doc: {
getMap: vi.fn(() => nodesMap)
},
loading: false,
awareness: null
})
})

it('keeps the canvas host at full height for the renderer shell', () => {
render(<CanvasView docId="canvas-1" />)

const canvas = screen.getByTestId('mock-canvas')
const canvasHost = canvas.parentElement?.parentElement
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


expect(canvasHost).not.toBeNull()
expect(canvasHost?.className).toContain('h-full')
})
})
2 changes: 1 addition & 1 deletion apps/electron/src/renderer/components/CanvasView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export const CanvasView = forwardRef<CanvasViewHandle, CanvasViewProps>(function
}

return (
<div className="relative flex-1 overflow-hidden">
<div className="relative h-full overflow-hidden">
<div className="pointer-events-none absolute left-6 top-6 z-20 rounded-full border border-border/60 bg-background/80 px-4 py-2 text-xs uppercase tracking-[0.24em] text-muted-foreground shadow-lg backdrop-blur-xl">
{canvas?.title || 'Workspace Canvas'}
</div>
Expand Down
6 changes: 1 addition & 5 deletions packages/ui/src/primitives/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,7 @@ export function MenuSeparator() {
* A label for use with the simple Menu component.
*/
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>
}
Comment on lines 117 to 119
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// ─── Compound Components (for advanced usage) ───────────────────────
Expand Down