Conversation
Add display helpers that inject attachment file sizes into campfire line output. HTML content goes through HTMLToMarkdown then gets size annotations on deterministic 📎 marker lines. Plain text and empty content paths synthesize attachment info directly. ByteSize == 0 is treated as omitted (no size parenthetical). Wire up WithDisplayData at three call sites (messages, line show, upload) so styled/markdown output shows sizes while JSON stays untouched. Upload summary now prefers the API-returned filename and includes file size.
Helper unit tests: HTML+attachments, plain text+attachments, empty content+attachments, title fallback, plain-text-only, mid-line non-rewrite, zero byte size, title fallback, duplicate filenames. Command-level tests: upload summary includes file size, styled output renders 📎 filename (size). Output-level tests: styled and markdown rendering with display data, JSON preserves original content.
github-actions
bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/chat_test.go">
<violation number="1" location="internal/commands/chat_test.go:989">
P2: Unchecked error from `os.WriteFile`. If the write fails, the test will produce a confusing failure downstream. Use `require.NoError` to surface the real cause.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR enhances human-readable chat output by surfacing Campfire attachment file sizes (from CampfireLineAttachment.ByteSize) in styled/markdown output while keeping raw JSON output unchanged.
Changes:
- Wire
WithDisplayData(...)throughchat messages,chat line, andchat uploadso styled/markdown rendering can show attachment sizes without altering JSON data. - Add chat-line display helpers to render attachment-only lines more informatively and to inject sizes into rich-text→markdown attachment markers.
- Add output/command tests covering the new display behavior and upload summaries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/commands/chat.go |
Adds display-data plumbing + helpers to format chat line content with attachment sizes and to update upload summaries. |
internal/commands/chat_test.go |
Adds unit tests for display-content helpers and command-level tests validating upload summary/styled output include sizes. |
internal/output/output_test.go |
Adds rendering coverage confirming WithDisplayData affects styled/markdown but not JSON for chat_line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NormalizeData returns []map[string]any (not []any) when all elements are objects, so chatLinesDisplayData was silently skipping content replacement. Assert the correct type. Also wrap os.WriteFile in require.NoError in test helpers.
Use min(len(items), len(lines)) bounded loop instead of range+break so gosec can verify the index is in bounds.
There was a problem hiding this comment.
Pull request overview
Adds “display data” helpers so chat line attachment markers can show file sizes in human-oriented outputs (styled/markdown) while keeping JSON output unchanged.
Changes:
- Introduces chat-line display helpers that inject attachment sizes into rendered content.
- Wires
WithDisplayDataintochat messages,chat line, andchat uploadresponses. - Enhances upload summary to prefer API-returned filename and include file size when available.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/output/output_test.go | Adds output-level tests verifying DisplayData is used for styled/markdown and ignored for JSON. |
| internal/commands/chat_test.go | Adds unit tests for display helper behavior and command-level upload output/summary assertions. |
| internal/commands/chat.go | Implements display-content helpers, injects DisplayData into chat commands, and updates upload summary formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6b8bc8d87
ℹ️ 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".
Summary
CampfireLineAttachment.ByteSize) into campfire line output for styled/markdown renderingWithDisplayDataat three call sites:chat messages,chat line, andchat upload— JSON output stays untouchedDetails
Attachment marker lines (
📎 filename) produced byrichtext.HTMLToMarkdownare matched line-by-line and annotated with(size). Plain text and empty content paths synthesize attachment info directly.ByteSize == 0is treated as "API omitted the field" (no size parenthetical).Test plan
📎 filename (size)bin/cipasses (lint, vet, unit tests, e2e, surface snapshot, skill drift)