Skip to content

fix: artsy lens improvements and fixes - #14014

Merged
nickskalkin merged 7 commits into
mainfrom
nickskalkin/artsy-lens-code-review-fixes
Sep 3, 2026
Merged

nickskalkin merged 7 commits into
mainfrom
nickskalkin/artsy-lens-code-review-fixes

Conversation

@nickskalkin

@nickskalkin nickskalkin commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

Addressing @MrSltun 's and @MounirDhahri 's code review comments from #13959. Each change is in a separate commit, so it should be easier to review commit by commit.

Here is what was done:

  • Pinned the new Expo dependencies to exact versions.
  • Updated Lens to use standard stack navigation and allow opening artworks normally.
  • Changed all “Search by photo” copy to “Search by Photo.”
  • Replaced React Native touchables with Palette touchables.
  • Simplified the camera error message.
  • Removed redundant comments.
  • Made useAppState reactive and removed the duplicate useIsForeground hook

Here is what was not done and why:

  • Did not turn LensAnalyzing into an overlay because the current screen-based flow is simpler and handles success, failure, and cleanup safely.
  • Did not replace expo-image-manipulator because the existing crop picker cannot apply our calculated crop rectangle automatically.
  • Did not add an RFC for VisionCamera/Nitro for now. The Nitro packages are required peer dependencies of VisionCamera.

Thank you both!

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

#nochangelog

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

@nickskalkin nickskalkin self-assigned this Sep 3, 2026
Comment thread src/app/Scenes/Lens/Screens/LensResults.tsx
Comment thread src/app/utils/useAppState.tsx Outdated
@nickskalkin
nickskalkin force-pushed the nickskalkin/artsy-lens-code-review-fixes branch from b4b0fb5 to a907805 Compare September 3, 2026 11:10
@github-actions github-actions Bot deleted a comment from claude Bot Sep 3, 2026
@artsyit

artsyit commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator
Warnings
⚠️

✅ No changelog changes

Generated by 🚫 dangerJS against a907805

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review

Summary

Follow-up cleanup for Artsy Lens (Search by Photo): switches the Lens flow from a modal presentation to standard in-tab stack navigation, so artwork results open through the normal href route and back/close use the global goBack(). Also pins Expo deps, standardizes "Search by Photo" copy, swaps RN touchables for Palette Touchable, simplifies the camera error copy, and replaces the useIsForeground wrapper with a reactive useAppState usage.

Issues Found

None blocking. The navigation change is internally consistent:

  • Dropping alwaysPresentModally and using goBack() everywhere (LensCamera, LensAnalyzing, LensResults header + Android hardware back) all leave the whole Lens route rather than popping the inner stack onto the still-mounted camera. The old dismissModal(() => navigate("/search")) dance is no longer needed now that Lens lives in the active tab stack.
  • Removing disableNavigation + the custom handlePress in LensResults is correct: results now navigate via RouterLink's to={artwork.href}, which resolves from inside the tab stack. Test updated to assert navigate("/artwork/some-artwork-slug"). disableNavigation on RouterLink and the other list components is untouched — only the ArtworkGridItem / MasonryInfiniteScrollArtworkGrid pass-through (Lens was the sole consumer) is removed.
  • useIsForeground is fully deleted with no remaining references; LensCamera now drives re-renders through useAppState({ onChange: setAppState }) plus a local appState state, which is the right pattern since the hook keeps its value in a ref and would not otherwise re-render.

Areas Reviewed

  • Bugs & edge cases: LensAnalyzing still calls navigation.replace("LensResults", …); the global goBack() pops the outer route as intended. No dangling imports — LensResults now imports only goBack, and the no-restricted-imports disable comment is gone with it.
  • Behavior change worth confirming (not a bug): removing onPress from the LensResults grid means taps now fire the default ArtworkGridItem tracking with contextScreenOwnerType={OwnerType.search}. updateRecentSearchesOnTap defaults to false and is not passed, so no recent-search write happens — only the tap tracking event, which looks intended.
  • Testing: good coverage — new useAppState unit tests (mount value, foreground/background/change callbacks, listener cleanup), plus goBack close/back tests for LensCamera, LensAnalyzing, and LensResults (header + hardware back), and a routes test asserting Lens no longer presents modally and still hides the bottom tabs.

Questions for Author

  • The Lens.tsx comment now says detachInactiveScreens={false} is retained from ReverseImage. The original all-caps warning was specific to RN modals freezing the camera. Now that Lens is no longer alwaysPresentModally, does that constraint still hold, or is this now purely precautionary? Fine to keep either way; just confirming the rationale is current.

Screenshots and AI-disclosure: the checklist already confirms Android + iOS testing with screenshots, and no AI co-author is present, so nothing to flag there.

@nickskalkin
nickskalkin requested a review from MrSltun September 3, 2026 11:16

@MrSltun MrSltun 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.

Looks good! Thank you!! 🌟

@nickskalkin
nickskalkin merged commit 917ad9f into main Sep 3, 2026
14 checks passed
@nickskalkin
nickskalkin deleted the nickskalkin/artsy-lens-code-review-fixes branch September 3, 2026 12:19
@MounirDhahri

Copy link
Copy Markdown
Member

LGTM thanks a lot. I would only suggest to revisit the decision of opening an RFC to adopt the new package. The rationale behind that is that nitro modules, although simple and open up a new world of opportunities for us, they are also sensitive to react native upgrades and it’s a decision that will be hard to roll back from (because others will probably build on top of it)

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.

4 participants