chore: delete Experimental code paths and assets#7322
Conversation
PR1 of the phased Experimental → Official migration.
- Adds an English-only DeprecationModal shown on every launch in Experimental
builds (gated by !isOfficial) pointing users to the official App Store and
Play Store listings.
- Repoints E2E to Official: new build_official_simulator fastlane lane,
assembleOfficialRelease on Android, IS_OFFICIAL=YES PlistBuddy on iOS,
Official keystore secrets, and Official APK / .app artifact paths.
- Templates all .maestro flow files appId to ${APP_ID}, with the value
injected per-platform by run-maestro.sh (chat.rocket.ios on iOS,
chat.rocket.android on Android).
No deletions of Experimental build infra, code paths, or store records —
those come in PR2 and PR3 once current Experimental users have migrated.
Without output_name the Official scheme produces RocketChat.app while e2e-build-ios.yml uploads from Rocket.Chat.app, causing the artifact step to fail and breaking the Maestro iOS install path. Mirrors the existing output_name in the build_official lane.
PR2 of the phased Experimental → Official migration (design doc + NATIVE-1118).
CI is no longer building or publishing the Experimental variant. The Official
publish pipeline is unchanged in behaviour; this just rips out the parallel
experimental arm from every workflow, composite action, and fastlane lane it
touched.
Workflows
- Delete .github/workflows/build-android.yml and build-ios.yml (the parameterised
workflows whose only remaining caller was the experimental path; Official
already had build-official-{android,ios}.yml).
- Remove android-build-experimental-store / ios-build-experimental-store from
build-pr.yml and build-develop.yml.
- Drop `type` workflow input from build-official-{android,ios}.yml and stop
passing GOOGLE_SERVICES_IOS_EXPERIMENTAL / BUGSNAG_KEY to the iOS composite.
Composite actions
- build-android: drop type/KEYSTORE_EXPERIMENTAL_*/BUGSNAG_KEY inputs and the
experimental arms in keystore decode, gradle.properties, gradle bundle, and
bugsnag upload. Only the Official AAB upload step remains.
- build-ios: drop type/BUGSNAG_KEY/GOOGLE_SERVICES_IOS_EXPERIMENTAL, drop the
experimental fastlane invocation and all `inputs.type == 'experimental'`
artifact uploads. Keep the PlistBuddy `Set IS_OFFICIAL YES` block until PR3
removes the key from the source Info.plists.
- upload-{android,internal-android,ios}: drop type input, the experimental
download branches, and the `Rocket.Chat Experimental` PR-comment label.
Fastlane
- ios/fastlane/Fastfile: delete build_experimental and build_experimental_simulator;
collapse `beta` to a no-options Official lane (chat.rocket.ios + BUGSNAG_KEY_OFFICIAL).
Keep build_official and its `prepare_ios_official.sh` call — PR3 deletes both
in lockstep with the pbxproj rewrite.
- android/fastlane/Fastfile: delete experimental_production; collapse `beta` and
`internal_app_sharing` to no-options Official lanes (chat.rocket.android).
Verified by `grep -rn experimental|Experimental|EXPERIMENTAL .github/ ios/fastlane/Fastfile android/fastlane/Fastfile` returning zero, and by YAML/Ruby
parse on every modified file.
Out-of-band steps (App Store Connect archive, Play Console internal entry, GH
Environments + Secrets cleanup) are tracked separately in the design doc and
are not part of this PR.
https://rocketchat.atlassian.net/browse/NATIVE-1118
PR3 of the phased Experimental → Official migration (NATIVE-1118). Stacked on PR2 (#7321). After this PR the only remaining Experimental references live inside ios/RocketChatRN.xcodeproj/project.pbxproj, the .xcscheme files, ios/Experimental.xcassets/, scripts/prepare_ios_official.sh, and the Podfile target line — all owned by the Xcode-side cleanup that ships alongside this PR (out-of-band, repo owner handles in Xcode). JS - Delete app/containers/DeprecationModal/ and its mount in app/index.tsx - Delete app/lib/constants/environment.ts (isOfficial switch) - app/lib/database/index.ts: drop -experimental suffix from DB path - Remove Experimental_retirement_* keys from 25 locale files iOS - Remove IS_OFFICIAL key from RocketChatRN/ShareRocketChatRN/NotificationService Info.plists - Drop PlistBuddy "Set IS_OFFICIAL YES" step from build-ios/action.yml and e2e-build-ios.yml — IS_OFFICIAL is no longer read by any consumer - Simplify Database.swift: stop reading IS_OFFICIAL, always use the non-suffixed DB path - ShareRocketChatRN/Info.plist CFBundleDisplayName: "Rocket.Chat Experimental" → "Rocket.Chat" Android - Remove `experimental` product flavor + the IS_OFFICIAL buildConfigField from android/app/build.gradle (keep `official` flavor — single-flavor collapse is left as follow-up to minimize CI surface churn) - Delete android/app/src/experimental/ resource directory (40 files: launcher icons, splash, strings, manifest) - Simplify Encryption.java getDatabaseName: drop BuildConfig.IS_OFFICIAL reflection - android/app/src/debug/res/values/strings.xml: "[DEBUG] Rocket.Chat Experimental" → "[DEBUG] Rocket.Chat"
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemove the experimental build variant and IS_OFFICIAL branch: delete experimental Android/iOS assets, flavors and schemes; stop exporting isOfficial; standardize database filenames (drop -experimental suffix); remove DeprecationModal and related i18n keys; update CI/workflows to set Bugsnag apiKey instead of writing Info.plist IS_OFFICIAL flags. (50 words) ChangesSingle-path: remove experimental variant and normalize builds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
216-224:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDatabase file naming mismatch between Android and JS platforms.
The Java code creates physical files with double
.dbextensions (*.db.db), but the JS implementation creates files with single.dbextensions. This causes the Android native code to access different database files than the JS layer, resulting in a cross-platform inconsistency.The Java
getDatabaseName()method appends.db, then passes that toWMDatabase.getInstance()which appends another.db(usingcontext.getDatabasePath(name + ".db")). Meanwhile, the JSgetDatabasePath()also appends.dbbefore passing toSQLiteAdapter, which uses the provided name as-is without further modification.Fix: Either remove the
.dbextension fromgetDatabaseName()in Java (lettingWMDatabasehandle it exclusively), or verify that this intentional difference is correct for the Android implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java` around lines 216 - 224, The getDatabaseName method is appending ".db" which causes WMDatabase (via WMDatabase.getInstance / context.getDatabasePath(name + ".db")) to produce a "*.db.db" file; remove the explicit ".db" suffix from getDatabaseName so it returns the base database name (matching the JS behavior) and let WMDatabase/context.getDatabasePath append the ".db" itself; update the method for serverUrl normalization (serverUrl.replaceFirst(...).replace("/", ".")) but do not add ".db".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java`:
- Around line 216-224: The getDatabaseName method is appending ".db" which
causes WMDatabase (via WMDatabase.getInstance / context.getDatabasePath(name +
".db")) to produce a "*.db.db" file; remove the explicit ".db" suffix from
getDatabaseName so it returns the base database name (matching the JS behavior)
and let WMDatabase/context.getDatabasePath append the ".db" itself; update the
method for serverUrl normalization (serverUrl.replaceFirst(...).replace("/",
".")) but do not add ".db".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf9ef8a-bd04-46aa-9ee7-5a382f584874
⛔ Files ignored due to path filters (16)
android/app/src/experimental/ic_launcher-web.pngis excluded by!**/*.pngandroid/app/src/experimental/res/drawable-hdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/experimental/res/drawable-mdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/experimental/res/drawable-xhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/experimental/res/drawable-xxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/experimental/res/drawable-xxxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/experimental/res/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.png
📒 Files selected for processing (46)
.github/actions/build-ios/action.yml.github/workflows/e2e-build-ios.ymlandroid/app/build.gradleandroid/app/src/debug/res/values/strings.xmlandroid/app/src/experimental/res/drawable-v24/ic_launcher_background.xmlandroid/app/src/experimental/res/drawable/ic_launcher_foreground.xmlandroid/app/src/experimental/res/drawable/ic_launcher_monochrome.xmlandroid/app/src/experimental/res/mipmap-anydpi-v26/ic_launcher.xmlandroid/app/src/experimental/res/mipmap-anydpi-v26/ic_launcher_round.xmlandroid/app/src/experimental/res/values/colors.xmlandroid/app/src/experimental/res/values/strings.xmlandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.javaapp/containers/DeprecationModal/index.tsxapp/containers/DeprecationModal/styles.tsapp/i18n/locales/ar.jsonapp/i18n/locales/bn-IN.jsonapp/i18n/locales/cs.jsonapp/i18n/locales/de.jsonapp/i18n/locales/en.jsonapp/i18n/locales/es.jsonapp/i18n/locales/fi.jsonapp/i18n/locales/fr.jsonapp/i18n/locales/hi-IN.jsonapp/i18n/locales/hu.jsonapp/i18n/locales/it.jsonapp/i18n/locales/ja.jsonapp/i18n/locales/nl.jsonapp/i18n/locales/nn.jsonapp/i18n/locales/no.jsonapp/i18n/locales/pt-BR.jsonapp/i18n/locales/pt-PT.jsonapp/i18n/locales/ru.jsonapp/i18n/locales/sl-SI.jsonapp/i18n/locales/sv.jsonapp/i18n/locales/ta-IN.jsonapp/i18n/locales/te-IN.jsonapp/i18n/locales/tr.jsonapp/i18n/locales/zh-CN.jsonapp/i18n/locales/zh-TW.jsonapp/index.tsxapp/lib/constants/environment.tsapp/lib/database/index.tsios/NotificationService/Info.plistios/RocketChatRN/Info.plistios/ShareRocketChatRN/Info.plistios/Shared/RocketChat/Database.swift
💤 Files with no reviewable changes (40)
- android/app/src/experimental/res/drawable-v24/ic_launcher_background.xml
- android/app/src/experimental/res/drawable/ic_launcher_monochrome.xml
- android/app/src/experimental/res/mipmap-anydpi-v26/ic_launcher.xml
- ios/RocketChatRN/Info.plist
- android/app/src/experimental/res/drawable/ic_launcher_foreground.xml
- app/i18n/locales/cs.json
- app/containers/DeprecationModal/styles.ts
- app/i18n/locales/es.json
- app/i18n/locales/tr.json
- ios/NotificationService/Info.plist
- app/lib/constants/environment.ts
- app/i18n/locales/nn.json
- app/i18n/locales/pt-BR.json
- app/i18n/locales/no.json
- app/i18n/locales/pt-PT.json
- app/i18n/locales/ja.json
- .github/actions/build-ios/action.yml
- app/i18n/locales/hi-IN.json
- android/app/src/experimental/res/mipmap-anydpi-v26/ic_launcher_round.xml
- android/app/src/experimental/res/values/strings.xml
- app/i18n/locales/hu.json
- android/app/build.gradle
- android/app/src/experimental/res/values/colors.xml
- app/i18n/locales/zh-CN.json
- app/i18n/locales/fr.json
- app/i18n/locales/bn-IN.json
- app/i18n/locales/zh-TW.json
- app/i18n/locales/sl-SI.json
- app/i18n/locales/en.json
- app/i18n/locales/de.json
- app/i18n/locales/ta-IN.json
- app/i18n/locales/ar.json
- app/containers/DeprecationModal/index.tsx
- app/i18n/locales/ru.json
- app/i18n/locales/sv.json
- app/i18n/locales/it.json
- app/index.tsx
- app/i18n/locales/fi.json
- app/i18n/locales/nl.json
- app/i18n/locales/te-IN.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/database/index.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/lib/database/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/index.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/index.ts
🔇 Additional comments (5)
.github/workflows/e2e-build-ios.yml (1)
73-77: LGTM!ios/ShareRocketChatRN/Info.plist (1)
14-15: LGTM!android/app/src/debug/res/values/strings.xml (1)
2-3: LGTM!ios/Shared/RocketChat/Database.swift (1)
32-35: LGTM!app/lib/database/index.ts (1)
32-32: LGTM!
Drops the Experimental RocketChatRN/Watch app targets, their schemes, the Experimental.xcassets bundle, the prepare_ios_official.sh rename shim, and its fastlane invocations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/NotificationService/Info.plist`:
- Around line 34-40: The NSExtensionAttributes dictionary (containing
IntentsSupported and INSendMessageIntent) is currently at the top level; move
that entire <key>NSExtensionAttributes</key> <dict>…</dict> block so it becomes
a child inside the existing <key>NSExtension</key> <dict>…</dict> (the same dict
that holds extension keys) ensuring IntentsSupported remains inside NSExtension,
the plist XML stays well-formed, and no duplicate NSExtensionAttributes keys
remain at top level.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f271184e-d1fb-4a8f-81db-c96a665ac142
⛔ Files ignored due to path filters (24)
ios/Experimental.xcassets/AppIcon.appiconset/100.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/1024 1.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/1024.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/114.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/120.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/144.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/152.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/167.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/180.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/20.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/29.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/40.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/50.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/57.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/58.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/60.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/72.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/76.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/80.pngis excluded by!**/*.pngios/Experimental.xcassets/AppIcon.appiconset/87.pngis excluded by!**/*.pngios/Experimental.xcassets/Launch Screen Icon.imageset/icon.pngis excluded by!**/*.pngios/Experimental.xcassets/Launch Screen Icon.imageset/icon@2x.pngis excluded by!**/*.pngios/Experimental.xcassets/Launch Screen Icon.imageset/icon@3x.pngis excluded by!**/*.pngios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
ios/Experimental.xcassets/AppIcon.appiconset/Contents.jsonios/Experimental.xcassets/Contents.jsonios/Experimental.xcassets/Launch Screen Icon.imageset/Contents.jsonios/Experimental.xcassets/splashBackgroundColor.colorset/Contents.jsonios/NotificationService/Info.plistios/Podfileios/PrivacyInfo.xcprivacyios/RocketChatRN.xcodeproj/project.pbxprojios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/NotificationService.xcschemeios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/RocketChatRN Watch.xcschemeios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/RocketChatRN.xcschemeios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/ShareRocketChatRN.xcschemeios/ShareRocketChatRN/Info.plistios/fastlane/Fastfilescripts/prepare_ios_official.sh
💤 Files with no reviewable changes (8)
- ios/Experimental.xcassets/Contents.json
- ios/Experimental.xcassets/AppIcon.appiconset/Contents.json
- ios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/RocketChatRN.xcscheme
- ios/Experimental.xcassets/splashBackgroundColor.colorset/Contents.json
- ios/fastlane/Fastfile
- scripts/prepare_ios_official.sh
- ios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/RocketChatRN Watch.xcscheme
- ios/Experimental.xcassets/Launch Screen Icon.imageset/Contents.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/ShareRocketChatRN/Info.plist
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (4)
ios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/ShareRocketChatRN.xcscheme (1)
67-77: LGTM!ios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/NotificationService.xcscheme (1)
58-68: LGTM!ios/PrivacyInfo.xcprivacy (1)
45-85: LGTM!ios/Podfile (1)
32-32: Target alignment is correct; no action needed.The Xcode project defines a
PBXNativeTargetnamedRocket.Chat, which matches the target declaration at line 32 of the Podfile. Pod install will succeed.
|
Android Build Available Rocket.Chat 4.72.0.108863 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTYI1nwS4jT5OUyvKlnkMBZuq_NPjiDJ3125hy-n4l1GdgHioZwV7ioF2ERgrqPupn47gjSoMjv1rTpmXff |
…figs The deleted scripts/prepare_ios_official.sh used to rewrite these IDs at build time. Make the rename permanent now that the Experimental targets are gone, so the App Store provisioning profiles match the bundle IDs.
The double-quoted echo lines let bash re-interpret `$` and other metacharacters in the substituted secret value, corrupting the password written to gradle.properties and producing `KeytoolException: keystore password was incorrect` at signing time. Mirror the single-quoted pattern already used in .github/actions/build-android/action.yml so the password reaches gradle intact.
|
Android Build Available Rocket.Chat 4.72.0.108865 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNS9Sz180vOWWaudN9kL2GNkX0YoQ071iSxDnZZuaZi7zBhYdC4cTdDr_glkb6zsQZEBwzyvetVaoTgXPTLF |
The new build_official_simulator Fastlane lane stripped codesign with skip_codesigning + CODE_SIGNING_ALLOWED=NO, producing a linker-signed binary with no entitlements. Without the App Group entitlement, containerURL(forSecurityApplicationGroupIdentifier:) returned nil, the SQLite path collapsed to /default.db, and the JSI bridge crashed on launch — failing every E2E shard. Restore the previous-working signing behavior and add an empty-groupDir guard in Database.swift matching MMKV.swift's existing guard.
|
Android Build Available Rocket.Chat 4.72.0.108871 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQ08XNqaYmn6I03bJHc5xoLz_oCTAUX9ZrveenUCkeBmKx5M5UEzrWORWsRAJCRn20Nqz2uJU4wijbowhp- |
diegolmello
left a comment
There was a problem hiding this comment.
Review verdict: ship-with-nits
Pure-deletion PR3 of the NATIVE-1118 phased migration. Diffed against remove-experimental-pr2-stop-publishing (the actual PR base, not develop). No genuine blockers found. All "missing" pieces I traced are intentionally deferred to PR4.
What I verified
- Stale-reference sweeps on the PR3 head tree:
isOfficial/IS_OFFICIAL→ zero hits anywhere.Experimental_retirement_*→ zero hits anywhere.DeprecationModal→ zero hits anywhere; both files that referenced it (app/containers/DeprecationModal/*, mount inapp/index.tsx) are removed cleanly.- Residual
experimental*matches are only WatermelonDB internals (experimentalSubscribe,experimentalUnsafeNativeReuse) and VoIP arch-doc stability markers — unrelated.
- All 25 locale files delete the same 6 keys uniformly (verified
en,ar,cs,de,zh-CN,zh-TW,hi-IN). Encryption.javacompiles:import java.lang.reflect.Fieldwas the only consumer of the removed reflection block; it's removed too.- Tests: no test or snapshot referenced
DeprecationModalpre-PR3, so no snapshot regen needed. The PR author's note that "existing snapshots cover the removed mount" is technically a no-op claim — there's literally nothing to cover — which is fine.
Migration / data-loss risk (the user's top concern)
No risk. The Official and Experimental apps have always been distinct installables with distinct package IDs (chat.rocket.android / chat.rocket.ios vs chat.rocket.reactnative), distinct App Group containers on iOS, and distinct app-private sandboxes on Android. The Official app's getDatabasePath has always produced ${name}.db (no -experimental suffix); only the Experimental flavor produced ${name}-experimental.db. The Official app never had a -experimental file to read from on disk, so removing the conditional is a pure dead-code cleanup, not a path change for any existing user. Experimental users on the now-dormant build had the PR1 modal nudging them off; their orphaned DBs sit untouched in the Experimental sandbox. No migration code needed.
Genuine issues
None blocking.
Nits / follow-ups (non-blocking)
ios/PrivacyInfo.xcprivacy— the file got reordered and lost a useful inline TODO comment ("Audit privacy manifest against full App Store Connect App Privacy disclosure for pre-existing data types..."). This is unrelated to Experimental removal and was likely an Xcode auto-format side effect. The audit reminder still applies — consider re-adding the comment or filing a tracking issue.- PR body is slightly stale — it says
prepare_ios_official.sh, thePodfile'starget 'RocketChatRN' # Experimental appline, andExperimental.xcassets/would be "handed off to the repo owner" and NOT in this PR, but commit031c39a51("chore(ios): remove Experimental Xcode targets and assets") actually does that work in-PR. The PR body should be refreshed to reflect that the Xcode-side cleanup already landed in PR3. The code is correct; the description just trails reality. - CI flakes — 4 of 28 E2E shards failed (Maestro emulator step), but Build iOS Official, Build Android Official, ESLint+Test all pass. Retry the failed shards.
Existing review reconciliation
- CodeRabbit on
ios/NotificationService/Info.plist:40(NSExtensionAttributes nested wrong): The complaint is technically valid — per Apple's schema,NSExtensionAttributesshould be a child ofNSExtension, not a top-level key. However, this misplacement is pre-existing (already present at the PR2 base and ondevelop; predates PR3 by several releases). PR3's diff on this file is purely theIS_OFFICIALremoval plus a tab/space indentation normalization on the existingNSExtensionAttributesblock. Disagree with treating this as a PR3 blocker. Acknowledge the bug and file a separate follow-up — fixing it here mixes scopes and would need a real test thatINSendMessageIntentstill resolves for the service extension. - CodeRabbit outside-diff on
Encryption.java:216-224(Android.db.dbdouble extension vs JS single.db): Also pre-existing. The explicit comment in the method body documents the intentional convention ("WMDatabase will resolve and append its own.dbinternally, so the physical file becomes*.db.db, matching the JS adapter"). The JS adapter passes its${name}.dbstring directly to native SQLite (no extra.dbappend), and the Java side replicates the final physical filename viacontext.getDatabasePath(name + ".db")which Android's framework converts to${name}.db.db. The convention is load-bearing for the notification-service Android side to open the same file WatermelonDB writes. Disagree with making this a PR3 change; if revisited, it needs a coordinated JS+Android test plan and likely belongs in its own PR. - CodeRabbit/PR1-carryover claim on
app/i18n/locales/hi-IN.json:331about Hindi translations contradicting an "English-only" claim: Does not apply to PR3. PR3's diff forhi-IN.jsonis purely deletion of the 6Experimental_retirement_*keys (the values were already in Hindi script in the file; PR3 doesn't touch values, only removes keys). PR1-era noise — ignore for this PR.
Summary
Solid deletion-only PR. Scope is tight, all sweeps come back clean, the stacked-on-PR2 base means the diff stays focused on what PR3 actually does. The Xcode-side cleanup that PR body said would land separately actually landed in-PR (good — the diff is more self-consistent that way; just refresh the body). Ship after PR1+PR2 land, retry the flaky E2E shards.
iOS shows a "Save Password?" sheet asynchronously after a password TextField is submitted, blocking the accessibility tree so subsequent extendedWaitUntil: rooms-list-view assertions time out (60 s) in navigate-to-e2ee-security.yaml. Tap "Not Now" on iOS after rooms-list-view is confirmed visible to dismiss the sheet before the next flow runs.
431ed09 to
694d654
Compare
…l-pr3-delete-code # Conflicts: # .github/actions/build-android/action.yml # .github/actions/build-ios/action.yml # .github/workflows/e2e-build-android.yml # ios/Podfile.lock # ios/ShareRocketChatRN/Info.plist # ios/fastlane/Fastfile
Proposed changes
PR3 of the phased Experimental → Official migration (NATIVE-1118). Stacked on PR2 (#7321) — merge PR1 (#7320) and PR2 (#7321) first, then rebase this PR onto
developbefore merging.After PR1 (in-app modal nudging Experimental users to migrate) and PR2 (stopping new Experimental publishes), this PR deletes the now-dormant Experimental code paths, assets, and Xcode-project entries.
JS
app/containers/DeprecationModal/and its mount inapp/index.tsx.app/lib/constants/environment.ts(theisOfficialswitch). No call sites remain.app/lib/database/index.ts: drop the-experimentalDB path suffix; the path is now always${appGroupPath}${name}.db. Safe by design — Experimental and Official ship under different bundle IDs / applicationIds, so they live in different sandboxes and never collide.Experimental_retirement_*i18n keys from the 25 locale files that had them (kept in PR1 to feed the modal; obsolete here).iOS
ios/RocketChatRN.xcodeproj/project.pbxproj: remove the Experimental main target (Rocket.Chat Experimental.app), the Experimental Watch target (Rocket.Chat Experimental Watch.app), theExperimental.xcassetsfile refs, and thePRODUCT_NAME = "Rocket.Chat Experimental"build settings.ios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/RocketChatRN.xcscheme,RocketChatRN Watch.xcscheme, andRocketChat Watch.xcscheme. Remaining schemes:RocketChat.xcscheme,ShareRocketChatRN.xcscheme,NotificationService.xcscheme.ios/Experimental.xcassets/.scripts/prepare_ios_official.shand its invocations inios/fastlane/Fastfile(build_official/build_official_simulator). The pbxproj now carries the Official bundle IDs directly — no CI-time sed rewrite needed.ios/Podfile: remove thetarget 'RocketChatRN' # Experimental appline.IS_OFFICIALkey fromRocketChatRN/Info.plist,ShareRocketChatRN/Info.plist, andNotificationService/Info.plist. No consumer reads it after this PR.PlistBuddy "Set IS_OFFICIAL YES"step from.github/actions/build-ios/action.ymland.github/workflows/e2e-build-ios.yml.ios/Shared/RocketChat/Database.swift: stop readingIS_OFFICIAL; always use the non-suffixed DB path (mirrors the JS change). Also addsguard !groupDir.isEmpty else { return nil }— the signature was alreadyString?but the function never returned nil, so this also closes a latent bug.ShareRocketChatRN/Info.plist:CFBundleDisplayNameRocket.Chat Experimental→Rocket.Chat.Android
experimentalproduct flavor and theIS_OFFICIALbuildConfigFieldfromandroid/app/build.gradle. The single remainingofficialflavor is kept to preserve all existing CI gradle task names (assembleOfficialRelease,bundleOfficialRelease,*/officialRelease/*artifact paths) — flattening to a no-flavor structure is handled in PR4 (NATIVE-1129).android/app/src/experimental/(launcher icons, splash assets,strings.xml, manifest — 40 files).android/app/src/main/java/.../notification/Encryption.java: drop theBuildConfig.IS_OFFICIALreflection ingetDatabaseName; the suffix is gone.android/app/src/debug/res/values/strings.xml:[DEBUG] Rocket.Chat Experimental→[DEBUG] Rocket.Chat.package.json:yarn androidandyarn android-whitelabelnow build--mode=officialDebug(theexperimentalflavor no longer exists).CI / E2E stabilization (driven by failures observed while landing the diff)
.github/workflows/e2e-build-android.yml: switch keystore-secret quoting from"…"to'…'. Bash re-interprets$inside double quotes, which was corrupting the base64-decoded keystore and breaking signing.ios/fastlane/Fastfile: dropskip_codesigning: true/CODE_SIGNING_ALLOWED=NOfrombuild_official_simulatorso the simulator build embeds entitlements (extensions wouldn't load on the simulator otherwise)..maestro/helpers/login.yaml: conditionally tap "Not Now" to dismiss the iOS Save Password sheet that occasionally interrupts the login flow.Post-merge ops (no diff) — once this PR lands:
official_android_buildandofficial_ios_buildenvironment gates on the merge run.chat.rocket.reactnativeExperimental app record (TestFlight already stopped from PR2).KEYSTORE_EXPERIMENTAL_*,EXPERIMENTAL_KEYSTORE_*,GOOGLE_SERVICES_IOS_EXPERIMENTAL, and any non-_OFFICIALlegacy keys that paired with them (cross-check — two historical naming conventions coexist). The_OFFICIAL-suffixed secrets are kept permanently — see ADR 0007.experimental_*GH environments are gone (experimental_android_build,experimental_ios_build, anyupload_experimental_*).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1122
How to test or reproduce
grep -rn 'isOfficial\|IS_OFFICIAL' app/ android/ ios/ .github/ scripts/→ zero hits in source (matches only under gitignoredandroid/app/build/andios/Pods/).grep -rn 'Experimental' app/ android/ ios/ .github/ scripts/→ only WatermelonDB internals (experimentalSubscribe,experimentalUnsafeNativeReuse) and the VoIP architecture-doc stability marker — both unrelated.TZ=UTC yarn test→ 1351/1351 pass, 357 snapshots stable.npx tsc --noEmit→ no errors../gradlew assembleOfficialRelease) succeeds; the single-flavorofficialkeeps all existing artifact paths intact.fastlane build_officialsucceeds withoutprepare_ios_official.sh(bundle IDs are now native to the pbxproj).Screenshots
n/a — code/assets deletion only.
Types of changes
Checklist
Further comments
The remaining
official/*_OFFICIALnaming in Gradle flavors, Fastlane lanes, GH Actions workflows/jobs/artifacts, and the iOS asset catalog is flattened in PR4 (NATIVE-1129), stacked on this branch. CI secret and GH Environment names retain the_OFFICIALsuffix permanently per ADR 0007.Summary by CodeRabbit
Features Removed
Style
Chores