[Payment due @ikevin127] [HOLD on https://github.com/Expensify/react-native-onyx/pull/848] fix: export Onyx state through Onyx - #101568
Conversation
|
|
chrispader
changed the title
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
chrispader
changed the title
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
The core move is right: stop opening OnyxDB a second time, read through Onyx, and collapse two platform-specific ExportOnyxState files into one.
Deleting the hand-rolled IndexedDB cursor and the raw SELECT * is a real simplification, and the web path finally propagates errors instead of hanging forever on a failed indexedDB.open (the old openRequest.onerror was never wired up, so the promise just never settled).
Findings below are ordered by what I would want fixed before approval.
…t-onyx-state-via-onyx # Conflicts: # config/eslint/eslint.seatbelt.tsv
chrispader
changed the title
chrispader
changed the title
|
Addressed all review comments by @ikevin127 ! |
chrispader
changed the title
chrispader
changed the title
| "../../src/libs/ExportOnyxState/index.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 | ||
| "../../src/libs/fileDownload/DownloadUtils.ts" "no-restricted-syntax" 1 | ||
| "../../src/libs/ExportOnyxState/masking.ts" "@typescript-eslint/no-unsafe-type-assertion" 14 | ||
| "../../src/libs/fileDownload/DownloadUtils.ts" "no-restricted-syntax" 2 |
There was a problem hiding this comment.
🟡 config/eslint/eslint.seatbelt.tsv
- "../../src/libs/fileDownload/DownloadUtils.ts" "no-restricted-syntax" 1
+ "../../src/libs/fileDownload/DownloadUtils.ts" "no-restricted-syntax" 2This raises the allowance without a matching violation. Verified:
main's seatbelt for that file is1.- At PR head
672f7277,DownloadUtils.tshas exactly oneimport * as(import * as ApiUtils from '@libs/ApiUtils'), which is the singleImportNamespaceSpecifierviolation.openExternalLinkis a named import, matching main. - I read all three
no-restricted-syntaxblocks inconfig/eslint/eslint.config.mjs. Thesrc/**selectors areTSEnumDeclaration,React.forwardRef, fourImportNamespaceSpecifiervariants, the PressablesentryLabelrule,LabeledStatementandWithStatement. None of them matchtry/finally,setTimeout,link.remove()orexport {createDownloadLink}.
So the file should still be 1. CI stays green because the seatbelt is a ceiling, which is exactly why this matters: it silently re-opens room for one more namespace import in a file this PR touches, and nobody will notice.
Most likely a merge artifact from 551329df0 resolving a generated file to the older value. Please re-run the seatbelt regeneration (or set it back to 1) and confirm lint still passes.
| const dataToShare = maskOnyxState(value, shouldMaskOnyxState); | ||
| await shareAsFile(JSON.stringify(dataToShare)); |
There was a problem hiding this comment.
🟢 NIT: No success-path assertion at the page level
TroubleshootPageTest.ts sets up a working happy path in beforeEach but every test forces a failure, so nothing ever asserts what shareAsFile receives. Concretely, if someone changed
const dataToShare = maskOnyxState(value, shouldMaskOnyxState);
await shareAsFile(JSON.stringify(dataToShare));to pass value instead of dataToShare, every test in this PR still passes and the app ships an unmasked Onyx dump containing auth tokens. That is the one regression in this flow with a real security consequence, and it is one assertion away:
it('shares the masked state', async () => {
render(React.createElement(TroubleshootPage));
fireEvent.press(screen.getByRole('button', {name: exportButtonName}));
await waitFor(() => {
expect(shareAsFile).toHaveBeenCalledWith(JSON.stringify(maskedState));
});
expect(maskOnyxState).toHaveBeenCalledWith(exportedState, undefined);
});
| jest.mock('@libs/ApiUtils', () => ({ | ||
| getApiRoot: jest.fn(() => 'https://example.com'), | ||
| })); | ||
| jest.mock('@libs/fileDownload/FileUtils', () => ({ | ||
| appendTimeToFileName: jest.fn((fileName: string) => fileName), | ||
| getFileName: jest.fn((fileName: string) => fileName), | ||
| })); | ||
| jest.mock('@libs/tryResolveUrlFromApiRoot', () => jest.fn((url: string) => url)); | ||
| jest.mock('@userActions/Link', () => ({openExternalLink: jest.fn()})); |
There was a problem hiding this comment.
🟢 NIT: createDownloadLink's home drags the API stack into the test
saveTextFileTest.ts now has to mock four modules that have nothing to do with saving a text file:
jest.mock('@libs/ApiUtils', () => ({getApiRoot: jest.fn(() => 'https://example.com')}));
jest.mock('@libs/fileDownload/FileUtils', () => ({...}));
jest.mock('@libs/tryResolveUrlFromApiRoot', () => jest.fn((url: string) => url));
jest.mock('@userActions/Link', () => ({openExternalLink: jest.fn()}));purely because importing createDownloadLink pulls in fetchFileDownload's whole dependency tree.
That is fallout from my own suggestion, so I will own it: the cleaner placement is its own module, say src/libs/fileDownload/createDownloadLink.ts, re-exported from DownloadUtils for existing callers. Then saveTextFile imports a pure DOM helper and the mock list collapses.
Not blocking, but worth doing while the code is fresh. Note also that the @userActions/Link mock is already dead: DownloadUtils imports @libs/openExternalLink now, so that mock targets a module this test never loads, and the real @libs/openExternalLink is pulled in unmocked.
| import saveTextFileNative from '@libs/saveTextFile/index.native'; | ||
| import type SaveTextFile from '@libs/saveTextFile/types'; | ||
|
|
||
| type ShareOptions = {url: string; failOnCancel: boolean}; |
There was a problem hiding this comment.
🟢 NIT: Re-derived type in a test file
type ShareOptions = {url: string; failOnCancel: boolean};This duplicates react-native-share's own options type. Prefer importing it so the mock's signature tracks the library rather than drifting from it. Same idea as saveTextFileWeb correctly importing the production SaveTextFile type rather than restating it.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 All seven of my previous comments are addressed. One new issue found, plus three nits.
Merge after the seatbelt line is corrected and swap the git pin github:margelo/react-native-onyx#a9da3ceb. The seatbelt is a one-line fix and the only thing I would hold on.
The other three 🟢 items are polish and can land here or as a follow-up, though the masked-state assertion is cheap enough that I would just add it.
melvin-bot
Bot
changed the title
|
🎯 @ikevin127, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
2 participants
Exporting Onyx state currently opens
OnyxDBdirectly from Expensify, creating a second connection to storage that Onyx already owns. NitroSQLite 9.8.2 rejects that duplicate open, so the Troubleshoot export can fail without producing a file. This prerequisite makes both native and web exports read through Onyx while keeping masking and file sharing in Expensify.@NicolasBonet @ikevin127
Explanation of Change
Pin
react-native-onyxto the head of Onyx PR #848 until that change is released.ExportOnyxState/index.tsexports the shared read and file-saving functions directly and re-exports masking frommasking.ts. A smallsaveTextFilehelper handles the platform difference: it reuseslocalFileCreateto stage a Blob download on web or a temporary share-sheet file on native, then removes the temporary resource. I did not reuselocalFileDownloadbecause it saves to Android's public Downloads directory and swallows errors, whereas this export shares from the app cache and reports failures.The focused export tests pass locally. The isolated install still has TypeScript/ESLint dependency errors outside these files, so those checks depend on CI.
This PR is a prerequisite for the NitroSQLite 9.8.2 upgrade. After the Onyx PR is merged and published, replace the Git pin with its released version before merging this PR.
Fixed Issues
Related prerequisite; the upgrade PR fixes the issue: #101448
No separate approved proposal applies to this prerequisite.
Tests
Manual device and browser verification is pending.
Offline tests
Offline verification is pending.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avatar, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
None attached. The normal export UI is unchanged, and manual device/browser verification is still pending.