feat(daemon): add shared UI transcript layer#4328
Conversation
📋 Review SummaryThis PR adds a shared daemon UI layer for web chat/terminal clients, providing typed daemon event normalization, transcript state management, and React bindings through 🔍 General FeedbackPositive aspects:
Architectural observations:
Recurring themes:
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
Reviewer Verification Steps: # 1. Verify SDK daemon UI tests pass
cd packages/sdk-typescript && npx vitest run test/unit/daemonUi.test.ts
# 2. Verify type checking passes
cd packages/sdk-typescript && npx tsc --noEmit
# 3. Verify WebUI builds
cd packages/webui && npm run build
# 4. Verify browser-safe export (check for Node imports in daemon UI bundle)
grep -r "require\('fs'\)\|require\('path'\)\|from 'node:" packages/sdk-typescript/src/daemon/ui/
# 5. Smoke test the daemon import
node -e "import('@qwen-code/sdk/daemon').then(m=>console.log('exports:', Object.keys(m).sort().join(', ')))" |
ceb2ae5 to
94d66e0
Compare
|
Generated by GPT-5 model. Follow-up from the review pass:
Review triage / false positives:
Validation run locally after the amend:
|
94d66e0 to
b3e102b
Compare
b3e102b to
fe77066
Compare
chiga0
left a comment
There was a problem hiding this comment.
Multi-round architectural + correctness + performance review on the shared daemon UI transcript layer. Findings as inline comments anchored to specific lines.
Organized by priority:
P0 (blocking)
normalizer.ts:173—_meta.usagepiggyback forassistant.doneis fragilenormalizer.ts:241—toolCallIdfallback toevent.idcreates phantom tool blockstranscript.ts:345—cloneTranscriptStatedoes full deep clone on every event (perf)transcript.ts:152—getBlockByIdlinear scan on streaming hot pathtranscriptAdapter.ts:139—normalizeToolStatusdefault →'failed'breaks forward-compattranscriptAdapter.ts:151—cancelled→'completed'loses informationtranscriptAdapter.ts:115— error block rendered as assistant message with[System Error]prefix is a UX antipatternsdk-typescript/src/index.ts:14— UI re-exports pollute Node-targeted main entry
P1 (strongly recommend)
DaemonSessionProvider.tsx:256— single context value causes unnecessary re-rendersstore.ts:53— dispatch immediately notifies, no microtask batchingtranscript.ts:289— silent fallback to status when permission block was trimmedwebui/package.json:47—"@qwen-code/sdk": "0.1.7"exact-pin causes monorepo skewbuild.js:131—assertBrowserSafeBundleonly string-scans, no bundle size cap
Direction is right — sinking shared UI primitives (normalizer / reducer / store / terminal sanitization) into SDK is exactly what 02-architectural-decisions.md §8 calls for. Concerns are around concrete implementation: reducer performance under streaming, UX antipatterns in the webui adapter, and main-entry pollution.
Not posted as P0 but worth flagging: zero React component tests (DaemonSessionProvider.tsx 341 LOC, 0 tests) and zero transcriptAdapter.ts tests (159 LOC mapping logic). The 5 existing unit tests cover the reducer happy paths but leave the consumer surfaces entirely uncovered. Recommend adding before next PR builds on this.
Deleted DaemonTuiAdapter.{ts,test.ts} (-1874 LOC) is the right call — spike-only, superseded by PR#4202.
Generated with assistance from Claude Opus 4.7 (claude-opus-4-7) — code references verified against PR head fe77066d.
fe77066 to
feddf8e
Compare
99f2952 to
5f48a92
Compare
|
Generated by GPT-5 model. Latest review pass handled in
Local validation run:
|
5f48a92 to
5532d6c
Compare
Summary
@qwen-code/webui.@qwen-code/sdk/daemonexport shape, and confirmation that native local TUI / ACP / channel / IDE defaults remain untouched.Validation
npm run buildreports an existing VSCode companion lint warning ineditorGroupUtils.tsbut exits successfully.true true.Scope / Risk
@qwen-code/webuinow has an optional dependency on the SDK daemon subpath; the subpath is built as browser-safe ESM to avoid pulling Node-only SDK query/transport code into browser bundles./webapp or full browser UI parity. It provides the shared UI layer those clients should consume.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Related to #3803 and #4175.