fix(insights): a session id is only unique inside its app (#248) - #532

Thank you for digging into #248, @snowyukitty — the GROUP BY source_app change is correct and it fixes a real bug. I reproduced it against main before reviewing, so that part is not in question:

With two apps sharing a session_id and running the same command (7 events each), main collapses them into a single insight — Possible loop · 14× identical command, attributed to whichever source_app SQLite happened to pick. With your insights.ts change applied, they split correctly into app-alpha:s-shared and app-beta:s-shared, 7× each. That's the bug, and you found it.

Three things before I can merge it.

1. The added test passes on main, unpatched. I ran server/test/insights-group-app.test.ts against an unmodified tree: 1 pass. It uses two different session ids (s-1, s-2), and the old GROUP BY session_id, cmd already separated those — so the test never exercises the grouping it's named for. To fail before the fix, the two apps have to collide on both session_id and cmd:

for (let i = 0; i < 7; i++) db.insertEvent(event("app-alpha", "s-shared", "pytest test_a.py", i) as any);
for (let i = 0; i < 7; i++) db.insertEvent(event("app-beta",  "s-shared", "pytest test_a.py", i) as any);

Then assert both app-alpha:s-shared and app-beta:s-shared are present. That version fails on main and passes with your change — I verified both directions.

2. The insight ids carry the same bug. id: \loop:${l.session_id}:${l.cmd}`(andspend:${s.session_id}, and the error-rate one) don't include source_app. In exactly the case this PR fixes, the two now-correctly-separated insights get **identical ids** — and Alerts.tsx:225renders them withkey={i.id}`. The grouping fix needs to carry into the ids, otherwise it splits the rows and then collides them again one layer up.

3. Please drop the normalize() change from this PR. isWithin takes an explicit separator parameter precisely so Windows paths can be checked on any host — that's what server/test/windows-paths.test.ts exercises. normalize is platform-bound, so on a mismatched platform it disagrees with the s argument that was passed in. It also doesn't fix what it sounds like it fixes: posix normalize("/a/b/") keeps the trailing slash, so the mismatch case is unchanged, and the callers already pass resolve()-derived paths (see the note at db.ts:927). If there's a concrete path-shape that fails today, that's worth its own PR with the failing case.

Also two stray blank lines snuck in — one after isWithin in config.ts, one inside the SQL template before HAVING.

Fix 1 and 2, drop 3, and I'll merge this. It's a good catch and I'd rather land it with a test that actually guards the behaviour.

Generated by Claude Code

Before changing anything I ran the existing test against unpatched 26ef8bc; it reported 1 pass, because its fixture never made the two apps collide.

1. The fixture collides now. One session id, one command, two apps. On 26ef8bc:

Expected to contain: "app-beta:s-shared"
Received: [ "app-alpha:s-shared" ]

It carries all three session-scoped insights on that shared id, so the id assertion below covers spend: and errors: too, not just loop:.

2. The ids carry the pair. rowId sits next to key, and the comment says why they differ: key is the label, it truncates the session id, and two rows are allowed to render the same one; an id is a React key and may not.

The fixture produces nine insight rows — three kinds for each of the two colliding apps, two more loops from the pair in the next paragraph, and the hourly burn card. With GROUP BY fixed and the ids left alone, expect(new Set(ids).size).toBe(ids.length) gives:

Expected: 9
Received: 6

Three duplicate ids, one for each of loop:, spend: and errors:.

One thing changed the shape of the fix: `${app}:${sid}` is not injective. App a:b in session c and app a in session b:c are two rows and one string, and nothing constrains source_app to exclude a colon — the pre-change fixture in insights-scope.test.ts left a whole C:\Users\… path in it on Windows, drive colon included. rowId encodes each half instead of joining them, and a third fixture pair pins it. With a plain join the same assertion gives Expected: 9 / Received: 8.

3. Dropped, with both stray blank lines. server/src/config.ts at this head hashes identical to 26ef8bc.

insights-scope.test.ts matched ids by prefix (loop:s-in:, spend:b-in), which the new shape breaks, so those four assertions had to move either way.

One change you didn't ask for, and can refuse. That file's fixture built source_app with project.split("/").pop(), which does not split a Windows path; I switched it to basename so an app-anchored assertion is not green on CI and red on a Windows checkout. That swap is independent of the requested fix — say the word and I will revert it or split it out.

A separate Windows finding. insights-scope.test.ts > "a loop whose turn ran inside the scope (via cwd) is included" fails on unmodified 26ef8bc on Windows. Line 53 builds the cwd as SCOPED + "/wt/x" while isWithin compares with the platform sep. A reduced probe, paths abbreviated:

sep      "\"
mixed    "…\scoped/wt/x"  ->  false
native   "…\scoped\wt\x"  ->  true     (control)
outside  "…\other\wt"     ->  false    (control)

ci.yml currently runs on ubuntu-latest only, so this case is not exercised there. I will open it separately with the failing case rather than fold it in here.

Validation. bun run typecheck in server/ and web/, bunx vite build, and the full server suite on both 26ef8bc and this branch on the same machine. The five files that touch insights fail on the same five test titles in both runs, and this branch adds three passing tests. The full suite is flaky on Windows in the git and network tests, so I compared failure titles rather than totals.

All three points check out, and I re-ran each one rather than reading the diff — same standard the first review held itself to.

1. The fixture collides now, and the test earns its name. Reverting only server/src/insights.ts to its state on the base commit, leaving your new test in place:

Expected to contain: "app-beta:s-shared"
Received: [ "app-alpha:s-shared" ]
2 fail, 1 pass

With insights.ts restored: 3 pass. That is the direction the earlier version never had.

2. encodeURIComponent is load-bearing, and I checked that specifically — it is the kind of thing that looks like defensive noise, so it is worth knowing which. Keeping your fix but degrading rowId to a plain `${app}:${sid}`:

Expected: 9
Received: 8

Exactly the number you reported. The third test fails alongside it at Expected: 2 / Received: 1. So both the encoding and the fixture pair that pins it are doing real work.

3. normalize() is gone. server/src/config.ts at this head is byte-identical to 26ef8bc, and the PR now touches three files. Both stray blank lines are gone with it.

On the id format change, which nobody asked about but is the thing that would bite later: I checked whether anything consumes an insight id as structure rather than as an opaque value. Nothing does — the only use is key={i.id} in Alerts.tsx, and no caller splits, slices or parses it. So re-encoding the halves is free.

The basename swap: keep it. You flagged it as unrequested and offered to revert. It belongs — an app-anchored assertion that is green on CI and red on a Windows checkout is a worse trade than a one-line fixture change, and it is in the same file as the assertions you had to move anyway.

And please do open the Windows isWithin finding separately. That is the right call and the probe you posted is the useful part of it. ci.yml runs ubuntu-latest only, so nothing here would have caught it.

Also ran: bun run typecheck clean, and all three insights suites at 12 pass / 0 fail.

CI on this PR is currently red for an unrelated reason — main was broken by an unpinned bun-version: latest picking up 1.4.0, fixed in #535 and now merged. I am updating this branch onto it so the run reflects your diff. Nothing here is waiting on you.

Thanks for the thorough second pass — the reproduction notes in your reply made this quick to verify.

Generated by Claude Code