Conversation
Parse diagnosis results into structured cards with severity badges, fix options, checklist checkboxes, and export (Markdown/JSON). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf4da5dbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {msg.diagnosisReport && msg.diagnosisReport.items.length > 0 && ( | ||
| <div className="mt-2"> | ||
| <DiagnosisCard items={msg.diagnosisReport.items} /> |
There was a problem hiding this comment.
Populate diagnosisReport before rendering DiagnosisCard
This UI branch is currently unreachable because no message source sets diagnosisReport: useDoctorAgent only binds doctor:chat-delta, doctor:chat-final, doctor:invoke, and doctor:invoke-result (see src/lib/use-doctor-agent.ts around lines 123-227), so every DoctorChatMessage created there lacks this field. As a result, the new structured diagnosis cards never render even when the backend emits diagnosis data.
Useful? React with 👍 / 👎.
| let summary = result | ||
| .summary | ||
| .clone() | ||
| .unwrap_or_else(|| format!("诊断完成,发现 {} 个问题。", result.items.len())); |
There was a problem hiding this comment.
Make diagnosis fallback summary locale-aware
When a parsed diagnosis JSON omits summary (which parse_diagnosis_result explicitly allows), this path emits a hardcoded Chinese chat-final message (诊断完成,发现 ...). That causes mixed-language responses for non-Chinese sessions and breaks the language-consistency behavior enforced elsewhere in the adapter. The fallback summary should be derived from the active language context rather than fixed Chinese text.
Useful? React with 👍 / 👎.
Keith-CY
left a comment
There was a problem hiding this comment.
Blocking issues found:
-
Runtime event bridge is missing
DiagnosisReporthandling (Rust compile/dispatch break).src-tauri/src/runtime/types.rsaddsRuntimeEvent::DiagnosisReport, butsrc-tauri/src/doctor_runtime_bridge.rsstill has exhaustive matches only forChatDelta/ChatFinal/Invoke/Error/Status. With this PR applied,map_runtime_event_name/emit_runtime_eventwill fail to compile unless the new variant is handled, and even if compiled it won’t be emitted to the frontend. -
Structured diagnosis parsing is added only in non-streaming paths (
ZeroclawDoctorAdapter::start/send), but the Doctor command path uses streaming methods (start_streaming/send_streaming) insrc-tauri/src/doctor_commands.rsandrun_zeroclaw_streaming_turnonly passesparse_tool_intent. Real diagnosis traffic will never produceDiagnosisReport, so the new card feature is effectively inactive. -
Frontend listeners never consume diagnosis report events.
src/lib/use-doctor-agent.tscurrently subscribes only todoctor:chat-delta,doctor:chat-final,doctor:invoke, etc.; nodoctor:diagnosis-reporthandling exists. ThereforeDiagnosisCardinsrc/components/DoctorChat.tsxwon’t receivemsg.diagnosisReportin normal flow even if backend produced it.
Address review feedback on PR #39: 1. doctor_runtime_bridge.rs: add DiagnosisReport arm to both map_runtime_event_name and emit_runtime_event (was a compile error) 2. use-doctor-agent.ts: add doctor:diagnosis-report event listener that attaches diagnosis data to the most recent assistant message, enabling DiagnosisCard rendering in DoctorChat 3. adapter.rs: replace hardcoded Chinese fallback summary with language-neutral English default
|
Addressed all 3 review findings in ad38ffb: 1. Bridge compile error (P0) — Added 2. Frontend listener missing (P0) — Added 3. Hardcoded Chinese fallback (P1) — Replaced Ready for re-review. |
|
感谢 review!不过仔细检查了一下,三个 blocking issue 在当前代码中其实都已经处理了: 1) Runtime event bridge 已处理
|
dev01lay2
left a comment
There was a problem hiding this comment.
Thanks for the detailed review Keith!
ad38ffb (pushed just after your review) addresses points 1 and 3:
-
Bridge handling —
map_runtime_event_name+emit_runtime_eventnow handleDiagnosisReport. ✅ -
Frontend listener —
use-doctor-agent.tsnow bindsdoctor:diagnosis-reportand attaches the report to the most recent assistant message (or creates a new one). ✅ -
Streaming path — Valid concern, but this is about the streaming methods from #34 (
start_streaming/send_streaming/run_zeroclaw_streaming_turn), which haven't landed on main yet. Currently,doctor_commands.rscallsadapter.start()andadapter.send()(the sync path), and diagnosis parsing is wired into both. When #34 merges, I'll addparse_diagnosisto the streaming path in the same pass. Happy to add a// TODO: wire parse_diagnosis into streaming path (#34)comment in the adapter if you'd like.
Could you re-review at ad38ffb?
Keith-CY
left a comment
There was a problem hiding this comment.
LGTM. No blocking issues found in the current patch. Non-streaming diagnosis parsing and event wiring are implemented correctly on both Rust and frontend, and CI checks are passing.
Summary
Closes #30 — Zeroclaw diagnosis results are now parsed into structured cards instead of plain text.
Changes
Backend (Rust)
DiagnosisSeverity,DiagnosisItem,DiagnosisResulttypes with serde support;parse_diagnosis_result()parser (mirrorsparse_tool_intentpattern);export_diagnosis()for Markdown/JSON export; new testsDiagnosisReport { items: Value }variant toRuntimeEventwithdiagnosis_report()constructorstart()andsend()now tryparse_diagnosis_resultbeforeparse_tool_intent— emits bothChatFinal(summary) andDiagnosisReportevents when structured diagnosis is detectedFrontend (React)
DiagnosisCardwhendiagnosisReportis present on a messageDoctorChatMessagewith optionaldiagnosisReportfieldTesting
parse_diagnosis_result(valid JSON, missing fields, embedded JSON in mixed text)