[WIP] Save CSV-import mapped comuns for later use #81885
Conversation
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Optimistically save the column layout for this card (replaces entire entry) | ||
| // First clear any existing entry, then set the new one to match backend behavior | ||
| optimisticData.push({ |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The optimistic save of column layout uses two sequential MERGE operations (first setting to null, then setting the new value). If the second merge fails or is interrupted, the column layout will be cleared without a replacement, resulting in data loss.
Reasoning: This pattern is fragile because it creates an intermediate state where data is deleted. If any error occurs between the two operations, the user loses their saved column mappings without a way to recover them.
Suggested fix:
Replace the two-step process with a single atomic operation that overwrites the value directly:
// Optimistically save the column layout for this card (replaces entire entry atomically)
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.NVP_SAVED_CSV_COLUMN_LAYOUT_LIST,
value: {[cardID]: columnLayout},
});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return; | ||
| } | ||
|
|
||
| if (!existingCardID || !spreadsheet?.data || !savedColumnLayouts) { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The hasAppliedSavedMappings ref is used to prevent re-running the effect, but this logic could be simplified by deriving whether mappings should be applied directly in the render phase or by structuring dependencies correctly.
Reasoning: Using a ref to track whether an effect has run is a pattern that indicates the dependencies might not be correct. The effect depends on existingCardID, spreadsheet?.data, and savedColumnLayouts, but these are explicitly excluded from dependencies.
Suggested fix:
Consider restructuring to either:
- Run the effect only when the specific data needed is available (proper dependency array)
- Move the logic outside useEffect if it can be derived synchronously
However, if the intent is truly to run once on mount regardless of prop changes, the current pattern with the ref is acceptable as a mount-only side effect.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| dateFormat: null, | ||
| accountDetails: { | ||
| bank: 'upload', | ||
| currency, |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The string 'upload' is a magic value representing the bank name for CSV imports, but it is not defined as a constant.
Reasoning: The value 'upload' appears to be a special identifier in the oldDot system for CSV-uploaded cards, but using it as a string literal makes it unclear what it represents and prone to typos.
Suggested fix:
Define a constant for this value or reference an existing constant if one exists:
accountDetails: {
bank: CONST.PERSONAL_CARD.BANK_NAME.CSV, // or create a constant if this does not exist
currency,
accountID: cardName,
},Note: Looking at line 201, CONST.PERSONAL_CARD.BANK_NAME.CSV already exists and is used elsewhere in this file, so this should use that constant instead of the hardcoded string 'upload'.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| * @param savedLayout - The saved column layout for this card | ||
| * @returns Promise that resolves when column mappings are applied | ||
| */ | ||
| function applySavedColumnMappings(spreadsheetData: string[][], savedLayout: SavedCSVColumnLayoutData): Promise<void | void[]> { |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The function returns Promise<void | void[]> but the actual return types are inconsistent. It returns Promise<void> from Promise.resolve() in early returns, but the main path returns the result of Onyx.merge() which may return Promise<void[]>.
Reasoning: This type inconsistency can lead to confusion about what the function returns and makes it harder for consumers to handle the promise correctly.
Suggested fix:
Standardize the return type to always return Promise<void>:
function applySavedColumnMappings(spreadsheetData: string[][], savedLayout: SavedCSVColumnLayoutData): Promise<void> {
// ... existing code ...
// If we found any matching columns, apply the mappings
if (Object.keys(columnUpdates).length > 0) {
return Onyx.merge(ONYXKEYS.IMPORTED_SPREADSHEET, {columns: columnUpdates}).then(() => undefined);
}
return Promise.resolve();
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return; | ||
| } | ||
|
|
||
| hasAppliedSavedMappings.current = true; |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The applySavedColumnMappings call is fire-and-forget with no error handling. If the Onyx merge fails, the user will not know that their saved mappings were not applied.
Reasoning: Silent failures in data operations can lead to user confusion. If applying saved mappings fails, the user might wonder why their previously saved column mappings are not being used.
Suggested fix:
Add error handling:
hasAppliedSavedMappings.current = true;
applySavedColumnMappings(spreadsheet.data, savedLayout).catch((error) => {
console.error('Failed to apply saved column mappings', error);
// Optionally show a user-facing error message
});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
1 participant
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/596277
PROPOSAL:
Tests
Log into newDot and go to Wallet
Import a CSV card. You can use this file:
fake_transactions-2026-01-16 10_56_55.629.csv
Click on the card. You should see an option to add transactions:
click it and follow the process. Use the same file. Confirm the columns get pre-mapped
COnfirm they are also pre-mapped in oldDot
Offline tests
Same as tests
QA Steps
Same as tests
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