Repository navigation
Fix screenshot comments sharing images and never being deleted - #1
Conversation
Give each uploaded diff image a name of its own, so a comment keeps showing the run that wrote it. * Scope the uploaded name to the repository, the pull request and the arch. The upload target is one flat directory shared by every project using this script, so a bare basename meant the last run to finish replaced the images that earlier comments already linked to - across repositories as well as within one. * Restore the rename the upload depends on: curl sends the basename as the stored name, so the file has to be renamed on disk for the URL to change.
4488a1f to
b1a7454
Compare
|
I've to make this on upstream as well to test it in gitx/gitx#658 |
|
This is the indication: https://github.com/gitx/gitx/actions/runs/35431177919/job/105865926899?pr=661 |
Read the comments payload without letting the shell rewrite it, so the cleanup can find the comments it is meant to remove. * Pass the JSON to jq with printf instead of echo. This script runs under zsh, whose echo expands backslash escapes, so every \n inside a comment body became a real newline in the middle of a JSON string and jq refused the whole payload. * The failure was invisible: the `|| echo ""` left an empty list, and the cleanup reported nothing to do while old comments accumulated on every run. It only showed up once a pull request carried a comment written by a person, since the comments this script posts use <br/> and contain no escapes. * Apply the same change to the DEBUG_INFO dump, which printed the same corrupted JSON.
|
That jq error you linked is fixed here now, as a second commit ( Line 74 is the whole story: this script runs under zsh, whose Replayed against the live payloads: the cleanup now finds all 7 stale comments I retitled the PR, since it covers two separate fixes now. No rush from my side - flagging it only because the commit landed after your |
Two independent defects in how the screenshot comments are produced and
cleaned up. Both surfaced in gitx/gitx#658, #661 and #666.
1. Each PR's images overwrite every other PR's (
b1a7454)Give each uploaded diff image a name of its own, so a comment keeps showing
the run that wrote it.
The upload target is one flat directory shared by every project using this
script, and the images were sent under their bare basename, so
https://www.mxtracks.info/github/uploads/00-commit-context-menu.pngis asingle global slot. The last run to finish replaces the images that earlier
comments already link to. Two pull requests compared at once end up rendering
the same pictures - and since the directory is shared across repositories,
AndroidChart and GitX can clobber each other the same way.
the way the error-screenshot branch just below already renames its file.
curl -F "file=@..."sends thebasename as the stored name, so the file has to be renamed on disk for the
URL to change - which is presumably why the name was left bare when the
mvwas commented out.$GITHUB_REPOSITORYis already used at lines 61, 92 and 160,$PRiscomputed at line 47, and
$emulatorApicomes from the calling workflow, sonothing else in the script changes.
2. Old comments are never deleted (
1f4c399)Read the comments payload without letting the shell rewrite it, so the
cleanup can find the comments it is meant to remove.
This script runs under zsh, whose
echoexpands backslash escapes. Line 74pipes the comments JSON through
echo, so every\ninside a comment bodybecomes a real newline in the middle of a JSON string and jq refuses the
payload:
The
|| echo ""then leaves an empty list, so the cleanup reports "nothingto do" while the old comments accumulate on every run.
It only bites once a pull request carries a comment written by a person. The
comments this script posts use
<br/>and contain no escapes, which is whyit worked until now.
printf '%s'instead ofecho.DEBUG_INFOdump, which printed the samecorrupted JSON.
Lines 76 and 82 keep their
echo, since those act on jq's own output, whichis ids and
true/falsewith no backslashes.Test plan
zsh -n screenShotCompare.shpasses.gitx-gitx-pr658-arm64-00-commit-context-menu.png,gitx-gitx-pr666-arm64-...andAppDevNext-AndroidChart-pr658-29-...where all three previously produced00-commit-context-menu.png.comment payloads of Screen shot false test gitx/gitx#661 and #666: the patched version finds all 7
stale bot comments on #661 and both on #666, and correctly leaves the human
comments alone. The current version returns an empty list for both.
mxtracks credentials. Worth one real run on two concurrent pull requests.