Skip to content

build(deps): bump view shot to better support new arch - #13864

Merged
gkartalis merged 2 commits into
mainfrom
gkartalis/bump-view-shot
Jul 31, 2026
Merged

gkartalis merged 2 commits into
mainfrom
gkartalis/bump-view-shot

Conversation

@gkartalis

@gkartalis gkartalis commented Jul 31, 2026 •

Copy link
Copy Markdown
Member

This PR resolves []

Description

Bumps react-native-view-shot to latest for better new arch support

Nothing changed behavior wise

Platform After
Android
debug.mov
iOS
debugafter.mp4
iOS share
shareAfter.mp4

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • build(deps): bump view shot to better support new arch

Need help with something? Have a look at our docs, or get in touch with us.

@gkartalis gkartalis self-assigned this Jul 31, 2026
@artsyit

artsyit commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Dev changes (build(deps): bump view shot to better support new arch - gkartalis)

Generated by 🚫 dangerJS against b33b8f7

@github-actions github-actions Bot deleted a comment from claude Bot Jul 31, 2026
Comment thread tsconfig.json
Comment on lines +13 to +19
"paths": {
// react-native-view-shot's `exports` map points the "react-native" condition at its raw
// `src/index.tsx`, so tsc type-checks the library source (skipLibCheck only covers `.d.ts`)
// and our noUnusedLocals flags its unused imports. Point tsc at the shipped declarations
// instead; Metro still bundles the react-native source. Remove once upstream is clean.
"react-native-view-shot": ["../node_modules/react-native-view-shot/lib/index.d.ts"]
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comment explaining the what, but "Remove once upstream is clean" has no way to be actioned. Add a link to the upstream issue (or open one) so the next person bumping this knows what to check.

Two notes on the approach:

  • This makes tsc read lib/index.d.ts while Metro and Jest bundle src/index.tsx. Fine as long as lib/ is emitted from that same source, but it's a divergence to be aware of if the library's build ever changes.
  • The repo already patches 12 packages (.yarn/patches/), so a yarn patch removing the unused imports from the library's src/index.tsx is an alternative that keeps tsc and Metro on the same file. Trade-off is patch maintenance on every bump, so the tsconfig override is a reasonable call — just flagging the option.

Also worth confirming this paths block didn't disturb baseUrl: "src" resolution for app/… imports. TypeScript falls back to baseUrl when no paths pattern matches, so it should be fine, but yarn type-check passing is the proof.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Bumps react-native-view-shot from 4.0.3 to 5.1.1 for better new-arch support. The type surface changed with the major: ViewShot's ref is now ViewShotRef instead of the class instance, so ShareSheet.tsx and InstagramStoryViewShot.tsx are updated. A tsconfig.json paths override is added to keep tsc off the library's raw source.

The code changes are minimal and correct for the new ref type. Comments below are about verification, not the diff.

Issues Found

🟡 Important

Android is unverified for a native-module major bump. The checklist has Android unchecked, and both video cells in the description are still placeholder xxx. android/gradle.properties:46 has newArchEnabled=true, and the Android snapshot implementation is precisely what changed between v4 and v5 — that's the stated motivation for the bump. CI builds Android (build-app-android), so compile breakage would be caught, but a Fabric view-hierarchy capture regression would not. Please run the Instagram Stories share on an Android device and attach the recording.

🟢 Suggestion

The capture path has no test coverage. src/setupJest.tsx:389 mocks the module to {} and src/app/Components/ShareSheet/__tests__/ only contains helpers.tests.ts. Nothing exercises shareOnInstagramStory (ShareSheet.tsx:54-138) — including the retry loop and the base64 handling. A test that mocks react-native-view-shot with a stub ViewShot plus a capture() returning fixture base64, then asserts the Share.shareSingle payload, would catch exactly the class of breakage a major bump can introduce. Not required for this PR, but this is the second time these call sites have moved.

See the inline comment on tsconfig.json for notes on the paths override.

Areas Reviewed

  • Architecture — ViewShotRef threaded correctly through InstagramStoryViewShotProps.shotRef as Ref<ViewShotRef>; the ViewShot default import stays only where the component is rendered, and ShareSheet now imports the type alone. Clean.
  • Native — ios/Podfile.lock shows only the version and checksum changing; the pod's own dependency list and the deployment target are untouched. scripts/utils/doctor.js:193 keys off the pod name, which is unchanged, so the pod check exception still holds. Android autolinks, and v5's react-native: ">=0.76.0" peer is satisfied.
  • Security — no new transitive deps; html2canvas was already the sole dependency in v4. enableScripts: false in .yarnrc.yml means no postinstall exposure.
  • Testing — see above.

Questions for Author

  1. Does v5 still return raw base64 for options={{ result: "base64" }}? ShareSheet.tsx:123 prefixes the result with data:image/png;base64,. If v5 ever returns a data URI, that double-prefixes and Instagram share fails silently rather than throwing, so it wouldn't hit any of the retry branches. You tested on iOS so presumably it's unchanged — just confirming it was the happy path you exercised.
  2. Related: is the line-break strip at ShareSheet.tsx:109 still needed, or did that upstream issue get fixed in the interim?
  3. Anything else in the v4 → v5 changelog that touches options, captureMode, or ref timing? The retry-with-delay scaffolding in shareOnInstagramStory exists because captures were flaky; if v5 fixes the underlying race, that's worth a follow-up ticket to simplify.

@MounirDhahri MounirDhahri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gkartalis
gkartalis merged commit 8d904fd into main Jul 31, 2026
19 of 20 checks passed
@gkartalis
gkartalis deleted the gkartalis/bump-view-shot branch July 31, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants