Skip to content

feat(FF): Add android in app update - #10976

Open
MrSltun wants to merge 12 commits into
mainfrom
mrsltun/feat/add-android-in-app-update-prompt
Open

feat(FF): Add android in app update#10976
MrSltun wants to merge 12 commits into
mainfrom
mrsltun/feat/add-android-in-app-update-prompt

Conversation

@MrSltun

@MrSltun MrSltun commented Oct 18, 2024

Copy link
Copy Markdown
Member

This PR resolves []

Description

This PR adds the native Android In-App Updates

android-in-app-updates-demo-low.mp4

@MrSltun MrSltun self-assigned this Oct 18, 2024
@MrSltun
MrSltun force-pushed the mrsltun/feat/add-android-in-app-update-prompt branch from a18fc14 to 2b81a87 Compare March 24, 2025 10:53
@MrSltun
MrSltun force-pushed the mrsltun/feat/add-android-in-app-update-prompt branch from 2b81a87 to 3a9bdad Compare July 30, 2025 09:55
@MrSltun
MrSltun force-pushed the mrsltun/feat/add-android-in-app-update-prompt branch from 3a9bdad to 57d181e Compare August 22, 2025 08:41
@MrSltun
MrSltun marked this pull request as ready for review August 25, 2025 13:28

Copilot AI left a comment

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.

Pull Request Overview

This PR implements Android In-App Updates functionality that allows users to update the app without going to the Play Store. The implementation uses Google Play's App Update API to check for available updates and handle the update process with user notifications.

Key changes:

  • Adds Android In-App Update functionality using Google Play's App Update API
  • Implements update checking and notification system with React Native bridge
  • Adds toast notifications to prompt users when updates are downloaded

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/setupJest.tsx Adds Jest mocks for new update-related native module methods
src/app/utils/usePromptForUpdate.tsx New hook that manages update checking and user notifications
src/app/Scenes/HomeView/HomeView.tsx Integrates the update prompt hook into the home screen
src/app/NativeModules/ArtsyNativeModule.tsx Adds React Native bridge methods for update functionality
android/app/src/main/java/net/artsy/app/MainActivity.kt Implements Android In-App Update logic using Google Play API
android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java Adds native module methods to bridge update functionality to React Native
android/app/build.gradle Adds Google Play App Update dependencies
Comments suppressed due to low confidence (1)

src/app/NativeModules/ArtsyNativeModule.tsx:27

  • The hardcoded timeout value 12000 is a magic number. Consider extracting this to a named constant with a descriptive name that explains why this delay is necessary.
        }

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/app/utils/usePromptForUpdate.tsx Outdated
Comment thread src/app/utils/usePromptForUpdate.tsx Outdated

Copilot AI left a comment

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

checkForAppUpdate:
Platform.OS === "ios"
? () => {
console.error("checkForAppUpdate is not supported on iOS")

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.

outside scope of this pr, but I wonder if there is a way to get errors at type check time or with eslint when we use a native module without a platform check.

brainbicycle
brainbicycle previously approved these changes Aug 25, 2025

@brainbicycle brainbicycle left a comment

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.

the code and experience looks really great and good to have the capability!

I am wondering what our policy should be about using this? maybe a discussion for mobile practice or a separate meeting?

@MounirDhahri

Copy link
Copy Markdown
Member

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot free trial expires on September 8, 2025
Learn more in the Cursor dashboard.

Comment thread src/app/NativeModules/ArtsyNativeModule.tsx
MounirDhahri
MounirDhahri previously approved these changes Aug 26, 2025

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

Nice one! LGTM as well. Let's discuss how aggressive we want to be with this

setTimeout(() => {
// delay prompt until homescreen loads
ArtsyNativeModule.checkForAppUpdate()
}, 12000)

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.

The home screen takes no more than 3 seconds to load - so we can maybe decrease this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried 5,7, and 10, and the update prompt shows up so quickly that before the homescreen loads 😅 so I went with 15 and was too long, so I ended up with 12 and it was perfect

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.

oki, thanks.

@MrSltun
MrSltun dismissed stale reviews from MounirDhahri and brainbicycle via 6073203 August 26, 2025 14:12
@MrSltun
MrSltun force-pushed the mrsltun/feat/add-android-in-app-update-prompt branch from 6073203 to a0650d9 Compare July 23, 2026 10:09
@artsy artsy deleted a comment from ArtsyOpenSource Jul 23, 2026
@artsyit

artsyit commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Android user-facing changes (Add Android In App Update - MrSltun)

Generated by 🚫 dangerJS against a0650d9

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Adds Android In-App Updates via the Play Core app-update library: native checkForAppUpdate/completeAppUpdate bridge methods on MainActivity, an onAppUpdateDownloaded event, and a usePromptForUpdate hook wired into HomeView that shows a "Reload to apply" toast. The Android flow looks solid. A few notes inline: the iOS completeAppUpdate shim has a bug, and the new hook has no test coverage or timer cleanup.

completeAppUpdate:
Platform.OS === "ios"
? console.error("completeAppUpdate is unsupported on iOS")
: NativeModules.ArtsyNativeModule.completeAppUpdate,

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.

This calls console.error at import time and assigns its return value (undefined) to completeAppUpdate. ArtsyNativeModule is imported at app startup, so every iOS launch logs "completeAppUpdate is unsupported on iOS" even when the method is never called, and ArtsyNativeModule.completeAppUpdate is undefined rather than a callable no-op. Wrap it in a function like checkForAppUpdate above.

Suggested change
: NativeModules.ArtsyNativeModule.completeAppUpdate,
completeAppUpdate:
Platform.OS === "ios"
? () => {
console.error("completeAppUpdate is unsupported on iOS")
}
: NativeModules.ArtsyNativeModule.completeAppUpdate,

Comment on lines +21 to +24
setTimeout(() => {
// delay prompt until homescreen loads
ArtsyNativeModule.checkForAppUpdate()
}, 12000)

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.

🟢 The 12s timer isn't cleared on unmount. If the user leaves Home before it fires, checkForAppUpdate still runs (and the whole flow re-arms every time Home remounts). Keep the id and clear it in the cleanup alongside eventListener.remove():

Suggested change
setTimeout(() => {
// delay prompt until homescreen loads
ArtsyNativeModule.checkForAppUpdate()
}, 12000)
const timeoutId = setTimeout(() => {
// delay prompt until homescreen loads
ArtsyNativeModule.checkForAppUpdate()
}, 12000)

Then add clearTimeout(timeoutId) to the returned cleanup.

* This is used to check for app updates on every app launch.
* Also monitors for downloaded updates and shows restart toast.
*/
export const usePromptForUpdate = () => {

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.

🟡 No test covers this hook. Worth one that mocks ArtsyNativeModule and asserts: on Android the toast shows when updateDownloaded is true and when the onAppUpdateDownloaded event fires, the toast CTA calls completeAppUpdate, and nothing runs on iOS. That would also have caught the completeAppUpdate shim bug flagged in ArtsyNativeModule.tsx.

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.

5 participants