feat: render structured Claude Code session detail messages#198
Conversation
# Conflicts: # src/data.js # src/frontend/detail.js # src/frontend/styles.css
|
Merge conflicts have been addressed |
NovakPAai
left a comment
There was a problem hiding this comment.
Review
Overall the approach is solid — extracting regex constants, using Map<tag, def> for hyphenated-tag-to-field mapping, and the cursor-based full-consumption check in parseStructuredFields are all good design decisions. The fallback between two descriptor sets for task_notification is pragmatic. A few things need attention before merge:
HIGH — must fix
[H1] XSS: fields.status embedded in HTML
fields.status originates from session JSONL content (arbitrary text), not from a controlled internal enum. Double-check it is run through escHtml() everywhere it appears in the rendered HTML — including any class attribute positions (e.g. class="status-badge status- + value). A session file with <status>ok" onmouseover="alert(1)</status> would be enough to trigger injection if escHtml is missing at even one call site.
[H2] No tests for the parsing layer
The PR adds ~255 lines of regex-heavy logic with non-trivial correctness requirements: hyphenated-tag-to-field mapping, required: false semantics, two-schema fallback. The project already has a node:test suite. Please add test/claude-structured-parse.test.js covering at minimum: each message type happy path, missing required field → null, optional field absent → success, malformed wrapper → null.
[H3] Nested usage parse failure drops the whole notification
In parseClaudeTaskNotification, if parsedFields.usage is non-null but fails parseStructuredFields (e.g. the usage format changes in a future Claude version), the entire notification is silently dropped even though status/summary/etc. parsed correctly. Usage should be treated as optional — return the notification without usage rather than returning null.
MEDIUM
[M1] The new RegExp(STRUCTURED_FIELD_RE.source, STRUCTURED_FIELD_RE.flags) clone inside parseStructuredFields is correct (resets lastIndex), but the intent is non-obvious — add a one-line comment explaining why, otherwise a future maintainer will "simplify" it and introduce a stale-lastIndex bug.
[M2] parseStructuredMessage signature changed from 3 to 4 params — please confirm with a grep that no other call site exists besides the two already updated.
[M3] isFilteredClaudeStructuredMessage checks a single hardcoded tag. Replace with a Set constant so future additions are a one-liner.
LOW / nits
getMessageRoleMetasilently falls through tomsg-assistantfor unknown roles — aconsole.warnin dev would surface future gaps early.- Unrecognized
queue-operationentries are silently dropped — intentional, but worth a one-line comment so it's not mistaken for a bug. data.jsis already well past 4 000 lines; the new Claude-specific parsing functions (parseClaudeStructuredMessage,parseClaudeTaskNotification, etc.) are cohesive enough to live in a separatesrc/claude-message-parser.js. Not blocking, but worth tracking as tech debt.
Verdict: request changes. H1 (XSS) and H3 (silent notification drop) are the most important; H2 (tests) is important for long-term reliability given how often Claude's internal message format evolves.
Addressed in dbc3499 |
|
Thanks for the quick turnaround on the fixes! |
#197 must be merged first. After that, merge conflicts should be resolved before code review to reduce line churn.
Summary
Render known structured Claude Code session messages in the detail panel instead of showing raw XML-like wrappers.
It currently supports:
command-name,command-message,command-args)bash-input)bash-stdout,bash-stderr)local-command-stdout)task-notification) from bothuserandqueue-operationentriesWhat Changed
local-command-caveatmessages from the detail panelqueue-operationtask notifications visible as structured queue entriesNotes
systementries with subtypelocal_commandare still ignoreduserandqueue-operationare intentionally kept for nowExamples
Slash Command
Bash Command
Task Notification