--attach stack 6/8: Add the flag to gh pr comment and gh issue comment - #14182
Conversation
--attach stack 6/8: Add the flag to gh pr comment and gh issue comment
There was a problem hiding this comment.
Pull request overview
Adds attachment uploads to pull request and issue comments through shared comment handling.
Changes:
- Adds the repeatable
--attachflag and help text. - Uploads assets, rewrites comment Markdown, and preserves edited comment bodies.
- Adds validation and comprehensive shared tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cmd/pr/shared/commentable.go |
Integrates attachment upload and body handling. |
pkg/cmd/pr/shared/commentable_test.go |
Tests shared attachment behavior and failures. |
pkg/cmd/pr/comment/comment.go |
Adds --attach to pull request comments. |
pkg/cmd/pr/comment/comment_test.go |
Tests pull request flag wiring. |
pkg/cmd/issue/comment/comment.go |
Adds --attach to issue comments. |
pkg/cmd/issue/comment/comment_test.go |
Tests issue flag wiring. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
7fa3f71 to
d6a297a
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
d6a297a to
7d1d9b7
Compare
babakks
left a comment
There was a problem hiding this comment.
Thanks for this one, @BagToad! 🙏
This is a clean integration into the shared commentable path. I really like how bodyForWrite treats a partially-successful upload: joining the upload error ahead of the write error so a user always learns about an asset that reached the server even when the comment itself failed to save. The reasoning comments throughout (the lone-attachment-as-body case, the KeepExistingBody handling on edits, guarding the uploader on len(Assets) > 0) make the flow easy to follow, and the new commentable_test.go coverage is thorough.
Only a few nitpicks from me, nothing blocking:
- A repeated-
--attachexample (without alt text) in the help for bothgh pr commentandgh issue comment, to show the flag can be given more than once. - A small test-formatting nit: a few case structs close their brace on the same line as the last field.
One thing I want to circle back on from my review of the previous PR (#14181): the flag mutual-exclusion checks (e.g. --web vs --attach) that currently live in the attachments package. I still think those belong at the command level, and this PR is a good illustration of why: the conflicts being enforced (--web, --delete-last, --dry-run) are exactly the flags these comment commands own. Happy for that to land in the follow-up we discussed rather than holding up the stack.
Nice work! 🚀
Part of the pull request stack tracked in #14186.
Description
This is the sixth layer of the stack, and the first one where a user can do anything. It adds
--attachtogh pr commentandgh issue comment.The flag uploads a local image or video and puts it in the comment. If the comment body already references the local path, that reference is rewritten to the uploaded asset URL. Otherwise the file is appended at the end. Alt text goes after a
#, and without it the file name is used.An image becomes a markdown image embed. A video becomes a bare URL on its own line, which is what GitHub needs to show a player.
An attachment counts as body input on its own, so
--attachwith no--bodyposts a comment that is just the image.With
--edit-last, attaching a file keeps the text of the comment being edited and adds the attachment below it. An attachment is not a body, so it does not replace one.Two combinations are refused.
--attachwith--webcannot work, because the browser is doing the writing.--attachwith--delete-lastis refused as well.How did you test this change?
Both commands were exercised by hand against a private test repository. That covered a comment with an image, a comment with a video, attaching with no body at all, alt text after the hash, a body whose reference was rewritten in place, a fenced code block left untouched, two files in one command,
--edit-lastkeeping the existing text,--edit-lastwhere the existing comment already named the file so the reference was rewritten instead of appended, and both refused flag combinations.Everything was run again from a second account with read only access on the repository. Every attach was refused before any upload, and a plain comment with no attachment still worked, so only the attaching is blocked rather than the command.
Key points
Resolving the flag happens in the shared comment code rather than in each command, so a command that offers the flag cannot forget to check it.
Notes for reviewers
This is one commit covering two commands, which is worth explaining. Both commands run through the same shared comment code, and that shared code now resolves the flag for whichever command is running. It reports an error if the command has not registered the flag. So the shared change and both registrations have to land together; splitting them would leave whichever command was not wired yet broken.
Authorship and follow-up
Who wrote this:
Who answers review comments: