Repository navigation
feat(commit): document attaching PR screenshots with gh --attach - #176
JoshuaKGoldberg wants to merge 4 commits into
Conversation
Agents were claiming gh can't attach images and asking users to drag screenshots in, though gh pr create and gh pr edit support --attach.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Older gh versions reject --attach as an unknown flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JoshuaKGoldberg
changed the title
|
|
||
| ## Attaching Images to the Pull Request | ||
|
|
||
| Since gh 2.99.0, `gh pr create` and `gh pr edit` upload images and videos with |
There was a problem hiding this comment.
i am not sure we need to index on the version of gh as much, the agent will figure out if it fails the first time and will update vs this might make the agent do a gh version check before any git command
i would also make sure we have a "skip this section if its not a visual change" so it doesn't try and put a screenshot in for all sort of things. or maybe the good models are smart enough to figure that out on their own, but there are probably also visual changes that might not need a screenshot either, i think like if text copy changes, will the llm think oh yeah i need to show before and after?
There was a problem hiding this comment.
I've had this locally for a while and at least my Claude & Opus >=5 combo is smart enough to not trip up on this.
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
Attaching images on a pull request reads as something distinct from a commit. I wonder if there's a better way (maybe a distinct skill for image attachment and then invoke the 2 skills together a distinct workflow/prompt)?
Oh, actually, I do like that. Since sending this PR I've evolved some practices around tables (when to use before-and-after vs just one row, what columns to put, etc.). Let me go rework this... |
There was a problem hiding this comment.
[Explanation] I, ah, kind of got excited. I directed Claude to look through my last 2 weeks of session chats/feedback and PR descriptions. It distilled them down into basic preferences of how I do things. I trimmed out a bunch of me-specific stuff (e.g. Logs area things). But I'm not sure I drew the line right - is everything here Sentry-general? There's still a lot new... 😅
| image references. Switch to an HTML table only when cells must span rows, such | ||
| as a light and dark row under one location | ||
| (`Location | Theme | Before | After` with `rowspan="2"` on the location cell). | ||
| Do not add `align` attributes. |
There was a problem hiding this comment.
alignattributes
[Explanation] For some reason, whenever there's a real table, Claude loves to add align="right" or whatnot on every column.
| Before writing a body the user may have edited, fetch it again and compare it | ||
| with the version you last read. Keep the user's wording and table edits, swap in | ||
| only your rows and URLs, and keep the body's line endings. Removing an image | ||
| from the body does not delete the uploaded file. |
There was a problem hiding this comment.
[Explanation] I can't tell you how many times Claude clobbered my edits...
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
To answer the question that you left in one of your comments - I think this is a generalizable skill because I can see this being used by people working on the UI or docs pages.
Left some "food-for-thought" comments but I imagine some of these can only be answered through usage/testing of the skill by others, so am considering them non-blocking.
| @@ -0,0 +1,123 @@ | |||
| --- | |||
| name: pr-screenshots | |||
| description: Capture UI screenshots of the real product and lay them out in a pull request description as before/after tables, then upload them with `gh --attach`. | |||
There was a problem hiding this comment.
I'm not sure if Claude knows what "real product" is 😅 Would specifying that it needs to capture screenshots of the browser lead to the skill being more effective?
| description: Capture UI screenshots of the real product and lay them out in a pull request description as before/after tables, then upload them with `gh --attach`. | |
| description: Capture screenshots of the Sentry UI in a web browser, lay them out in a pull request description as before/after tables, and upload them with `gh --attach`. |
| List every place in the product that the diff reaches: each page, drawer, | ||
| modal, or widget that renders a changed component, found by tracing its | ||
| callers and consumers. Then add the states the change affects at each of them, | ||
| such as loading, error, empty, hover, selected, menu open, expanded, scrolled, | ||
| narrow viewport, and light or dark theme. | ||
|
|
||
| Each location and state gets its own row. Never group surfaces into one row: | ||
| "GridEditable, as in Discover, dashboards and insights" is three rows. |
There was a problem hiding this comment.
I worry that this will cause the AI to create 2 screenshots for every permutation, which may not be desirable (there could be a lot of states!).
Maybe there needs to be an analysis step so that a developer can choose just how many options to show? Otherwise I worry that pull request descriptions will be very noisy 😬
| --- | ||
| name: pr-screenshots | ||
| description: Capture UI screenshots of the real product and lay them out in a pull request description as before/after tables, then upload them with `gh --attach`. | ||
| --- |
There was a problem hiding this comment.
I wonder if it'd be worth adding a when_to_use frontmatter field here to indicate that this skill is intended to be used on the frontend project (at least for now) 🤔
I can see a scenario where devs make backend changes that result in FE changes with no changes being applied in FE components also wanting to use this, so feel free to disregard this comment if this scenario is a more common flow than I'm thinking it is at the moment.
| |------|--------| | ||
| | Real build only | Screenshot the running app on the PR branch for "after" and on the base branch for "before". Never screenshot a story, mock page, or harness built for the PR, unless the change is to that story. | | ||
| | No injected changes | Never inject CSS or change the DOM to imitate the change. Stubbing API responses is fine; the frontend code must be the real code. | | ||
| | Seed missing data | If a state needs data no account has, create it in a local environment. Do not report a location as impossible to screenshot until seeding has been tried, and then say exactly what failed. | |
There was a problem hiding this comment.
If a state needs data no account has, create it in a local environment.
Is the assumption that there's "seed data initialization" scripts, etc. on hand in the project that the AI will notice? Or does this need to be specified as part of this workflow?
| | Alternatives with no "before", such as prototype treatments or design options | `Treatment \| Screenshot` table, one row per alternative. | | ||
| | A change that differs by theme | Add a light and a dark row for each location, such as `Explore › Logs, dark`. For new UI, use `Light \| Dark` columns instead. | | ||
|
|
||
| Pixel-diff every before/after pair, then confirm by eye. Move a pair with no |
There was a problem hiding this comment.
Pixel-diff every before/after pair, then confirm by eye.
Is the "confirm by eye" part intended to be an instruction to the AI to pause in the workflow to get a dev's 👍🏻 / 👎🏻 on the image before continuing?
4 participants
Notes that
gh pr createandgh pr editcan now upload images with--attach, so agents attach screenshots directly.This speeds up local PR description creations for me (which I do as a form of QA too, a lot). Previously it would ask to upload to a branch, or chrome-devtools drag them in, etc.