Skip to content

Follow-ups from reviewing #17 and #24: CI, a queue perf fix, and issue #19 - #26

Merged
g8row merged 4 commits into
mainfrom
fix/review-followups
Sep 20, 2026
Merged

g8row merged 4 commits into
mainfrom
fix/review-followups

Conversation

@g8row

@g8row g8row commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Three follow-ups to the two pull requests just merged, plus the bug they surfaced.

Run the tests on every pull request

Both #17 and #24 merged with no checks reported — there was no workflow that built the app or ran its tests. #24 was 1,941 lines across 25 files, verified only by its author running the suite locally.

tests.yml now runs xcodebuild test on every PR and every push to main. It resolves its simulator from what the runner actually has installed rather than naming a device that disappears on the next image bump, and it reads the verdict back out of the log, because piping xcodebuild through grep for a readable summary otherwise swallows the exit status.

The release build's Xcode pin also failed open:

if [ -d /Applications/Xcode_16.4.app ]; then
  echo "DEVELOPER_DIR=..." >> "$GITHUB_ENV"
fi

If GitHub retires that image, this silently builds on whatever Xcode ships instead — which defeats the point of building in CI at all, since the release note claims a specific toolchain. It now fails the run and prints what is installed.

Ask the photo library once per finished row, not twice

#24's follow-up hook runs on the main actor as every row finishes, and fetched the asset twice — once for the Live Photo check, once for the Google Photos edit check. On a full-library backup that is two PHAsset.fetchAssets calls per item on the thread drawing the activity list, which is the list #8 reports as slow.

One fetch now answers both, with the cheap checks first so the resource read behind the edit-base test still only runs for an adjusted asset.

Issue #19: "Items getting cancelled without knowing why"

Root cause found. A CancellationError that neither cancel nor a requeue claimed reached this:

if error is CancellationError {
    setState(.cancelled, at: index); cleanCheckpoint(for: index); persist(); return
}

That is an interruption, not a decision — a staging read dropped as iOS suspended the app, say. It became a terminal row labelled only "Cancelled": no reason, no way back, and still sitting in the list when a later scan backed the same photo up and added an "Already backed up" row beside it. That pair is exactly the screenshot in #19.

Such a row now retries like any other interruption, and if it keeps happening it fails in words the user can act on. .cancelled is left meaning one thing only — the user stopped it — so the label now says "Stopped by you".

Three regression tests cover the retry, the recovery, and the label.

Also: a dropped row counts as settled

A continued backup reports its Live Activity total as settled plus unfinished. #24 drops a row whose photo left the library without counting it as either, so the total walked backwards mid-run. The test that pinned the old count is updated with the reasoning rather than quietly flipped.

Test plan

  • xcodebuild test on Xcode 16.4: 191 tests, 2 skipped, 0 failures (188 before, 3 added here).
  • Builds with the iOS 26 continued-backup path compiled out, as make-ipa.sh does.
  • Simulator-resolution step verified against local simctl output.

Not verified: the workflows themselves have never run — this PR is the first thing that will exercise them.

🤖 Generated with Claude Code

g8row and others added 4 commits September 20, 2026 18:31
A CancellationError that neither `cancel` nor a requeue claimed is an
interruption, not a decision — a staging read dropped as iOS suspended the
app, say. It landed in a terminal `.cancelled` row labelled only
"Cancelled": no reason, no way back, and still sitting there when a later
scan backed the same photo up and added an "Already backed up" row beside
it. That pair is what #19 reported.

Such a row now retries like any other interruption and, if it keeps
happening, fails in words the user can act on. `.cancelled` is left
meaning one thing only — the user stopped it — so the label can say so.

Also count a row dropped because its photo left the library as settled. A
continued backup reports its total as settled plus unfinished, so dropping
one without counting it walked the Live Activity's total backwards
mid-run. The test that pinned the old count is updated with the reason.

Fixes #19

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The follow-up hook runs on the main actor as every row finishes, and it
fetched the asset twice: once to see whether the item is a Live Photo,
once to see whether it carries a Google Photos edit. On a full-library
backup that is two library reads per item on the thread drawing the
activity list, which is the list #8 reports as slow.

One fetch now answers both. The cheap checks go first, so the resource
read behind the edit-base test still only runs for an adjusted asset.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both of the last two pull requests merged with no checks reported: there
was no workflow that built the app or ran its 191 tests. One now runs on
every pull request and every push to main, reading its simulator from
what the runner actually has so an image bump cannot silently break it.

The release build's Xcode pin also failed open — a missing
Xcode_16.4.app fell through to whatever the image shipped, which would
quietly have broken the one thing building in CI is for: a binary tied to
the commit and the toolchain it claims. It now fails the run and says
what is installed instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first thing CI caught. `pauseForRateLimit` sets `rateLimitPauseReason`
synchronously but only reaches the sleeper once its task body runs, so
waiting on the reason alone could observe the pause before any delay had
been requested. That ordering held on a fast machine and lost on a loaded
GitHub runner, where the assertion saw no delay at all.

The wait now covers both. The queue itself is not affected: the reason is
what gates `pump`, and it is in place before either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@g8row
g8row merged commit 2b04b09 into main Sep 20, 2026
1 check passed
@g8row
g8row deleted the fix/review-followups branch September 20, 2026 15:48
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.

1 participant