[Onyx audit] Migrate keys to RAM-only - part 1/2#82309
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
The submodule update was unintentionally included and is unrelated to the RAM-only keys migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-migrate-keys-to-ram-only-part-1
…91-migrate-keys-to-ram-only-part-1
|
@Krishna2323 @roryabraham One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I’ll be reviewing this shortly. |
|
Works well 🎉 fix_part_1.mp4fix_part_2.mp4 |
|
@JKobrynski Is there anything left? I tried checking for similar cases but found that it shouldn’t happen anywhere else.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e9b00ca33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Promise.race([ | ||
| Linking.getInitialURL(), | ||
| new Promise<null>((resolve) => { | ||
| setTimeout(() => resolve(null), CONST.TIMING.GET_INITIAL_URL_TIMEOUT); | ||
| }), |
There was a problem hiding this comment.
Handle late getInitialURL resolution after timeout
Racing Linking.getInitialURL() against a 10s null timeout unblocks startup, but it also permanently drops deep links when the native bridge returns the initial URL after that timeout (a case this comment already calls out in HybridApp). Once the timeout wins, onInitialUrl(null) runs and the eventual URL resolution is ignored, so users can land on the default route instead of the intended deep-linked report on slower/lagging startup paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@Krishna2323 do you have any thoughts on this ☝️ ?
There was a problem hiding this comment.
Not a practical concern. getInitialURL() resolves in milliseconds (it's a synchronous native read). The 10s timeout would only fire if the native bridge is broken, in which case the URL is unavailable anyway. Without the timeout, a broken bridge would leave the app stuck on the splash screen forever -- so this seems to be the right tradeoff.
These are just some suggestions for further investigation on similar cases, and potential improvements to consider! |
@JKobrynski I was asking about this :) |
|
@Krishna2323 posted on the issue yesterday! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_native.mp4Android: mWeb Chromeandroid_chrome.mp4iOS: HybridApptest_1.mp4test_2.mp4iOS: mWeb Safariios_safari.mp4MacOS: Chrome / Safariloader_test.mp4onfido_test.mp4 |
|
@JKobrynski could you please check the failing tests? |
|
@Krishna2323 on it |
…-migrate-keys-to-ram-only-part-1
|
@Krishna2323 I updated the branch with the latest main and the test started passing locally, let's see if CI follows |
|
@Krishna2323 all green now! |
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM and works well! 🚀 ![]()
|
Dang there are confclits now @JKobrynski |
…91-migrate-keys-to-ram-only-part-1
|
@mountiny conflicts resolved 🫡 |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.39-3 🚀
|
Explanation of Change
This PR is part 1/2 of RAM-only migration - it migrates the obvious keys.
Fixed Issues
$ #80091
PROPOSAL: N/A
Tests
1. App Launch & Deep Linking (
IS_CHECKING_PUBLIC_ROOM)2. App Update Modals (
UPDATE_AVAILABLE,UPDATE_REQUIRED)It's difficult to test these, as the related actions are only triggered by API
3. Search / User Selection Loading States (
IS_SEARCHING_FOR_REPORTS)New Chat
Money Request (Expense) Participant Selector
Workspace Invite
Search Router
4. Wallet / Onfido Identity Verification (
WALLET_ONFIDO)5. General Regression Checks
Offline tests
N/A
QA Steps
Same as Tests section above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web-compressed.mov