Skip to content

APP-2977: Handle AppView polling errors - #11590

Open
mozzius wants to merge 2 commits into
mainfrom
samuel/app-2977-handle-appview-polling-errors
Open

APP-2977: Handle AppView polling errors#11590
mozzius wants to merge 2 commits into
mainfrom
samuel/app-2977-handle-appview-polling-errors

Conversation

@mozzius

@mozzius mozzius commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • make the until predicate type accurately reflect its existing undefined-response and error behavior
  • audit every until callsite and guard predicates that assumed a successful response
  • preserve deletion polls that intentionally complete when AppView starts returning an error
  • add regression coverage for retries and error-aware completion

Test plan

  • Temporarily reject an AppView follow lookup during onboarding and confirm polling retries without throwing.
  • Delete a list or starter pack and confirm the flow completes once AppView begins returning an error for it.

Fixes APP-2977
Fixes APP-T4QA

Sentry: https://blueskyweb.sentry.io/issues/7692780353/?project=4508807082278912

@linear-code

linear-code Bot commented Aug 28, 2026

Copy link
Copy Markdown

APP-2977

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @mozzius's task in 2m 3s —— View job


Claude review

  • Read .github/claude-review-prompt.md and CLAUDE.md
  • Reviewed the diff (until.ts, until.test.ts, oxlint-suppressions.json)
  • Audited all 11 call sites of until for reliance on the removed err arg / cond(undefined) on rejection

The core change is sound: previously cond was invoked with (undefined as T, err) on every rejection, which is exactly the undefined blowup this PR is fixing. Dropping the second cond parameter and retrying on rejection is the right shape, the test coverage is good, and removing the lint suppression is correct. But there's one caller that depended on cond being invoked on the rejection path.

Finding 1 — Delete-starter-pack poll no longer short-circuits (non-blocking, behavioral regression)

src/state/queries/starter-packs.ts:327-329

await whenAppViewReady(appviewClient, uri, v => {
  return Boolean(v?.starterPack) === false
})

This condition detects the pack being absent. Once the AppView has processed the deletion, getStarterPack for that URI returns an error (not an empty body), so previously the poll terminated via the catch branch: cond(undefined, e)Boolean(undefined?.starterPack) === falsetrue → return early. The optional chaining (v?.starterPack) and the old explicit cond(undefined, e) call are what made this work.

After this change cond is never called on rejection, so this poll can no longer be satisfied by the deletion-indexed error. It will burn all 5 retries (~5s) and until resolves false. whenAppViewReady ignores the boolean so nothing crashes, but the delete mutation's onSuccess (cache invalidation + onSuccess() callback) is now delayed by up to ~5s instead of firing as soon as the deletion is indexed — defeating the purpose of the poll for this path.

I'm confident about the code path; the one thing I couldn't verify from here is whether app.bsky.graph.getStarterPack on a deleted pack throws vs. returns {starterPack: undefined}. If it throws (the likely case, and what the old cond(undefined, e) handling implies), this is a real regression.

Suggested direction: give this specific caller a "treat a not-found rejection as satisfied" path — e.g. have until optionally accept an onError predicate, or have the delete cond's fn catch the not-found error and resolve to {starterPack: undefined} so the existing cond(v) still fires. Fix this →

Notes (not blocking)

  • The other two res?-optional conds — the create checks at generate-starterpack.ts:112 and starter-packs.ts:142 (typeof v?.starterPack.uri === 'string') — correctly evaluated to false on the old error path, so retrying instead of calling cond is equivalent. No change in behavior.
  • The update cond at starter-packs.ts:268-270 (currentStarterPack.cid !== v?.starterPack.cid) previously returned true on an error (cid !== undefined), i.e. it short-circuited on rejection too. That old behavior was arguably a bug (an error falsely signaling "indexed"), so retrying now is an improvement — just noting the behavior change.
  • All remaining callers (profile.ts, list.ts, Composer.tsx, Onboarding/util.ts, GermButton.tsx, both verification mutations, CreateListFromStarterPackDialog.tsx) use single-arg conds that never read err, so they're unaffected.

Overall: a clean, well-tested fix for the reported crash. The one item worth confirming before merge is the delete-starter-pack poll above.
· samuel/app-2977-handle-appview-polling-errors

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor
Old size New size Diff
8.06 MB 8.06 MB 87 B (0.00%)

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @mozzius's task in 3m 50s —— View job


Claude review

  • Read .github/claude-review-prompt.md and CLAUDE.md
  • Reviewed the diff (until.ts, until.test.ts, oxlint-suppressions.json)
  • Re-audited all until/whenAppViewReady call sites for null-safety on the rejection path (the approach changed since my last review)

This looks good. The approach was revised since my previous review, and the revision is the right one.

What changed: rather than dropping cond from the rejection path, until now keeps calling it as cond(undefined, err) with an accurate signature — cond: (v: T | undefined, err: unknown). The original crash (cond(undefined as unknown as T, e) then a caller dereferencing undefined.follows inside the un-guarded cond call, which propagated out of until and rejected the mutation) is fixed by making every cond null-safe on v, either via optional chaining or an explicit if (!x) return false guard.

Earlier Finding 1 is resolved. My prior concern was that a version which never invoked cond on rejection would break the delete-starter-pack poll (Boolean(v?.starterPack) === false), since that poll relies on the not-found error to terminate. Because this revision still calls cond on rejection, starter-packs.ts:331 evaluates Boolean(undefined?.starterPack) === falsetrue and short-circuits as before. Same for list.ts delete (v => !v). No regression to the delete timing.

Call-site audit (all null-safe on v === undefined):

  • Optional-chained, resolve false on error → retry: generate-starterpack.ts:112, starter-packs.ts:142, Onboarding/util.ts:46, GermButton.tsx:140/161, CreateListFromStarterPackDialog.tsx, Composer.tsx:1215 (res?.thread.at(0)anchor?.value).
  • Explicit guard if (!x) return false → retry: pinned-post.ts, profile.ts default cond, AutomationLabelSettings.tsx, PwiOptOut.tsx, both verification mutations.
  • Short-circuit true on error (intended delete behavior): list.ts delete, starter-packs.ts delete.

Notes (non-blocking):

  • list.ts:157 list?.name === record.name && list.description === record.description mixes guarded/unguarded access, but it typechecks and is runtime-safe: record.name is the required (non-nullable) list name field, so on the error path undefined === <string> is false and && short-circuits before list.description is read. TS also narrows list to defined inside the truthy && for the same reason. Using list?.description would read more consistently, but it's not required.
  • starter-packs.ts:269 (update cond, currentStarterPack.cid !== v?.starterPack.cid) still resolves true on a rejection, so a transient error during the update poll would stop polling early. This is pre-existing behavior (identical on main) and the update path shouldn't normally throw, so it's out of scope here — just flagging it isn't newly introduced.

Test coverage matches the implementation (rejection passes (undefined, err), retries on unmet conditions, and can short-circuit on an error), and dropping the no-explicit-any suppression is correct now that the any casts are gone.
· samuel/app-2977-handle-appview-polling-errors

@github-actions

Copy link
Copy Markdown
Contributor

The OTA deployment for this PR was successful! You may now apply it by either scanning the QR code or opening the deep link below in your browser:

QR code for the PR OTA deployment

bluesky://intent/apply-ota?channel=pull-request-11590&releaseVersion=1.132.0&iosBuildNumber=2052&androidBuildNumber=1529

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