From 81e08b76ed003f426ec9eb30d73fc25b38c1a086 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Thu, 1 Oct 2026 20:23:31 +0000 Subject: [PATCH] =?UTF-8?q?fix(viewer):=20pages=20that=20went=20blank=20?= =?UTF-8?q?=E2=80=94=20repeated=20ids,=20and=20Steps=20on=20a=20function?= =?UTF-8?q?=20full=20of=20checks=20(#2264)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(viewer): a repeated id no longer blanks Entry points or a component's Symbol view The viewer keys its lists by id and a repeat stops the page drawing. The routes list was a row per (route, edge), so an inline handler came back once per call in its body; it is now one row per route — its bound handler, or the route itself as an inline handler. A Vue/Svelte/Astro component's members listed its script's symbols twice, plus the component and its file; each member is now listed once. Co-Authored-By: Claude Opus 5.5 * fix(viewer): Steps reads a function full of checks in one pass The in-order reading kept every way forward through an `if` with no `else` twice — under its condition and without — so eighty checks that draw nothing (jsoup's `parse`) made 2^80 ways and the page died on a stack overflow. Ways waiting at the same place are merged, keeping the conditions they all share. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- __tests__/ui-program-model.test.ts | 30 +++++++++ __tests__/ui-repeated-keys.test.ts | 97 ++++++++++++++++++++++++++++++ docs/viewer-launch-changelog.md | 4 ++ src/ui-server/api/node.ts | 11 +++- src/ui-server/api/routes.ts | 70 +++++++++++++++++++-- ui/src/lib/entry-model.ts | 3 +- ui/src/lib/program-model.ts | 26 +++++++- ui/src/lib/wire.ts | 4 ++ 8 files changed, 233 insertions(+), 12 deletions(-) create mode 100644 __tests__/ui-repeated-keys.test.ts diff --git a/__tests__/ui-program-model.test.ts b/__tests__/ui-program-model.test.ts index 467976bb2..3ea4d0852 100644 --- a/__tests__/ui-program-model.test.ts +++ b/__tests__/ui-program-model.test.ts @@ -328,3 +328,33 @@ describe('the picture in the code’s order', () => { expect(stepEdgeVisible(model, sibling, 'inside')).toBe(true); }); }); + +describe('a body full of checks that draw nothing', () => { + it('reads in one pass, not one per combination of checks', () => { + // jsoup's `parse` inlines eighty-odd `if`s with no `else` and no step in + // them; each one doubled every way on, and the page died overflowing the + // stack. They are all one way on, under no condition of their own. + const checks: WireItem[] = Array.from({ length: 80 }, (_, i) => ({ + kind: 'fork', + form: 'if', + on: `c${i}`, + arms: [arm(`c${i}`, [])], + })); + const root: WireBlock = [{ kind: 'step', step: 'a' }, ...checks, { kind: 'step', step: 'b' }]; + expect(shape(root)).toEqual(['anchor → a', 'a → b']); + expect(buildOrderModel(payload([step('a'), step('b')], root))?.nodes.size).toBe(3); + }); + + it('claims no side when the code goes on either way', () => { + const root: WireBlock = [ + { + kind: 'fork', + form: 'if', + on: 'user', + arms: [arm('user', [{ kind: 'fork', form: 'if', on: 'admin', arms: [arm('admin', [])] }])], + }, + { kind: 'step', step: 'a' }, + ]; + expect(shape(root)).toEqual(['anchor → a']); + }); +}); diff --git a/__tests__/ui-repeated-keys.test.ts b/__tests__/ui-repeated-keys.test.ts new file mode 100644 index 000000000..e848f5348 --- /dev/null +++ b/__tests__/ui-repeated-keys.test.ts @@ -0,0 +1,97 @@ +/** + * The viewer draws its lists keyed by id, and a repeated id stops the page + * from drawing at all — Entry points on hono, the Symbol view of every Vue / + * Svelte / Astro component. Two payloads repeated themselves: + * + * - the routes list was a row per (route, edge), so an inline handler came + * back once per call in its body (`GET /stream/text` as `streamText`, + * `writeln` and `sleep`). A route is one row: its bound handler, or the + * route itself as an inline handler; + * - a component's members listed its script's symbols twice, and the + * component and its file among them. + */ +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import { buildRoutes } from '../src/ui-server/api/routes'; +import { buildNode } from '../src/ui-server/api/node'; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ui-repeated-')); + const files: Record = { + 'package.json': JSON.stringify({ name: 'app', private: true, dependencies: { express: '^4.0.0', astro: '^4.0.0' } }), + 'server/app.js': `const express = require('express'); +const app = express(); + +function loadUser(id) { return { id }; } +function audit(id) { return id; } +function render(user) { return user; } +function listUsers(req, res) { res.json([]); } +function showUser(req, res) { res.json(loadUser(req.params.id)); } + +app.get('/users/:id/card', async (req, res) => { + const user = loadUser(req.params.id); + audit(user.id); + res.send(render(user)); +}); + +app.get('/users', listUsers); +app.get('/users/:id', showUser); +`, + 'src/components/Card.astro': `--- +import Badge from './Badge.astro'; +import { format } from '../lib/format'; +const title = format('card'); +--- +
{title}
+`, + 'src/components/Badge.astro': `badge +`, + 'src/lib/format.ts': `export function format(s: string): string { + return s.trim(); +} +`, + }; + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + cg = await CodeGraph.init(root, { index: true }); +}); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +describe('the routes list', () => { + it('has one row per route', () => { + const { entries } = buildRoutes(cg, new URLSearchParams()); + const ids = entries.map((e) => e.routeId); + expect(new Set(ids).size).toBe(ids.length); + }); + + it('names an inline handler as one, and a bound handler by its name', () => { + const { entries } = buildRoutes(cg, new URLSearchParams()); + const card = entries.find((e) => e.url === 'GET /users/:id/card')!; + expect(card).toMatchObject({ handler: 'inline handler', inline: true, handlerId: card.routeId }); + expect(entries.find((e) => e.url === 'GET /users/:id')).toMatchObject({ handler: 'showUser', inline: false }); + }); +}); + +describe('a component’s members', () => { + it('lists each symbol once, and never the component or a file', async () => { + const component = cg.getNodesByKind('component').find((n) => n.name === 'Card')!; + const payload = (await buildNode(cg, root, component.id)) as { members: { items: Array<{ id: string; kind: string }> } }; + const ids = payload.members.items.map((m) => m.id); + expect(ids.length).toBeGreaterThan(0); + expect(new Set(ids).size).toBe(ids.length); + expect(ids).not.toContain(component.id); + expect(payload.members.items.map((m) => m.kind)).not.toContain('file'); + }); +}); diff --git a/docs/viewer-launch-changelog.md b/docs/viewer-launch-changelog.md index 2df404954..8d2864db2 100644 --- a/docs/viewer-launch-changelog.md +++ b/docs/viewer-launch-changelog.md @@ -141,6 +141,8 @@ These describe `codegraph ui` and its screens. They were taken out of `## [Unrel ## Fixes — Steps — what a call does, and when +- **Steps no longer goes blank on a function full of checks.** Reading a function in its code's order doubled the ways forward at every `if` with no `else`. A body with dozens of checks that draw nothing, like jsoup's `parse`, never finished, and the page died with a stack overflow. Ways that arrive at the same place are now one way, carrying only the conditions they all share. + - **A Swift app's Steps picture labels what a call leaves for correctly.** `rawValue.data(using: .utf8)` was drawn as a network call, Foundation's `Timer` as telemetry, `Calendar` and a SiriKit `intent` as device calls, and `viewModel.votes.firstIndex(of:)` as a database read. Now only a session's `data`, `upload` and `download` are network, `Timer`, `Calendar` and `intent` are no effect at all, a view model is not a table, and a keychain is storage. A call through a type the app declares itself, like IceCubes' `Notifications` endpoint enum, is no longer read as the library of the same name. - **A server-rendered endpoint's Steps picture shows how it answers.** A Spring MVC handler answers by what it returns: a view name, a constant holding one, `"redirect:/owners/" + id`, a `ModelAndView` or a `RedirectView`. Each is now a reply, a render (`200`) or a redirect (`302`), under the condition it's returned in. petclinic's `POST /owners/new` draws the form re-rendered `WHEN result.hasErrors()` and the redirect otherwise. A Laravel controller's `view(…)`, `redirect(…)`, `redirect()->back()`, `back()`, `to_route(…)` and `response()->json(…)` are replies too, so BookStack's book page draws the page it renders beside its `404` and its old-slug redirect. PHP's own programming-error exceptions (`InvalidArgumentException`, `LogicException`, …) thrown deep in a helper are no longer drawn as one of the endpoint's answers. @@ -169,6 +171,8 @@ These describe `codegraph ui` and its screens. They were taken out of `## [Unrel ## Fixes — Symbols, tests and the viewer +- **Entry points and the Symbol view no longer go blank.** The Symbol view of every Vue, Svelte and Astro component stayed on "Loading…", because its member list held the component's script symbols twice, along with the component and its file. Entry points went blank on apps with inline route handlers like `app.get('/x', async (c) => { … })`, which came back once per call in the handler, each call listed as if it were the handler. Each member is now listed once, and each route is one row: the handler it is bound to, or "inline handler" when the handler is written at the registration. + - **Entry points' most-depended-on list shows your code, not a vendored bundle's.** A minified library or bundled docs script (`n`, `t`, `Buffer` with thousands of callers, all from inside the bundle) could top the list. Symbols in generated files are now left out of it, like test files. - **Stopping `codegraph ui` stops the server.** Killing the command by its process id, as a process manager, an editor task or `kill` does, left the server running on its port with no way to reach it, until the machine restarted. The server now notices it has been left behind and shuts down, closing the index first. Ctrl+C was never affected. diff --git a/src/ui-server/api/node.ts b/src/ui-server/api/node.ts index bc7d6b4a6..c5431a0d3 100644 --- a/src/ui-server/api/node.ts +++ b/src/ui-server/api/node.ts @@ -244,10 +244,17 @@ function buildMembers( endpoints: Map, overrides?: Map ): { items: WireMember[]; total: number } { + // Each member once, and never the symbol itself or a file: a component in a + // Vue / Svelte / Astro file was recorded as containing its script's file node + // and every symbol in it, so the walk one level down met those symbols + // again — and a member listed twice is a repeated key the viewer cannot draw. + const seen = new Set([focal.id]); + const admit = (child: Node | undefined): child is Node => + !!child && child.kind !== 'file' && !seen.has(child.id) && !!seen.add(child.id); const direct: Array<{ node: Node; parentId: string; depth: number }> = []; for (const edge of containsOut) { const child = endpoints.get(edge.target); - if (child) direct.push({ node: child, parentId: focal.id, depth: 1 }); + if (admit(child)) direct.push({ node: child, parentId: focal.id, depth: 1 }); } const containerIds = direct @@ -260,7 +267,7 @@ function buildMembers( const grandNodes = cg.getNodesByIds(grandEdges.map((e) => e.target)); for (const edge of grandEdges) { const child = grandNodes.get(edge.target); - if (child) nested.push({ node: child, parentId: edge.source, depth: 2 }); + if (admit(child)) nested.push({ node: child, parentId: edge.source, depth: 2 }); } } diff --git a/src/ui-server/api/routes.ts b/src/ui-server/api/routes.ts index 44429b6b5..ca20c9a74 100644 --- a/src/ui-server/api/routes.ts +++ b/src/ui-server/api/routes.ts @@ -23,6 +23,8 @@ import type { CodeGraph } from '../../index'; import { intParam } from './respond'; import { toPosixPath } from './wire'; +import { routeRoots } from './route-roots'; +import { isTestFile } from '../../search/query-utils'; /** * HTTP verbs a route name may lead with, plus the two stand-ins the resolvers @@ -56,6 +58,8 @@ export interface WireRoute { routeFile: string; routeLine: number; routeId: string; + /** The handler is written inline at the registration: the route stands in for it. */ + inline: boolean; } export interface WireRoutes { @@ -86,11 +90,62 @@ export function splitRouteName(url: string): { method: string | null; path: stri */ const MIN_LIMIT = 3; +/** Manifest rows fetched per route shown, so folding repeats still fills the page. */ +const ROWS_PER_ROUTE = 4; + +type ManifestRow = NonNullable>['entries'][number]; + +/** + * The manifest's rows folded to one per route, in its order: the row naming + * the route's root (the handler a resolver bound, or the component a screen + * renders), else the route's first row. A route whose root is its own inline + * body is that route, labelled as an inline handler — not whichever call in + * the body sorted first — and is left out when it is written in a test, as + * the manifest leaves out test handlers. + */ +function oneRowPerRoute(cg: CodeGraph, entries: readonly ManifestRow[]): Array { + const byRoute = new Map(); + for (const entry of entries) { + const list = byRoute.get(entry.routeId); + if (list) list.push(entry); + else byRoute.set(entry.routeId, [entry]); + } + const routes = [...cg.getNodesByIds([...byRoute.keys()]).values()]; + const roots = routeRoots(cg, routes); + const out: Array = []; + for (const [routeId, list] of byRoute) { + const root = roots.get(routeId); + if (root?.inline) { + const first = list[0]!; + if (isTestFile(first.routeFile)) continue; + out.push({ + ...first, + handler: 'inline handler', + handlerKind: 'route', + handlerFile: first.routeFile, + handlerLine: first.routeLine, + inline: true, + }); + continue; + } + const named = root + ? list.find((e) => e.handlerFile === root.node.filePath && e.handlerLine === root.node.startLine && e.handler === root.node.name) + : undefined; + out.push({ ...(named ?? list[0]!), inline: false }); + } + return out; +} + export function buildRoutes(cg: CodeGraph, query: URLSearchParams): WireRoutes { const limit = intParam(query, 'limit', { min: MIN_LIMIT, max: 500, default: 200 }); - // One row over the limit, purely to learn whether there were more. - const manifest = cg.getRoutingManifest(limit + 1); + // The engine's manifest is a row per (route, edge): a route bound to two + // symbols, or an inline handler whose every call reads as a "handler" + // (hono's `GET /stream/text` came back three times, as `streamText`, + // `writeln` and `sleep`), repeats. A route has ONE answer to "what serves + // this" — route-roots.ts's — so rows are over-fetched and folded onto it. + const fetched = limit * ROWS_PER_ROUTE + 1; + const manifest = cg.getRoutingManifest(fetched); const routeCount = cg.getStats().nodesByKind.route ?? 0; if (!manifest) { @@ -105,8 +160,9 @@ export function buildRoutes(cg: CodeGraph, query: URLSearchParams): WireRoutes { }; } - const truncated = manifest.entries.length > limit; - const rows = manifest.entries.slice(0, limit); + const folded = oneRowPerRoute(cg, manifest.entries); + const truncated = folded.length > limit || manifest.entries.length >= fetched; + const rows = folded.slice(0, limit); // Every row's handler file in one batched query, so a project that scatters // handlers across hundreds of files still costs one query per chunk, and no @@ -127,11 +183,13 @@ export function buildRoutes(cg: CodeGraph, query: URLSearchParams): WireRoutes { handlerKind: entry.handlerKind, file: toPosixPath(entry.handlerFile), line: entry.handlerLine, - handlerId: - byFileLineName.get(`${entry.handlerFile} ${entry.handlerLine} ${entry.handler}`) ?? null, + handlerId: entry.inline + ? entry.routeId + : byFileLineName.get(`${entry.handlerFile} ${entry.handlerLine} ${entry.handler}`) ?? null, routeFile: toPosixPath(entry.routeFile), routeLine: entry.routeLine, routeId: entry.routeId, + inline: entry.inline, })); return { diff --git a/ui/src/lib/entry-model.ts b/ui/src/lib/entry-model.ts index 854d2cbfd..ef7061b37 100644 --- a/ui/src/lib/entry-model.ts +++ b/ui/src/lib/entry-model.ts @@ -140,7 +140,8 @@ export function routeRow(route: WireEntryRoute): EntryRow { target: route.handlerId ? { type: 'symbol', id: route.handlerId, name: route.handler, kind: route.handlerKind } : null, - flowFrom: route.handlerId ? route.handler : null, + // An inline handler has no name of its own to draw a Flow from. + flowFrom: route.handlerId && !route.inline ? route.handler : null, title: `${route.url} → ${route.handler} (${route.file}:${route.line}), registered at ${route.routeFile}:${route.routeLine}`, }; } diff --git a/ui/src/lib/program-model.ts b/ui/src/lib/program-model.ts index 8d38559ac..bbcbe48dc 100644 --- a/ui/src/lib/program-model.ts +++ b/ui/src/lib/program-model.ts @@ -174,12 +174,12 @@ export function orderGraph(program: NonNullable, an // An arm that answers, returns or throws does not rejoin — nothing // leaves the last box in it, which is what says so on a canvas. const armTails = flow(arm.body, entry, []); - if (arm.ends === null) out.push(...armTails); + if (arm.ends === null) for (const t of armTails) out.push(t); } // An `if` with no `else` runs on either way; a fork with both sides // covered runs on only through the arms that did not end. - if (item.arms.length < 2) out.push(...tails.map((t) => ({ ...t, runs: [...t.runs, ...runs] }))); - tails = out; + if (item.arms.length < 2) for (const t of tails) out.push({ ...t, runs: [...t.runs, ...runs] }); + tails = mergeTails(out); } else if (item.kind === 'block') { const label = runWords(item); const inner = flow(item.body, tails, [...runs, label]); @@ -206,6 +206,26 @@ export function orderGraph(program: NonNullable, an return { edges, depth: rows(anchor, ids, edges), forks }; } +/** + * Ways on that wait at the same place are one place to go on from. Without + * this, an `if` with no `else` — or a fork whose arms draw nothing — doubles + * every way it crosses (once under its condition, once without), and a body + * with eighty such checks (jsoup's `parse`) never finished: the page died + * overflowing the stack. The merged way keeps what every way to it had to + * hold; a condition only some of them needed is dropped, so the line + * under-claims rather than claims a side it does not have. + */ +function mergeTails(tails: readonly Tail[]): Tail[] { + const byPlace = new Map(); + for (const t of tails) { + const key = `${t.id}\0${t.arm ?? ''}\0${t.runs.join('\0')}`; + const found = byPlace.get(key); + if (found) found.when = found.when.filter((w) => t.when.includes(w)); + else byPlace.set(key, { ...t, when: [...t.when], runs: [...t.runs] }); + } + return [...byPlace.values()]; +} + /** Whether anything in this block draws a box — a fork with one drawn arm is a guard clause, not a point. */ function hasStep(block: WireBlock): boolean { return block.some((item) => diff --git a/ui/src/lib/wire.ts b/ui/src/lib/wire.ts index 227989e65..74b4462ed 100644 --- a/ui/src/lib/wire.ts +++ b/ui/src/lib/wire.ts @@ -377,6 +377,8 @@ export interface WireRoute { routeFile: string; routeLine: number; routeId: string; + /** The handler is written inline at the registration: the route stands in for it. */ + inline: boolean; } export interface WireRoutes { @@ -410,6 +412,8 @@ export interface WireEntryRoute { routeFile: string; routeLine: number; routeId: string; + /** The handler is written inline at the registration: the route stands in for it. */ + inline: boolean; } export interface WireEntryFile extends WireNodeRef {