From 0e650258dac52402d2b393d5062fb956ede8d5fe Mon Sep 17 00:00:00 2001 From: Thalida Noel Date: Mon, 15 Jun 2026 21:47:56 -0400 Subject: [PATCH 1/3] fix(camera): frame the city on initial load to match R (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first auto-frame on load differed from pressing R because it was captured from a transient early state, while R re-captures after everything settles. All framing inputs now come from layout data, and the camera re-snaps through the load's applies: 1. Tallest building height — getTallest() looped the building cell MESHES, which build asynchronously, so it was null when the camera framed and only valid by the time R ran. Replaced with a `cityState.tallestBuilding` computed derived from `layout.buildings` (which already includes the worker's media-silhouette sizing). It tracks `layout` (reassigned every apply), so the height is fresh on the skeleton→final reuse apply, where placeholder heights become real without bumping structureRevision. The rig reads it from cityState like its other framing inputs; the now-dead buildings.getTallest is removed. 2. The final manifest applies as a REUSE (same tree_signature → structureRevision and bbox frozen), so the old bbox-tracking reframe never re-fired and the camera stayed on the early framing (empty boot, then skeleton placeholder heights). The composer now follows the framing on every apply (cityRevision) until the user first takes control of the camera (OrbitControls 'start'), so it tracks the world settling but never yanks a view the user has set. reset() also recomputes the framing from current state instead of a cached pose, removing an effect-ordering hazard. 3. Repo-label width — the height-fit sampled the label's left/right corners, so the label's world width (from the text-texture aspect, which only settles once the Orbitron web font loads) pulled the camera back and shifted the frame after load. The label is centered on the gem and already sits inside the width-fit frame, so the fit now uses only the label's top-edge HEIGHT, not its width — removing the last rendered/font dependency from framing. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src/city/components/buildings/index.ts | 12 -- app/src/city/index.ts | 34 ++-- app/src/city/render/cameraRig.ts | 39 ++--- app/src/city/state/index.ts | 27 ++- app/tests/city/cityState.test.ts | 35 +++- .../city/components/buildings/index.test.ts | 5 +- app/tests/city/initialFraming.test.ts | 165 ++++++++++++++++++ app/tests/city/render/cameraRig.test.ts | 58 ++++-- 8 files changed, 299 insertions(+), 76 deletions(-) create mode 100644 app/tests/city/initialFraming.test.ts diff --git a/app/src/city/components/buildings/index.ts b/app/src/city/components/buildings/index.ts index 9fd4589a..8fe201fe 100644 --- a/app/src/city/components/buildings/index.ts +++ b/app/src/city/components/buildings/index.ts @@ -66,8 +66,6 @@ export interface Buildings extends SceneComponent { disposeAdPanels(): void; /** Building lookup by file path → { mesh, building, instanceId }. */ getBuildingByPath(p: string): { mesh: THREE.Mesh; building: Building; instanceId: number } | null; - /** Tallest building (layout pos + dims), or null when empty. */ - getTallest(): { x: number; y: number; w: number; d: number; h: number } | null; /** Cell map (consumed by picker / fader / outline / diff mirror). */ getCells(): Map; /** Building index, or null pre-rebuild. */ @@ -479,16 +477,6 @@ export function createBuildings(ctx: SceneContext): Buildings { onResize, dispose, getBuildingByPath: (p) => _buildingsByPath[p] || null, - getTallest: () => { - let tallest: Building | null = null; - for (const cell of _cells.values()) { - for (const b of cell.buildings) { - if (b && (!tallest || b.h > tallest.h)) tallest = b; - } - } - if (!tallest) return null; - return { x: tallest.x, y: tallest.y, w: tallest.w, d: tallest.d, h: tallest.h }; - }, getCells: () => _cells, getBuildingIndex: () => _buildingIndex, getMeshForBuilding, diff --git a/app/src/city/index.ts b/app/src/city/index.ts index 882ca96c..4c9f1ad6 100644 --- a/app/src/city/index.ts +++ b/app/src/city/index.ts @@ -108,33 +108,38 @@ export async function createCity(canvas: HTMLCanvasElement, manifest: Manifest): // BEFORE the rig (so bbox is set and the rig's first frame can frame the city). await applyManifest(manifest); - // cityState is threaded so the rig re-frames reactively when bbox changes - // and reads its world-framing inputs (bbox/gem/root street) directly. deps - // carries only the component-geometry accessors the rig can't reach via state. + // cityState is threaded so the rig re-frames reactively when bbox changes and + // reads its world-framing inputs (bbox/gem/root street/tallest building) + // directly. deps carries only the component-geometry accessors the rig can't + // reach via state. const rig = createCameraRig({ canvas, cityState, deps: { - getTallestBuilding: () => buildings.getTallest(), getRepoLabelBounds: () => repoLabel.getPanelBounds(), getTreeBoundsBySha: (sha) => trees.getRenderer()?.getTreeBoundsBySha(sha) ?? null, }, }); - // Reframe the camera on a SOURCE change (repo switch) — the one camera action - // that used to live in the view. bbox is reassigned on every non-reuse apply, - // so the rig's framing is freshly captured by its own effect just above by the - // time this fires; peeking CURRENT_SOURCE_KEY filters to repo switches only — - // a config rebuild / live-update / the skeleton apply (before the fetch layer - // commits the new key) all keep the same key → no snap. reset() is untracked - // so this effect depends only on bbox. + // Re-snap the camera on every apply (cityRevision), not just bbox changes: the + // final manifest is a REUSE apply (placeholder heights → real) that leaves bbox + // frozen, so a bbox-only reframe would miss it and strand the camera on the + // early framing (issue #62). Follow until the user first takes control + // (OrbitControls 'start') so a later apply never yanks their view. let lastReframedSourceKey: string | null = null; + let followFraming = false; + const onUserControl = () => { + followFraming = false; + }; + rig.controls.addEventListener('start', onUserControl); const stopReframe = effect(() => { - void cityState.bbox.value; + void cityState.cityRevision.value; const key = CURRENT_SOURCE_KEY.peek(); - if (key !== null && key !== lastReframedSourceKey) { - untracked(() => rig.reset()); + if (key === null) return; + if (key !== lastReframedSourceKey) { lastReframedSourceKey = key; + followFraming = true; } + if (followFraming) untracked(() => rig.reset()); }); const postFx = createPostFx(renderer, scene, rig.camera); @@ -228,6 +233,7 @@ export async function createCity(canvas: HTMLCanvasElement, manifest: Manifest): dispose(): void { stopFrameLoop(); stopReframe(); + rig.controls.removeEventListener('start', onUserControl); handlers.dispose(); picker.dispose(); rig.dispose(); diff --git a/app/src/city/render/cameraRig.ts b/app/src/city/render/cameraRig.ts index 045575a2..d241afec 100644 --- a/app/src/city/render/cameraRig.ts +++ b/app/src/city/render/cameraRig.ts @@ -46,11 +46,10 @@ import type { Building, PickTarget, Street } from '@/types'; import type { CityState } from '@/city/state'; /** Narrow accessor surface the rig needs from the composer for component - * geometry (buildings/repoLabel/trees). World framing inputs (bbox, gem, - * root street) come from cityState directly, not through here. createCity + * geometry (repoLabel/trees). World framing inputs (bbox, gem, root street, + * tallest building) come from cityState directly, not through here. createCity * (city/index.ts) passes a small deps literal that satisfies this. */ export interface CameraRigDeps { - getTallestBuilding(): { x: number; y: number; w: number; d: number; h: number } | null; getRepoLabelBounds(): { centerX: number; centerY: number; @@ -258,7 +257,7 @@ export function createCameraRig({ // Take the max across the 4 roof corners. HEADROOM scales D up for // breathing room above the roof (1.0 = spire flush against top edge). let heightDist = 0; - const tallest = gemPos && rootStreet ? deps.getTallestBuilding() : null; + const tallest = gemPos && rootStreet ? cityState.tallestBuilding.value : null; const labelBounds = gemPos && rootStreet ? deps.getRepoLabelBounds() : null; if (tallest || labelBounds) { const sinElev = dir.y; @@ -293,22 +292,13 @@ export function createCameraRig({ } } } - // Include the floating repo-label panel so empty worlds (no - // buildings) still frame to show the label, and crowded worlds - // never crop it off the top edge. The panel billboards to face - // the camera, so its horizontal extent could rotate either way — - // sample the top corners along BOTH world axes to bound it. + // Fit the label's top-edge HEIGHT only, not its width: the panel is + // centered on the gem and already inside the width-fit frame, and its + // width comes from the text-texture aspect, which only settles on web-font + // load — sampling it made the frame jump after load (issue #62). if (labelBounds) { const topY = labelBounds.centerY + labelBounds.halfHeight; - const r = labelBounds.halfWidth; - for (const [dx, dz] of [ - [-r, 0], - [r, 0], - [0, -r], - [0, r], - ]) { - _fitPoint(labelBounds.centerX + dx, topY, labelBounds.centerZ + dz); - } + _fitPoint(labelBounds.centerX, topY, labelBounds.centerZ); } heightDist *= TALLEST_BUILDING_HEADROOM_MULT; } @@ -397,15 +387,10 @@ export function createCameraRig({ } function reset() { - // If framing wasn't captured yet (or was cleared by a disposal cycle), - // try once more before giving up — silent no-op on R is worse than - // re-running the cheap framing computation. Returns false only when - // the city has no bbox at all (e.g., pre-manifest), in which case - // there's nothing to reset to. - if (!initialCamPos || !initialTarget) { - if (!_captureFraming()) return; - if (!initialCamPos || !initialTarget) return; - } + // Recapture from the CURRENT layout every call: the composer's follow-snap + // can fire before the rig's own bbox effect re-captures (preact gives no + // effect ordering), so a cached pose could be stale. False only pre-manifest. + if (!_captureFraming() || !initialCamPos || !initialTarget) return; // Cancel any in-flight focus/reset animation so it can't keep // walking the camera away from the snap target. camAnimToken++; diff --git a/app/src/city/state/index.ts b/app/src/city/state/index.ts index 4a463f85..52cff6fa 100644 --- a/app/src/city/state/index.ts +++ b/app/src/city/state/index.ts @@ -7,16 +7,16 @@ // trees) rebuild off it. treePlacements is a source signal trees writes. // structureRevision / cityRevision / decorationRevision — change-notification // counters; consumers track them and peek the data. -// bbox / sceneBbox / cityHeight / latestWorldBounds / rootStreet / gemWorldPos — -// COMPUTED, never written. bbox = union of street rects + building footprints -// + footprint halo. +// bbox / sceneBbox / cityHeight / latestWorldBounds / rootStreet / gemWorldPos / +// tallestBuilding — COMPUTED, never written. bbox = union of street rects + +// building footprints + footprint halo. // // applyManifest + invalidateLayoutCache are defined here (the store owns its // transition); every scene component rebuilds reactively off the signals above. import { signal, computed, batch, type Signal, type ReadonlySignal } from '@preact/signals'; import * as THREE from 'three'; import { FOOTPRINT } from '@/state/stores/settings/footprint'; -import type { CityBbox, CityLayout, Manifest, Street } from '@/types'; +import type { Building, CityBbox, CityLayout, Manifest, Street } from '@/types'; import { getWorldBounds, type WorldBounds } from '../utils/floorBounds'; import { rectOfStreet } from '@/city/layout/rect'; import type { TreePlacement } from '../components/trees/treePlacement'; @@ -45,6 +45,9 @@ export interface CityState { treePlacements: Signal; readonly rootStreet: ReadonlySignal; readonly gemWorldPos: ReadonlySignal; + // Tallest building (by height) for the camera start-framing height-fit. From + // layout data, not the async building meshes (see the computed). + readonly tallestBuilding: ReadonlySignal; // { street dir.path → Street } from the layout. The buildings fader, pathLine, // picker, and the CityWorld debug API resolve a street by directory off this // instead of reaching into the streets component. @@ -176,6 +179,21 @@ export function createCityState(layoutClient: ReturnType(() => { + const l = layout.value; + if (!l) return null; + let tallest: Building | null = null; + for (const b of l.buildings) { + if (!tallest || b.h > tallest.h) tallest = b; + } + return tallest; + }); + // Street lookup by directory path. Tracks structureRevision + peeks layout // (ref-stable on a reuse apply, recomputes on a structure change) — a // { dir.path → Street } map derived straight from layout.streets. @@ -274,6 +292,7 @@ export function createCityState(layoutClient: ReturnType { expect(cs.rootStreet.value).toBe(r2); expect(cs.gemWorldPos.value!.x).not.toBe(z1); }); + + describe('tallestBuilding', () => { + const bld = (over: Partial): Building => + ({ x: 0, y: 0, w: 10, d: 10, h: 10, ...over }) as unknown as Building; + const layoutOf = (buildings: Building[]): CityLayout => + ({ buildings, streets: [] }) as unknown as CityLayout; + + it('is null with no layout and with no buildings', () => { + const cs = makeCityState(); + expect(cs.tallestBuilding.value).toBeNull(); + cs.layout.value = layoutOf([]); + expect(cs.tallestBuilding.value).toBeNull(); + }); + + it('returns the max-height building', () => { + const cs = makeCityState(); + const tall = bld({ x: 5, y: 6, w: 3, d: 4, h: 90 }); + cs.layout.value = layoutOf([bld({ h: 10 }), tall, bld({ h: 50 })]); + expect(cs.tallestBuilding.value).toBe(tall); + }); + + // Crux of #62: a reuse apply (skeleton placeholder → real heights) reassigns + // `layout` without bumping structureRevision, so tallestBuilding must track + // `layout` or it'd frame to the stale placeholder height. + it('updates when layout is reassigned without a structureRevision bump', () => { + const cs = makeCityState(); + cs.layout.value = layoutOf([bld({ h: 20 })]); // skeleton placeholder height + expect(cs.tallestBuilding.value!.h).toBe(20); + + cs.layout.value = layoutOf([bld({ h: 200 })]); // final real height — no structureRevision++ + expect(cs.tallestBuilding.value!.h).toBe(200); + }); + }); }); diff --git a/app/tests/city/components/buildings/index.test.ts b/app/tests/city/components/buildings/index.test.ts index 1906e956..75496eb6 100644 --- a/app/tests/city/components/buildings/index.test.ts +++ b/app/tests/city/components/buildings/index.test.ts @@ -3,7 +3,7 @@ // Tests for the persistent createBuildings(ctx) component. // API: createBuildings(ctx) → { group, rebuild(layout, dateRanges), // disposeAdPanels, tick(dt, frame), onResize(), dispose(), -// getBuildingByPath, getTallest, getCells, getBuildingIndex, +// getBuildingByPath, getCells, getBuildingIndex, // getMeshForBuilding }. // // The shared-material theme effect reacts to BUILDINGS/SCENE/BLOOM at @@ -166,9 +166,6 @@ describe('createBuildings()', () => { expect(buildings.getBuildingByPath('src/b.ts')!.building).toBe(b1); expect(buildings.getBuildingByPath('nope')).toBeNull(); - // getTallest reflects b1 (h=9). - expect(buildings.getTallest()).toEqual({ x: -20, y: -20, w: b1.w, d: b1.d, h: 9 }); - // getMeshForBuilding resolves through the live cell (cellId/slotId were // assigned during rebuild). const resolved = buildings.getMeshForBuilding(b0); diff --git a/app/tests/city/initialFraming.test.ts b/app/tests/city/initialFraming.test.ts new file mode 100644 index 00000000..d5d218db --- /dev/null +++ b/app/tests/city/initialFraming.test.ts @@ -0,0 +1,165 @@ +// Regression for issue #62: a post-load layout rebuild (per-source settings +// hydrating on a same-source apply) changed the framing, but the source-change +// snap didn't re-fire, so the initial frame differed from R. The camera should +// follow the framing until the user first takes control. Reproduced here via a +// STREET_TIERS change + rebuild (the same path) and a synthesized 'start' event. +// +// jsdom has no WebGL — mock the renderer + post pipeline like city/index.test.ts. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as THREE from 'three'; +import { EMPTY_MANIFEST } from '@/constants/manifest'; +import { CURRENT_SOURCE } from '@/state/stores/source'; +import { STREET_TIERS } from '@/state/stores/settings/streets'; +import type { Manifest } from '@/types'; +import { mkDir } from '../_helpers/cityFixtures'; + +vi.mock('three', async () => { + const actual = await vi.importActual('three'); + class FakeWebGLRenderer { + domElement: HTMLCanvasElement; + constructor(opts: { canvas: HTMLCanvasElement }) { + this.domElement = opts.canvas; + } + setPixelRatio() {} + setSize() {} + getSize(v: THREE.Vector2) { + return v; + } + render() {} + dispose() {} + copyTextureToTexture() {} + setRenderTarget() {} + getContext() { + return {}; + } + } + return { ...actual, WebGLRenderer: FakeWebGLRenderer }; +}); + +vi.mock('@/city/render/postFx', () => ({ + createPostFx: () => ({ + render: () => {}, + setSize: () => {}, + refresh: () => {}, + dispose: () => {}, + }), +})); + +// buildIconAtlas loads icon images, which never fire onload in jsdom and hang +// applyManifest. Icons are irrelevant to camera framing — stub it out. +vi.mock('@/city/components/buildings/atlas', async () => { + const actual = await vi.importActual( + '@/city/components/buildings/atlas' + ); + return { ...actual, buildIconAtlas: async () => null }; +}); + +import { createCity } from '@/city/index'; + +describe('initial-load framing (issue #62)', () => { + let rafSpy: ReturnType; + const DEFAULT_TIERS = STREET_TIERS.peek(); + + beforeEach(() => { + CURRENT_SOURCE.value = null; + rafSpy = vi + .spyOn(globalThis, 'requestAnimationFrame') + .mockImplementation((cb: FrameRequestCallback) => { + setTimeout(() => cb(performance.now()), 0); + return 0 as unknown as number; + }); + }); + + afterEach(() => { + rafSpy.mockRestore(); + CURRENT_SOURCE.value = null; + STREET_TIERS.value = DEFAULT_TIERS; + vi.clearAllMocks(); + }); + + function makeCanvas(): HTMLCanvasElement { + const canvas = document.createElement('canvas'); + Object.defineProperty(canvas, 'clientWidth', { value: 1280, configurable: true }); + Object.defineProperty(canvas, 'clientHeight', { value: 720, configurable: true }); + return canvas; + } + + // Directory-only tree (no files → no buildings), so the framing is driven + // purely by the root street width — which we control via STREET_TIERS below. + function makeManifest(): Manifest { + const tree = mkDir('repo', [mkDir('a', [mkDir('b', [])]), mkDir('c', []), mkDir('d', [])]); + return { + ...EMPTY_MANIFEST, + tree, + tree_signature: 'sig-repo', + signature: 'full-sig', + } as unknown as Manifest; + } + + // Force every street (incl. the root) to a single width, so a rebuild after + // changing it deterministically moves the framing. + function setRootWidth(width: number): void { + STREET_TIERS.value = { TIERS: [{ min_descendants: 0, width }] }; + } + + it('re-snaps when a post-load rebuild changes the framing, before the user interacts', async () => { + setRootWidth(100); + const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); + try { + // Source commits, then the first real manifest applies → initial frame. + CURRENT_SOURCE.value = { src: 'test://repo' }; + const m = makeManifest(); + await handle.applyManifest(m); + const posInitial = handle.rig.camera.position.clone(); + + // Per-source settings hydrate: a Rebuild-route setting flips and the + // layout is rebuilt for the SAME source. The framing changes. + setRootWidth(400); + handle.invalidateLayoutCache(); + await handle.applyManifest(m); + const posAfterRebuild = handle.rig.camera.position.clone(); + + // The rebuild's framing = what R would produce now. + handle.rig.reset(); + const posReset = handle.rig.camera.position.clone(); + + // Sanity: the rebuild actually changed the framing. + expect(posReset.distanceTo(posInitial)).toBeGreaterThan(1); + // Fix: the camera followed the rebuild to the settled framing — no manual R + // needed. Before the fix it stayed at posInitial (the pre-hydration frame). + expect(posAfterRebuild.distanceTo(posReset)).toBeLessThan(0.5); + } finally { + handle.dispose(); + } + }); + + it('stops following once the user takes control of the camera', async () => { + setRootWidth(100); + const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); + try { + CURRENT_SOURCE.value = { src: 'test://repo' }; + const m = makeManifest(); + await handle.applyManifest(m); + + // User grabs the camera (orbit/pan/zoom all emit OrbitControls 'start'). + handle.rig.controls.dispatchEvent({ type: 'start' } as unknown as never); + const posBeforeRebuild = handle.rig.camera.position.clone(); + + // A later settings rebuild must NOT yank the user's view. + setRootWidth(600); + handle.invalidateLayoutCache(); + await handle.applyManifest(m); + const posAfterRebuild = handle.rig.camera.position.clone(); + + expect(posAfterRebuild.distanceTo(posBeforeRebuild)).toBeLessThan(0.5); + + // Confirm the framing genuinely would have changed (R still re-frames). + handle.rig.reset(); + const posReset = handle.rig.camera.position.clone(); + expect(posReset.distanceTo(posBeforeRebuild)).toBeGreaterThan(1); + } finally { + handle.dispose(); + } + }); +}); diff --git a/app/tests/city/render/cameraRig.test.ts b/app/tests/city/render/cameraRig.test.ts index 18c0f2b2..49adf286 100644 --- a/app/tests/city/render/cameraRig.test.ts +++ b/app/tests/city/render/cameraRig.test.ts @@ -13,12 +13,13 @@ function makeStubWorld(overrides: Partial> = {}) { return { ..._baseWorld(), ...overrides }; } -// The rig reads its world-framing inputs (bbox / gemWorldPos / rootStreet) -// from cityState computeds, not from deps — so seed a layout that produces a -// non-empty bbox + a root street. Two crossing 1000-long streets span XZ to -// ±500 and a 200-tall building gives the Y extent: bbox = (-500,0,-500)..(500, -// 200,500). Exact magnitudes don't drive the focus assertions (they key off -// the focused node + a sub-distance clamp), only that framing captures at all. +// The rig reads its world-framing inputs (bbox / gemWorldPos / rootStreet / +// tallestBuilding) from cityState computeds, not from deps — so seed a layout +// that produces a non-empty bbox + a root street. Two crossing 1000-long streets +// span XZ to ±500 and a 200-tall building gives the Y extent: bbox = +// (-500,0,-500)..(500,200,500). Exact magnitudes don't drive the focus +// assertions (they key off the focused node + a sub-distance clamp), only that +// framing captures at all. function seedFramedCity(): CityState { const cs = makeCityState(); cs.layout.value = { @@ -34,14 +35,6 @@ function seedFramedCity(): CityState { function _baseWorld() { return { - getTallestBuilding: () => - ({ x: 100, y: 0, w: 30, d: 30, h: 200 }) as { - x: number; - y: number; - w: number; - d: number; - h: number; - } | null, getRepoLabelBounds: () => null as { centerX: number; @@ -197,3 +190,40 @@ describe('cameraRig top-down focus', () => { }); }); }); + +describe('cameraRig start framing', () => { + // Issue #62: the label's width (from the text aspect, which settles on web-font + // load) must not affect framing — only its top-edge height. A tall label drives + // heightDist; widening it 100× must not move the camera. + function labelDeps(halfWidth: number) { + return makeStubWorld({ + getRepoLabelBounds: () => ({ + centerX: 0, + centerY: 400, + centerZ: 0, + halfWidth, + halfHeight: 50, + }), + }); + } + + it('ignores repo-label width (only its top-edge height matters)', () => { + const narrow = createCameraRig({ + canvas: makeCanvas(), + deps: labelDeps(40), + cityState: seedFramedCity(), + }); + narrow.update(16); + const narrowPos = narrow.camera.position.clone(); + + const wide = createCameraRig({ + canvas: makeCanvas(), + deps: labelDeps(4000), + cityState: seedFramedCity(), + }); + wide.update(16); + const widePos = wide.camera.position.clone(); + + expect(widePos.distanceTo(narrowPos)).toBeLessThan(0.001); + }); +}); From 782285a86c726a3b1ee0a3796f89edb5d956ac2a Mon Sep 17 00:00:00 2001 From: Thalida Noel Date: Mon, 15 Jun 2026 22:29:32 -0400 Subject: [PATCH 2/3] refactor(camera): snap on source change only; drop follow-until-interaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit reframed on every apply until the user's first interaction ("follow"). That silently reframed the camera on config-saves and live-updates when the user hadn't yet touched it — an unintended change to interaction behavior — and it's unnecessary: the empty boot has no source key (already skipped), and the final manifest is the apply where the source key first commits, so a plain snap-on-source-key-change lands on the settled city. Same-source re-applies (live-updates, config saves) no longer reframe, matching the prior semantics. reset() still recomputes (the final reuse apply leaves bbox frozen, so a cached pose would be stale). Tests updated: initial load frames the city (not the empty boot); a same-source re-apply does not reframe. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src/city/index.ts | 22 +++----- app/src/city/render/cameraRig.ts | 6 +-- app/tests/city/initialFraming.test.ts | 77 +++++++++++++-------------- 3 files changed, 46 insertions(+), 59 deletions(-) diff --git a/app/src/city/index.ts b/app/src/city/index.ts index 4c9f1ad6..b5eb0ac3 100644 --- a/app/src/city/index.ts +++ b/app/src/city/index.ts @@ -120,26 +120,19 @@ export async function createCity(canvas: HTMLCanvasElement, manifest: Manifest): getTreeBoundsBySha: (sha) => trees.getRenderer()?.getTreeBoundsBySha(sha) ?? null, }, }); - // Re-snap the camera on every apply (cityRevision), not just bbox changes: the - // final manifest is a REUSE apply (placeholder heights → real) that leaves bbox - // frozen, so a bbox-only reframe would miss it and strand the camera on the - // early framing (issue #62). Follow until the user first takes control - // (OrbitControls 'start') so a later apply never yanks their view. + // Snap the camera when a NEW source's city has applied (initial load or repo + // switch). Track cityRevision (every apply), not bbox: the final manifest is a + // reuse apply that leaves bbox frozen, so a bbox-only effect would miss it. The + // key guard skips the empty boot (key null — no source yet) and same-source + // re-applies (live-updates, config saves) — only a real source change reframes. let lastReframedSourceKey: string | null = null; - let followFraming = false; - const onUserControl = () => { - followFraming = false; - }; - rig.controls.addEventListener('start', onUserControl); const stopReframe = effect(() => { void cityState.cityRevision.value; const key = CURRENT_SOURCE_KEY.peek(); - if (key === null) return; - if (key !== lastReframedSourceKey) { + if (key !== null && key !== lastReframedSourceKey) { lastReframedSourceKey = key; - followFraming = true; + untracked(() => rig.reset()); } - if (followFraming) untracked(() => rig.reset()); }); const postFx = createPostFx(renderer, scene, rig.camera); @@ -233,7 +226,6 @@ export async function createCity(canvas: HTMLCanvasElement, manifest: Manifest): dispose(): void { stopFrameLoop(); stopReframe(); - rig.controls.removeEventListener('start', onUserControl); handlers.dispose(); picker.dispose(); rig.dispose(); diff --git a/app/src/city/render/cameraRig.ts b/app/src/city/render/cameraRig.ts index d241afec..9a1c9c81 100644 --- a/app/src/city/render/cameraRig.ts +++ b/app/src/city/render/cameraRig.ts @@ -387,9 +387,9 @@ export function createCameraRig({ } function reset() { - // Recapture from the CURRENT layout every call: the composer's follow-snap - // can fire before the rig's own bbox effect re-captures (preact gives no - // effect ordering), so a cached pose could be stale. False only pre-manifest. + // Recapture from the CURRENT layout every call: the final manifest is a reuse + // apply (bbox frozen), so the bbox effect that normally caches the pose + // doesn't re-fire there — a cached pose could be stale. False only pre-manifest. if (!_captureFraming() || !initialCamPos || !initialTarget) return; // Cancel any in-flight focus/reset animation so it can't keep // walking the camera away from the snap target. diff --git a/app/tests/city/initialFraming.test.ts b/app/tests/city/initialFraming.test.ts index d5d218db..57d7446f 100644 --- a/app/tests/city/initialFraming.test.ts +++ b/app/tests/city/initialFraming.test.ts @@ -1,8 +1,8 @@ -// Regression for issue #62: a post-load layout rebuild (per-source settings -// hydrating on a same-source apply) changed the framing, but the source-change -// snap didn't re-fire, so the initial frame differed from R. The camera should -// follow the framing until the user first takes control. Reproduced here via a -// STREET_TIERS change + rebuild (the same path) and a synthesized 'start' event. +// Regression for issue #62: the camera must snap to a NEW source's city once it +// applies (not stay on the empty boot), and must NOT reframe on a same-source +// re-apply (live-update / config save). The snap rides cityRevision (every +// apply) so it catches the final reuse apply, gated on a CURRENT_SOURCE_KEY +// change. // // jsdom has no WebGL — mock the renderer + post pipeline like city/index.test.ts. @@ -85,10 +85,16 @@ describe('initial-load framing (issue #62)', () => { return canvas; } - // Directory-only tree (no files → no buildings), so the framing is driven - // purely by the root street width — which we control via STREET_TIERS below. + // Directory-only tree (no files → no buildings), so the framing is driven by + // the root street width. 10 child dirs → root descendants_count 20 → a high + // street tier, so the real city frames clearly differently from the empty boot + // (root descendants 0 → the narrowest tier). STREET_TIERS can also be forced + // to a single width below. function makeManifest(): Manifest { - const tree = mkDir('repo', [mkDir('a', [mkDir('b', [])]), mkDir('c', []), mkDir('d', [])]); + const tree = mkDir( + 'repo', + Array.from({ length: 10 }, (_, i) => mkDir(`d${i}`, [])) + ); return { ...EMPTY_MANIFEST, tree, @@ -103,61 +109,50 @@ describe('initial-load framing (issue #62)', () => { STREET_TIERS.value = { TIERS: [{ min_descendants: 0, width }] }; } - it('re-snaps when a post-load rebuild changes the framing, before the user interacts', async () => { - setRootWidth(100); + it('frames the city on initial load, not the empty boot', async () => { const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); try { - // Source commits, then the first real manifest applies → initial frame. - CURRENT_SOURCE.value = { src: 'test://repo' }; - const m = makeManifest(); - await handle.applyManifest(m); - const posInitial = handle.rig.camera.position.clone(); + // firstFrame framed the empty boot (no source committed yet → no snap). + const bootPos = handle.rig.camera.position.clone(); - // Per-source settings hydrate: a Rebuild-route setting flips and the - // layout is rebuilt for the SAME source. The framing changes. - setRootWidth(400); - handle.invalidateLayoutCache(); - await handle.applyManifest(m); - const posAfterRebuild = handle.rig.camera.position.clone(); + // The fetch layer commits the source, then the real manifest applies. + CURRENT_SOURCE.value = { src: 'test://repo' }; + await handle.applyManifest(makeManifest()); + const loadPos = handle.rig.camera.position.clone(); - // The rebuild's framing = what R would produce now. + // What R produces now = the real city's framing. handle.rig.reset(); - const posReset = handle.rig.camera.position.clone(); + const resetPos = handle.rig.camera.position.clone(); - // Sanity: the rebuild actually changed the framing. - expect(posReset.distanceTo(posInitial)).toBeGreaterThan(1); - // Fix: the camera followed the rebuild to the settled framing — no manual R - // needed. Before the fix it stayed at posInitial (the pre-hydration frame). - expect(posAfterRebuild.distanceTo(posReset)).toBeLessThan(0.5); + // Sanity: the city frames differently from the empty boot. + expect(resetPos.distanceTo(bootPos)).toBeGreaterThan(1); + // The camera snapped to the city on load — no manual R needed. + expect(loadPos.distanceTo(resetPos)).toBeLessThan(0.5); } finally { handle.dispose(); } }); - it('stops following once the user takes control of the camera', async () => { + it('does not reframe on a same-source re-apply (live-update / config save)', async () => { setRootWidth(100); const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); try { CURRENT_SOURCE.value = { src: 'test://repo' }; const m = makeManifest(); await handle.applyManifest(m); + const posLoaded = handle.rig.camera.position.clone(); - // User grabs the camera (orbit/pan/zoom all emit OrbitControls 'start'). - handle.rig.controls.dispatchEvent({ type: 'start' } as unknown as never); - const posBeforeRebuild = handle.rig.camera.position.clone(); - - // A later settings rebuild must NOT yank the user's view. - setRootWidth(600); + // A same-source rebuild that moves the framing (here a Rebuild-route + // setting + invalidate; a live-update is the same shape) must leave the + // camera where it is — the source key didn't change. + setRootWidth(500); handle.invalidateLayoutCache(); await handle.applyManifest(m); - const posAfterRebuild = handle.rig.camera.position.clone(); - - expect(posAfterRebuild.distanceTo(posBeforeRebuild)).toBeLessThan(0.5); + expect(handle.rig.camera.position.distanceTo(posLoaded)).toBeLessThan(0.5); - // Confirm the framing genuinely would have changed (R still re-frames). + // Confirm the framing genuinely changed (R re-frames to the new width). handle.rig.reset(); - const posReset = handle.rig.camera.position.clone(); - expect(posReset.distanceTo(posBeforeRebuild)).toBeGreaterThan(1); + expect(handle.rig.camera.position.distanceTo(posLoaded)).toBeGreaterThan(1); } finally { handle.dispose(); } From 9f4f44e130405992780ad361a22192bfdd25dc8f Mon Sep 17 00:00:00 2001 From: Thalida Noel Date: Mon, 15 Jun 2026 22:33:51 -0400 Subject: [PATCH 3/3] =?UTF-8?q?chore(deps):=20bump=20starlette=201.2.1=20?= =?UTF-8?q?=E2=86=92=201.3.1=20(CVE-2026-54283)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's Trivy scan fails on starlette 1.2.1 for CVE-2026-54283 (HIGH, fixed in 1.3.1): request.form() size limits silently ignored for application/x-www-form-urlencoded, enabling DoS. Transitive via fastapi / sse-starlette; bumped via `uv lock --upgrade-package starlette` (lock-only, no pyproject change needed). Backend tests pass (242). Co-Authored-By: Claude Opus 4.8 (1M context) --- uv.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/uv.lock b/uv.lock index c59cbd8a..50704fb6 100644 --- a/uv.lock +++ b/uv.lock @@ -730,15 +730,15 @@ wheels = [ [[package]] name = "starlette" -version = "1.2.1" +version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "anyio" }, { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/25/44/ec35f1b6e83094b997da438a02c8c9b0ade2b1e84cfc48bd4656780760a6/starlette-1.2.1.tar.gz", hash = "sha256:9b9b5ebb992e67d6093741e63c2f59e4f6fff986f81163c087867bd7b924b3f6", size = 2701854, upload-time = "2026-05-31T01:07:51.847Z" } +sdist = { url = "https://files.pythonhosted.org/packages/eb/e3/7c1dc7381d9f8ab7d854328ebfa884e62cb3f3d8549ddfd37c7814f42afa/starlette-1.3.1.tar.gz", hash = "sha256:05d0213193f2fbaae60e2ecb593b4add4262ad4e46536b54abe36f11a71724e0", size = 2703240, upload-time = "2026-06-12T09:23:11.602Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/1c/54/196d0c1db10af76baa4f64894448505d60d3cdf70ef92cbb35f46a4e4c71/starlette-1.2.1-py3-none-any.whl", hash = "sha256:4de0082d08c8f6764a85a54cf1120d6939507a19905c7768acad2a9f875d2b89", size = 73350, upload-time = "2026-05-31T01:07:50.09Z" }, + { url = "https://files.pythonhosted.org/packages/ec/bb/2799cc2ede3ed41131f8975621e7213dfc7ef4acbbaadfa440f32500c370/starlette-1.3.1-py3-none-any.whl", hash = "sha256:c7372aae11c3c3f26a42df7bd626cec2f47d03483d261d369516a615a53714c6", size = 73632, upload-time = "2026-06-12T09:23:10.017Z" }, ] [[package]]