Skip to content

Commit 4488806

Browse files
alicodingclaude
andcommitted
fix: the Extensions list gets its industry-standard treatment -- grouped, noun-named, contained details, no escaping knobs
Five hands-on-review findings against Settings > Extensions: - The toggle knob's own compositor layer could paint outside its row after scrolling with a disclosure open; contain: paint on the row scopes every descendant's paint (and gives it a containing block) so this can never happen structurally. - The list groups into Knowledge/Files/Drawing sections (registry `group`, now shared by tray tools and tool-less nouns via the new AtlasNounGroup type) instead of repeating the group word on every row's own meta line. - Row titles read as the noun ("Card", "Pencil") via a new nounName field, separate from the command-verb label ("Add a card") that still drives the palette/tray tooltips. - The bare On/Off status text next to each switch is hidden (it was always aria-hidden in Primer's own markup, so nothing is lost for screen readers). - The Annotate tray group's own collapsed trigger now derives its face icon from the first enabled tool in the group instead of a hardcoded paintbrush, so disabling that tool never leaves a stale glyph. atlasNounRegistry.ts split along its own real seam (board-object content registry vs. the tray-tool registry) to stay under the 500-line file limit, re-exported in full so no other importer moves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012im1JxQQV2ahnXzZDdVmZq
1 parent 5e14385 commit 4488806

25 files changed

Lines changed: 533 additions & 233 deletions

frontend/e2e/settings-extensions.spec.ts

Lines changed: 112 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,18 @@ import { deleteViaContextMenu, shapeDrawPoints, shapeObjects } from './fixtures/
55
import { paletteDialog } from './fixtures/palette'
66

77
// Settings > Extensions (goal 0237 S2, extended by goal 0237 S3's
8-
// rider): a registry-derived list of every registered canvas NOUN --
9-
// every tray tool plus every tool-less noun (diagram, sheet -- native
10-
// file-drop only, no tray button), each toggleable off. Shared pool:
11-
// the only global state this spec writes (Shape's own disabled flag)
12-
// is restored to its default (enabled) before the file ends, same
13-
// cleanup discipline display-density.spec.ts already establishes for a
14-
// Settings toggle in the shared pool; every board object created here
15-
// is deleted here.
8+
// rider and its own hands-on-review follow-up): a registry-derived
9+
// list of every registered canvas NOUN -- every tray tool plus every
10+
// tool-less noun (diagram, sheet -- native file-drop only, no tray
11+
// button), each toggleable off. Grouped into three sections
12+
// (Knowledge/Files/Drawing, one per registry `group` -- goal 0237 S3's
13+
// review rider); each row's own title is the noun ("Shape"), never the
14+
// command-verb phrase ("Draw a shape") that still surfaces in the tray
15+
// tooltip and command palette. Shared pool: the only global state this
16+
// spec writes (Shape's own disabled flag) is restored to its default
17+
// (enabled) before the file ends, same cleanup discipline
18+
// display-density.spec.ts already establishes for a Settings toggle in
19+
// the shared pool; every board object created here is deleted here.
1620
//
1721
// Disabling diagram/sheet gates useAtlasNativeFileDrop.ts's own drop
1822
// routing (a disabled drop falls through to the plain-card path). The
@@ -45,6 +49,9 @@ test('A row expands to show its description, an honest reach line, and the app v
4549
await openExtensionsSection(page)
4650

4751
const shapeRow = page.locator('[data-testid="extensions-row"][data-extension-id="shape"]')
52+
// The row's own title is the bare noun, never the tray/palette's own
53+
// command-verb phrase ("Draw a shape").
54+
await expect(shapeRow.getByTestId('extensions-row-title')).toHaveText('Shape')
4855
const expanded = shapeRow.getByTestId('extensions-row-expanded')
4956
await expect(expanded).toBeHidden()
5057

@@ -57,12 +64,109 @@ test('A row expands to show its description, an honest reach line, and the app v
5764
await expect(shapeRow.getByTestId('extensions-row-description')).toHaveText('Draws a rectangle, ellipse, or arrow.')
5865
await expect(shapeRow.getByTestId('extensions-row-reach')).toHaveText('Reaches nothing outside Mill.')
5966
await expect(shapeRow.getByTestId('extensions-row-version')).toHaveText(/^Ships with Mill v/)
67+
// The group chip survives in the expanded view even though the
68+
// collapsed meta line below no longer repeats it.
69+
await expect(expanded.getByText('Drawing', { exact: true })).toBeVisible()
6070

6171
// Collapses again on a second click of the same summary.
6272
await shapeRow.locator('summary').click()
6373
await expect(expanded).toBeHidden()
6474
})
6575

76+
test('The list groups into three sections; every row title is a noun, and the collapsed meta line never repeats the group', async ({ page }) => {
77+
await page.goto('/')
78+
await openExtensionsSection(page)
79+
80+
// Three sections, registry-derived, in knowledge/files/drawing order.
81+
const knowledge = page.getByTestId('extensions-group-knowledge')
82+
const files = page.getByTestId('extensions-group-file')
83+
const drawing = page.getByTestId('extensions-group-annotate')
84+
await expect(knowledge.getByRole('heading', { name: 'Knowledge' })).toBeVisible()
85+
await expect(files.getByRole('heading', { name: 'Files' })).toBeVisible()
86+
await expect(drawing.getByRole('heading', { name: 'Drawing' })).toBeVisible()
87+
88+
// card/note/area/table land in Knowledge; image/diagram/sheet in
89+
// Files (image/diagram/sheet are the file-backed family); the
90+
// freehand-marking tools in Drawing.
91+
for (const id of ['card', 'note', 'area', 'table']) {
92+
await expect(knowledge.locator(`[data-testid="extensions-row"][data-extension-id="${id}"]`)).toBeVisible()
93+
}
94+
for (const id of ['image', 'diagram', 'sheet']) {
95+
await expect(files.locator(`[data-testid="extensions-row"][data-extension-id="${id}"]`)).toBeVisible()
96+
}
97+
for (const id of ['pencil', 'eraser', 'laser', 'shape']) {
98+
await expect(drawing.locator(`[data-testid="extensions-row"][data-extension-id="${id}"]`)).toBeVisible()
99+
}
100+
101+
// Every row's own title is the bare noun, one word.
102+
const expectedTitles: Record<string, string> = {
103+
card: 'Card', note: 'Note', area: 'Area', table: 'Table', image: 'Image',
104+
pencil: 'Pencil', eraser: 'Eraser', laser: 'Laser', shape: 'Shape',
105+
diagram: 'Diagram', sheet: 'Sheet',
106+
}
107+
for (const [id, title] of Object.entries(expectedTitles)) {
108+
const row = page.locator(`[data-testid="extensions-row"][data-extension-id="${id}"]`)
109+
await expect(row.getByTestId('extensions-row-title')).toHaveText(title)
110+
}
111+
112+
// The collapsed meta line states source/edit-route facts, never the
113+
// group word its own section heading already carries.
114+
const imageMeta = page.locator('[data-testid="extensions-row"][data-extension-id="image"]').getByTestId('extensions-row-meta')
115+
await expect(imageMeta).toHaveText('Backed by a file · Opens in your default app')
116+
const tableMeta = page.locator('[data-testid="extensions-row"][data-extension-id="table"]').getByTestId('extensions-row-meta')
117+
await expect(tableMeta).toHaveText('Live view of a List · Edits in place')
118+
const shapeMeta = page.locator('[data-testid="extensions-row"][data-extension-id="shape"]').getByTestId('extensions-row-meta')
119+
await expect(shapeMeta).toHaveText('Stored on the board')
120+
})
121+
122+
test('The collapsed meta line stays single-line at 1000px viewport width', async ({ page }) => {
123+
await page.setViewportSize({ width: 1000, height: 660 })
124+
await page.goto('/')
125+
await openExtensionsSection(page)
126+
127+
// diagram's own meta line is the longest in the list (a file source
128+
// plus its per-object edit-route resolver's own generic phrase) --
129+
// the stress case for wrapping.
130+
const diagramMeta = page.locator('[data-testid="extensions-row"][data-extension-id="diagram"]').getByTestId('extensions-row-meta')
131+
await expect(diagramMeta).toBeVisible()
132+
const box = await diagramMeta.boundingBox()
133+
if (!box) throw new Error('extensions-row-meta has no bounding box')
134+
// A single line of this small-text token is well under 24px tall;
135+
// two wrapped lines would roughly double it.
136+
expect(box.height).toBeLessThan(24)
137+
})
138+
139+
test('The toggle knob stays contained within its own row, even scrolled with a disclosure open (regression: it painted over the sticky search bar)', async ({ page }) => {
140+
await page.setViewportSize({ width: 1000, height: 660 })
141+
await page.goto('/')
142+
await openExtensionsSection(page)
143+
144+
// Scrolling to a row well below the fold pins the sticky search bar
145+
// (.filterRow, SettingsView.module.css) to the top of the scroll
146+
// pane -- the exact live condition the bug needed.
147+
const sheetRow = page.locator('[data-testid="extensions-row"][data-extension-id="sheet"]')
148+
await sheetRow.scrollIntoViewIfNeeded()
149+
await sheetRow.locator('summary').click()
150+
await expect(sheetRow.getByTestId('extensions-row-expanded')).toBeVisible()
151+
152+
const searchBar = page.getByTestId('settings-filter')
153+
await expect(searchBar).toBeVisible()
154+
const box = await searchBar.boundingBox()
155+
if (!box) throw new Error('settings-filter has no bounding box')
156+
const point = { x: box.x + box.width / 2, y: box.y + box.height / 2 }
157+
158+
const hit = await page.evaluate(({ x, y }) => {
159+
const el = document.elementFromPoint(x, y)
160+
return {
161+
isSearchBar: el?.closest('[data-testid="settings-filter"]') !== null,
162+
isToggleKnob: el?.className?.toString().includes('ToggleKnob') ?? false,
163+
}
164+
}, point)
165+
166+
expect(hit.isToggleKnob).toBe(false)
167+
expect(hit.isSearchBar).toBe(true)
168+
})
169+
66170
test('Turn all off empties the tray of every non-built-in tool; turn all on restores them', async ({ page }) => {
67171
await page.goto('/')
68172
await openExtensionsSection(page)

frontend/src/atlas/AtlasCreationTray.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,14 @@ export function AtlasCreationTray({ armedTool, locked, onToggle, tablePickerOpen
124124
)
125125
const primaryTools = PRIMARY_GROUP_ORDER.flatMap((group) => tools.filter((tool) => tool.group === group))
126126
const annotateTools = tools.filter((tool) => tool.group === 'annotate')
127+
// The collapsed group trigger's own face glyph (goal 0237 S3's review
128+
// rider): derived from the first ENABLED annotate tool rather than a
129+
// hardcoded icon, so disabling that tool from Settings > Extensions
130+
// never leaves the trigger showing a glyph for a tool that's no
131+
// longer in the group at all. Falls back to the generic paintbrush
132+
// only when every annotate tool is disabled and the drawer would
133+
// have nothing left to show anyway.
134+
const AnnotateGroupIcon = annotateTools[0]?.icon ?? PaintbrushIcon
127135
const annotateGroupAnchorRef = useRef<HTMLButtonElement>(null)
128136
// The Annotate group's own disclosure state (goal 0224). At most ONE
129137
// AnchoredOverlay for the whole annotate family is ever mounted at a
@@ -354,7 +362,7 @@ export function AtlasCreationTray({ armedTool, locked, onToggle, tablePickerOpen
354362
setManualOpen((open) => !open)
355363
}}
356364
>
357-
<PaintbrushIcon size={14} />
365+
<AnnotateGroupIcon size={14} />
358366
<ChevronDownIcon size={12} />
359367
</button>
360368
<AnchoredOverlay

0 commit comments

Comments
 (0)