Skip to content

Fix screenshot comments sharing images and never being deleted - #1

Merged
hannesa2 merged 2 commits into
hannesa2:mainfrom
goneng:fix-per-pr-upload-names
Sep 19, 2026
Merged

hannesa2 merged 2 commits into
hannesa2:mainfrom
goneng:fix-per-pr-upload-names

Conversation

@goneng

@goneng goneng commented Sep 18, 2026 •

Copy link
Copy Markdown

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.png is a
single 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.

  • Scope the uploaded name to the repository, the pull request and the arch,
    the way the error-screenshot branch just below already renames its file.
  • Restore the rename the upload depends on. curl -F "file=@..." sends the
    basename 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
    mv was commented out.

$GITHUB_REPOSITORY is already used at lines 61, 92 and 160, $PR is
computed at line 47, and $emulatorApi comes from the calling workflow, so
nothing 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 echo expands backslash escapes. Line 74
pipes the comments JSON through echo, so every \n inside a comment body
becomes a real newline in the middle of a JSON string and jq refuses the
payload:

jq: parse error: Invalid string: control characters from U+0000 through
U+001F must be escaped at line 101, column 1

The || echo "" then leaves an empty list, so the cleanup reports "nothing
to 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 why
it worked until now.

  • Pass the JSON to jq with printf '%s' instead of echo.
  • Apply the same change to the DEBUG_INFO dump, which printed the same
    corrupted JSON.

Lines 76 and 82 keep their echo, since those act on jq's own output, which
is ids and true/false with no backslashes.

Test plan

  • zsh -n screenShotCompare.sh passes.
  • Running the upload loop with the script's surrounding variables gives
    gitx-gitx-pr658-arm64-00-commit-context-menu.png,
    gitx-gitx-pr666-arm64-... and
    AppDevNext-AndroidChart-pr658-29-... where all three previously produced
    00-commit-context-menu.png.
  • Replaying the full deletion chain (lines 74-83) under zsh against the live
    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.
  • I have not been able to exercise the upload itself, since it needs the
    mxtracks credentials. Worth one real run on two concurrent pull requests.

@goneng
goneng marked this pull request as ready for review September 18, 2026 08:46
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.
@goneng
goneng force-pushed the fix-per-pr-upload-names branch from 4488a1f to b1a7454 Compare September 18, 2026 08:57
@hannesa2

Copy link
Copy Markdown
Owner

I've to make this on upstream as well to test it in gitx/gitx#658

@hannesa2

Copy link
Copy Markdown
Owner

This is the indication: https://github.com/gitx/gitx/actions/runs/35431177919/job/105865926899?pr=661

==> Delete all old comments, starting with 'Screenshot differs:
DEBUG_INFO is unset or empty
742.187 (0.011325)742.187 (0.011325)742.187 (0.011325)742.187 (0.011325)jq: parse error: Invalid string: control characters from U+0000 through U+001F must be escaped at line 101, column 1
oldCommentsList=
oldCommentsList=
==> oldCommentsList is empty, there is nothing to do

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.
@goneng goneng changed the title Fix one PR's screenshots overwriting another's Fix screenshot comments sharing images and never being deleted Sep 19, 2026
@goneng

goneng commented Sep 19, 2026

Copy link
Copy Markdown
Author

That jq error you linked is fixed here now, as a second commit (1f4c399).

Line 74 is the whole story: this script runs under zsh, whose echo expands
backslash escapes, so any \n in a comment body becomes a real newline inside
a JSON string and jq refuses the payload. printf '%s' instead. It only
started biting once a pull request carried a comment written by a person -
the ones this script posts use <br/> and contain no escapes.

Replayed against the live payloads: the cleanup now finds all 7 stale comments
on gitx/gitx#661 and both on #666, and leaves the human ones alone.

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
last look here, and gitx sees nothing until the pin moves off b1a7454.

@hannesa2
hannesa2 merged commit 86db24d into hannesa2:main Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants