From 68a0aba86cc05c69732e5646dd91e29e726d7274 Mon Sep 17 00:00:00 2001 From: brainstorm-os Date: Wed, 29 Jul 2026 19:04:09 +0200 Subject: [PATCH] =?UTF-8?q?fix(shell):=20three=20VID-build-apps=20capture?= =?UTF-8?q?=20blockers=20=E2=80=94=20tile=20collision,=20truncated=20grant?= =?UTF-8?q?s,=20icon=20404s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POLISH-LAY-6 — a newly installed app's tile landed ON TOP of Notes. Units-vs-cells mismatch: the renderer stores icon positions in 8px GRID_UNITs and paints an ICON_FOOTPRINT_W × ICON_FOOTPRINT_H (11×14) box per icon, while `placeDashboardIcon` scanned a fictional 12-column grid of 1×1 cells — so with Notes at 0,0 the "free" cell 1,0 was inside Notes' box. Replaying the old scan against the captured fleet (0,0 · 11,0 · 22,0 · 0,14) reproduces exactly the reported 1,0. Fixed by making one placer, not two: `shared/dashboard-icon-grid.ts` owns the footprint constants, the overlap test and the first-free-slot scan; the renderer's `dashboard/grid.ts` and main's `dashboard/grid-placement.ts` both build on it, and every install/pin path (marketplace, sideload, vault-source, dev seeder, entity pin, Bin pin) now computes its cell there. Also fixes the Bin's shell-surface pin, which wrote {0,0} on the stale premise that "the collision layer repositions it" — icon layout has been free-placement with no collision resolution for some time, so it stacked on the origin tile. POLISH-LAY-7 — the grants popover ellipsised the capability scope (`entities.read:brainstorm/Pro…`), i.e. the panel that answers "what can this app read?" truncated the only part that answers it. The row drops from three columns to two, stacking capability over "granted via" in one text column — the same shape as the app rows behind the popover — and the capability wraps (`overflow-wrap: anywhere`) instead of ellipsising. POLISH-LAY-8 — `brainstorm://app-icon/` 404'd for icon-less sideloaded apps, the only console errors in the dry-run. Two sides: * the handler now distinguishes "installed, declares no icon" (204 — not an error, the caller draws initials) from "resolves to nothing installed" (404, still worth surfacing). A declared-but-missing icon file resolves as no-icon too, instead of handing the caller a missing `file://` to fetch (ERR_UNEXPECTED in the renderer). Rules extracted to a testable `apps/app-icon-resolver.ts`. * the request side converges on the shared `resolveAppIconSrc`, which already answers `null` for a known icon-less app — the running strip, the window switcher, widget headers and marketplace cards each hand-built their own URL and so opted out of it. The URL builder itself moves to `shared/app-icon-url.ts` (preload + renderer share it). Tests: pure placement + occupancy (incl. the exact captured fleet, the wrap case and an off-lattice dragged icon), install-path placement over the real DashboardStore, icon resolution + miss-status, icon-cache epoch/suppression, and a CSS ratchet pinning the grants row's non-truncating layout. `bun run verify`, `bun run lint`, `bun run typecheck`, full `bun run test` (1640 files / 16340 tests) green. Co-Authored-By: Claude Fable 5 --- .../src/main/apps/app-icon-resolver.test.ts | 101 ++++++++++++++++++ .../shell/src/main/apps/app-icon-resolver.ts | 88 +++++++++++++++ .../main/dashboard/dashboard-store.test.ts | 86 +++++++++++++++ .../src/main/dashboard/grid-placement.test.ts | 85 ++++++++++++--- .../src/main/dashboard/grid-placement.ts | 52 ++++----- packages/shell/src/main/dev/seed-demo-apps.ts | 52 ++++----- packages/shell/src/main/index.ts | 46 ++++---- packages/shell/src/preload/index.ts | 6 +- packages/shell/src/renderer/bin/bin.tsx | 17 ++- packages/shell/src/renderer/dashboard.tsx | 7 +- .../renderer/dashboard/app-icon-cache.test.ts | 30 +++--- .../src/renderer/dashboard/app-icon-cache.ts | 25 +++-- packages/shell/src/renderer/dashboard/grid.ts | 60 +++++------ .../src/renderer/dashboard/widgets-layer.tsx | 14 ++- .../src/renderer/dashboard/window-strip.tsx | 3 +- .../renderer/dashboard/window-switcher.tsx | 3 +- .../src/renderer/marketplace/listing-card.tsx | 3 +- .../renderer/marketplace/listing-detail.tsx | 3 +- .../settings/grants-panel-layout.test.ts | 48 +++++++++ .../src/renderer/settings/grants-panel.tsx | 18 +++- .../shell/src/renderer/settings/settings.css | 20 +++- packages/shell/src/shared/app-icon-url.ts | 17 +++ .../shell/src/shared/dashboard-icon-grid.ts | 72 +++++++++++++ 23 files changed, 675 insertions(+), 181 deletions(-) create mode 100644 packages/shell/src/main/apps/app-icon-resolver.test.ts create mode 100644 packages/shell/src/main/apps/app-icon-resolver.ts create mode 100644 packages/shell/src/renderer/settings/grants-panel-layout.test.ts create mode 100644 packages/shell/src/shared/app-icon-url.ts create mode 100644 packages/shell/src/shared/dashboard-icon-grid.ts diff --git a/packages/shell/src/main/apps/app-icon-resolver.test.ts b/packages/shell/src/main/apps/app-icon-resolver.test.ts new file mode 100644 index 00000000..5777f4ec --- /dev/null +++ b/packages/shell/src/main/apps/app-icon-resolver.test.ts @@ -0,0 +1,101 @@ +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + AppIconMiss, + appIconMissStatus, + isAppIconHit, + resolveAppIconInBundle, +} from "./app-icon-resolver"; + +const BUNDLE = join("/vaults", "v1", "apps", "studio.northbound.client-pulse", "1.0.0"); + +/** Bundle IO stub: a manifest object plus the files that exist on disk. */ +const io = (manifest: unknown, files: readonly string[] = []) => ({ + readManifest: async () => JSON.stringify(manifest), + fileExists: async (path: string) => files.includes(path), +}); + +describe("resolveAppIconInBundle", () => { + it("resolves a declared icon inside the bundle", async () => { + const resolved = await resolveAppIconInBundle( + BUNDLE, + io({ id: "x", icon: "icon.svg" }, [join(BUNDLE, "icon.svg")]), + ); + expect(isAppIconHit(resolved)).toBe(true); + expect(resolved).toEqual({ path: join(BUNDLE, "icon.svg") }); + }); + + it("resolves an icon in a subdirectory", async () => { + const resolved = await resolveAppIconInBundle( + BUNDLE, + io({ icon: "assets/icon.png" }, [join(BUNDLE, "assets", "icon.png")]), + ); + expect(resolved).toEqual({ path: join(BUNDLE, "assets", "icon.png") }); + }); + + // POLISH-LAY-8 — the sideloaded Client Pulse / Hello case: no `icon` key at + // all. Not an error; the caller draws initials. + it("reports NoIcon for a manifest with no icon key", async () => { + expect(await resolveAppIconInBundle(BUNDLE, io({ id: "x" }))).toEqual({ + miss: AppIconMiss.NoIcon, + }); + }); + + it("reports NoIcon for an empty or non-string icon", async () => { + expect(await resolveAppIconInBundle(BUNDLE, io({ icon: "" }))).toEqual({ + miss: AppIconMiss.NoIcon, + }); + expect(await resolveAppIconInBundle(BUNDLE, io({ icon: 7 }))).toEqual({ + miss: AppIconMiss.NoIcon, + }); + }); + + it("reports NoIcon for a declared icon that isn't on disk", async () => { + // Never hand the caller a path it will fetch as a missing `file://` + // (ERR_UNEXPECTED in the renderer) — the app simply has no artwork. + expect(await resolveAppIconInBundle(BUNDLE, io({ icon: "icon.svg" }, []))).toEqual({ + miss: AppIconMiss.NoIcon, + }); + }); + + it("refuses a path that escapes the bundle", async () => { + expect( + await resolveAppIconInBundle(BUNDLE, io({ icon: "../../secrets.png" }, ["/vaults/secrets.png"])), + ).toEqual({ miss: AppIconMiss.NoIcon }); + }); + + it("contains an absolute-looking icon path inside the bundle", async () => { + // `join` treats a leading slash as relative, so `/etc/passwd` can only + // ever address `/etc/passwd` — and that doesn't exist. + expect( + await resolveAppIconInBundle(BUNDLE, io({ icon: "/etc/passwd" }, ["/etc/passwd"])), + ).toEqual({ miss: AppIconMiss.NoIcon }); + }); + + it("reports Unresolved when the app has no active bundle", async () => { + expect(await resolveAppIconInBundle(null)).toEqual({ miss: AppIconMiss.Unresolved }); + }); + + it("reports Unresolved when the manifest is missing or unparseable", async () => { + expect( + await resolveAppIconInBundle(BUNDLE, { + readManifest: async () => { + throw new Error("ENOENT"); + }, + }), + ).toEqual({ miss: AppIconMiss.Unresolved }); + expect(await resolveAppIconInBundle(BUNDLE, { readManifest: async () => "{ not json" })).toEqual({ + miss: AppIconMiss.Unresolved, + }); + }); +}); + +describe("appIconMissStatus", () => { + it("answers 204 for a declared-no-icon app so no console 404 is logged", () => { + expect(appIconMissStatus(AppIconMiss.NoIcon)).toBe(204); + }); + + it("keeps 404 meaningful for an id that resolves to nothing installed", () => { + expect(appIconMissStatus(AppIconMiss.Unresolved)).toBe(404); + }); +}); diff --git a/packages/shell/src/main/apps/app-icon-resolver.ts b/packages/shell/src/main/apps/app-icon-resolver.ts new file mode 100644 index 00000000..c913131e --- /dev/null +++ b/packages/shell/src/main/apps/app-icon-resolver.ts @@ -0,0 +1,88 @@ +/** + * `brainstorm://app-icon/` resolution — which file (if any) backs an + * app's icon, and WHY there isn't one. + * + * The distinction is the point. An installed app that declares no `icon` is a + * normal, expected state: every icon surface already draws a deterministic + * initials tile for it. Answering that with a 404 turned "this app has no + * artwork" into a console error on every surface that renders the app, every + * boot — four per run for two icon-less sideloaded apps, and the only console + * errors in the VID-build-apps capture (POLISH-LAY-8). It answers 204 instead. + * A 404 stays reserved for an id that resolves to nothing installed, which IS + * worth surfacing. + * + * Path containment (the icon must stay inside the app's own bundle) is part of + * the resolution, not the caller's job. + */ + +import { readFile, stat } from "node:fs/promises"; +import { join, normalize, sep } from "node:path"; + +export enum AppIconMiss { + /** No session, no active record for this id, or an unreadable manifest — a + * real miss the caller should see. */ + Unresolved = "unresolved", + /** Installed, but the manifest declares no in-bundle icon. Not an error. */ + NoIcon = "no-icon", +} + +export type AppIconResolution = { path: string } | { miss: AppIconMiss }; + +export function isAppIconHit(resolution: AppIconResolution): resolution is { path: string } { + return "path" in resolution; +} + +/** HTTP status for a miss — 204 (no content) for a declared-no-icon app so the + * caller's initials fallback runs quietly; 404 for an unresolvable id. */ +export function appIconMissStatus(miss: AppIconMiss): number { + return miss === AppIconMiss.NoIcon ? 204 : 404; +} + +export type AppIconResolverIo = { + /** Read `/manifest.json`. */ + readManifest: (dir: string) => Promise; + /** Does the resolved icon file exist on disk? */ + fileExists: (path: string) => Promise; +}; + +const DEFAULT_IO: AppIconResolverIo = { + readManifest: (dir) => readFile(join(dir, "manifest.json"), "utf8"), + fileExists: (path) => + stat(path).then( + () => true, + () => false, + ), +}; + +/** Resolve the icon file inside an installed app's bundle. `bundleDir` is + * `null` when no active record exists for the id. `io` is injectable for + * tests. + * + * A declared icon that isn't actually on disk resolves to `NoIcon`, not to a + * path: letting the caller `net.fetch` a missing `file://` surfaces as an + * `ERR_UNEXPECTED` in the renderer rather than a clean status (the same trap + * the emoji handler already guards). Either way the app has no usable + * artwork, which is exactly what `NoIcon` means. */ +export async function resolveAppIconInBundle( + bundleDir: string | null, + io: Partial = {}, +): Promise { + if (!bundleDir) return { miss: AppIconMiss.Unresolved }; + const { readManifest, fileExists } = { ...DEFAULT_IO, ...io }; + let manifest: { icon?: unknown }; + try { + manifest = JSON.parse(await readManifest(bundleDir)) as { icon?: unknown }; + } catch { + return { miss: AppIconMiss.Unresolved }; + } + if (typeof manifest.icon !== "string" || manifest.icon.length === 0) { + return { miss: AppIconMiss.NoIcon }; + } + if (manifest.icon.includes("..")) return { miss: AppIconMiss.NoIcon }; + const target = normalize(join(bundleDir, manifest.icon)); + if (!target.startsWith(bundleDir + sep) && target !== bundleDir) { + return { miss: AppIconMiss.NoIcon }; + } + if (!(await fileExists(target))) return { miss: AppIconMiss.NoIcon }; + return { path: target }; +} diff --git a/packages/shell/src/main/dashboard/dashboard-store.test.ts b/packages/shell/src/main/dashboard/dashboard-store.test.ts index 4b55a273..09390a25 100644 --- a/packages/shell/src/main/dashboard/dashboard-store.test.ts +++ b/packages/shell/src/main/dashboard/dashboard-store.test.ts @@ -12,6 +12,11 @@ import { import { OsHandoffConsent } from "@brainstorm-os/sdk-types"; import { ThemeName } from "@brainstorm-os/tokens"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + ICON_FOOTPRINT_H, + ICON_FOOTPRINT_W, + footprintsOverlap, +} from "../../shared/dashboard-icon-grid"; import { placeDashboardIcon } from "../dev/seed-demo-apps"; import { YDocStore } from "../storage/ydoc-store"; import { DASHBOARD_DOC_ID, DashboardStore, applyLegacyMigration } from "./dashboard-store"; @@ -665,6 +670,87 @@ describe("DashboardStore — shell prefs (locale / regional / chrome / notificat await store.close(); }); + // POLISH-LAY-6 — the install path, end to end over the real store. Main + // used to place onto a fictional 12-column grid of 1×1 cells while the + // renderer stores 8px units and paints an ICON_FOOTPRINT_W × _H box per + // icon, so a newly installed app landed ON TOP of Notes. + it("places an installed app's icon clear of every existing icon's footprint", async () => { + const store = await DashboardStore.open(yStore); + // The seeded fleet as captured on the real dashboard. + const seeded = [ + { id: "io.brainstorm.notes", x: 0, y: 0 }, + { id: "io.brainstorm.files", x: 11, y: 0 }, + { id: "io.brainstorm.database", x: 22, y: 0 }, + { id: "io.brainstorm.books", x: 0, y: 14 }, + ]; + for (const app of seeded) { + store.upsertIcon(`icon_${app.id}_demo`, { + x: app.x, + y: app.y, + kind: "app", + target: app.id, + label: app.id, + }); + } + + expect(placeDashboardIcon(store, "studio.northbound.client-pulse", "Client Pulse")).toBe(true); + const placed = store.snapshot().icons["icon_studio.northbound.client-pulse_demo"]; + expect(placed).toBeDefined(); + if (!placed) throw new Error("unreachable"); + for (const app of seeded) { + expect(footprintsOverlap({ col: placed.x, row: placed.y }, { col: app.x, row: app.y })).toBe( + false, + ); + } + + // A second install stacks on neither the fleet nor the first newcomer. + expect(placeDashboardIcon(store, "io.brainstorm.hello", "Hello")).toBe(true); + const second = store.snapshot().icons["icon_io.brainstorm.hello_demo"]; + if (!second) throw new Error("unreachable"); + for (const other of [...seeded.map((a) => ({ x: a.x, y: a.y })), placed]) { + expect(footprintsOverlap({ col: second.x, row: second.y }, { col: other.x, row: other.y })).toBe( + false, + ); + } + await store.close(); + }); + + it("wraps an install onto the next footprint row once the first row is full", async () => { + const store = await DashboardStore.open(yStore); + // Fill the first footprint row wall to wall for a 1440px-wide surface + // (≈ 16 slots at ICON_FOOTPRINT_W); the placer keeps stepping across + // (the grid is unbounded) but never overlaps. + for (let i = 0; i < 16; i++) { + store.upsertIcon(`icon_seed${i}_demo`, { + x: i * ICON_FOOTPRINT_W, + y: 0, + kind: "app", + target: `seed${i}`, + label: `Seed ${i}`, + }); + } + // One icon parked on the second footprint row's origin: the newcomer + // must clear that too. + store.upsertIcon("icon_seedRow2_demo", { + x: 0, + y: ICON_FOOTPRINT_H, + kind: "app", + target: "seedRow2", + label: "Row 2", + }); + + expect(placeDashboardIcon(store, "io.brainstorm.newcomer", "Newcomer")).toBe(true); + const placed = store.snapshot().icons["icon_io.brainstorm.newcomer_demo"]; + if (!placed) throw new Error("unreachable"); + for (const icon of Object.values(store.snapshot().icons)) { + if (icon.target === "io.brainstorm.newcomer") continue; + expect(footprintsOverlap({ col: placed.x, row: placed.y }, { col: icon.x, row: icon.y })).toBe( + false, + ); + } + await store.close(); + }); + it("isWithinDnd handles same-day and wrap-past-midnight windows", () => { const at = (h: number, m: number) => new Date(2026, 0, 1, h, m); const wrap = { enabled: true, start: "22:00", end: "08:00" }; diff --git a/packages/shell/src/main/dashboard/grid-placement.test.ts b/packages/shell/src/main/dashboard/grid-placement.test.ts index 3c69856f..bc4a6ed3 100644 --- a/packages/shell/src/main/dashboard/grid-placement.test.ts +++ b/packages/shell/src/main/dashboard/grid-placement.test.ts @@ -1,32 +1,87 @@ import { describe, expect, it } from "vitest"; -import { GRID_COLS, firstFreeCell, occupiedCells } from "./grid-placement"; +import { + ICON_FOOTPRINT_H, + ICON_FOOTPRINT_W, + footprintsOverlap, +} from "../../shared/dashboard-icon-grid"; +import { firstFreeCell, occupiedCells } from "./grid-placement"; describe("occupiedCells", () => { - it("collects only integer-cell records (legacy pixel records occupy nothing)", () => { - const taken = occupiedCells({ - a: { x: 0, y: 0 }, - b: { x: 3, y: 1 }, - pixel: { x: 12.5, y: 40.2 }, // legacy float — ignored - }); - expect([...taken].sort()).toEqual(["0:0", "3:1"]); + it("keeps every record — a dropped record is one the placer would land on", () => { + expect( + occupiedCells({ + a: { x: 0, y: 0 }, + b: { x: 11, y: 0 }, + fractional: { x: 12.5, y: 40.2 }, + }), + ).toEqual([ + { col: 0, row: 0 }, + { col: 11, row: 0 }, + { col: 12, row: 40 }, + ]); + }); + + it("clamps negatives to the origin", () => { + expect(occupiedCells({ a: { x: -4, y: -1 } })).toEqual([{ col: 0, row: 0 }]); + }); + + it("skips non-finite records", () => { + expect(occupiedCells({ a: { x: Number.NaN, y: 0 } })).toEqual([]); }); it("is empty for no icons", () => { - expect(occupiedCells({}).size).toBe(0); + expect(occupiedCells({}).length).toBe(0); }); }); describe("firstFreeCell", () => { it("returns 0:0 on an empty grid", () => { - expect(firstFreeCell(new Set())).toEqual({ col: 0, row: 0 }); + expect(firstFreeCell([])).toEqual({ col: 0, row: 0 }); + }); + + it("steps a whole icon footprint, not one grid unit (POLISH-LAY-6)", () => { + // The regression: with Notes at 0,0 the OLD 12-column 1×1-cell scan + // answered {col:1,row:0} — inside Notes' 11×14-unit box — so the newly + // installed app's tile and label printed on top of it. + const cell = firstFreeCell([{ col: 0, row: 0 }]); + expect(cell).toEqual({ col: ICON_FOOTPRINT_W, row: 0 }); + expect(footprintsOverlap(cell, { col: 0, row: 0 })).toBe(false); + }); + + it("never overlaps ANY icon of a populated seeded fleet", () => { + // The real seeded layout the dry-run captured: one icon every + // ICON_FOOTPRINT_W across, wrapping every ICON_FOOTPRINT_H down. + const fleet = [ + { col: 0, row: 0 }, + { col: ICON_FOOTPRINT_W, row: 0 }, + { col: 2 * ICON_FOOTPRINT_W, row: 0 }, + { col: 3 * ICON_FOOTPRINT_W, row: 0 }, + { col: 0, row: ICON_FOOTPRINT_H }, + ]; + const cell = firstFreeCell(fleet); + for (const icon of fleet) expect(footprintsOverlap(cell, icon)).toBe(false); + }); + + it("wraps to the next footprint row when the first row is full", () => { + const row0 = Array.from({ length: 6 }, (_, c) => ({ col: c * ICON_FOOTPRINT_W, row: 0 })); + // Six columns is not the whole (unbounded) row, so the placer keeps + // going across before it wraps. + expect(firstFreeCell(row0)).toEqual({ col: 6 * ICON_FOOTPRINT_W, row: 0 }); }); - it("scans row-major, skipping taken cells", () => { - expect(firstFreeCell(new Set(["0:0", "1:0"]))).toEqual({ col: 2, row: 0 }); + it("places clear of an icon the user dragged off the slot lattice", () => { + // Free drags snap to the 8px grid, not to install slots — a dragged + // icon at 3,3 still blocks the 0,0 slot it overlaps. + const dragged = { col: 3, row: 3 }; + const cell = firstFreeCell([dragged]); + expect(footprintsOverlap(cell, dragged)).toBe(false); }); - it("wraps to the next row when the first is full", () => { - const fullRow0 = new Set(Array.from({ length: GRID_COLS }, (_, c) => `${c}:0`)); - expect(firstFreeCell(fullRow0)).toEqual({ col: 0, row: 1 }); + it("fills a hole left by an uninstalled app before extending the grid", () => { + const fleet = [ + { col: 0, row: 0 }, + { col: 2 * ICON_FOOTPRINT_W, row: 0 }, + ]; + expect(firstFreeCell(fleet)).toEqual({ col: ICON_FOOTPRINT_W, row: 0 }); }); }); diff --git a/packages/shell/src/main/dashboard/grid-placement.ts b/packages/shell/src/main/dashboard/grid-placement.ts index b3160dec..51ec511f 100644 --- a/packages/shell/src/main/dashboard/grid-placement.ts +++ b/packages/shell/src/main/dashboard/grid-placement.ts @@ -1,33 +1,37 @@ /** * Dashboard grid placement — the single source of "where does a new - * pinned tile land". Extracted so the entity-pin path (dashboard-service) - * and the shell-surface-pin path (Bin overlay) compute the first free - * cell identically (DRY — previously inlined only in dashboard-service). + * pinned tile land" on the MAIN side. Every install path (marketplace, + * sideload, vault-source install, dev seeder) and every pin path + * (dashboard-service entity pin, Bin overlay) computes its cell here. * - * Pure: same icon map → same cell. `GRID_COLS` mirrors the renderer's - * fixed-width grid (`dashboard/grid.ts`). + * The geometry itself is NOT defined here: it lives in + * `shared/dashboard-icon-grid`, which the renderer's `dashboard/grid.ts` + * also builds on. This module is only the main-side adapter from a stored + * icon map to that placer. + * + * That indirection is the POLISH-LAY-6 fix. Main used to scan a fictional + * 12-column grid of 1×1 cells while the renderer stores 8px grid units and + * paints an 11×14-cell footprint per icon — so with Notes at `0,0` the cell + * `1,0` looked free and a freshly installed app's tile landed on top of it. + * + * Pure: same icon map → same cell. */ -export const GRID_COLS = 12; +import { + type IconGridCell, + firstFreeIconCell, + occupiedIconCells, +} from "../../shared/dashboard-icon-grid"; + +export type { IconGridCell }; -/** Integer-cell coordinates already occupied by an icon. Legacy pixel - * records (non-integer x/y, rare, renderer-migrated) occupy no cell — - * the renderer's collision layer handles any residual display overlap. */ -export function occupiedCells(icons: Record): Set { - const taken = new Set(); - for (const icon of Object.values(icons)) { - if (Number.isInteger(icon.x) && Number.isInteger(icon.y)) { - taken.add(`${icon.x}:${icon.y}`); - } - } - return taken; +/** Footprint origins already occupied by an icon, in 8px grid units. */ +export function occupiedCells(icons: Record): IconGridCell[] { + return occupiedIconCells(icons); } -/** First free `{col, row}` scanning row-major over the fixed-width grid. */ -export function firstFreeCell(taken: ReadonlySet): { col: number; row: number } { - for (let row = 0; ; row++) { - for (let col = 0; col < GRID_COLS; col++) { - if (!taken.has(`${col}:${row}`)) return { col, row }; - } - } +/** First install slot (row-major, footprint-stepped) whose icon box clears + * every existing icon's box. */ +export function firstFreeCell(occupied: readonly IconGridCell[]): IconGridCell { + return firstFreeIconCell(occupied); } diff --git a/packages/shell/src/main/dev/seed-demo-apps.ts b/packages/shell/src/main/dev/seed-demo-apps.ts index d169647a..55efbabb 100644 --- a/packages/shell/src/main/dev/seed-demo-apps.ts +++ b/packages/shell/src/main/dev/seed-demo-apps.ts @@ -35,14 +35,13 @@ import { FIRST_PARTY_APPS, type FirstPartyApp, firstPartyAppById } from "../apps import { DEV_INSTALL_PROVENANCE, type InstallProvenance } from "../apps/install-provenance"; import { AppInstaller } from "../apps/installer"; import type { DashboardStore } from "../dashboard/dashboard-store"; +import { firstFreeCell, occupiedCells } from "../dashboard/grid-placement"; import { pruneOrphanAppIcons } from "../dashboard/prune-orphan-app-icons"; import { getActiveShortcutRegistry } from "../shortcuts/active-registry"; import { AppsRepository } from "../storage/registry-repo/apps-repo"; import { getActiveVaultSession } from "../vault/session"; import { isBundleBuildStale } from "./bundle-staleness"; -const GRID_COLS = 12; - // Re-exported so existing import sites (`./seed-demo-apps`) keep working; // the canonical catalog now lives in `../apps/first-party`. export { FIRST_PARTY_APPS, type FirstPartyApp } from "../apps/first-party"; @@ -310,14 +309,15 @@ export async function reinstallFirstPartyApp( return outcome.ok ? { ok: true } : { ok: false, reason: outcome.reason }; } -/** Place a dashboard icon at the next free grid cell. Returns false when +/** Place a dashboard icon at the next free install slot. Returns false when * an icon already targets this app (no-op for re-seeds). * - * Wire format is cell-index `{x: col, y: row}` (small integers) — matches - * the renderer's grid in `packages/shell/src/renderer/dashboard/grid.ts`. - * Storing pixel coordinates here would put `col=0` (`x=16`) below the - * legacy-detection threshold and strand it as `col=16` on the rendered - * grid (clamped off-screen). */ + * Wire format is 8px grid units `{x: col, y: row}` — the same units the + * renderer paints with. The free slot comes from the SHARED placer + * (`shared/dashboard-icon-grid` via `dashboard/grid-placement`), which knows + * an icon's box spans `ICON_FOOTPRINT_W × ICON_FOOTPRINT_H` units: a + * local "next free 1×1 cell" scan dropped a new app straight on top of + * Notes (POLISH-LAY-6). */ export function placeDashboardIcon( dashboard: DashboardStore, appId: string, @@ -325,30 +325,18 @@ export function placeDashboardIcon( ): boolean { // The user removed this app's icon on purpose — don't resurrect it. if (dashboard.isAppIconDismissed(appId)) return false; - const existingIcons = Object.entries(dashboard.snapshot().icons); - if (existingIcons.some(([, icon]) => icon.target === appId)) return false; - - const occupied = new Set(existingIcons.map(([, icon]) => keyFor(icon.x, icon.y))); - let cursor = 0; - while (true) { - const col = cursor % GRID_COLS; - const row = Math.floor(cursor / GRID_COLS); - if (!occupied.has(keyFor(col, row))) { - dashboard.upsertIcon(`icon_${appId}_demo`, { - x: col, - y: row, - kind: "app", - target: appId, - label, - }); - return true; - } - cursor += 1; - } -} - -function keyFor(x: number, y: number): string { - return `${x}:${y}`; + const existingIcons = dashboard.snapshot().icons; + if (Object.values(existingIcons).some((icon) => icon.target === appId)) return false; + + const cell = firstFreeCell(occupiedCells(existingIcons)); + dashboard.upsertIcon(`icon_${appId}_demo`, { + x: cell.col, + y: cell.row, + kind: "app", + target: appId, + label, + }); + return true; } async function pathExists(path: string): Promise { diff --git a/packages/shell/src/main/index.ts b/packages/shell/src/main/index.ts index 5819706f..33b4cabe 100644 --- a/packages/shell/src/main/index.ts +++ b/packages/shell/src/main/index.ts @@ -55,6 +55,13 @@ import { createGeminiProvider } from "./ai/gemini-provider"; import { type OllamaHttp, createOllamaProvider } from "./ai/ollama-provider"; import { createOpenAiProvider } from "./ai/openai-provider"; import { ProviderRegistry } from "./ai/provider-registry"; +import { + AppIconMiss, + type AppIconResolution, + appIconMissStatus, + isAppIconHit, + resolveAppIconInBundle, +} from "./apps/app-icon-resolver"; import { maskAppWindowsForLock } from "./apps/app-window-lock"; import { setAppsChangedTarget } from "./apps/apps-changed"; import { wireExternalLinkRouting } from "./apps/external-link-routing"; @@ -556,8 +563,11 @@ function registerBrainstormProtocol(): void { } // brainstorm://app-icon/ — manifest-declared icon asset. // The handler reads the installed bundle's manifest, validates the - // icon path stays inside the bundle, and streams it. Apps without a - // declared icon get 404 — the renderer falls back to initials. + // icon path stays inside the bundle, and streams it. An app that + // declares NO icon answers 204 (no content, not an error) so the + // caller's initials fallback runs without a console 404 per surface + // per boot (POLISH-LAY-8); an id that resolves to no installed app + // still 404s, because that IS worth surfacing. if (url.host === "app-icon") { const session = getActiveVaultSession(); if (!session) return new Response(null, { status: 404 }); @@ -565,9 +575,11 @@ function registerBrainstormProtocol(): void { if (!appId || appId.includes("..") || appId.includes("/")) { return new Response(null, { status: 400 }); } - const target = await resolveAppIconPath(appId); - if (!target) return new Response(null, { status: 404 }); - const response = await net.fetch(pathToFileURL(target).toString()); + const resolved = await resolveAppIconPath(appId); + if (!isAppIconHit(resolved)) { + return new Response(null, { status: appIconMissStatus(resolved.miss) }); + } + const response = await net.fetch(pathToFileURL(resolved.path).toString()); const headers = new Headers(response.headers); // A `?v=` request is immutable: the renderer cache-busts on // app update, so the bytes for a given version never change. Caching @@ -1175,27 +1187,15 @@ async function drainAssetUploads(): Promise { } } -async function resolveAppIconPath(appId: string): Promise { +/** Locate the active bundle for an app id, then resolve its declared icon + * inside it. The resolution rules (and why a no-icon app is NOT a 404) live + * in `apps/app-icon-resolver`. */ +async function resolveAppIconPath(appId: string): Promise { const session = getActiveVaultSession(); - if (!session) return null; + if (!session) return { miss: AppIconMiss.Unresolved }; const registry = await session.dataStores.open("registry"); const repo = new AppsRepository(registry); - const record = repo.getActive(appId); - if (!record) return null; - let manifest: { icon?: unknown }; - try { - const raw = await readFile(join(record.bundleDir, "manifest.json"), "utf8"); - manifest = JSON.parse(raw) as { icon?: unknown }; - } catch { - return null; - } - if (typeof manifest.icon !== "string" || manifest.icon.length === 0) return null; - if (manifest.icon.includes("..")) return null; - const target = normalize(join(record.bundleDir, manifest.icon)); - if (!target.startsWith(record.bundleDir + sep) && target !== record.bundleDir) { - return null; - } - return target; + return resolveAppIconInBundle(repo.getActive(appId)?.bundleDir ?? null); } function resolveIconPath(): string { diff --git a/packages/shell/src/preload/index.ts b/packages/shell/src/preload/index.ts index 4c0021d2..bc3f313b 100644 --- a/packages/shell/src/preload/index.ts +++ b/packages/shell/src/preload/index.ts @@ -44,6 +44,7 @@ import { } from "@brainstorm-os/sdk-types"; import type { ThemeName } from "@brainstorm-os/tokens"; import { contextBridge, ipcRenderer } from "electron"; +import { appIconUrl } from "../shared/app-icon-url"; export type { AppearanceMode, AppearancePair, AppearanceSlot, AppearanceState }; export type { ReleaseInfo, UpdateCheckResult, UpdatePrefs }; @@ -1097,10 +1098,7 @@ const apps = { * app-derived caches (widget titles + entries, icon cache) on this edge * instead of racing the installer once at mount (F-380). */ onChanged: (listener: () => void): (() => void) => subscribe("apps:changed", listener), - iconUrl: (appId: string, version?: string): string => { - const base = `brainstorm://app-icon/${encodeURIComponent(appId)}`; - return version ? `${base}?v=${encodeURIComponent(version)}` : base; - }, + iconUrl: (appId: string, version?: string): string => appIconUrl(appId, version), launch: (appId: string): Promise => ipcRenderer.invoke("apps:launch", appId), uninstall: (appId: string): Promise => ipcRenderer.invoke("apps:uninstall", appId), diff --git a/packages/shell/src/renderer/bin/bin.tsx b/packages/shell/src/renderer/bin/bin.tsx index 1d0a59c9..e81f4e0c 100644 --- a/packages/shell/src/renderer/bin/bin.tsx +++ b/packages/shell/src/renderer/bin/bin.tsx @@ -14,6 +14,8 @@ import { Orientation, useCompositeKeyboard, useFocusTrap } from "@brainstorm-os/ import { useVirtualizer } from "@tanstack/react-virtual"; import { motion } from "framer-motion"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { occupiedIconCells } from "../../shared/dashboard-icon-grid"; +import { firstFreeCell } from "../dashboard/grid"; import { ShellSurfaceId, shellSurfacePinIconId } from "../dashboard/shell-surfaces"; import { t } from "../i18n/t"; import { Button, ButtonVariant } from "../ui/button"; @@ -86,12 +88,17 @@ export function Bin({ onClose }: BinProps) { setPinned(false); return; } - // {0,0} is a deterministic seed; the dashboard's collision layer - // repositions onto the first free cell on display (the same - // guarantee 7.13 entity pins rely on). + // Place on the first free install slot, exactly as the app-pin and + // install paths do. {0,0} used to be written as a "deterministic + // seed the collision layer repositions" — but icon layout is FREE + // placement with no collision resolution, so that seed dropped the + // Bin tile on top of whatever sits at the origin (POLISH-LAY-6's + // family). + const snap = await window.brainstorm.dashboard.snapshot(); + const cell = firstFreeCell(occupiedIconCells(snap?.icons ?? {})); await window.brainstorm.dashboard.upsertIcon(pinId, { - x: 0, - y: 0, + x: cell.col, + y: cell.row, kind: "shell-surface", target: ShellSurfaceId.Bin, label: t("shell.bin.title"), diff --git a/packages/shell/src/renderer/dashboard.tsx b/packages/shell/src/renderer/dashboard.tsx index 33f4d35c..f0152acf 100644 --- a/packages/shell/src/renderer/dashboard.tsx +++ b/packages/shell/src/renderer/dashboard.tsx @@ -26,6 +26,7 @@ import { import { AnimatePresence, motion } from "framer-motion"; import { Suspense, lazy, memo, useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { DashboardIcon, InstalledApp, VaultEntry, VaultSessionMeta } from "../preload"; +import { occupiedIconCells } from "../shared/dashboard-icon-grid"; import { onSystemPreferenceChange, systemPrefersDark } from "./dashboard/appearance-watcher"; import "./dashboard.css"; import { AppGrid } from "./dashboard/app-grid"; @@ -520,11 +521,7 @@ export function Dashboard() { const onPinApp = useCallback( (app: InstalledApp) => { const id = `icon_${app.id}_${Date.now()}`; - const occupied = Object.values(snapshot?.icons ?? {}).map(({ x, y }) => ({ - col: Math.max(0, Math.floor(x)), - row: Math.max(0, Math.floor(y)), - })); - const cell = firstFreeCell(occupied); + const cell = firstFreeCell(occupiedIconCells(snapshot?.icons ?? {})); void window.brainstorm.dashboard.upsertIcon(id, { x: cell.col, y: cell.row, diff --git a/packages/shell/src/renderer/dashboard/app-icon-cache.test.ts b/packages/shell/src/renderer/dashboard/app-icon-cache.test.ts index 7927ffc0..b78ff8b7 100644 --- a/packages/shell/src/renderer/dashboard/app-icon-cache.test.ts +++ b/packages/shell/src/renderer/dashboard/app-icon-cache.test.ts @@ -1,7 +1,7 @@ /** * @vitest-environment jsdom */ -import { beforeAll, describe, expect, it } from "vitest"; +import { describe, expect, it } from "vitest"; import { APP_ICON_CACHE_KEY, appHasIcon, @@ -11,19 +11,6 @@ import { setAppIcons, } from "./app-icon-cache"; -beforeAll(() => { - // The module reads `window.brainstorm.apps.iconUrl` when resolving a src. - Object.defineProperty(window, "brainstorm", { - configurable: true, - value: { - apps: { - iconUrl: (appId: string, version?: string): string => - version ? `brainstorm://app-icon/${appId}?v=${version}` : `brainstorm://app-icon/${appId}`, - }, - }, - }); -}); - describe("app-icon cache", () => { // Module state starts cold (nothing persisted at import) — this case can // only be observed before the first `setAppIcons`, so it runs first. @@ -62,6 +49,21 @@ describe("app-icon cache", () => { expect(changed).toBe(false); }); + // POLISH-LAY-8 — the widget header wants a per-install-epoch retry (F-380) + // AND the icon-less suppression. It used to hand-build its own URL to get + // the first, and so lost the second. + it("appends the caller's apps-changed epoch to a versioned URL", () => { + setAppIcons([{ id: "com.example.widget", hasIcon: true, version: "1.0.0" }]); + expect(resolveAppIconSrc("com.example.widget", 3)).toBe( + "brainstorm://app-icon/com.example.widget?v=1.0.0&e=3", + ); + }); + + it("suppresses an epoch request for a known icon-less app too", () => { + setAppIcons([{ id: "com.example.noart", hasIcon: false, version: "1.0.0" }]); + expect(resolveAppIconSrc("com.example.noart", 3)).toBeNull(); + }); + it("reports a change when a version bumps", () => { setAppIcons([{ id: "com.example.bump", hasIcon: true, version: "1.0.0" }]); const changed = setAppIcons([{ id: "com.example.bump", hasIcon: true, version: "2.0.0" }]); diff --git a/packages/shell/src/renderer/dashboard/app-icon-cache.ts b/packages/shell/src/renderer/dashboard/app-icon-cache.ts index 992063fd..4c5fa8c1 100644 --- a/packages/shell/src/renderer/dashboard/app-icon-cache.ts +++ b/packages/shell/src/renderer/dashboard/app-icon-cache.ts @@ -10,6 +10,8 @@ * (the protocol handler serves versioned requests `immutable`). */ +import { appIconUrl } from "../../shared/app-icon-url"; + export const APP_ICON_CACHE_KEY = "brainstorm.dashboard.app-icons.v1"; // `null` until the first authoritative `apps.listInstalled()` of a fresh @@ -71,14 +73,25 @@ function sameMap(a: Map, b: Map): boolean { } /** - * Resolve the `src` for an app squircle. Returns a versioned (cacheable) URL - * for a known-iconed app, `null` for a known-iconless one (so no wasted 404), + * Resolve the `src` for an app squircle — the ONE place a renderer surface + * turns an app id into an icon URL. Returns a versioned (cacheable) URL for a + * known-iconed app, `null` for a known-iconless one (so no wasted request), * and an optimistic unversioned URL while the cache is still cold — the * AppIcon's gradient+initials paint behind it, so an optimistic miss degrades * to the placeholder rather than a broken tile. + * + * Hand-built `brainstorm://app-icon/` URLs are what made the running + * strip, the window switcher and the widget headers each fire their own 404 + * for an app that ships no artwork (POLISH-LAY-8) — they all come through + * here now. + * + * `epoch` (optional) appends the caller's apps-changed counter so a load that + * raced an (re)install retries on the next edge instead of pinning the + * initials fallback for the session (F-380 / POLISH-LAY-3). */ -export function resolveAppIconSrc(appId: string): string | null { - if (appHasIcon(appId)) return window.brainstorm.apps.iconUrl(appId, appIconVersion(appId)); - if (appIconsKnown()) return null; - return window.brainstorm.apps.iconUrl(appId); +export function resolveAppIconSrc(appId: string, epoch?: number): string | null { + if (appIconsKnown() && !appHasIcon(appId)) return null; + const base = appIconUrl(appId, appIconVersion(appId)); + if (epoch === undefined) return base; + return `${base}${base.includes("?") ? "&" : "?"}e=${epoch}`; } diff --git a/packages/shell/src/renderer/dashboard/grid.ts b/packages/shell/src/renderer/dashboard/grid.ts index 528ae4ed..3f36cd47 100644 --- a/packages/shell/src/renderer/dashboard/grid.ts +++ b/packages/shell/src/renderer/dashboard/grid.ts @@ -17,7 +17,27 @@ * `layoutIcons`. */ -export const GRID_UNIT = 8; +import { + GRID_UNIT, + ICON_BUTTON_H, + ICON_BUTTON_W, + ICON_FOOTPRINT_H, + ICON_FOOTPRINT_W, + firstFreeIconCell, +} from "../../shared/dashboard-icon-grid"; + +/** The placement geometry lives in `shared/dashboard-icon-grid` so the main + * process places a newly installed tile with the SAME units and the SAME + * occupancy test the renderer paints with (POLISH-LAY-6). Re-exported here so + * every existing renderer call site keeps importing from `./grid`. */ +export { + GRID_UNIT, + ICON_BUTTON_W, + ICON_BUTTON_H, + ICON_FOOTPRINT_W, + ICON_FOOTPRINT_H, +} from "../../shared/dashboard-icon-grid"; + export const GRID_OUTER_MARGIN = 16; /** A layout whose every icon sits within this many cells of the origin is a @@ -41,16 +61,11 @@ export function getCellSize(_viewport?: GridPoint): GridSize { * `cell` argument is ignored, kept for call-site compatibility. */ export type IconSize = { w: number; h: number; tile: number }; -/** Fixed icon-button box (tile + two label lines) and inner tile. Keep in - * lockstep with `--grid-icon-w/-h` + the tile size in `icons-layer.css`. */ -export const ICON_BUTTON_W = 80; -export const ICON_BUTTON_H = 104; +/** Inner tile of the icon button. The button box itself (`ICON_BUTTON_W/H`) and + * its footprint come from the shared placement module re-exported above; keep + * all three in lockstep with `--grid-icon-w/-h` + the tile size in + * `icons-layer.css`. */ export const ICON_TILE = 56; -/** Footprint of one icon, in `GRID_UNIT` cells (button box + a 1-cell gutter) — - * the slot spacing the install placer (`firstFreeCell`) steps by, and the box it - * checks for overlap. Free drags ignore this; only new installs avoid piling. */ -export const ICON_FOOTPRINT_W = Math.ceil(ICON_BUTTON_W / GRID_UNIT) + 1; -export const ICON_FOOTPRINT_H = Math.ceil(ICON_BUTTON_H / GRID_UNIT) + 1; /** * Vertical slot reserved *below the tile, above the label* by every icon @@ -109,29 +124,10 @@ export function clampCell(cell: GridCell): GridCell { return { col: Math.max(0, Math.floor(cell.col)), row: Math.max(0, Math.floor(cell.row)) }; } -/** Do two icon footprints (top-left cells `a`, `b`, each `ICON_FOOTPRINT_W` × - * `ICON_FOOTPRINT_H`) overlap? Used only to place a NEW icon clear of existing - * ones — user drags are free and never overlap-checked. */ -function footprintsOverlap(a: GridCell, b: GridCell): boolean { - return ( - a.col < b.col + ICON_FOOTPRINT_W && - b.col < a.col + ICON_FOOTPRINT_W && - a.row < b.row + ICON_FOOTPRINT_H && - b.row < a.row + ICON_FOOTPRINT_H - ); -} - -/** First free install slot: scan footprint-stepped slots (row-major) for one - * whose footprint clears every occupied icon. New installs land in a tidy grid; - * the user can then freely drag them anywhere on the 8px grid. */ +/** First free install slot — the shared placer, under the name the renderer's + * existing call sites already use. */ export function firstFreeCell(occupied: readonly GridCell[]): GridCell { - for (let r = 0; r < 1024; r++) { - for (let c = 0; c < 1024; c++) { - const slot = { col: c * ICON_FOOTPRINT_W, row: r * ICON_FOOTPRINT_H }; - if (!occupied.some((o) => footprintsOverlap(slot, o))) return slot; - } - } - return { col: 0, row: 1024 * ICON_FOOTPRINT_H }; + return firstFreeIconCell(occupied); } // --- Widget grid (Stage 7.3 / 7.3b) --- diff --git a/packages/shell/src/renderer/dashboard/widgets-layer.tsx b/packages/shell/src/renderer/dashboard/widgets-layer.tsx index faa916ed..e47e7852 100644 --- a/packages/shell/src/renderer/dashboard/widgets-layer.tsx +++ b/packages/shell/src/renderer/dashboard/widgets-layer.tsx @@ -28,6 +28,7 @@ import { launchApp } from "../analytics/track-app-launch"; import { t } from "../i18n/t"; import { Icon, IconName } from "../ui/icon"; import { AppIcon } from "./app-icon"; +import { resolveAppIconSrc } from "./app-icon-cache"; import { GRID_OUTER_MARGIN, type GridCell, @@ -849,14 +850,11 @@ const WidgetCard = memo(function WidgetCardImpl(props: WidgetCardProps) { request racing the installer's uninstall→install window 404s, AppIcon falls back to initials, and a constant src pins that fallback for the whole session (the same F-380 edge the - iframe entry and titles already re-resolve for). */} - + iframe entry and titles already re-resolve for). The shared + resolver answers `null` for an app that ships no icon, so an + icon-less app's widget draws initials with no request at all + (POLISH-LAY-8). */} + {title}