mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
* feat(web): make chat telemetry name the run it is talking about `setChatCorrelation()` has had no production caller since #7518. Every `client_chat_*` event therefore shipped with an empty correlation block, and `client_chat_interaction_latency` — the one chat observability event with real traffic (2,694 events / 30 days) — derives its `streaming` flag from `chatCorrelation().run_id`. With no producer the flag was false on every event ever sent, so the panel read "every chat stall happens while the app sits idle": a self-consistent, wrong story, and the reason the metric was worse than no metric. Wire the four producers that already had their values in hand: - run id + agent id, opened at every run start (`streamViaDaemon`, the next physical run of a strategy-task chain, and `reattachDaemonRun` — the path a page refresh takes back onto a run still in flight) and closed in the same `finally` that settles the stuck-run watchdog. Named helpers, because the pair is load-bearing: only an opener and `streaming` is stuck true forever; only a closer and it is stuck false. - conversation id + project id, once `listMessages` resolves. - release channel, off `/api/version` — the daemon has always answered with `channel` and the web read `version` and dropped the rest. - replay session id, handed over from the posthog-js instance inside `loaded()`. This is the only possible source: the ESM build never publishes itself as `window.posthog`, so the global-lookup fallback returns undefined forever while looking implemented. No new events. This only makes the traffic already being sent tell the truth. `model_id` stays absent — it is not in scope at either run-start site, and a guess would be worse than the gap. * feat(analytics): record which failure sentence the user actually read `run_failed_toast` says a user was shown a failure card and names the daemon's `error_code`. It cannot say what the user READ — those two facts are separated by a mapping table, and `resolveRunErrorCardDescription` states in as many words that the table can always be one row short. When it is, the card falls back to a blank "the task failed" with no diagnosis. That is the worst impression this surface produces, and it was the one we could not count. Add two fields to the exposure: - `message_key` — the i18n key of the mapped copy, or `generic_fallback` when the table had no line for it. Always present, never omitted: the fallback RATE is the number that matters, and a rate needs a denominator, so an omitted key would drop the fallback impressions out of the very count meant to measure them. - `failure_category` — the daemon's own classification off the run's error event, defaulting to the enum's existing `unknown` member rather than vanishing, so this event stays joinable against `run_finished` and `run_recovery_action`. `message_key` is typed as `string`, not the web's `RunFailureMessageKey` union: that union lives in apps/web and grows with every copy change, and pinning it in contracts would make adding a sentence a contracts change. * feat(web): attach the chat-health monitor to the chat log `chat-health.ts` has answered four questions since #7518 — how long until the user can read the conversation, is the DOM growing without bound, who is about to OOM, how janky is the UI while a run streams — and nothing but its own unit spec has ever called it. All four tiles were empty in production. Open a surface on the `.chat-log` element while the Chat tab is showing it, and tear it down when it goes away. Two effect dependencies, each guarding a real failure: - `tab`, because the log is conditionally rendered and `logRef.current` is null on the Comments tab — without it, coming back to Chat never reattaches. - `activeConversationId`, because that div carries no conversation key, so React reuses the very same DOM node across a switch. There is no node-level signal that the surface should be replaced; the id is the only one. The spec pins this by asserting node identity is unchanged AND that a second surface opened anyway. `markFirstPaint` lands together with `setStreamEventCount`, deliberately: a first-paint duration on its own cannot be attributed, because a slow open is either "many messages" or "one message with hundreds of tool events behind it", and only the event count separates them. Rendered-row count uses the same `:scope > *` definition `client_chat_dom_growth` already uses, so the two events are comparable. Run start / end feed the jank window from the daemon provider, which is where the authoritative run lifecycle lives. Both no-op when no chat surface is mounted. Also extracts `isChatVirtualized()` so the renderer and the telemetry read the same predicate. Two copies of `items.length > THRESHOLD` are identical today and drift the first time the rule grows a second term — at which point the telemetry would describe a render mode the renderer no longer uses, silently. `markChatOpenIntent` is NOT wired here. Hanging it off the two conversation handlers would only cover `conversation_switch`; `project_switch` has 10+ navigate sites in App.tsx and `cold_boot` passes through no handler at all. The right home is `router.ts`, which is the single navigation entry point and can derive the open kind from the route transition. That is its own change. Until then `open_kind` honestly reads `remount` rather than a fabricated `cold_boot`. * fix(web): stop the wiring from reintroducing the bug it removes Three review findings, all real, all the same family: an event that cannot name what it measured. 1. `client_chat_stream_health` was going out with an empty `run_id`. `chatSurfaceRunEnded()` does not merely bookkeep — it FLUSHES the jank window, and the flush spreads `chatCorrelation()`. The terminal path cleared the correlation one line earlier, so the one event this PR newly enables shipped anonymous: the exact defect the PR exists to remove, reintroduced by the fix for it. One rule now covers both ends of a run: the chat-health call goes FIRST, because it is the one that can emit; the correlation mutation goes second. Applied symmetrically at the three run-start sites too — `runStarted()` flushes any window a previous run left open (its terminal event never arrived), and that flush belongs to the OLD run, so it has to happen before the block is repointed. `trackRunTerminal` is unaffected either way; it carries its own context and never reads this block. 2. A surface opened mid-run did not know a run was running. Surfaces are recreated on every conversation switch and every return to the Chat tab, but `chatSurfaceRunStarted` only ever reached the surface that existed when the provider fired it. The new surface kept its window shut and dropped every long task until the next run start — precisely the stretch a user is watching, because they came back to the tab to see something generate. `openChatSurface` now adopts the in-flight run. It reads it from the correlation context rather than keeping a second flag: `chat-interaction.ts` already derives its `streaming` breakdown the same way, and one source cannot disagree with itself. No double counting — `runCount` and the window clock are per-surface, and adoption only ever runs against the surface constructed a line above it. The spec pins this with three long tasks split across a mid-run swap and asserts they add up to three, not four. 3. `conversation_id` / `project_id` moved from ProjectView to the ChatPane boundary. ChatPane has three hosts — ProjectView, DesignSystemFlow and workspace/SideChatTab — feeding one set of observability modules and one dashboard. Setting it in a single host left the other two emitting orphan rows indistinguishable from genuinely un-correlated ones. Both already pass the ids in as props. The effect is declared ahead of the surface-open effect so the opening `conversation_open` sample is already stamped.
116 lines
3.9 KiB
TypeScript
116 lines
3.9 KiB
TypeScript
// @vitest-environment jsdom
|
|
|
|
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
|
|
|
/**
|
|
* The two boot-time halves of the chat correlation block.
|
|
*
|
|
* `replay_session_id` and `release_channel` are the fields that turn a
|
|
* `client_chat_*` row from a number into something a triager can act on — one
|
|
* opens the session replay, the other separates "the packaged stable build is
|
|
* janky" from "a dev daemon is janky". Both had a producer sitting one line
|
|
* away from a value that was already in hand, and neither was ever called.
|
|
*
|
|
* `registerChatReplaySessionSource` in particular is a trap with no failure
|
|
* mode: we load posthog-js through `await import('posthog-js')`, whose ESM
|
|
* build never publishes itself as `window.posthog`, so the global-lookup
|
|
* fallback silently returns undefined forever and every replay link on the
|
|
* dashboard is dead while the code reads as implemented.
|
|
*/
|
|
|
|
let lastRegisterPayload: Record<string, unknown> | null = null;
|
|
|
|
vi.mock('posthog-js', () => {
|
|
const stub = {
|
|
init: (_key: string, config: Record<string, unknown>) => {
|
|
const loaded = config.loaded as ((i: unknown) => void) | undefined;
|
|
loaded?.(stub);
|
|
return stub;
|
|
},
|
|
register: (payload: Record<string, unknown>) => {
|
|
lastRegisterPayload = payload;
|
|
},
|
|
setPersonProperties: () => undefined,
|
|
opt_in_capturing: () => undefined,
|
|
opt_out_capturing: () => undefined,
|
|
reset: () => undefined,
|
|
identify: () => undefined,
|
|
get_session_id: () => 'replay-sess-1',
|
|
};
|
|
return { default: stub };
|
|
});
|
|
|
|
const ORIGINAL_FETCH = globalThis.fetch;
|
|
|
|
beforeEach(() => {
|
|
lastRegisterPayload = null;
|
|
vi.resetModules();
|
|
globalThis.fetch = vi.fn(async (input: RequestInfo | URL) => {
|
|
const url =
|
|
typeof input === 'string' ? input : input instanceof URL ? input.href : input.url;
|
|
if (url.endsWith('/api/analytics/config')) {
|
|
return new Response(
|
|
JSON.stringify({
|
|
enabled: true,
|
|
env: 'local_development',
|
|
key: 'phc_test_key',
|
|
host: 'https://us.i.posthog.com',
|
|
installationId: 'install-123',
|
|
}),
|
|
{ status: 200, headers: { 'content-type': 'application/json' } },
|
|
);
|
|
}
|
|
if (url.endsWith('/api/version')) {
|
|
return new Response(
|
|
JSON.stringify({
|
|
version: {
|
|
version: '1.2.3',
|
|
channel: 'prerelease',
|
|
packaged: true,
|
|
platform: 'darwin',
|
|
arch: 'arm64',
|
|
},
|
|
}),
|
|
{ status: 200, headers: { 'content-type': 'application/json' } },
|
|
);
|
|
}
|
|
return new Response('not found', { status: 404 });
|
|
}) as unknown as typeof fetch;
|
|
});
|
|
|
|
afterEach(() => {
|
|
globalThis.fetch = ORIGINAL_FETCH;
|
|
vi.restoreAllMocks();
|
|
});
|
|
|
|
describe('chat correlation — boot-time identity', () => {
|
|
it('takes the replay session id from the posthog instance, not from a global', async () => {
|
|
const { getAnalyticsClient } = await import('../../src/analytics/client');
|
|
const { chatCorrelation } = await import('../../src/observability/chat-context');
|
|
|
|
// No `window.posthog` on purpose: that is exactly the shape production
|
|
// runs in, and the shape the global-lookup fallback cannot serve.
|
|
expect((globalThis as { posthog?: unknown }).posthog).toBeUndefined();
|
|
|
|
await getAnalyticsClient({
|
|
anonymousId: 'anon-1',
|
|
sessionId: 'sess-1',
|
|
clientType: 'web',
|
|
locale: 'en',
|
|
appVersion: '1.2.3',
|
|
});
|
|
|
|
expect(lastRegisterPayload).not.toBeNull();
|
|
expect(chatCorrelation().replay_session_id).toBe('replay-sess-1');
|
|
});
|
|
|
|
it('keeps the release channel the daemon already answers with', async () => {
|
|
const { resolveAppVersionForCapture } = await import('../../src/analytics/provider');
|
|
const { chatCorrelation } = await import('../../src/observability/chat-context');
|
|
|
|
await resolveAppVersionForCapture('0.0.0');
|
|
|
|
expect(chatCorrelation().release_channel).toBe('prerelease');
|
|
});
|
|
});
|