fix(shell): the renderer owns dashboard icon placement — installs stop landing off-screen (POLISH-LAY-9) - #370
Merged
Conversation
`firstFreeIconCell` had no column bound — only a MAX_SLOT_SCAN — so the install slot marched along one unbounded row. With the captured 20-app fleet at `0,0 · 11,0 … 209,0`, the next install landed at `220,0` = 1760px on a 1440px stage: off the edge of `.dashboard-icons`, which does not scroll horizontally, i.e. permanently unreachable. Books/Chat/Forms/ Agent were already past the fold for the same reason. A column bound needs the icon surface's width, and only the renderer knows it (`widget-host.ts` already documents the viewport as "which only the renderer knows"). So placement moves: - Main (`placeDashboardIcon`, `dashboard.pin`) writes `UNPLACED_ICON_POSITION` — a typed `-1,-1` sentinel, unambiguous because every real cell is ≥ 0, and not a magic `0,0` that would read as a deliberate top-left placement. `main/dashboard/grid-placement.ts` is gone: main no longer places anything. - The icons layer resolves those against its measured viewport via `resolveIconPlacements` and persists the cell through the existing `dashboard.moveIcon` path. It resolves in the render memo, so a fresh install paints in its free on-screen slot the same frame the snapshot arrives — the live no-restart reveal is unchanged. - The two renderer-side pin paths (app-grid pin, Bin pin) stop picking cells too; everything funnels through the one placer. Wrapping derives from the shared footprint constants; a viewport too narrow for one icon clamps to a single column. Pre-existing overflow is re-flowed in the VIEW MODEL only (`IconPlacementReason.Offscreen`), never persisted — mirroring `clampWidgetOrigin` (F-379): stranded icons become reachable immediately, widening the window restores the user's own arrangement, and dragging a rescued icon commits it normally. The geometry stays in the one shared module (#369's point); what moved is who calls it and with what bound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
The bug
firstFreeIconCell(the shared placer #369 introduced) had no column bound — only aMAX_SLOT_SCAN— so it filled a single unbounded row. On the captured dashboard (20 seeded apps at0,0 · 11,0 · 22,0 … 209,0, 8px grid units, 11×14-unit footprint) a newly installed app was placed at220,0= 1760px on a 1440px stage..dashboard-iconsdoes not scroll horizontally, so that icon is permanently unreachable — and Books / Chat / Forms / Agent were already past the fold for the same reason.This is a regression exposed, not caused by #369: before it, the seeder wrote 1×1 cell indices that tripped
isLegacyIconLayout, so the renderer's one-shotrepackIconsmigration incidentally re-wrapped the fleet every launch. That accident is correctly gone, which is what made the missing column bound visible.It bites any user whose window is narrower than their icon count needs — not just the video shoot.
The architecture change: the renderer owns placement
A column bound needs the icon surface's width, and main cannot know it (
main/dashboard/widget-host.tsalready documents the viewport as "which only the renderer knows"). Main was guessing, and guessing wrong.placeDashboardIcon(seeder / marketplace / sideload / packaged seed) and thedashboard.pinbroker method now writeUNPLACED_ICON_POSITION— a typed-1,-1sentinel. Unambiguous (every real cell is ≥ 0) and deliberately not a magic0,0, which would read as a genuine top-left placement and drop the tile on Notes.main/dashboard/grid-placement.tsis deleted; nothing on the main side computes a cell any more.resolveIconPlacements(icons, surfaceWidthPx)runs in the icons layer's render memo, so an unplaced icon paints in a free on-screen slot the same frame its snapshot arrives — the live no-restart install reveal is preserved, with no flash at the sentinel — and an effect persists the chosen cell through the existingdashboard.moveIconpath (guarded by a ref so it isn't re-issued while the store round-trips, and gated on the surface actually having been measured).shared/dashboard-icon-grid.ts— that was fix(shell): three VID-build-apps capture blockers — tile collision, truncated grants, icon 404s #369's whole point. What moved is who calls it and with what column bound.pointToCell's drop clamp now shares its right-edge bound with the placer viamaxIconCol, so a drop and an install agree on where the edge is.Wrapping derives from
ICON_FOOTPRINT_W/H(no hardcoded 14). A viewport too narrow for even one icon clamps to a single column and stacks vertically.Pre-existing overflow: rescued in the view model, not repacked
The four already-stranded apps become reachable, but their new cells are not persisted (
IconPlacementReason.OffscreenvsUnplaced). This mirrorsclampWidgetOrigin(F-379), the established pattern for exactly this problem: the widgets layer rescues off-surface records into the view model and lets the next touch commit them.Why not a full repack: a persisted reflow means shrinking the window permanently rewrites a layout the user arranged in a wide one, with no way back. With the view-model rescue, stranded icons are immediately reachable, widening the window restores the user's own arrangement, and dragging a rescued icon persists its new spot through the normal drop path. Only icons that genuinely have no position are written.
Evidence — the captured fleet, 1440px stage
(
r= the icon box's right edge in surface px; the stage is 1440.) The 16 icons that already fit are untouched; the five that didn't wrap onto row 2.Tests
shared/dashboard-icon-grid.test.ts, new, 28 cases): wraps at the column bound; every slot handed out stays on-screen for its bound; a narrow viewport clamps to one column and stacks; existing occupied slots (including hand-dragged, off-lattice icons) are respected; idempotent — re-resolving a layout it just placed moves nothing; assignment order is the map's key order so a seeded fleet keeps its order; a 0/NaN surface resolves nothing.icons-layer-placement.test.tsx, new): an icon arriving without a position is painted on-screen and persisted, and the write is not re-issued while the store round-trips; a fleet wider than the viewport wraps to row 2 rather than running off the edge, with only the genuinely-unplaced icon written back; a 140px viewport stacks into one column.placeDashboardIconwrites the sentinel and touches no existing icon;dashboard.pindoes the same; a 20-app seed of sentinels resolves into a wrapped, fully on-screen grid. The old "main picks a distinct free cell" assertions are replaced, not deleted.0,0 · 11,0 … 209,0on 1440px) asserted at both the pure and renderer levels — the newcomer lands inside the viewport, the four stranded apps come back, and no two resolved footprints overlap.bun run verify✅ ·bun run lint✅ ·bun run typecheck(packages + apps) ✅ · fullbun run test✅ 1644 files / 16380 tests passing.Four paths
--icon-col/--icon-row); the renderer test asserts the painted custom properties, not just the model.useCompositeKeyboard,Orientation.Spatial) navigates by each icon's{col, row}. Those cells previously included icons at col 209+, so arrowing right walked the focus off-screen to an invisible tile; every cell it can reach is now on-screen.group, same per-icon accessible names. No new controls, no new announcements; a placed icon is announced identically to a dragged one.Security
No new capability, IPC method or dependency.
dashboard:move-iconwas already renderer-reachable and vault-scoped; the sentinel is validated by the samereadIconRecordnumeric check as any other position, andresolveIconPlacementsis pure with a bounded row scan.(Also carries a one-line
biome formatfix togrants-panel-layout.test.ts, which is unformatted onmainand failsbun run lintthere.)🤖 Generated with Claude Code