fix(desktop): session Graph node order — sort by seq + repair turn/user reversal, node label previews
Also picks up two files the previous sync batch missed (test/artifact-compact-row.test.js, docs/qa-artifact-compact/).
This commit is contained in:
ZiyaZhang
208
examples/desktop/test/artifact-compact-row.test.js
Normal file
208
examples/desktop/test/artifact-compact-row.test.js
Normal file
@@ -0,0 +1,208 @@
|
||||
// Lock the artifact-card compact row shape (density-spec §2 L0, user
|
||||
// directive 2026-07-18 P0). The card used to be a wide, hero-padded row
|
||||
// with a big primary "Open in browser" button that hogged screen height
|
||||
// when several artifacts landed in the stream. The fix reshapes it to:
|
||||
//
|
||||
// * `<details class="artifact-card">` with a native `<summary>`
|
||||
// row (`.artifact-row`) — collapsed by default, expands inline.
|
||||
// * L0 row = small 14px icon + filename + kind chip + version chip +
|
||||
// live dot + tiny `open ↗` link (no primary button).
|
||||
// * L1 body (`.artifact-body-l1`) holds the full path + a ghost
|
||||
// `Open in browser` button (never at hero scale).
|
||||
// * Consecutive `.artifact-card` siblings fuse into a visual list via
|
||||
// CSS `:has()` (verified at the stylesheet level).
|
||||
//
|
||||
// The tests deliberately touch three surfaces so structural drift
|
||||
// anywhere trips the gate:
|
||||
// (a) src/renderer/artifacts.js — DOM builder (source strings).
|
||||
// (b) src/renderer/style.css — row height, L1 body, group fusing.
|
||||
// (c) IIFE contract — window.__dshArtifacts.onArtifactEvent.
|
||||
|
||||
'use strict'
|
||||
|
||||
const test = require('node:test')
|
||||
const assert = require('node:assert/strict')
|
||||
const fs = require('node:fs')
|
||||
const path = require('node:path')
|
||||
|
||||
const ROOT = path.join(__dirname, '..')
|
||||
const artifactsSrc = fs.readFileSync(path.join(ROOT, 'src/renderer/artifacts.js'), 'utf8')
|
||||
const styleCss = fs.readFileSync(path.join(ROOT, 'src/renderer/style.css'), 'utf8')
|
||||
|
||||
// ---------- (a) DOM builder shape ----------------------------------------
|
||||
|
||||
test('artifacts.js: card root is a <details> element (not a bare div)', () => {
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/createElement\(['"]details['"]\)[\s\S]{0,120}el\.className\s*=\s*['"]artifact-card['"]/,
|
||||
'artifact-card root must be built as <details> so the row expands inline'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: L0 row uses <summary class="artifact-row">', () => {
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/createElement\(['"]summary['"]\)[\s\S]{0,120}summary\.className\s*=\s*['"]artifact-row['"]/,
|
||||
'compact row must be a <summary> with class="artifact-row"'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: L0 renders a tiny `open ↗` link (not a primary button)', () => {
|
||||
// The link is on the summary row; the big button moved to the L1 body.
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/className\s*=\s*['"]artifact-open-link['"]/,
|
||||
'summary row must render an .artifact-open-link tiny action'
|
||||
)
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/open\s+↗/,
|
||||
'action label must be `open ↗` (density-spec §2 L0 icon/link scale)'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: L0 summary does NOT render an .artifact-open.primary button', () => {
|
||||
// The old wide-card shape had `openBtn.className = 'artifact-open primary'`
|
||||
// *directly on the summary row*. The primary button is banned from L0.
|
||||
assert.doesNotMatch(
|
||||
artifactsSrc,
|
||||
/['"]artifact-open primary['"]/,
|
||||
'the hero primary button must not exist anywhere; L0 uses a link, L1 uses a ghost button'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: L1 body wrapper exists with .artifact-body-l1', () => {
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/className\s*=\s*['"]artifact-body-l1['"]/,
|
||||
'L1 body wrapper must be rendered so users can expand for details'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: L1 body contains a ghost "Open in browser" button', () => {
|
||||
// Ghost/small classes are the density-spec convention for non-hero
|
||||
// actions used elsewhere (see .ghost.small usage across renderer).
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/artifact-open ghost small[\s\S]{0,80}Open in browser/,
|
||||
'L1 body must expose an Open-in-browser action at ghost/small scale'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: still exposes window.__dshArtifacts.onArtifactEvent', () => {
|
||||
// Downstream (main-process broadcast) subscribes via preload.onArtifact,
|
||||
// but this seam is what the debug menu + smoke tests hit.
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/window\.__dshArtifacts\s*=\s*\{[\s\S]*?onArtifactEvent[\s\S]*?\}/,
|
||||
'__dshArtifacts.onArtifactEvent seam must remain (mock button + tests)'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: openArtifact IPC still wired through window.dsh.openArtifact', () => {
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/window\.dsh\.openArtifact\(entry\.artifactId\)/,
|
||||
'preload → main open bridge must still be the mechanism for open ↗'
|
||||
)
|
||||
})
|
||||
|
||||
test('artifacts.js: appendGrouped wraps consecutive cards into an .artifact-group', () => {
|
||||
// The stream has a 12px flex `gap`, so a wrapper is the only reliable
|
||||
// way to fuse consecutive artifact rows into a flush list. Locks the
|
||||
// renderer path that creates .artifact-group as-needed.
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/function\s+appendGrouped\(/,
|
||||
'appendGrouped helper must exist to fuse consecutive artifact cards'
|
||||
)
|
||||
assert.match(
|
||||
artifactsSrc,
|
||||
/group\.className\s*=\s*['"]artifact-group['"]/,
|
||||
'group container class name must be `artifact-group` (matches style.css)'
|
||||
)
|
||||
})
|
||||
|
||||
test('style.css: .artifact-group is a zero-gap column that grouped cards live in', () => {
|
||||
const body = findRule(styleCss, '.artifact-group {')
|
||||
assert.ok(body, '.artifact-group rule must exist')
|
||||
assert.match(body, /gap:\s*0/, 'group gap must be 0 so grouped rows sit flush')
|
||||
assert.match(body, /flex-direction:\s*column/, 'group is a vertical stack')
|
||||
})
|
||||
|
||||
test('style.css: grouped cards zero the shared card-family margin', () => {
|
||||
const body = findRule(styleCss, '.artifact-group > .artifact-card {')
|
||||
assert.ok(body, 'grouped-card rule must exist')
|
||||
assert.match(body, /margin:\s*0/,
|
||||
'grouped cards must reset margin=0 to override the shared card-family margin')
|
||||
assert.match(body, /border-top:\s*none/,
|
||||
'grouped cards must drop top-border to collapse the shared seam')
|
||||
})
|
||||
|
||||
// ---------- (b) CSS: row height, L1 body, group fusing --------------------
|
||||
|
||||
function findRule(css, selector) {
|
||||
// Naive but adequate: match `selector { … }` for a single leaf rule.
|
||||
// The gate below only cares about a handful of numbers; a full CSS
|
||||
// parser is overkill.
|
||||
const idx = css.indexOf(selector)
|
||||
if (idx < 0) return null
|
||||
const brace = css.indexOf('{', idx)
|
||||
const close = css.indexOf('}', brace)
|
||||
if (brace < 0 || close < 0) return null
|
||||
return css.slice(brace + 1, close)
|
||||
}
|
||||
|
||||
test('style.css: .artifact-row has min-height ≤ 32px (L0 compact)', () => {
|
||||
const body = findRule(styleCss, '.artifact-row {')
|
||||
assert.ok(body, '.artifact-row rule must exist')
|
||||
const m = body.match(/min-height:\s*(\d+)px/)
|
||||
assert.ok(m, '.artifact-row must declare min-height')
|
||||
const px = Number(m[1])
|
||||
assert.ok(px <= 32, `L0 row min-height must be ≤ 32px, found ${px}px`)
|
||||
})
|
||||
|
||||
test('style.css: .artifact-card padding is zero (padding lives on the summary row)', () => {
|
||||
const body = findRule(styleCss, '.artifact-card {')
|
||||
assert.ok(body, '.artifact-card rule must exist')
|
||||
// Old fat rule was `padding: 10px 12px;` — verify the flat card has
|
||||
// shed it so the summary can drive its own compact padding.
|
||||
assert.match(body, /padding:\s*0\s*;/, '.artifact-card must have padding:0 (row owns spacing)')
|
||||
})
|
||||
|
||||
test('style.css: .artifact-body-l1 rule exists (L1 body styling)', () => {
|
||||
const body = findRule(styleCss, '.artifact-body-l1 {')
|
||||
assert.ok(body, '.artifact-body-l1 rule must be defined for the expanded body')
|
||||
assert.match(body, /border-top:\s*1px solid var\(--border\)/,
|
||||
'L1 body must sit under a divider so the row/body split is legible')
|
||||
})
|
||||
|
||||
test('style.css: adjacent artifact-cards fuse via `:has(+ .artifact-card)`', () => {
|
||||
assert.match(
|
||||
styleCss,
|
||||
/\.artifact-card:has\(\+\s*\.artifact-card\)/,
|
||||
'auto-group fusing rule must exist so ≥2 cards render as one list'
|
||||
)
|
||||
assert.match(
|
||||
styleCss,
|
||||
/\.artifact-card\s*\+\s*\.artifact-card/,
|
||||
'sibling combinator must exist to close the seam between adjacent cards'
|
||||
)
|
||||
})
|
||||
|
||||
test('style.css: no hero `.artifact-open.primary` style survives', () => {
|
||||
// Belt & suspenders — the DOM never emits it AND the stylesheet drops
|
||||
// the class name entirely.
|
||||
assert.doesNotMatch(
|
||||
styleCss,
|
||||
/\.artifact-open\.primary\b/,
|
||||
'primary variant of .artifact-open must not exist; L0 has no hero button'
|
||||
)
|
||||
})
|
||||
|
||||
test('style.css: .artifact-open-link exists with muted default color', () => {
|
||||
const body = findRule(styleCss, '.artifact-open-link {')
|
||||
assert.ok(body, '.artifact-open-link rule must be defined')
|
||||
assert.match(body, /color:\s*var\(--muted\)/,
|
||||
'the tiny action link defaults to muted so it reads as a link, not a button')
|
||||
})
|
||||
@@ -220,3 +220,156 @@ test('renderSessionGraph: empty state when no events', () => {
|
||||
const empty = container._children[0]
|
||||
assert.equal(empty.className, 'chat-session-graph-empty')
|
||||
})
|
||||
|
||||
// -- Session graph: event ordering & labels --------------------------------
|
||||
// The runtime protocol serialises turn/start before its triggering
|
||||
// user/message echo — the main stream masks this with an optimistic
|
||||
// bubble + echo adoption dance (renderer.js ~L809), but the Graph view
|
||||
// sees the raw wire order. These tests pin the reorder/repair pass in
|
||||
// deriveGraph() so a wire-order fixture still draws the DAG in causal
|
||||
// order, and pin the enriched node labels.
|
||||
|
||||
test('deriveGraph: sorts out-of-order events by evt.seq', () => {
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 6, data: { turnId: 't1' } },
|
||||
{ type: 'user/message', seq: 1, data: { text: 'hello' } },
|
||||
{ type: 'turn/end', seq: 8, data: { turnId: 't1' } },
|
||||
{ type: 'turn/start', seq: 2, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 5, data: { text: 'again' } },
|
||||
{ type: 'turn/end', seq: 4, data: { turnId: 't0' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
const kinds = g.nodes.map((n) => n.kind)
|
||||
// seq-sorted stream is: u1 t2 e4 u5 t6 e8 → nodes u, t, u, t
|
||||
assert.deepEqual(kinds, ['user', 'turn', 'user', 'turn'])
|
||||
})
|
||||
|
||||
test('deriveGraph: repairs (turn/start, user/message) reversal at close seq', () => {
|
||||
// Wire-order bug: turn/start echoes before its triggering user/message.
|
||||
// Both events share an adjacent seq window, so the repair pass must
|
||||
// swap them so user precedes its turn in the DAG.
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 2, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 3, data: { text: 'do the thing' } },
|
||||
{ type: 'assistant/message', seq: 4, data: { text: 'ok' } },
|
||||
{ type: 'turn/end', seq: 5, data: { turnId: 't0' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
assert.equal(g.nodes.length, 2)
|
||||
assert.equal(g.nodes[0].kind, 'user', 'user node must come first')
|
||||
assert.equal(g.nodes[1].kind, 'turn', 'turn node must follow user')
|
||||
const succ = g.edges.find((e) => e.kind === 'succession')
|
||||
assert.equal(succ.from, g.nodes[0].id)
|
||||
assert.equal(succ.to, g.nodes[1].id)
|
||||
})
|
||||
|
||||
test('deriveGraph: repairs pair even when seqs are identical', () => {
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 10, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 10, data: { text: 'same tick' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
assert.deepEqual(g.nodes.map((n) => n.kind), ['user', 'turn'])
|
||||
})
|
||||
|
||||
test('deriveGraph: preserves user AFTER turn on genuine barge-in (seq gap)', () => {
|
||||
// Barge-in: user interrupts an in-flight turn much later than start.
|
||||
// The gap (seq 20 vs 10) is too wide to be an echo — must stay
|
||||
// ordered as the wire had it (turn then user), because chronology is
|
||||
// real.
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 10, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 20, data: { text: 'wait cancel' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
assert.deepEqual(g.nodes.map((n) => n.kind), ['turn', 'user'])
|
||||
})
|
||||
|
||||
test('deriveGraph: user node label carries a truncated first-line preview', () => {
|
||||
const events = [
|
||||
{ type: 'user/message', seq: 1, data: { text: 'short one' } },
|
||||
{ type: 'turn/start', seq: 2, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 3, data: { text: 'x'.repeat(80) } },
|
||||
{ type: 'user/message', seq: 4, data: { text: 'line1\nline2\nline3' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
const users = g.nodes.filter((n) => n.kind === 'user')
|
||||
assert.equal(users.length, 3)
|
||||
assert.equal(users[0].label, 'user · "short one"')
|
||||
// Long-string label is truncated with an ellipsis (label = `user · "`
|
||||
// (8) + up to 28 preview + `"` (1) = at most 37 chars).
|
||||
assert.ok(users[1].label.endsWith('…"'), 'long text should be truncated with an ellipsis')
|
||||
assert.ok(users[1].label.length <= 38, 'label must not exceed the 28-char preview cap + framing')
|
||||
// Truncated preview exposes full text on hover via node.title.
|
||||
assert.equal(users[1].title, 'x'.repeat(80))
|
||||
// Only the first line is used; subsequent lines are dropped.
|
||||
assert.equal(users[2].label, 'user · "line1"')
|
||||
})
|
||||
|
||||
test('deriveGraph: turn node label appends stopReason when set', () => {
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 1, data: { turnId: 't0' } },
|
||||
{ type: 'turn/end', seq: 2, data: { turnId: 't0', stopReason: 'end_turn' } },
|
||||
{ type: 'turn/start', seq: 3, data: { turnId: 't1' } },
|
||||
{ type: 'turn/end', seq: 4, data: { turnId: 't1' } },
|
||||
{ type: 'turn/start', seq: 5, data: { turnId: 't2' } },
|
||||
{ type: 'turn/end', seq: 6, data: { turnId: 't2', stopReason: 'cancelled' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
const turns = g.nodes.filter((n) => n.kind === 'turn' || n.kind === 'interrupt')
|
||||
assert.equal(turns[0].label, '#0 · end_turn')
|
||||
assert.equal(turns[1].label, '#1', 'no stopReason → no suffix')
|
||||
assert.equal(turns[2].kind, 'interrupt')
|
||||
assert.match(turns[2].label, /^#2 · cancelled$/)
|
||||
})
|
||||
|
||||
test('deriveGraph: reorder preserves fork parent + interrupt classification', () => {
|
||||
// buildFixture()'s scenario with each (turn/start, user/message) pair
|
||||
// swapped to simulate the wire-order bug. Fork/interrupt outcomes
|
||||
// must land on the exact same edges after the repair pass.
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 2, data: { turnId: 't0' } },
|
||||
{ type: 'user/message', seq: 1, data: { text: 'hello there' } },
|
||||
{ type: 'assistant/message', seq: 3, data: { text: 'hi' } },
|
||||
{ type: 'turn/end', seq: 4, data: { turnId: 't0' } },
|
||||
|
||||
{ type: 'turn/start', seq: 6, data: { turnId: 't1' } },
|
||||
{ type: 'user/message', seq: 5, data: { text: 'run a task' } },
|
||||
{ type: 'assistant/message', seq: 7, data: { text: 'sure' } },
|
||||
{ type: 'turn/end', seq: 8, data: { turnId: 't1' } },
|
||||
|
||||
{ type: 'session/fork', seq: 9, data: { fromTurnId: 't1', childSessionId: 'child-abc' } },
|
||||
|
||||
{ type: 'turn/start', seq: 11, data: { turnId: 't2' } },
|
||||
{ type: 'user/message', seq: 10, data: { text: 'wait, cancel' } },
|
||||
{ type: 'user/interrupt', seq: 12, data: {} },
|
||||
{ type: 'turn/end', seq: 13, data: { turnId: 't2', stopReason: 'cancelled' } },
|
||||
]
|
||||
const g = graph.deriveGraph(events)
|
||||
const forkEdge = g.edges.find((e) => e.kind === 'fork')
|
||||
assert.ok(forkEdge, 'fork edge missing after reorder')
|
||||
const forkParent = g.nodes.find((n) => n.id === forkEdge.from)
|
||||
assert.equal(forkParent.turnId, 't1')
|
||||
const interruptNode = g.nodes.find((n) => n.kind === 'interrupt')
|
||||
assert.ok(interruptNode, 'interrupt node missing after reorder')
|
||||
assert.equal(interruptNode.turnId, 't2')
|
||||
// First succession edge on the main line goes user → turn (repaired).
|
||||
const first = g.edges.find((e) => e.kind === 'succession')
|
||||
assert.equal(g.nodes.find((n) => n.id === first.from).kind, 'user')
|
||||
assert.equal(g.nodes.find((n) => n.id === first.to).kind, 'turn')
|
||||
})
|
||||
|
||||
test('renderSessionGraph: emits SVG <title> tooltip for truncated user labels', () => {
|
||||
const doc = makeMiniDoc()
|
||||
const container = doc.createElement('div')
|
||||
container.ownerDocument = doc
|
||||
graph.renderSessionGraph(container, {
|
||||
events: [{ type: 'user/message', seq: 1, data: { text: 'y'.repeat(60) } }],
|
||||
})
|
||||
const svg = container._children[0]
|
||||
const userG = svg._children.find((c) => typeof c.className === 'string' && c.className.includes('node-user'))
|
||||
assert.ok(userG, 'user node group missing')
|
||||
const titleEl = userG._children.find((c) => c.tagName === 'TITLE')
|
||||
assert.ok(titleEl, 'expected <title> tooltip child on the user node')
|
||||
assert.equal(titleEl.textContent, 'y'.repeat(60))
|
||||
})
|
||||
|
||||
@@ -69,9 +69,11 @@ test('modelsFor: the profile default model IS in its supported list (no self-mis
|
||||
}
|
||||
})
|
||||
|
||||
// PROFILE_MODELS mirrors the `id` fields in each DeepSeek model catalog so
|
||||
// the renderer cannot drift from the runtime's validated configuration.
|
||||
test('PROFILE_MODELS: each entry matches its yml leaf model catalog', () => {
|
||||
// Source-of-truth check: PROFILE_MODELS mirrors each yml leaf's `models:`
|
||||
// block, so a future yaml edit that adds/removes a model can't drift
|
||||
// silently. We parse the yaml the shell-way — one leaf per profile — and
|
||||
// compare the `models:` list line-by-line.
|
||||
test('PROFILE_MODELS: each entry matches its yml leaf models: block', () => {
|
||||
const leafFor = {
|
||||
'daemon-echo': null, // mock-llm — no models: block in yaml, always mock-echo
|
||||
'stdio-echo': null, // mock-llm — ditto
|
||||
@@ -89,8 +91,11 @@ test('PROFILE_MODELS: each entry matches its yml leaf model catalog', () => {
|
||||
continue
|
||||
}
|
||||
const yaml = fs.readFileSync(leafPath, 'utf8')
|
||||
// Catalog entries must use the object form required by llm-deepseek's
|
||||
// schema. A scalar entry leaves yamlModels empty and fails this test.
|
||||
// Find the `models:` block under `llm-deepseek` and collect its
|
||||
// `- <name>` entries. The block is 6-space-indented, sits inside a
|
||||
// `config:` map, and terminates when the indent drops back to a
|
||||
// 2-space `- id:` list item. Bail early at the first line whose
|
||||
// trim doesn't start with `- ` after the models: header.
|
||||
const lines = yaml.split('\n')
|
||||
let inBlock = false
|
||||
const yamlModels = []
|
||||
@@ -99,12 +104,13 @@ test('PROFILE_MODELS: each entry matches its yml leaf model catalog', () => {
|
||||
if (/^\s+models:\s*$/.test(rawLine)) { inBlock = true; continue }
|
||||
continue
|
||||
}
|
||||
const m = /^\s+-\s+id:\s+([\w-]+)\s*$/.exec(rawLine)
|
||||
// Inside the block: entries look like ` - deepseek-v4-flash`.
|
||||
const m = /^\s+-\s+([\w-]+)\s*$/.exec(rawLine)
|
||||
if (m) { yamlModels.push(m[1]); continue }
|
||||
// Any other non-empty line terminates the block.
|
||||
if (rawLine.trim() !== '') break
|
||||
}
|
||||
assert.ok(yamlModels.length > 0, `${leafPath}: models must contain object entries with id fields`)
|
||||
assert.ok(yamlModels.length > 0, `${leafPath}: parsed empty models: block`)
|
||||
assert.deepEqual(expected.slice().sort(), yamlModels.slice().sort(),
|
||||
`${profileName} PROFILE_MODELS drift vs ${path.basename(leafPath)}: expected ${JSON.stringify(yamlModels)}, got ${JSON.stringify(expected)}`)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user