fix: view-mapped plugin restrict to mapped sheet#720
Conversation
WalkthroughThis pull request introduces a bug fix for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
plugins/view-mapped/src/view-mapped.ts (1)
Line range hint
127-134: Consider enhancing error context for sheet-related failuresWhile the error handling is solid, consider adding sheet-specific context to help debug issues in multi-sheet scenarios.
logError( '@flatfile/plugin-view-mapped', - JSON.stringify(error, null, 2) + JSON.stringify({ + error, + context: { + destinationSheetId, + workbookId + } + }, null, 2) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/dull-apes-deny.md(1 hunks)plugins/view-mapped/src/view-mapped.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dull-apes-deny.md
🔇 Additional comments (2)
plugins/view-mapped/src/view-mapped.ts (2)
51-54: LGTM: Crucial fix for sheet-specific mapping
The addition of destinationSheetId extraction is a key improvement that helps restrict field updates to the mapped sheet only, addressing the core issue where fields were being incorrectly deleted from unmapped sheets.
74-80: LGTM: Sheet-specific field updates implemented correctly
The conditional update ensures that only fields in the mapped sheet are modified, preventing unintended changes to other sheets.
Let's verify that no other parts of the codebase assume all sheets are updated:
✅ Verification successful
Let me check the full context of this usage to ensure it aligns with the sheet-specific update logic.
Comment verified - implementation matches documentation
The code correctly implements the sheet-specific update logic despite the broader comment. While the comment suggests "Looping through all sheets", the actual implementation properly restricts updates to only the destination sheet through the if (sheet.id === destinationSheetId) condition. No other parts of the codebase assume all sheets are updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any code that might assume all sheets are updated
rg -g '!*.{json,md}' -i "sheets.*fields.*mapped.*true"
Length of output: 240
Script:
#!/bin/bash
# Get more context around the comment about mapped fields
rg -g '!*.{json,md}' -B 5 -A 5 "sheets.*fields.*mapped.*true"
Length of output: 1246
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
plugins/view-mapped/src/view-mapped.ts (2)
74-81: Enhance error handling for trackChanges validationWhile the validation is good, consider throwing a specific error instead of just logging and returning. This would make it easier for consumers to handle this case.
if (!workbook.settings.trackChanges) { - console.log('Skipping because trackChanges is not enabled') - return + throw new Error('plugins.viewMapped.trackChangesRequired') }
84-90: LGTM: Bug fix implemented correctly with room for minor improvementThe code now correctly restricts field updates to the mapped sheet, fixing the original bug. Consider simplifying the nested conditionals for better readability.
-workbook.sheets.forEach((sheet) => { - if (sheet.id === destinationSheetId) { - sheet.config.fields.forEach((field) => { - if (mappedFields.includes(field.key)) { - field.metadata = { mapped: true } - } - }) - } -}) +const targetSheet = workbook.sheets.find(sheet => sheet.id === destinationSheetId) +if (targetSheet) { + targetSheet.config.fields + .filter(field => mappedFields.includes(field.key)) + .forEach(field => field.metadata = { mapped: true }) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/view-mapped/src/view-mapped.ts(4 hunks)
🔇 Additional comments (2)
plugins/view-mapped/src/view-mapped.ts (2)
1-1: LGTM: Well-structured utility function addition
The sleep utility function is appropriately implemented for managing API request rates.
Also applies to: 8-9
53-56: Add validation for destinationSheetId
The previous review comment about adding validation for the destination sheet is still relevant. This would prevent potential runtime errors if the sheet doesn't exist.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
plugins/view-mapped/src/view-mapped.ts (2)
74-80: Well-implemented race condition preventionThe trackChanges check effectively prevents race conditions between hooks and workbook updates. The implementation is clean and well-documented.
Consider documenting this requirement in the plugin's README to ensure users enable trackChanges when using this plugin.
84-90: Add defensive checks for field propertiesWhile the sheet-specific update logic is correct, consider adding defensive checks for null/undefined fields and metadata.
if (sheet.id === destinationSheetId) { - sheet.config.fields.forEach((field) => { + sheet.config?.fields?.forEach((field) => { if (mappedFields.includes(field.key)) { - field.metadata = { mapped: true } + field.metadata = field.metadata || {} + field.metadata.mapped = true } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/dull-apes-deny.md(1 hunks)plugins/view-mapped/src/view-mapped.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/dull-apes-deny.md
🔇 Additional comments (3)
plugins/view-mapped/src/view-mapped.ts (3)
1-1: LGTM! Well-structured type imports and utility function.
The type import enhances type safety, and the sleep utility function is properly implemented for managing API request rates.
Also applies to: 8-8
53-56: Add validation for sheet existence
Consider adding validation to handle edge cases where the destination sheet might not exist or was deleted during processing.
123-139: Add timeout mechanism to commit completion check
While the commit completion check is well implemented, consider adding a timeout mechanism to prevent potential infinite loops if commits never complete.
| // Check that all commits are completed before running this job | ||
| let hasUncompletedCommits = true | ||
| do { | ||
| const { data: commits } = await api.sheets.getSheetCommits( |
There was a problem hiding this comment.
Is there any other way to trigger this other than polling? Can we listen for completed commits or do we need to make another mechanism to control the order of these event responses?
2 participants
Please explain how to summarize this PR for the Changelog:
This PR fixes a bug in the view-mapped plugin when multiple sheets have fields with the same key by limiting the view-mapped update to the sheet being mapped. Additionally, the view-mapped plugin now only runs when the
trackChangessetting in the workbook is enabled to prevent a race condition where the view-mapped plugin runs before hooks have completed.Closes https://github.com/FlatFilers/support-triage/issues/1696
Tell code reviewer how and what to test: