Ask the server which images it still shows before dropping an upload

[SunkenInTime]

Base icarus-cloud. First half of the page trash; #228 is the second half and stacks on this one.

Merge order

1. Merge this PR. Deploy Web runs: Convex, then the web build.
2. Wait until that Deploy Web run has completed successfully: the Convex deploy and the Cloudflare Pages publish of the web build. Deploy Web cancels a running deploy when a newer push starts, so merging Keep deleted pages in a 30-day trash, with Recently deleted and restore #228 early could cancel this web deploy after its Convex step.
3. Retarget Keep deleted pages in a 30-day trash, with Recently deleted and restore #228 to icarus-cloud if GitHub hasn't, check that it shows only its own changes, then merge it. That turns the trash on.

What changes for users
Nothing today. Before the upload queue drops a pending image upload as no longer wanted, it now asks the server whether the strategy's content still shows that image. Before, it read the strategy's full snapshot and looked for the image there.

Why this ships first
#228 moves deleted pages to a 30-day trash. The trash hides them from the full snapshot, but their images are still wanted, because the page can be restored. A web build that still reads the snapshot can drop the bytes of an image that is still uploading when a teammate trashes its page. After a restore, that image shows as missing. Greptile and Astra both flagged this on #228. Convex deploys before the web build, so the live web build must stop reading the snapshot before the trash is on the server.

Server change (additive only)

• New query images:listReferencedAssetIds({ strategyPublicId, assetPublicIds }) (viewer role). It returns which of the asked images the strategy's content shows, looking each one up in assetReferences through the strategy/asset/deleted index. Tombstoned content is left out.
• The query answers at most 100 ids per call (MAX_REFERENCED_ASSET_IDS_PER_QUERY), so it reads at most 100 rows however large the strategy is. It returns null until the reference backfill has finished; production finished it with Serve the web beta from the production Convex deployment #206.
• strategy:getFullSnapshot takes a new optional argument, acceptsTrashedPagesLeftOut, ignored here. It tells Keep deleted pages in a 30-day trash, with Recently deleted and restore #228's server that this client doesn't decide uploads from the snapshot. Keep deleted pages in a 30-day trash, with Recently deleted and restore #228 refuses, with CLIENT_UPGRADE_REQUIRED, a snapshot to any client that doesn't send it while the strategy holds trashed pages. A tab from before this PR's deploy can then fail an export until it reloads, but it can't drop image bytes: its upload check keeps them when the read fails.
• No schema change. The web build live now never calls the new query and never sends the new argument.

Client change

• ConvexStrategyRepository.fetchFullSnapshot always sends acceptsTrashedPagesLeftOut: true (export, apply-to-all-pages, video export, share-link reads all go through it).
• ConvexStrategyRepository.fetchReferencedAssetIds asks in batches of 100. If any batch can't tell, the whole answer is null.
• The upload queue's three reference checks now ask about exactly the uploads they are deciding on: the recheck after discarded work, dropping a stale staged job, and promoting jobs at restart.
• If the server can't tell (null), or the query fails or is missing (for example on a deployment that lags, like dev), nothing is dropped. A staged upload (its placing change has not landed) keeps its check and its bytes and waits for a real answer. A durable upload (its change landed) may upload, as it could before when the snapshot read failed.

Correct on its own, before and after #228

• Today's server has no trash. For the image references this client writes (an image element's data.id, a lineup link's images[*].id), the reference rows and the snapshot name the same images. Astra found two edge differences:
  • The old lineup check matched any nested id in a lineup payload, so an upload whose id equalled some lineup's entity id was kept by accident. The references count only screenshots.
  • References briefly still name images on a page an older delete removed, until its orphan purge runs. That keeps bytes; it never drops them.
• After Keep deleted pages in a 30-day trash, with Recently deleted and restore #228, the trash keeps a trashed page's content and reference rows, so its images still count as referenced. That is what this client needs to know.
• If this build ever ran without its server half, the query would be missing: the call throws, and the queue keeps everything.

Left for Dara

• Nothing blocks this PR. The old-tab gap is closed by Keep deleted pages in a 30-day trash, with Recently deleted and restore #228's snapshot refusal together with the argument this PR adds. What remains is a cost: a tab from before this deploy that stays open through Keep deleted pages in a 30-day trash, with Recently deleted and restore #228's deploy can't export a strategy that has trashed pages until it reloads.

Review
Static review by Codex (GPT-6 Astra), STATIC ONLY.

• Round 1: one blocking finding. The null-answer test claimed nothing uploads, but a durable job may. The test now uses a staged job. Two non-blocking findings:
  • The first version read every reference row, which could pass Convex's 32k read limit on huge strategies. The query is now bounded, as described above.
  • The body overstated equivalence with the snapshot check. It is corrected.
• Round 2: no blocking findings; "good to ship". Its one note, this body being out of date, is fixed here.
• A separate rollout pass found that tabs from before this PR could still lose bytes once Keep deleted pages in a 30-day trash, with Recently deleted and restore #228 deploys. This PR now adds the snapshot argument, and Keep deleted pages in a 30-day trash, with Recently deleted and restore #228 refuses snapshots without it. A second rollout pass confirmed the path closed.

Tests (at 79e444e)

• npm run test:convex: 175 passed, including the new convex/referencedAssetIds.test.ts (5, the fifth a full snapshot read with the new argument):

  • answers only for images the content shows, deleted content left out;
  • refuses more than 100 ids;
  • returns null before the backfill;
  • a viewer can read it; a stranger is refused.

• flutter test --no-pub: 1356 passed, 5 skipped. New tests:

  • test/collab/referenced_asset_ids_test.dart: asks in batches of 100, deduplicated; a null batch makes the whole answer null; full snapshots are read with acceptsTrashedPagesLeftOut.
  • test/cloud_media_upload_queue_provider_test.dart: an upload whose image the server still names is kept, with its bytes, even when the full snapshot doesn't show it. A staged upload keeps its job and bytes and is not sent while the server can't tell.

  Both queue tests fail when the queue goes back to reading the full snapshot (checked by swapping the loader back).

• npx tsc --noEmit, npm run audit:convex-contract (46 public functions, 38 error codes). convex/function_spec.json was hand-edited in its sorted format, as Refuse lineup links whose origin or landing is gone #209/Refuse deleting a lineup origin or landing a live link still names #213 did. lib/collab/generated was regenerated with tool/icarus_convex_codegen.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • Media upload checks now verify only the requested asset IDs instead of loading a full strategy snapshot, reducing unnecessary data transfer.
  • Checks handle large requests in batches and defer decisions when reference information is unavailable, helping preserve uploads until their status is confirmed.
• Reliability
  • Added coverage for access permissions, deleted or unreferenced images, batch limits, and pending upload recovery.

Confidence Score: 4/5

Do not merge until snapshot requests work with older deployments, or the updated server contract is deployed before this client.

Findings

1.  Older servers reject snapshots ▶
2.  Stacked image calls fail ▶

Summary

The upload queue now asks the server which images remain referenced instead of relying on a full snapshot. Snapshot requests also send a new argument that older server deployments reject, preventing snapshot-dependent actions during a client/server rollout.

_{Reviews (2) · Last reviewed commit: "Say, when reading a full snapshot, that ..."}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f1cdd89b-e90d-4f61-a68b-faa0296b89b2

📥 Commits

Reviewing files that changed from the base of the PR and between 3cbd0f7 and d5f9209.

⛔ Files ignored due to path filters (2)

• lib/collab/generated/convex_models.dart is excluded by !**/generated/**
• lib/collab/generated/icarus_convex_api.dart is excluded by !**/generated/**

📒 Files selected for processing (8)

• convex/function_spec.json
• convex/images.ts
• convex/referencedAssetIds.test.ts
• lib/collab/convex_strategy_repository.dart
• lib/providers/collab/cloud_media_upload_queue_provider.dart
• test/cloud_media_upload_queue_provider_test.dart
• test/collab/referenced_asset_ids_test.dart
• test/web_media_bytes_upload_test.dart

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds a Convex query for referenced asset IDs, a client method that batches requests, and upload-queue checks that use those results instead of fetching full strategy snapshots.

Changes

Referenced asset lookup

Layer / File(s)	Summary
Server query and access contract convex/function_spec.json, convex/images.ts, convex/referencedAssetIds.test.ts	Adds a viewer-authorized query that accepts up to 100 asset IDs. It returns null before backfill readiness; otherwise, it returns sorted IDs with non-deleted references. Tests cover results, request limits, readiness, and access.
Client batching and result aggregation lib/collab/convex_strategy_repository.dart, test/collab/referenced_asset_ids_test.dart	Adds a repository method that deduplicates IDs, fetches batches of up to 100, and combines results. It returns null if any batch result is null. Tests verify batching and null handling.
Upload queue reference checks lib/providers/collab/cloud_media_upload_queue_provider.dart, test/cloud_media_upload_queue_provider_test.dart, test/web_media_bytes_upload_test.dart	Replaces full-snapshot reference checks with requested-ID lookups for staged admission, missing-source cleanup, and job reconciliation. Tests cover the loader change, referenced and unknown results, pending bytes, and recovery behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant UploadQueue as Cloud media upload queue
  participant ReferenceLoader as CloudMediaReferenceLoader
  participant Repository as ConvexStrategyRepository
  participant Convex as images:listReferencedAssetIds
  UploadQueue->>ReferenceLoader: Request references for asset IDs
  ReferenceLoader->>Repository: Fetch referenced IDs
  Repository->>Convex: Query batches of up to 100 IDs
  Convex-->>Repository: Return referenced IDs or null
  Repository-->>ReferenceLoader: Return combined IDs or null
  ReferenceLoader-->>UploadQueue: Return lookup result

Loading

Merge Risk: ⚪ Minimal · up to d5f92

The referenced-image lookup appears ready to merge after normal checks. Unavailable answers preserve staged uploads rather than treating their images as unreferenced.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d5f92

The new lookup limits the data fetched and defers deletion when the server cannot answer. Image retention still depends on the lookup’s accuracy and on completing this web deployment before the planned trash rollout. No introduced security vulnerability was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — An authenticated viewer can ask about up to 100 supplied IDs per call, but results are limited to references in a strategy for which that user has an effective viewer role. The inspected path does not expose cross-strategy reference rows.

Trust Boundaries and Controls

• observed — The viewer check obtains the caller identity and effective strategy role server-side. Unlike the snapshot’s share-token-aware read path, this query has no share-token argument; the inspected authorization path therefore does not broaden access to anonymous share-link holders.

Resilience and Maintainability Implications

• inferred — Deletion still relies on a prior server-reference answer: local account and job checks do not make that answer atomic with local byte removal. This limits what a negative lookup proves under concurrent content changes, although the supplied comparison does not establish a newly introduced race.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (6 skipped: 6…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: querying the server for referenced images before removing uploads.

Full details: Docstring Coverage

Explanation

Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (6 skipped: 6 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[SunkenInTime]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[greptile-apps]

Stacked image calls fail

PR #228 still calls images:listReferencedAssetIds with only strategyPublicId, including from its repository method and page-trash tests. Requiring assetPublicIds rejects those calls before the query runs, so the stacked client cannot check image references. Update PR #228’s callers and generated contract before merging the stack.

Artifacts

Higher-PR caller and candidate validator source

• Captured the executed git-show commands identifying the one-argument caller and the new required validator field, establishing the contract mismatch.

Query test against higher PR before the change

• Ran the higher PR’s query test against its own revision; the one-argument call passed.

Same query test against candidate PR after the change

• Ran the unchanged higher-PR test against the candidate revision; validation rejected the missing `assetPublicIds` field.

_{Ran code and verified through T-Rex}

[SunkenInTime]

Right that the stack must move together, and it already has. #228 (branch page-trash) merged this change in acdcb33: its repository method passes assetPublicIds in batches of 100, its tests pass them, and lib/collab/generated was regenerated from the updated function_spec.json. The one-argument calls you saw are at f3eec36, which predates that merge. No shipped client ever called the one-argument form: the query is new in this PR, and the live web build doesn't call it.

[greptile-apps]

Comments Outside Diff

These findings could not be posted inline.

•  New snapshot argument breaks clients connected to a legacy Convex deployment ▶

  • Bug
    • When the updated client reaches a server running the previous query contract, strategy:getFullSnapshot rejects its request. Cloud strategy export, cloud video export, and the cloud settings operation that call fetchFullSnapshot cannot proceed on that version-skew path.
  • Cause
    • fetchFullSnapshot unconditionally sends acceptsTrashedPagesLeftOut: true, while the previous server’s argument validator does not define that field; the repository does not retry without it.
  • Fix
    • Deploy the server contract before clients send the field, or provide a narrowly scoped compatibility fallback for the legacy argument-validation error.

[greptile-apps]

Older servers reject snapshots

If this client connects to a deployment that has not yet accepted acceptsTrashedPagesLeftOut, fetchFullSnapshot sends an argument the server rejects and does not retry without it. Cloud strategy export, video export, and the all-pages settings action then cannot complete. Deploy the server contract first or handle the older contract.

Artifacts

Local version-skew validation command

• The authored command reconstructs the previous query from Git, runs both local contract checks and the client check, then removes the temporary copy; it provides a reproducible safe run.

Previous and current server contract test

• The authored test sends the same new-client arguments to the previous and current query validators; it isolates the version-skew difference.

Local contract test configuration

• The authored configuration selects only the version-skew test in the local Convex test environment; no deployment is involved.

Previous server rejects the new argument

• A local run against the previous query contract succeeded without the field and rejected the new-client request with an unexpected-field validator error; the legacy contract is incompatible.

Current server accepts the new argument

• A local run sent the same request to the current query and received a one-page snapshot; the updated contract accepts it.

Dart repository retry test

• The authored test exercises `fetchFullSnapshot` against a transport returning the observed validation failure; it checks whether the client retries.

Dart repository propagates rejection after one attempt

• The executed Dart test records the sent arguments, propagated exception, and one query attempt; there is no fallback retry.

Complete local version-skew execution

• The captured command, working directory, exit code, and output cover both server-contract runs and the Dart retry check; all checks reproduced the compatibility failure without deployment.

_{Ran code and verified through T-Rex}

[SunkenInTime]

Production can't hit this: Deploy Web deploys Convex before it publishes the web build, and a cancelled run can leave a new server with the old web, never the reverse. This PR's server accepts the argument before any client sends it. The only backend that lags is the dev deployment (majestic-eel-413), which CI doesn't deploy. It would also be missing images:listReferencedAssetIds, so it has to be redeployed from icarus-cloud before a dev build uses it; both PR bodies say so. I haven't added a retry without the argument. The reason isn't that a flagless read would lose data: against a server that predates the trash it is harmless. It's that telling a validation error apart from other failures means matching error text, and the only case it covers is the dev deployment.

[greptile-apps]

That rollout constraint addresses the finding. Deploy Web publishes the Convex contract before the client bundle, so production cannot receive this client while still serving a server that rejects acceptsTrashedPagesLeftOut; a cancelled run can only leave the new server with the old client, which is compatible.

The lagging majestic-eel-413 deployment is an explicitly known dev-only case that also lacks images:listReferencedAssetIds and must be redeployed before using this build. Given that scope, adding a retry based on matching validation-error text would be brittle and is not necessary. I’m withdrawing this finding.