fix(windows,ios): releaseCapture, error rejection, file:// URIs, podspec minimum#642
Merged
fix(windows,ios): releaseCapture, error rejection, file:// URIs, podspec minimum#642
Conversation
…dspec iOS - Android: ViewShot.proposeSize now uses Math.max so the ReusableByteArrayOutputStream is sized to the bitmap (was capped at 32 bytes, forcing a realloc on every capture). - Windows: implement releaseCapture (was a TODO stub leaking temp files in ApplicationData.Current.LocalFolder). - Windows: switch error paths from tcs.SetResult(ex.Message) to tcs.SetException so JS promises actually reject; same fix for the unsupported-format early return. - Podspec: bump iOS minimum from 9.0 to 15.1 to match the React Native floor (the code already requires UIGraphicsImageRenderer / iOS 10+). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR is a low-blast-radius cleanup focusing on fixing platform-specific capture correctness/performance issues across Android, Windows, and iOS packaging.
Changes:
- Android: fixes
proposeSizecalculation used to pre-size the reusable output buffer. - Windows: implements
releaseCapturecleanup for tmpfiles and fixes promise rejection behavior on native errors / unsupported formats. - iOS: raises the podspec minimum iOS version to
15.1.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| windows/RNViewShot/RNViewShotModule.cs | Implements Windows tmpfile cleanup and corrects error handling to reject JS promises on failure. |
| react-native-view-shot.podspec | Updates iOS minimum deployment target declared by the podspec. |
| android/src/main/java/fr/greweb/reactnativeviewshot/ViewShot.java | Adjusts Android buffer size proposal logic to avoid always choosing a tiny size. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review feedback on PR #642. - ViewShot.proposeSize: revert to Math.min. The function's return value is set on a ReusableByteArrayOutputStream that is immediately discarded (lines 245-247) — innerBuffer() returns the same array that was passed in, and the actual capture methods build their own streams from the static outputBuffer. So the buffer-sizing intent never reaches anything that matters; changing the formula has no functional effect. The existing ProposeSizeTest pinned this dead-code behaviour to flag any change as deliberate, which is exactly what it caught. - Windows releaseCapture: replace the StartsWith prefix check with Path.GetRelativePath so sibling paths like "...\LocalState2\file.png" cannot match a "...\LocalState" prefix and slip through. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+25
to
+36
| if (string.IsNullOrEmpty(uri)) return; | ||
| // Only delete files that actually live inside the app's local | ||
| // folder (where ViewShot.GetStorageFile writes). Use | ||
| // Path.GetRelativePath rather than a string prefix check, which | ||
| // would otherwise let sibling paths like | ||
| // "...\LocalState2\file.png" pass a "...\LocalState" prefix. | ||
| var localFolder = ApplicationData.Current.LocalFolder.Path; | ||
| string fullPath; | ||
| try | ||
| { | ||
| fullPath = Path.GetFullPath(uri); | ||
| } |
| throw new ArgumentException("Unsupported image format: " + format + ". Try one of: png | jpg | jpeg"); | ||
| } | ||
|
|
||
| var tcs = new TaskCompletionSource<string>(); |
| catch (Exception ex) | ||
| { | ||
| tcs.SetResult(ex.Message); | ||
| tcs.SetException(ex); |
Address Copilot feedback round 2 on PR #642. - releaseCapture: parse file:// URIs first via Uri.TryCreate; previous Path.GetFullPath would throw on a `file:///...` input and silently leak the file. JS callers commonly pass URIs (matches Android Uri.parse). - TaskCompletionSource: opt into RunContinuationsAsynchronously so promise continuations don't pile up on the dispatcher thread; switch SetResult/SetException to TrySetResult/TrySetException for idempotent completion. - Drop verbose comments and collapse the trivial best-effort try/catch.
gre
changed the title
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
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.
Summary
Tier 1 of a 3-PR cleanup pass — concrete bugs with low blast radius.
releaseCapture— was a// TODO implement mestub, leaking temp files written toApplicationData.Current.LocalFolderindefinitely. Now usesPath.GetRelativePath(not a string prefix check) to confirm the URI lives inside the local folder before deleting. Also handlesfile://URIs (JS callers commonly pass them).tcs.SetResult(ex.Message)was returning the exception string as if it were a successful capture URI, so JS promises never rejected on Windows. Switched toTrySetException(ex)(andTrySetResulton the success path) withTaskCreationOptions.RunContinuationsAsynchronouslyso continuations don't pile up on the dispatcher thread. Same shape for the unsupported-format early return (throw new ArgumentExceptioninstead of returning the message).s.platform = :ios, "9.0"is misleading (the code already usesUIGraphicsImageRenderer, iOS 10+). Bumped to15.1to match the current React Native floor (the lib's existing peerDependencyreact-native >=0.76.0already requires this, so no new floor for valid consumers — but worth a CHANGELOG line on release).Test plan
npm run type-check— passesnpm run lint:ci— passesnpm test— 46 / 46releaseCapture("file:///…/LocalState/foo.png")deletes the file🤖 Generated with Claude Code