Render hook attachment entries at FULL detail (#128)#149
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThis PR models Claude Code ChangesHook Attachment Support
Sequence Diagram(s)sequenceDiagram
participant JSONL
participant TranscriptFactory
participant AttachmentFactory
participant Renderer
participant Formatter
JSONL->>TranscriptFactory: parse entry type="attachment"
TranscriptFactory->>AttachmentFactory: AttachmentTranscriptEntry
AttachmentFactory-->>TranscriptFactory: HookAttachmentMessage or None
TranscriptFactory->>Renderer: pass through message stream
Renderer->>Formatter: format_hook_attachment_content / format_HookAttachmentMessage
Formatter-->>Renderer: HTML/Markdown fragment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/test_hook_attachment_rendering.py (2)
326-326: 💤 Low valueNit:
delto silence unused-variable warnings is unusual.Idiomatic Python uses
_-prefixed names (already done for_nav) or simply doesn't bind the values. Sincerootsis destructured out of the tuple, you could rename it to_rootsupfront and drop thedel. Not a correctness concern.♻️ Suggested refactor
- roots, _nav, ctx = generate_template_messages( + _roots, _nav, ctx = generate_template_messages( _hook_messages(), detail=DetailLevel.FULL ) - del roots, _nav # only inspect ctx.messagesAlso applies to: 436-436, 478-478
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_hook_attachment_rendering.py` at line 326, The test currently uses "del roots, _nav" to silence unused-variable warnings; instead, rename the destructured "roots" binding to "_roots" where the tuple is unpacked (keeping "_nav" as-is) and remove the "del" statement(s); update the other occurrences that use the same pattern (the other spots that delete roots/_nav) so they also bind to "_roots" and drop the del lines, leaving only the variables you actually inspect (e.g., ctx.messages).
113-115: 💤 Low valueOptional: replace
# type: ignore[arg-type]with anisinstancenarrow.
create_transcript_entryreturnsTranscriptEntry, so the type checker can't see that the parsed result is concretely anAttachmentTranscriptEntry. The first test (line 78) already does the right thing with anisinstanceassert before invoking the factory. Mirroring that here would let you drop the three# type: ignore[arg-type]comments and add a useful invariant check at the same time.♻️ Suggested refactor
entry = create_transcript_entry(raw) - msg = create_attachment_message(entry) # type: ignore[arg-type] + assert isinstance(entry, AttachmentTranscriptEntry) + msg = create_attachment_message(entry)Also applies to: 136-138, 159-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_hook_attachment_rendering.py` around lines 113 - 115, Replace the ad-hoc type ignores by narrowing the transcript entry with an isinstance check before calling create_attachment_message: assert isinstance(entry, AttachmentTranscriptEntry) where entry comes from create_transcript_entry(raw), then call msg = create_attachment_message(entry) without # type: ignore[arg-type] and assert isinstance(msg, HookAttachmentMessage); apply the same change to the other two occurrences that currently use # type: ignore[arg-type] so the type checker sees the concrete AttachmentTranscriptEntry prior to invoking create_attachment_message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude_code_log/html/templates/components/message_styles.css`:
- Around line 399-408: The CSS blocks for .hook-attachment-output and
.hook-attachment-content use the deprecated word-wrap property; replace
word-wrap: break-word; with overflow-wrap: break-word; in those selector blocks
(and likewise update other occurrences you noted) so the behavior remains the
same but the deprecation warning is cleared—search for the symbol names
.hook-attachment-output and .hook-attachment-content (and other selectors where
word-wrap appears) and swap each word-wrap line to overflow-wrap: break-word;.
In `@claude_code_log/markdown/renderer.py`:
- Around line 478-503: The hook inline fields are currently inserted using raw
backticks which breaks if values contain backticks; update the places that
interpolate inline hook values to use the helper `_inline_code(...)` instead of
raw backticks: in the earlier hook-list block replace f"`{info.command}`" with
`_inline_code(info.command)` (where info.command is used), and inside
format_HookAttachmentMessage replace f"`{content.hook_name}`" with
`_inline_code(content.hook_name)`, f"`{content.hook_event}`" with
`_inline_code(content.hook_event)`, format the exit code as `f"exit
{_inline_code(content.exit_code)}"` and the duration as
`f"{_inline_code(content.duration_ms)} ms"` so all inline code fragments use the
central sanitizer.
---
Nitpick comments:
In `@test/test_hook_attachment_rendering.py`:
- Line 326: The test currently uses "del roots, _nav" to silence unused-variable
warnings; instead, rename the destructured "roots" binding to "_roots" where the
tuple is unpacked (keeping "_nav" as-is) and remove the "del" statement(s);
update the other occurrences that use the same pattern (the other spots that
delete roots/_nav) so they also bind to "_roots" and drop the del lines, leaving
only the variables you actually inspect (e.g., ctx.messages).
- Around line 113-115: Replace the ad-hoc type ignores by narrowing the
transcript entry with an isinstance check before calling
create_attachment_message: assert isinstance(entry, AttachmentTranscriptEntry)
where entry comes from create_transcript_entry(raw), then call msg =
create_attachment_message(entry) without # type: ignore[arg-type] and assert
isinstance(msg, HookAttachmentMessage); apply the same change to the other two
occurrences that currently use # type: ignore[arg-type] so the type checker sees
the concrete AttachmentTranscriptEntry prior to invoking
create_attachment_message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a2880bc-8b17-4f08-98b5-8683ca8ce0e3
📒 Files selected for processing (16)
claude_code_log/converter.pyclaude_code_log/dag.pyclaude_code_log/factories/attachment_factory.pyclaude_code_log/factories/transcript_factory.pyclaude_code_log/html/renderer.pyclaude_code_log/html/system_formatters.pyclaude_code_log/html/templates/components/message_styles.cssclaude_code_log/html/utils.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/__snapshots__/test_snapshot_html.ambrtest/test_data/hook_attachments.jsonltest/test_hook_attachment_rendering.pytest/test_hook_summary.pytest/test_silent_skip.py
| .hook-attachment-output, | ||
| .hook-attachment-content { | ||
| margin: 0; | ||
| padding: 0.5em; | ||
| background-color: var(--code-bg); | ||
| border-radius: 4px; | ||
| font-size: 0.85em; | ||
| white-space: pre-wrap; | ||
| word-wrap: break-word; | ||
| overflow-x: auto; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify deprecated property usage in the changed hook attachment style block.
rg -n 'hook-attachment-(output|content)|word-wrap|overflow-wrap' claude_code_log/html/templates/components/message_styles.cssRepository: daaain/claude-code-log
Length of output: 313
Replace deprecated word-wrap with overflow-wrap in hook attachment blocks.
The word-wrap property is deprecated. Replace with overflow-wrap: break-word; at line 407 to preserve behavior and clear the deprecation warning. Note: the same deprecated property appears at lines 470, 492, 746, 900, and 1006 elsewhere in the file—consider addressing all instances for consistency.
Suggested patch
.hook-attachment-output,
.hook-attachment-content {
- word-wrap: break-word;
+ overflow-wrap: break-word;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .hook-attachment-output, | |
| .hook-attachment-content { | |
| margin: 0; | |
| padding: 0.5em; | |
| background-color: var(--code-bg); | |
| border-radius: 4px; | |
| font-size: 0.85em; | |
| white-space: pre-wrap; | |
| word-wrap: break-word; | |
| overflow-x: auto; | |
| .hook-attachment-output, | |
| .hook-attachment-content { | |
| margin: 0; | |
| padding: 0.5em; | |
| background-color: var(--code-bg); | |
| border-radius: 4px; | |
| font-size: 0.85em; | |
| white-space: pre-wrap; | |
| overflow-wrap: break-word; | |
| overflow-x: auto; |
🧰 Tools
🪛 Stylelint (17.10.0)
[error] 407-407: Expected "word-wrap" to be "overflow-wrap" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude_code_log/html/templates/components/message_styles.css` around lines
399 - 408, The CSS blocks for .hook-attachment-output and
.hook-attachment-content use the deprecated word-wrap property; replace
word-wrap: break-word; with overflow-wrap: break-word; in those selector blocks
(and likewise update other occurrences you noted) so the behavior remains the
same but the deprecation warning is cleared—search for the symbol names
.hook-attachment-output and .hook-attachment-content (and other selectors where
word-wrap appears) and swap each word-wrap line to overflow-wrap: break-word;.
7373a53 to
b8f3b81
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
CodeRabbit review on PR #149 flagged five places in the Markdown hook formatters where inline fields are wrapped in raw backticks via ``f"`{value}`"``. If any value contains a backtick (a hook name like ``PostToolUse:`weird``` or a command like ``echo `pwd```), the naive span closes at the inner tick and the surrounding Markdown breaks. Switched all five to ``_inline_code(...)``, the existing helper that widens the fence past the longest internal backtick run and pads with a space when the value starts/ends with a backtick: - ``format_HookSummaryMessage``: ``info.command``. - ``format_HookAttachmentMessage``: ``content.hook_name``, ``content.hook_event``, ``content.exit_code``, ``content.duration_ms``. ``exit_code`` and ``duration_ms`` are typed ``Optional[int]`` so they go through ``str()`` first to keep the helper's ``str``-only contract intact. Two regression tests added under ``TestHookAttachmentMarkdownInlineCode``: a hook name and a hook command containing backticks both render with the widened fence + space padding pattern (``"`` echo `pwd` ``"``) instead of breaking the span. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hook callbacks recorded as ``type: "attachment"`` JSONL entries (``hook_success``, ``hook_additional_context``, ``hook_blocking_error``, ``hook_non_blocking_error``) were previously dropped at parse time as ``PassthroughTranscriptEntry``, which meant the user couldn't inspect any hook output at any detail level. Promote attachment entries into a typed ``AttachmentTranscriptEntry`` so their payload survives parsing. A new ``HookAttachmentMessage`` content type carries the hook event/name, command, exit code, duration and stdout/stderr/blocking-error text, and renders as a collapsible ``<details>`` block at ``DetailLevel.FULL``. ``HIGH`` and below drop it alongside the existing ``HookSummaryMessage`` noise. Anchoring follows ``parentUuid`` (not ``toolUseID``) per the issue — ``UserPromptSubmit`` hook attachments carry ``toolUseID`` values that match nothing in the transcript, while ``parentUuid`` correctly anchors on the prompt that fired them. Non-hook attachment flavours (``deferred_tools_delta``, ``queued_command``, ``skill_listing``, ``task_reminder``, ``edited_text_file``, etc.) keep the historical "structural in DAG, hidden from rendering" behaviour by returning ``None`` from the factory; future flavours can grow their own factory branch without touching the dispatch. DAG-level structural-subtree detection now treats ``AttachmentTranscriptEntry`` the same as ``PassthroughTranscriptEntry`` so fork-collapse and orphan-root classification still apply to chains of hook attachments. Includes a fixture ``test/test_data/hook_attachments.jsonl`` covering all four hook flavours plus a non-hook attachment (deferred_tools_delta) to lock in the structural carve-out, and 16 unit/integration tests verifying parsing, factory dispatch, parent-uuid anchoring, full-detail rendering, and HIGH/LOW filtering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Visual feedback after #128 landed: 1. **Hook attachments** (the new ``HookAttachmentMessage``): the body summary doubled the title's 🪝 icon. Kept the label bold ("Hook output" / "Hook blocked" / "Hook errored" / "Hook added context") plus the event/exit/duration metadata, dropped the icon — the message header carries it once at the title level. 2. **Hook summaries** (the legacy ``HookSummaryMessage``): same treatment, plus a stronger trim. The body's generic "🪝 Hook output" / "🪝 Hook failed" subhead repeated the title with no extra signal. Replaced with the actual command preview (first ``hookInfo`` command, with ``(+N more)`` when multiple). Returns empty when there's neither command nor error so a content-free hook renders as a single-line title (matches the request to keep "only that title"). 3. **HookSummaryMessage title icon**: ``get_message_emoji`` now returns 🪝 for both ``HookAttachmentMessage`` and ``HookSummaryMessage`` instead of falling back to ⚙️ for the summary case. Aligns the two hook-flavoured content types under the same title glyph. 4. **stderr padding**: the warning-bar ``border-left`` on ``.hook-attachment-stderr`` butted up against the text. Bumped ``padding-left`` to ``calc(0.5em + 3px)`` so the border breathes. Markdown side gets the matching trim: ``format_HookSummaryMessage`` no longer emits "Hook produced output" — the body is just the command list and any error text. Tests updated: the previous "Hook failed" / "Hook output" body-label assertions on ``test_hook_summary.py`` now look for the rendered command preview instead, and ``test_hook_attachment_rendering.py`` asserts the body deliberately omits the icon (header carries it). Snapshots regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues surfaced when reviewing transcripts where hooks fire on every turn (ClMail-style plugins). ## Visual weight (CSS) Hook entries (both ``HookAttachmentMessage`` from #128 and the legacy ``HookSummaryMessage``) accumulate in dense bursts and were drowning out actual conversation. Dropped them from 90% font-size to 70%, added 0.9 opacity, and de-bolded the message header — they now read as secondary signal without losing the affordance: ```css div.system-hook-attachment, div.system-hook { font-size: 70%; opacity: 0.9; } div.system-hook-attachment .header, div.system-hook .header { font-weight: 400; } ``` ## Hierarchy-level mis-anchor (regression fix) ``HookAttachmentMessage`` carries ``message_type == "system"`` but isn't a ``SystemMessage`` instance, so ``_get_message_hierarchy_level`` skipped the system info/warning level-3 branch (which gates on ``isinstance(msg.content, SystemMessage)``) and fell through to level 2 — same as ``stop_hook_summary`` entries. At level 2, a hook attachment claimed any subsequent ``system-info`` (level 3) entries as descendants. Symptom: a ``UserPromptSubmit`` hook ended up parenting a ``/color`` system entry from a *later* turn, AND the ``/color`` → ``Session color set to: green`` pair (which should anchor under their own user turn) was split because ``/color`` was already "claimed" by the hook. Added an explicit branch placing both ``HookAttachmentMessage`` and ``HookSummaryMessage`` at level 3. Both are out-of-band callbacks, not conversation turns; they sit alongside other system noise rather than acting as containers for it. Regression test ``test_hook_does_not_claim_system_info_as_child`` builds the exact shape that triggered the bug (user → hook → /color → Session color set) and asserts the hook stays a leaf and the system_infos anchor on the user turn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Iteration on the visual-impact reduction:
- font-size 70% → 75%, opacity 0.9 → 65% on hook entries — readable
at lower density, still clearly secondary content.
- ``.header`` font-weight 400 → 500 — distinct enough from body text
to anchor the eye without competing with conversation turns.
- Dropped the ``<strong>`` around the hook attachment's body summary
label ("Hook output" / "Hook blocked" / etc.). The 500-weight
header already gives it the right level of emphasis; the extra
bold made it feel double-weighted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hierarchy-level fix in c0cefe1 stopped hooks from claiming system-info entries as ancestry children, but a separate pairing pass kept producing the same visual symptom: every hook attachment in a dense transcript rendered with a "▼ 1 system" fold-bar because ``_try_pair_by_index`` paired it with a chained system entry by uuid. The mechanism: ``_build_pairing_indices`` indexes every ``type == "system"`` message by uuid for the parent/child pairing path; ``_try_pair_by_index`` then looks up ``current.parent_uuid`` in that index. ``HookAttachmentMessage`` and ``HookSummaryMessage`` both carry ``type == "system"`` (their ``message_type`` is ``"system"``), so any chained system entry whose ``parentUuid`` points at a hook ended up pairing the hook as its parent. In the alice fixture transcript, 132 ``stop_hook_summary`` entries have a hook attachment as their ``parentUuid`` — every one of them triggered the bug. Real-world rendering: spurious fold-bar on hundreds of hooks per session. Fix: exclude ``HookAttachmentMessage`` and ``HookSummaryMessage`` from both sides of the system→system pairing — the index build and the lookup. Hook flavours stand on their own; they don't participate in the conversational system-entry chain pairing (which is intended for ``/context``-style multi-step output and similar cases). Verified on the alice fixture: 1022 hook divs, 0 with ``pair_first``/``pair_last``, 0 with a fold-bar inside their own message div. Regression test ``test_hook_does_not_pair_with_chained_system_entry`` builds the exact shape (hook → stop_hook_summary parented on the hook) and asserts the hook has no pair links. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review on PR #149 flagged five places in the Markdown hook formatters where inline fields are wrapped in raw backticks via ``f"`{value}`"``. If any value contains a backtick (a hook name like ``PostToolUse:`weird``` or a command like ``echo `pwd```), the naive span closes at the inner tick and the surrounding Markdown breaks. Switched all five to ``_inline_code(...)``, the existing helper that widens the fence past the longest internal backtick run and pads with a space when the value starts/ends with a backtick: - ``format_HookSummaryMessage``: ``info.command``. - ``format_HookAttachmentMessage``: ``content.hook_name``, ``content.hook_event``, ``content.exit_code``, ``content.duration_ms``. ``exit_code`` and ``duration_ms`` are typed ``Optional[int]`` so they go through ``str()`` first to keep the helper's ``str``-only contract intact. Two regression tests added under ``TestHookAttachmentMarkdownInlineCode``: a hook name and a hook command containing backticks both render with the widened fence + space padding pattern (``"`` echo `pwd` ``"``) instead of breaking the span. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up the .filter-toggle[data-type="sidechain"] block landed in `7c2e6f6` directly on main (CSS-only, no snapshot refresh in that commit). 28 lines added across the 7 affected fixtures (4 lines per insertion); no other drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0ef9e5f to
dda4479
Compare
Summary
Closes #128. Promotes Claude Code's
type: "attachment"JSONL entries (hook callbacks, deferred-tool deltas, queued commands, file references, todo reminders, etc.) from a structurally-onlyPassthroughTranscriptEntryinto a typedAttachmentTranscriptEntry, and surfaces the four hook flavours —hook_success,hook_additional_context,hook_blocking_error,hook_non_blocking_error— as a renderableHookAttachmentMessageat--detail full. Non-hook flavours stay structural (DAG continuity preserved, dropped from the rendered output). Anchored onparentUuidper the issue, since the exampleUserPromptSubmithook attachment carries atoolUseIDmatching nothing in the project.What's in the diff
AttachmentTranscriptEntry(BaseTranscriptEntry)inmodels.py, dispatched viatranscript_factory.ENTRY_CREATORS;HookAttachmentMessagedataclass withkinddiscriminator (success / additional_context / blocking_error / non_blocking_error) carrying hook event/name, command, exit code, duration, content/stdout/stderr, blocking_error.factories/attachment_factory.pyreturnsHookAttachmentMessagefor the four hookattachment.typevalues; returnsNonefor everything else (silent drop, mirroring pre-existing Passthrough semantics)._StructuralEntry = (PassthroughTranscriptEntry, AttachmentTranscriptEntry)tuple covers fork-collapse, parallel-tool_use chain detection, and orphan-root classification._is_expected_root_typeaccepts orphan attachment roots —SessionStarthooks legitimately fire pre-prompt.HookAttachmentMessagejoins_HIGH_EXCLUDE_CLASSES(drops at HIGH and below alongsideHookSummaryMessage); pre-render_filter_by_detaildrops attachments at non-FULL since they're not inallowed_types.system-infoentries as descendants. Both are also excluded from the system→system parent/child uuid-pairing path (_try_pair_by_index) so astop_hook_summarywhoseparentUuidis a hook attachment doesn't pair the hook as its parent and produce a spurious "▼ 1 system" fold-bar on every hook in dense transcripts.<details>block (header summary + per-stream fenced bodies). Markdown mirrors the structure with adaptive code fences. JSON gets the new content type for free viadataclasses.asdict.Test plan
test/test_hook_attachment_rendering.pycovering parsing of the issue's example payload, all four hook flavours, non-hook structural carve-out, formatter output, parent-uuid anchoring, hierarchy-level isolation (no claiming of system-info children), and pair-link isolation (no spurious system→system pairing) — plus end-to-end across detail levels (FULL renders, HIGH/LOW drop).test/test_data/hook_attachments.jsonlcovering all four hook flavours + a non-hook attachment for the structural carve-out.test/test_silent_skip.pyupdated and extended to assert attachments now parse asAttachmentTranscriptEntry; existingtest/test_hook_summary.pyupdated to match the trimmed body shape.just ciclean: 1609 unit + 66 TUI + 39 browser + format + lint + pyright + ty.<details>at FULL, 0 at HIGH (matches the population's hook/non-hook ratio).pair_first/pair_last, 0 with a fold-bar inside their own message div.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests