Repo audit: dead code, test guards, and the two ways data left the machine - #146
Open
thalida wants to merge 37 commits into
Open
Repo audit: dead code, test guards, and the two ways data left the machine#146thalida wants to merge 37 commits into
thalida wants to merge 37 commits into
Conversation
A timed-out axe.run never settles, so axe's `_running` latch stays set and every later surface throws "Axe is already running" — one slow surface bounced the whole push. axe.teardown() alone does not fix this: it clears the caches, element tree and selector data, but leaves `_running` untouched. Releasing that latch is what actually breaks the cascade, so afterEach does both. `_running` is absent from axe's public types, hence the cast. Adds a regression test that wedges the latch and asserts the next scan still runs; it fails with teardown() alone. Closes #108
The syntax-highlighting stylesheet was fetched from cdn.jsdelivr.net on every theme change, which told a third party the viewer's IP, user agent and chosen theme, broke the app offline, and trusted un-pinned remote CSS (attribute selectors with url() can exfiltrate, so this was not purely cosmetic). highlight.js is already a direct dependency and ships these stylesheets, so they now come from the bundle same-origin — matching how constants/fileIcons.ts already sources its Material icons. `?url` keeps each theme a separate asset rather than inlining all 21 into the JS bundle, so the browser still fetches only the selected one. SYNTAX_THEME_OPTIONS becomes `as const` so its values form a literal union; THEME_HREF is keyed on that union, making a theme without a stylesheet a typecheck failure instead of a silently unstyled pane.
`--host` defaulted to 0.0.0.0, so a pip-installed `codecity` served every interface. The API has no authentication, and once a manifest scan registers a root, /api/file will serve anything beneath it — on shared wifi that hands the scanned tree to whoever asks. Local scanning is gated behind CODECITY_ALLOW_LOCAL_REPOS, but anyone pointing codecity at their own repos has already turned that on. Default is now 127.0.0.1, with --host 0.0.0.0 available to opt in. Containers need the wide bind and now ask for it explicitly: in a container 0.0.0.0 is the container's own namespace, and only published ports are reachable. docker-compose.dev.yml repeats the flag because its `command:` replaces the Dockerfile CMD outright rather than appending to it.
`DELETE /api/manifest/cache` had no caller. The frontend's clearManifestCache() was never invoked either — its only reference was a RecentsList test asserting it is never called, which is what surfaced it. Removing the endpoint made its whole backing chain dead: cache_clear_all (only the endpoint called it), cache_clear_manifests (only cache_clear_all), remove_clone (only the endpoint), and CacheClearResponse. cache_clear_timeline stays — a no_cache scan still calls it. Also drops api/tests/test_server_cache.py and the five cache_clear_manifests cases, and rewrites the RecentsList test around what it actually verifies (the confirm step), since the cache assertion no longer has a subject. Consequence worth knowing: nothing clears a per-root cache now. A corrupt manifest cache or clone is recovered by deleting ~/.cache/codecity, not through the API. Nothing exercised that path, so this removes an unused recovery route rather than a used one. Types regenerated via `just gen-types`.
#124) Removed rules nothing applies: .file-path-ellipsis / .file-path-segment / .file-path-sep (residue from stripping PathBreadcrumbs), .street-label (street labels render on canvas, and the overlay fallback is gone), and the .timeline-notice arm of the cc-showcase hide list. Kept the .text-*, .dot--* and .row--* families: those are documented vocabulary in shared styles/ partials, not leftovers from a removed feature. The comments were the worse half. Each described a relationship that no longer holds, which is actively misleading when tracing styling: - TreePane.css claimed selection lives in .row--selected; the tree keys off .tree-item.tree-selected. - InfoPane.css claimed .info-markdown code takes its shell from .code-inline; it is a bare element selector that redeclares everything, because marked emits <code> with no class. - overlaps.ts cited tests/city/layout.test.ts, which does not exist. - manifest_types.py said to hand-sync with app/types/manifest.ts — wrong path, and the TS side is generated from the OpenAPI schema, so no hand-sync exists. - stats.py's _author_hue narrated a completed migration. Also drops `export` from eight values used only inside their own module, so the compiler will flag them if they ever do become dead.
mediaBatch.ts and fingerprint.ts each declared `BATCH_SIZE = 32` under a comment saying it "mirrors the server-side cap". The server's cap is 64, so the comment was already false — the clients were sending half-size batches. Nothing caught it because the drift is invisible from either side. The cap now lives in api/config.py and ships over /api/config, which already exists for boot-time server capabilities. Both batchers chunk to the published value, so the client cannot ask for more than the server accepts. Two related mirrors fell out while wiring it: - api/config.ts declared a second ServerConfig interface, duplicating the one in state/stores/serverConfig.ts. The store's type is now derived from the generated OpenAPI schema and api/config.ts imports it. - fetchServerConfig rebuilt the response field by field, so `maxBatchPaths` would have been silently dropped on arrival. It now spreads over the defaults and only overrides what the body actually carries, which also keeps a truncated response from producing a zero batch size. Same for useManifestSource, which re-projected allowLocalRepos by hand.
Neither constant is actually duplicated, but both comments said otherwise, which invites someone to "fix" a drift that cannot happen: - gem/mesh.ts said RADIUS_AS_STREET_FRAC "MUST match what layout.ts uses". layout/algorithm.ts reads the same GEM_SIZING signal, so they agree by construction — and layout.ts is not a file. - buildingKind.ts said to mirror its values into the shaders by hand. building-shader.test.ts already asserts every BuildingKind has a matching `const int KIND_*`, so the pairing is enforced, not manual.
`npm audit fix` — lockfile only, no package.json change, and nothing here reaches the shipped bundle (production deps already audited clean). Four advisories remain and are accepted rather than fixable: all four come from openapi-typescript@7.13.0 -> @redocly/openapi-core, and 7.13.0 is the current release, so there is no version to move to. They are DoS-on-hostile-input bugs (brace-expansion OOM, js-yaml quadratic parse) in a tool that runs only during `just gen-types`, against an OpenAPI document this repo generates itself.
Two of the issue's five findings held up; recording all five. (1) The golden guards ran nowhere automatically. `npm test`, CI and the pre-push gate all pass --project=unit, and the bit-identical layout and decoration goldens sat in bench/, which only `npm run bench` executes. They are correctness guards, not timing harnesses, so they move to tests/ — 2.5s against a ~22s suite. bench/ keeps the four profiling harnesses. (3) Fixtures had no size edge cases: mkFile only ever emits size >= 200, lines >= 5, extension .c — never the 0-byte, 0-line, binary or media files that produced NaN geometry. edgeCaseFiles() adds them as a separate export so the captured golden digests stay valid. The new tests assert the invariant (finite, positive dimensions) rather than a digest, because a baseline captured from a broken run would enshrine the NaN. Note where the guard actually bites: statsFromTree builds ranges from non-zero files only, exactly like the server's stats.py, so edge files alone cannot reach Math.log(0) — a stats range whose min is 0 is what does it. Dropping the >= 1 clamp in dimensions.ts turns those cases NaN and fails the test; without the zero-floored range case it would not. (5) No production caller lacks stats: layout/worker.ts and layout/index.ts both pass the whole manifest. The tree-walk fallback serves tests only, so it stays as a degenerate default rather than becoming a required argument. (2) was already fixed — layoutGolden passes statsFromTree today. (4) did not reproduce: the duplicate test titles are same-file across describe blocks. The modal Escape/backdrop/close cases repeat across three components, which is parallel coverage rather than redundancy.
I flagged replay.ts and timeline.py as duplicated logic. They are not. Both walk bundle.deltas, but replay.ts builds a per-path index that the scrub queries read at fractional positions, several times per frame per building — that cannot be a round trip — while compute_commit_date_ranges produces per-commit aggregate ranges that ship in the bundle. Different outputs, different consumers. What misled: timeline.py said it "mirrors the client replay it replaces", which reads as though replay.ts is leftover. The client-side date-range computation is genuinely gone (the client only consumes commitDateRanges / commitLineRanges now); the per-path replay never moved. Both comments now say which walk produces what, so the next reader does not go looking for dead code. _iso_ms also lost its migration narration, per the comment convention.
Everything under manifests/ is keyed by repo CONTENT, not by repo — a new content signature per edit, a new ref key per commit visited, a new bundle per HEAD — so the directory grew for the life of the install. 844 files / 281 MB on one dev machine, and nothing ever pruned it: the delete endpoint removed in this PR was the only sweep, and nothing ever called it. Each save now trims its repo's entries to a per-family cap. Families are capped separately so a scrub session writing many ref manifests cannot evict the live content-signature manifest out from under a running scan. Callers pass the path they just wrote as `protect`. Sorting by mtime alone is not enough: mtime resolves to one second on some filesystems, so a burst of saves ties and the newest entry can rank into the evicted tail — deleting the very manifest the caller is about to read back. Ordering is write-recency, not LRU, since atime is not tracked under relatime. An old signature that keeps being re-read still ages out, which is fine: these are pure performance caches, so eviction costs a rescan and never correctness.
thalida
force-pushed
the
chore/audit-sweep
branch
from
July 26, 2026 19:16
ff3707d to
2e3e185
Compare
Two structural problems, found by adding cycle + layering analysis that the audit had never run. useManifestSource imported loadTimelineScene while useTimelineMode imported loadSource: a genuine runtime cycle between two data hooks. The dependency should run one way, since timeline mode is layered on top of source loading. The refresh handler is now injected instead, and registration cannot be missed because TIMELINE_MODE only turns on by calling into useTimelineMode, which registers as it loads. The api -> state edges were mine, added with the batch-cap change: api/config.ts and api/fingerprint.ts both reached into the state store. The API layer is the one that speaks to the server, so it now owns the ServerConfig shape and defaults, and the store is the reactive mirror. The batch coalescers read the API layer's own resolved config via serverConfigNow(), which is written only from the same memoized fetch the signal mirrors, so there is still one source. Zero import cycles and zero api -> state edges now.
settingsTips.test.ts was green but checked far less than it appeared to. Scope: it only walked settings-store field tips, so every other user-facing string was unguarded. There were 13 em-dashes sitting in copy it could not see — nine almanac tips and four ControlsPane section descriptions. Drift: the store list was hand-written imports and had fallen behind. Four of the sixteen stores on disk (blueprints, ruins, scrubber, theme) were never imported, so they never self-registered and their tips were never checked. That is the same hand-synced-list failure as the batch cap, but worse: a passing test implies coverage that is not there. Both halves now come from import.meta.glob, which cannot drift. The copy half reads source text via `?raw` rather than computing an almanac, because reaching those tips at runtime needs a manifest fixture that hits every branch — and a fixture missing a branch would under-enforce exactly like the old guard did. Verified by reintroducing an em-dash and watching it fail, then pass once fixed. Also drops a `(Task 4)` history comment in test_scan_dirty.py, per the comment convention.
fingerprint.ts and mediaBatch.ts were ~60% structurally identical: same queue Map, same debounce timer, same snapshot-and-clear flush, same chunk loop, same resolve-waiters-or-null. They differed only in endpoint, request body and how a response entry decodes, which is now all a caller passes. I edited both files for the batch-cap change without noticing they were the same file twice. Production clone coverage drops 2.7% -> 2.3%, and cross-file clone sets 28 -> 12. The shared core sits in api/ because it is a transport concern, so city/ keeps importing api/ rather than the reverse. Adds tests for the core, including the chunk-at-the-published-cap behaviour that nothing covered before — the point of the cap being served at all. Verified by hardcoding the chunk size and watching those two cases fail.
findSmallestValidStem is 76-88% of layout, climbing with repo size. Profiling split it almost evenly: ~44% rbush queries, ~42% interval arithmetic. No single smoking gun, so this takes the mechanical half and leaves the structural change (one query per placement instead of one per child rect) to #147. Three changes, all output-preserving by construction: - WorldOccupancy.query compacts in place instead of `.filter()`. rbush already allocated that array; a second array and a second pass across ~32M results was pure overhead. hasOverlap now stops at the first hit rather than compacting everything to read `.length > 0`. - The interval endpoints go into reused Float64Array scratch instead of two fresh growing arrays per call — ~32M pushes across ~240k calls otherwise spend their time allocating and regrowing. - The orientation test is hoisted out of the inner loop; it was re-evaluated twice per candidate. | repo | layout before | after | |------|--------------:|------:| | 10k | 399ms | 274ms | | 30k | 1344ms | 919ms | | 93k | 5541ms | 3629ms | ~39% off findSmallestStem, ~34% off total layout; a 93k-file repo lays out 1.9s faster. Stable across repeat runs. Both goldens pass unchanged, which is the real check: the digests cover every building and street coordinate, so identical hashes mean nothing moved. An idea worth recording as dead: the query looked unbounded along the parent axis and therefore wasteful, but instrumenting for intervals that could never bind found 4 of 7,394,614. The query is narrow perpendicular to the street, so it only ever returns real neighbours. Bounding it buys nothing.
update() was 306 lines at cyclomatic complexity 88 — comfortably the worst
function in the codebase, and the one that runs on every scrub frame.
It is now five phases that say what they do:
readScrubFrame settings, the commit's ranges, hover/selection targets
applyBuildingAtScrub per building, calling
resolveScrubState present / ruin / future / opacity
writeBuildingShape matrix + iFloors by state
resolveBuildingKind the iKind lookup
writeBuildingWeathering colour + age attributes
flushSinks the needsUpdate pass
applyStreetsAtScrub the street/footprint rollup
update() itself is now orchestration.
No behaviour change: the 53 existing tests in scrubController.test.ts cover this
function's observable output closely (height, opacity, ruin/future/empty states,
facade fades, weathering, per-instance attributes, street rollup, needsUpdate
dedup) and all pass untouched, as do both goldens.
Kept allocation-free where it matters. This runs per building per frame, so
resolveScrubState writes into reused scratch rather than returning a fresh
object — the same pattern _m/_pos/_scale/_color already use in this file.
Per-frame objects (the frame, the sinks) are one allocation each, as before.
The three remaining functions sit at 16/20/21. Left there deliberately: they are
flat dispatch — an opacity ternary, a per-street tint decision — not nested
logic, and chasing them under an arbitrary threshold would cost clarity.
The frontend measured coverage without enforcing anything while the backend has failed under 80% all along. Thresholds are set a few points under the real numbers (84.0 lines / 81.6 statements / 82.0 functions / 70.4 branches), so ordinary movement passes and a regression fails. Verified by raising one above actual and watching it fail. The config claimed a "~51% lines / 39% branches" baseline. It has been in the eighties for some time, and I repeated that stale figure in a report before measuring. Also trims seven comment blocks I had added at 5-13 lines each down to the non-obvious why. Density is the problem, not accuracy.
Fifteen test files hand-rolled makeCtx(). cityFixtures.ts already existed to hold exactly this, and its header still said "Task 8 migrates the consumers" — that migration never happened, which is why the copies kept accumulating. Two shapes were actually in use, so there are two helpers: makeSceneContext for components that never touch the picker, and makePickableSceneContext for those that do, returning the selection/hover signals so a test can drive them. Both take an optional cityState, which is what the three parameterised copies needed. Both now hand back a canvas whose clientWidth/clientHeight track a mutable `size` — the trees copy had invented that so onResize() could be tested, and it is useful everywhere. One test was redefining clientHeight by hand and now just sets size.h. Twelve migrated. Three stay deliberately: armOnFirstTick and frameLoop build a one-field stub, and handing them a whole scene would hide how little they need. Test clone coverage 6.1% -> 5.5%, cross-file clone sets 127 -> 97. Also drops the imports the copies left behind, and the stale audit log at the top of the file.
getBuildingDimensions (33) mixed three independent curves in one body. The floors-from-lines and width-from-bytes normalizations are now their own functions, which also puts the log(0) clamp next to the reason it exists. Both goldens and the new edge-case tests pass unchanged. interpretHit (38) dispatched over hit kinds with each branch's body inlined. Each kind now has its own resolver. The conditions stay in interpretHit deliberately rather than becoming a `??` chain: a null from inside a branch means "this kind, nothing selectable", not "try the next kind", and chaining would have let a scrub-hidden building fall through to the street test. Both are now under 15. Repo max drops 38 -> 42 only because _layoutDir was always the ceiling; the count over 15 is 34.
Running `prettier --write ../api` reformatted api/tests/fixtures/sample-repo, which broke test_resolved_dates_prefer_git: the scan reads file content, line counts and mtimes from those files, and rewriting them made the scanner treat them as dirty and report a working-tree mtime where the test expects the git date. It hid well. sample-repo is its own git repo, so the outer `git status` stayed clean and only the containerized pytest in the pre-push gate caught it.
_cache held a whole reconstructed repo tree per visited commit, was written on every successful reconstruction, and was never cleared — resetScrubbedManifest aborts the request and blanks the signal but does not touch it. Scrubbing a long history, or switching repos (src is part of the key, so old entries never became reachable again), grew it for the life of the tab. Now a 16-entry LRU. A quick scrub back and forth stays warm; a long session or a repo switch cannot grow without limit. The frontend twin of the manifests/ retention added earlier in this branch. Verified by disabling the eviction and watching the test fail. Rest of the error-handling sweep came back clean: no empty catch blocks (every one carries a reason), and the unbalanced addEventListener counts are all worker/EventSource listeners dropped wholesale by terminate()/close().
shots.ts read `h.rig`, `const a = h.rig.captureAnchors()`, `m.tree` — handle,
anchors and manifest now say so. picker.ts had `const b = getBuildingByPath()`,
which is not a Building but a resolved hit of {mesh, building, instanceId}, so
`b.building` was doing real work behind a one-letter name; it is `resolved` now.
Also `t` -> `target`, `sw`/`st` -> `sidewalk`/`street`.
Deliberately not a repo-wide sweep. The single letters in colors.ts and
islandGeometry.ts are domain notation (l/c/h for lightness/chroma/hue, r/g/b)
where a longer name reads worse, and renaming several hundred loop counters
would bury the rest of this branch in diff for no gain.
Switching repos out of Timeline rendered part of the new city as translucent, grime-shaded buildings with ruin crosses on their roofs, spreading as more frames ticked. Reproduces on main; not introduced by this branch. getMeshForBuilding resolved a Building purely by its cellId/slotId against the live cells, never checking the building belonged to the current city. Those ids are small integers, so they collide across manifests. The scrub controller captures the old manifest's Building objects at install, and buildings.tick() calls its update() every frame while TIMELINE_MODE holds. Between the new manifest's rebuild swapping in fresh cells and the timeline teardown landing, each stale building resolved to whatever now occupied its old slot, and the old city's ruin iKind/iFade/matrix got written into it. That is why only some buildings were affected and why it grew frame by frame. Fixed at the resolver: the slot must still hold that exact Building. cellAssembly already stamps both directions (b.cellId/b.slotId and cell.buildings[slot] = b), so the identity check is free and catches every stale-city write rather than just the timeline one. Test rebuilds twice and asserts a building from the first city no longer resolves, including that its old cell id really is still live so the test is exercising the collision rather than passing by luck.
Replaces the identity check from the previous commit. That guarded the symptom inside getMeshForBuilding, a shared hot path, and would have silently swallowed this whole class of lifecycle bug instead of surfacing it. The real defect is an asymmetry. rebuild() already clears _tweens, and the comment above it describes exactly this hazard: stale Building objects whose cellId/slotId resolve into the NEW cells and write into a slot now held by a different building. The scrub controller has the same problem and never got the same treatment, so a repo switch out of Timeline left it driving the old manifest's buildings against the new city's meshes every frame. Clearing it is safe because reapplyTimelineScene awaits applyManifest and then installs a fresh controller, so the legitimate in-Timeline rebuild re-seeds the same way _tweens.onDiff does. The only path left holding null is the repo switch, which is the one that should. Comparing ids instead of identity would not have worked either: the stale building's cellId/slotId are valid ids in the new city. That collision is the bug. Test asserts the lifecycle rather than the resolver, and fails without the fix.
Leaving Timeline told the trees, fireflies, streets and footprints to go back to normal. It never told the buildings. Their iKind/iFade/matrices kept whatever the last scrub frame drew, and only an incidental later rebuild ever fixed them. Toggling Timeline off hid that, because it reloads the same repo straight away. Switching repos does not: the switcher keeps the outgoing city on screen as a backdrop, so the frozen city is exactly what you look at for the whole load. Entering Timeline lands at SCRUB_POS 0, the first commit, where almost nothing exists yet, so no scrubbing was needed to see it. The city was never blending two manifests. It was left with no owner: Timeline takes the instance buffers and stops driving them without handing them back. restoreLiveView() is now a required member of a ModeDrivable interface that all five implement, and uninstall walks a typed list instead of naming four of them by hand. Buildings reuse writeBuildingToSlot rather than restating what live means. A sixth subsystem cannot be added without answering the question. Not fixed by feeding Timeline through the manifest pipeline instead: SCRUB_POS is a float and heights interpolate between commits, so that means applyManifest per frame, and layout alone is 1.3s at 30k buildings. Reverts the two earlier attempts on this branch. The trace showed the controller was already uninstalled before the incoming repo rebuilt, so neither the identity check nor dropping the controller on rebuild could have helped.
…d-hoc calls" This reverts commit 892b6f5.
…ad-hoc calls" This reverts commit f78482a.
…ot four ad-hoc calls"" This reverts commit c8d5206.
…kdrop live The project switcher shows the current city as a backdrop. Entering Timeline swaps the UNION manifest into the scene, and the union holds buildings that do not exist at HEAD, so there is no way to repaint it into a live city. Picking a different repo from Timeline therefore left an invalid city on screen for the whole load: ruins, wrong heights, ghosting. Two changes. Leaving Timeline now rebuilds from the live MANIFEST, which Timeline never overwrites; it applies the union straight to the scene. That replaces the hand-rolled restores that each exit path had accumulated, and which always missed something. It also removes the trees/fireflies scrub resets: rebuild disposes and recreates both renderers, so the gate resets itself. The switcher parks Timeline on open and puts it back on dismiss, so the backdrop is always a plain live city. useSwitcherShowcase already snapshots camera pose and selection and already bows out when the source actually changed, so the mode and scrub position just join that snapshot. Restoring re-packs from the saved bundle rather than refetching. The rebuild is best-effort: a dispose or a newer apply can supersede it mid-flight, which was surfacing as an unhandled rejection. Earlier attempts on this branch are reverted. Neither could have worked: the first guarded getMeshForBuilding, the second dropped the scrub controller on rebuild, and the trace showed the controller was already uninstalled before the incoming repo rebuilt.
The repo name vanished partway through a load, at the Building city step, and was sometimes missing on the switcher entirely. PENDING_SOURCE_LABEL had no owner. Five call sites maintained its lifetime by hand, and three of them were literally `hideLoadingOverlay(); LABEL = null;` on consecutive lines — the pairing was the contract, written out longhand every time. loadSource cleared it in its finally, which fires when the STREAM ends, while the overlay lives on through Building and Decorating. Hence the gap. The label is overlay state, so it now lives in the overlay store and hideLoadingOverlay clears it. All five hand-maintained clears are gone. It is no longer possible to hide the overlay and leave a stale label, or clear the label with the overlay still up. Also seeds the header from RECENTS at the start of a load: the server's label arrives with the first stream event, which is after the overlay is on screen. That is the "sometimes missing" half. Found by a check the audit did not have: writers-per-signal. The clone detector works on six-line windows and could not see a two-line pairing repeated five times, and nothing else looked for shared state with no owner. SCRUB_POS is the worst remaining at five writer files across eleven sites.
CI has failed on every push to this branch. layoutGolden timed out at 15s, taking 22s on the runner against 2.5s locally. Not a digest mismatch — the guards are compute-bound (30k buildings, 43k commits) and CI runs `npm run coverage`, while `npm test` and the pre-push gate do not. v8 instrumentation alone takes the layout golden 2.5s to 7s here, and the runner is slower again. I moved these out of bench/ (60s timeout, ran nowhere) into unit/, whose 15s default is tuned for jsdom tests, and only ever measured them uninstrumented on a fast laptop. Both guards now set 60s explicitly. decorationGolden was not failing but sat at 5.8s of the 15s budget, which is a flake waiting for a slow runner. Verified the way CI runs it: `npm run coverage` exits 0, 2855 passing, and the thresholds this PR added are met (lines 84.0/82, statements 81.7/79, functions 81.8/80, branches 70.6/68).
SCRUB_POS was written from 5 files across 11 sites, and no writer owned its range. The consequences were all visible in the tree: - `Math.max(0, commits.length - 1)` re-derived in 4 places - 3 readers clamped defensively (TimeTravelBar on read, readScrubFrame twice) because none of them could trust the value - the 2 restore paths clamped nothing at all: exitTimelineMode's cancel and the switcher dismiss both replay a position saved against an older bundle SCRUB_POS is now a computed that clamps a private raw signal against the current bundle, so it is in range by construction. Clamping on read rather than on write is what closes the restore case: a bundle swap alone re-clamps, with no write to forget. A shrink no longer destroys the saved position either, so it comes back if the bundle grows again. Exported as ReadonlySignal, which makes bypassing the setter a type error rather than a convention. That is how the writers were found: the compiler listed them. Also drops the now-redundant clamps, the duplicated max derivations, and the in-place branch in loadTimelineScene that existed only to re-clamp by hand. Six scrubController tests turned out to hand a bundle to the controller while leaving TIMELINE_BUNDLE null, so the store and the deps disagreed about how many commits existed. They only passed because nothing clamped. They now publish the bundle, like setup() already did. New store tests cover the shrink case; removing the clamp fails 3 of 4.
The two enter paths had diverged in their ordering: loadTimelineScene bundle -> pack scene -> flip mode switcher restore bundle -> flip mode -> pack scene (async) The switcher turned TIMELINE_MODE on before reapplyTimelineScene had repacked the union city, so for the length of that await the mode said Timeline while the scene still held live geometry. That is the same shape as the ghosting this PR already chased, just a narrower window. Both now call enterTimelineMode(pos?) in the store, which flips the mode and sets the position in one batch, and both call it only once the city is packed. TIMELINE_MODE is no longer written outside its store. BEHAVIOR CHANGE worth re-testing: dismissing the switcher back into Timeline now enters a frame or two later than before, after the repack rather than before it. The guard is `!active`, so reopening the switcher during that repack keeps it parked instead of having the stale continuation turn Timeline back on. TIMELINE_BUNDLE still has three writers. The two outside the store both mean "arm the bundle we are about to pack", and wrapping an assignment with no invariant to enforce would be ceremony, so it stands.
Coming back from the project switcher left the tick canvas blank until a drag, which repainted it. Two things had to be wrong at once, and I supplied the second: The mode check sat above every hook, so the component skipped its hooks while rendering nothing but kept the instance and its hook state. The bundle and position are restored to the exact values they had before, so on re-entry [scale, pos, accentTheme] all compared equal and the effect never re-ran. Its ResizeObserver was still watching the detached old track, so nothing fired there either, while the canvas the effect had painted was long gone and the newly built one was blank. That stayed hidden because the old restore set the position in a separate step after flipping the mode, so pos went 0 -> scrubPos and dragged a repaint along with it. Setting both in one batch removed the accidental dep change. So: hooks now run unconditionally and the bail happens after them, and the draw depends on `mounted`, since leaving Timeline destroys the canvas and returning builds a fresh blank one. Element identity was never in the dep list; the old deps only described what to paint, not what to paint onto. The regression test toggles the mode with every other dep held equal. Removing either half of the fix fails it.
thalida
commented
Jul 27, 2026
| return "ref" | ||
| if name_rest.startswith("timeline-"): | ||
| return "timeline" | ||
| return "content" |
Owner
Author
There was a problem hiding this comment.
these should be a proper enum
| _FAMILY_KEEP = { | ||
| "content": _KEEP_CONTENT_MANIFESTS, | ||
| "ref": _KEEP_REF_MANIFESTS, | ||
| "timeline": _KEEP_TIMELINE_BUNDLES, |
Owner
Author
There was a problem hiding this comment.
enum, that's also ideally used here.
| here, since re-reading it only ever saved a rescan. | ||
|
|
||
| Best-effort, like the rest of this module: a failed unlink (or a file a | ||
| concurrent request removed first) must never break the response.""" |
| if not manifests_dir.exists(): | ||
| return 0 | ||
|
|
||
| prefix = f"{repo_key(abs_root)}__" |
Owner
Author
There was a problem hiding this comment.
if this is the cache prefix, this should be a general shared constant, so that it doesn't become out of date w/ the the cache naming.
| "ref": [], | ||
| "timeline": [], | ||
| } | ||
| for path in manifests_dir.glob(f"{prefix}*.json.gz"): |
Owner
Author
There was a problem hiding this comment.
same thing w/ the file ending, there could be a shared helper function with shared constnats.
| let _cached: Promise<ServerConfig> | null = null; | ||
| // The resolved value, for callers that need it synchronously mid-request (the | ||
| // batch coalescers). Not a second source of truth: it is written only from the | ||
| // memoized fetch below, which is the same one the SERVER_CONFIG signal mirrors. |
| // Spread over the defaults rather than re-projecting field by field: the | ||
| // old shape listed each key by hand, so a field added on the server was | ||
| // silently dropped here. Only override what the body actually carries, so | ||
| // a truncated response can't yield a zero batch size. |
| // Per-instance building render kind (the iKind attribute). building.frag.glsl | ||
| // redeclares these as `const int KIND_*`; building-shader.test.ts asserts the | ||
| // two agree, since drift would silently render a whole class of buildings in | ||
| // the wrong mode. |
| hi.push(upper); | ||
| if (loCount === lo.length) { | ||
| lo = _loScratch = _growScratch(lo); | ||
| hi = _hiScratch = _growScratch(hi); |
Owner
Author
There was a problem hiding this comment.
why double set vars here? do we need both? can they be merged?
| // The server walks the same deltas, but for a different output: per-commit | ||
| // aggregate ranges, shipped in the bundle. This builds a per-path index the | ||
| // scrub queries below read at any fractional position, several times per frame | ||
| // per building, so it has to be local — it is not a copy of the server's pass. |
Owner
Author
There was a problem hiding this comment.
same issue w/ comment length. i like short commetns because they're fast to scan and read.
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
1 participant
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.
Closes #124
Closes #73
Closes #108
A whole-repo pass for dead code, duplicated logic, and centralization, plus a security audit of dependencies and granted access. Analysis was done with two throwaway scanners over 823 TS exports, 271 Python module symbols and 375 CSS classes; every candidate below was hand-verified, and a lot of plausible-looking findings were discarded (FastAPI route handlers, runtime-applied
hljs-*classes, private helpers called by their own module).Security
Syntax themes came from jsDelivr. Every theme change fetched CSS from
cdn.jsdelivr.net, telling a third party the viewer's IP, user agent and chosen theme, breaking offline use, and trusting un-pinned remote CSS — which is not merely cosmetic, since attribute-selectorurl()rules can exfiltrate.highlight.jswas already a direct dependency shipping those stylesheets, so they now come from the bundle same-origin. This is the only place data left the machine.The API bound to
0.0.0.0by default. A pip-installedcodecityserved every interface. There is no authentication, and once a scan registers a root,/api/fileserves anything beneath it. Default is now127.0.0.1; containers pass--host 0.0.0.0explicitly, where the bind is the container's own namespace and only published ports are reachable.Dependencies. Production npm deps audit clean.
npm audit fixcleared the vite/undici/postcss dev advisories. Four remain and are accepted: all come fromopenapi-typescript@7.13.0→@redocly/openapi-core, and 7.13.0 is current, so there is nowhere to move. They are DoS-on-hostile-input bugs in a tool that runs only duringjust gen-types, against an OpenAPI document this repo generates itself.Worth knowing, not fixed: Pillow and hachoir parse arbitrary files from scanned repos, so a hostile git URL makes malformed-image CVEs reachable. That is inherent to the feature.
#124 — dead code
Most of what the issue named was already gone (
uTiltRad,attachLeanAwareRaycast,CommitChip,LOOK_AHEAD,.commit-open,.commit-timeline-btn);BLUEPRINTSandscanlineare live features. The Python backend had zero dead symbols.What was real:
An unreachable cross-stack API.
DELETE /api/manifest/cachehad no caller, and the frontend'sclearManifestCache()was never invoked — its only reference was a test asserting it is never called, which is what surfaced it. Removing it madecache_clear_all,cache_clear_manifests,remove_cloneandCacheClearResponsedead too.No user-facing capability is lost. The existing Skip cache (fresh scan) toggle already bypasses every cache read (manifest, file-stat, git-history, blob), rewrites each one fresh in a single pass, and explicitly evicts timeline bundles — strictly better recovery than deleting and forcing a later rescan. Corrupt clones are handled by
ensure_clone, which re-clones from scratch when an update fails for any non-user-facing cause. Chasing that down surfaced a real pre-existing gap, now fixed here: nothing ever prunedmanifests/. Every entry is keyed by repo content — a signature per edit, a ref key per commit visited, a bundle per HEAD — so it grew for the life of the install (844 files / 281 MB on one dev machine). The removed endpoint was the only sweep and was never called, so this predates the sweep rather than following from it. Each save now trims its repo to a per-family cap, protecting the entry just written.Orphaned CSS, including the
.file-path-*residue from stripping PathBreadcrumbs. The.text-*,.dot--*and.row--*families were kept as documented vocabulary.Comments describing code that moved — the worse half, since they actively misdirect. TreePane.css pointed at
.row--selectedwhen the tree uses.tree-selected; InfoPane.css claimed a shell from.code-inlinethat it redeclares;overlaps.tscited a test file that does not exist;manifest_types.pysaid to hand-sync with a path that is both wrong and generated.#73 — test suite
Two of five findings held up. The golden guards ran nowhere automatically —
npm test, CI and the pre-push gate all pass--project=unit, and the bit-identical goldens sat inbench/. They are correctness guards, not timing harnesses, so they moved totests/(2.5s against a ~22s suite). Fixtures had no size edge cases, soedgeCaseFiles()adds the 0-byte, 0-line, binary and media files that produced NaN geometry.The new tests assert the invariant rather than a digest, because a baseline captured from a broken run would enshrine the NaN. They bite where it counts: dropping the
>= 1clamp indimensions.tsturns widths NaN and fails them.Finding 2 was already fixed. Finding 4 did not reproduce — the duplicate titles are same-file across
describeblocks. Finding 5 is answered: no production caller lacks stats.#108 — flaky a11y audit
The issue's proposed fix would not have worked.
axe.teardown()clears caches and the element tree but leaves_runningset, and_runningis the latch that makes the next surface throw after a timeout. Releasing it is what breaks the cascade. There is a regression test that wedges the latch; it fails withteardown()alone.Duplication
BATCH_SIZE = 32existed in two frontend files under a comment saying it mirrors the server cap. The server's cap is 64, so the comment was already false and both clients were sending half-size batches. The cap now ships over/api/config. Two related mirrors fell out: a secondServerConfiginterface, and a parser that rebuilt the response field by field and so would have silently dropped the new field on arrival.The two asset fetchers had each hand-rolled the same coalescing batcher, now shared as
createPathBatcher.Two flagged items turned out not to be duplication, and are documented rather than consolidated:
RADIUS_AS_STREET_FRACis read from one shared signal, andreplay.tsvstimeline.pyare different computations for different consumers.Complexity and performance
scrubController.updatewas 306 lines at cyclomatic complexity 88. Split into named phases, worst function in the file is now 21. Layout got ~35% faster with bit-identical goldens, by compacting in place, exiting overlap checks early, and reusing scratch buffers. The larger layout rework is filed separately rather than done here.Two caches were unbounded. The backend manifest cache had grown to 844 files / 281 MB on one dev machine; it now trims per family on write.
scrubbedManifestcached whole manifests forever; now a 16-entry LRU.Shared state with no owner
The audit missed a real defect, so I added the check it lacked: writers-per-signal. Clone detection works on six-line windows and could not see a two-line pairing repeated five times.
PENDING_SOURCE_LABELwas maintained by hand at five call sites, three of them literallyhideLoadingOverlay(); LABEL = null;on consecutive lines.loadSourcecleared it when the stream ended, while the overlay lives on through Building and Decorating, so the repo name vanished mid-load. It now lives in the overlay store andhideLoadingOverlayclears it.SCRUB_POSwas written from five files across eleven sites.Math.max(0, commits.length - 1)was re-derived in four places, three readers clamped defensively because none could trust the value, and the two restore paths clamped nothing at all: both replay a position saved against a possibly-shorter bundle. It is now a computed that clamps against the current bundle, so a swap alone re-clamps with no write to forget, and it is exportedReadonlySignalso bypassing the setter is a type error. That is how the writers were found: the compiler listed them.This surfaced a test defect. Six
scrubControllertests handed a bundle to the controller while leavingTIMELINE_BUNDLEnull, so the store and the deps disagreed about how many commits existed. They passed only because nothing clamped.The two Timeline entry paths had also diverged:
loadTimelineScenepacked the scene then flipped the mode, while the switcher restore flipped the mode then packed. Both now enter throughenterTimelineMode, after the pack.Two rendering bugs found while testing
Entering Timeline swaps a UNION manifest into the scene, containing buildings that do not exist at HEAD. Every exit path hand-rolled a partial restore, so leaving Timeline could leave a city that was neither. Exit now rebuilds from the live manifest, and the switcher parks Timeline on open and restores it on dismiss. Pre-existing on
main, not introduced here.Verification
Full gate green: 361 pytest, 2859 vitest, ruff, eslint, prettier, tsc, and CI green end to end.
Verified as CI runs it (
npm run coverage), not as the pre-push gate runs it (npm test) — that distinction cost three red CI runs. The relocated golden guards are compute-bound, and coverage instrumentation tooklayoutGoldenfrom 2.5s locally to 22s on the runner, past the 15s timeout of the project I had moved them into. Both goldens now set their own timeout. The gap is still there: nothing local exercises the coverage path, which is the same shape as the #73 finding this PR opened with.Container smoke-tested: api bound to
0.0.0.0:8000inside the container, cross-container health check ok,/api/configserved end to end through the vite proxy.🤖 Generated with Claude Code