Take screenshots on the web beta, and of cloud strategies anywhere - #229
Merged
Merged
Conversation
The screenshot button read the open strategy back from the local Hive box after a forced save. Cloud strategies are never in that box, so a screenshot of one silently did nothing, on desktop as well as web. It then wrote the PNG with dart:io, which a browser cannot do, which is why the web beta hid the button behind "desktop-only". The capture now reads the page as it is on the canvas (the editor's live providers, drawings copied so their cached paths stay the editor's), so local and cloud strategies capture the same way and nothing waits on a save or a sync. The PNG goes out through FilePicker.saveFile(bytes:), which writes the chosen file on desktop and downloads it in a browser. Images: the capture renders in its own provider container, which has no live cloud page, upload queue or pending bytes. Before it starts, every image on the page is resolved to what it will paint and handed in through captureImageSourcesProvider: files and in-memory bytes as they are, cloud URLs fetched to bytes (downloadCloudImageBytes, shared with the desktop media cache, including its one signed-URL refresh), and images the editor shows as unavailable stay unavailable. An image still loading or a fetch that fails stops the capture with a message instead of saving a picture with the image missing. PendingImageBytes becomes ImageBytes, since bytes fetched for a capture are not an upload. Screenshot leaves PlatformPolicy.webBeta.desktopOnly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review follow-up. A capture awaits image downloads before it renders, and the editor keeps editing its own objects in place meanwhile, so the page snapshot now clones agents, abilities, text, images and utilities (and deep-copies the lineup graph) instead of sharing them; an edit made while a slow image downloads can no longer land in half the picture. Fetched bytes were also never proven to be an image: a 200 with truncated bytes, or a decode slower than the 800 ms settle, would have saved a PNG with the picture missing. resolveCaptureImages now decodes every image that paints into the image cache and holds it there (keepAlive) until the capture releases it, so the capture's widgets find each picture already decoded, and a decode failure stops the capture with the same message as a failed fetch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A signed-out reader who opened a strategy link (#225) can take a screenshot too. Their only access to its images is that link, so the capture's signed-URL refresh carries the link's token, as the media cache does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Review follow-up. A keepAlive handle keeps a decoded image's completer alive, but once the last listener goes the image cache stops tracking it as live, so with enough large images an earlier one could be evicted and decoded again mid-capture, which is the missing-image timing the decode step exists to prevent. Each image now stays listened to until the capture releases it, which keeps it among the cache's live images. A stalled image request could also hold a capture forever: each download now times out after 60 s and fails the capture like any failed fetch. resolveCaptureImages takes a checkpoint, run before each image, so a caller can stop the work between images. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review follow-up: an image whose later frame failed to decode let go of its stream in the error handler, and release() then removed the listener again, which throws once the stream is disposed and would have stopped the cleanup of the images after it. Letting go is now idempotent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Found in the browser run: each screenshot was followed by "Cloud connection lost". A capture renders in its own provider container, and hydrating its strategy provider builds that container's own auth provider, since the strategy provider listens to auth. A real auth provider configures the one Convex client the app syncs through: built with no session it clears that client's auth, with one it replaces the editor's token fetcher, and disposing it tears the auth down. Either way the editor's session ends. createCaptureContainer now builds every capture's container with an inert signed-out auth provider (nothing a capture paints needs auth) and the capture's image sources. A test records every call a capture makes to the Convex auth API and requires none; before this change it saw clearAuth. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Comments Outside DiffThese findings could not be posted inline.
|
Greptile follow-ups. A page's cloud images downloaded one after another, so the wait was the sum of them; they now start together and decode in order, with the same checkpoints and cleanup, and an image still loading stops the capture before any download starts. And the screenshot guard cleared before the save dialog closed, so a second click could capture again and open a second dialog; the guard now holds until the save is done. Both have tests; the second fails without the fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Greptile follow-up: when one image failed or the capture was cancelled, the downloads already started for the others kept running, and a retry started them again alongside. A capture's downloads now share one HTTP client that is closed when resolution ends, which aborts whatever is still in flight (BrowserClient and IOClient both abort on close). A test fails one image while another waits and requires the client closed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment on lines
+79
to
+82
| imageId: http.runWithClient( | ||
| () => _guard(() => fetch(imageId, url)), | ||
| () => client, | ||
| )..ignore(), |
Contributor
There was a problem hiding this comment.
When a cloud image’s signed URL has expired, the downloader requests a fresh URL. The first http.get closes the client shared here, so the retry fails even when the fresh URL is valid. The screenshot cannot be saved; video export in PR #230 uses the same image-resolution path. Keep the capture-scoped client open until all requests finish.
Artifacts
- The log includes the exact executed before command and its output, showing a successful refreshed-URL capture.
- The log includes the exact executed after command and its output, showing the closed-client failure.
- The authored test exercises both capture implementations against the same concurrent-request and signed-URL retry scenario.
- The parent-commit source supplies the pre-change implementation exercised by the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gjorgji asked on Discord for record and screenshot on the web. On the web beta the Screenshot button only said "Screenshot is desktop-only for now." That gate was a line in
PlatformPolicy.webBeta, set when the beta shell landed (#183). Nobody had checked whether screenshots could work in a browser. Two things did stop it:strategiesBox, and cloud strategies are never there.newStrat == null → returnthen made the button do nothing, silently, on desktop too for every cloud strategy.dart:io, which a browser can't do.Now the capture reads the page as it is on the canvas, and the PNG goes out through
FilePicker.saveFile(bytes:). On desktop that writes the chosen file; in a browser it downloads it.Taken by the real capture code in headless Edge (dart2js build), from a cloud strategy whose image is served by URL. The magenta block is that image.
What changed
lib/screenshot/page_screenshot.dart:captureEditorPagebuilds the page from the editor's live providers.lib/screenshot/capture_images.dart: the capture renders in its ownProviderContainer, which has no live cloud page, upload queue or pending bytes. SoresolveCaptureImagesworks out every image before rendering starts:downloadCloudImageBytes, now shared with the desktop media cache, including its one signed-URL refresh and Let anyone with a strategy link view it without an account #225's share token.captureImageSourcesProvider(strategy_image_source.dart) is how the capture container gets those sources.watchStrategyImageSourcechecks it first; everywhere else it is null, so the editor behaves exactly as before.PendingImageBytesbecomesImageBytes, since bytes fetched for a capture aren't an upload.createCaptureContainer(offscreen_capture.dart) builds the capture's container with an inert, signed-out auth provider. Found in the browser run: the capture's strategy provider listens to auth, so the capture built its own realAuthProvider. That provider cleared or replaced auth on the app's one Convex client, and disposing it tore the auth down. Every screenshot showed "Cloud connection lost" and paused sync. A test records every call a capture makes to the Convex auth API and requires none; before the fix it caughtclearAuth.PlatformPolicy.webBeta.desktopOnly. Local-strategy screenshots no longer force a save first, because nothing reads the saved copy any more.Reproduction
Web beta, a cloud strategy open, click the camera. Expected: a PNG downloads. Actual: "Screenshot is desktop-only for now." Desktop, a cloud strategy: nothing happens.
Verification
test/page_screenshot_test.dart: a cloud page with a web-style image fetched from its URL renders to a 1920x1080 PNG with the image's pixels in it; loading, failed-fetch and undecodable images stop the capture; edits after the snapshot don't reach it; the toolbar spinner clears after a failure.test/capture_images_test.dart: the image resolution, and that decoded images are held until released.captureEditorPagefor a cloud strategy in headless Edge. It produced a 1920x1080 PNG in 1.5 to 1.8 s with the cloud image painted (image above).flutter build webwith CI's flags compiles.Not covered
icarus-cloud(it was missing Refuse deleting a lineup origin or landing a live link still names #213's validator and an index), so sync from any current client failed there. I pushed the currentconvex/to dev. Production is deployed on merge and wasn't touched.🤖 Generated with Claude Code
Not safe to merge until refreshed cloud-image downloads can complete.
Findings
Summary
The PR adds screenshots for the web beta and cloud strategies. When a cloud image needs a refreshed signed URL, its download can fail because the shared HTTP client has already closed. Video export in PR #230 uses the same image-resolution path. This must be fixed before merging.
Reviews (3) · Last reviewed commit: "Abort a capture's other downloads when i..."