add plugin-copy-column #611
Conversation
WalkthroughThe changes introduce a new functionality for copying values between columns within a workbook in the Flatfile API. A new file, Changes
Possibly related PRs
🪧 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 (
|
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
| sheets.forEach(async (sheet) => { | ||
| const loadAllFields = await getAllFields(sheets, sheet.slug); | ||
| setSheetActions(sheet, [ | ||
| copyColumnValuesBlueprint(loadAllFields), |
There was a problem hiding this comment.
Neat to populate all the field enums for each sheet!
Does this workbook:created event get triggered for File based Workbooks? Would we want to add the Action on every workbook:created or sheet:created?
There was a problem hiding this comment.
I'm not seeing sheet:created when creating a space. What do you mean by File based Workbooks?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
plugins/copy-column/src/copy-column.ts (1)
94-95: Remove debug logging statements from production codeThe
console.logstatements ingetAllFieldsappear to be for debugging purposes. It's best practice to remove or disable such statements in production to prevent unnecessary console output.Apply this diff to remove the debug statements:
const _fields = sheets.reduce((acc, sheet) => { - console.log("in getallfields, sheet.config.fields here: ") - console.log(sheet.config) if (sheet.config.slug == sheetSlug) { acc.push( ...sheet.config.fields
| console.log({ e }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the 'setSheetActions' function
Currently, the error is logged using console.log({ e });, which might not be sufficient for debugging and monitoring. Consider implementing a more robust error handling mechanism, such as rethrowing the error or logging it using a dedicated logging service.
Apply this diff to improve error handling:
} catch (e) {
- console.log({ e });
+ console.error('Error updating sheet actions:', e);
+ throw e;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log({ e }); | |
| } | |
| console.error('Error updating sheet actions:', e); | |
| throw e; | |
| } |
| sheets.forEach(async (sheet) => { | ||
| const loadAllFields = await getAllFields(sheets, sheet.slug); | ||
| setSheetActions(sheet, [ | ||
| copyColumnValuesBlueprint(loadAllFields), | ||
| ...(sheet.config.actions?.filter( | ||
| (a) => | ||
| a.operation !== 'copy-column' | ||
| ) || []), | ||
| ]); | ||
| }) |
There was a problem hiding this comment.
Use 'for...of' loop instead of 'forEach' with async callbacks
Using Array.forEach with an async callback does not wait for the promises to resolve, which can lead to unexpected behavior. It's recommended to use a for...of loop to handle asynchronous operations sequentially.
Apply this diff to fix the issue:
- sheets.forEach(async (sheet) => {
+ for (const sheet of sheets) {
+ const loadAllFields = await getAllFields(sheets, sheet.slug);
+ await setSheetActions(sheet, [
+ copyColumnValuesBlueprint(loadAllFields),
+ ...(sheet.config.actions?.filter(
+ (a) =>
+ a.operation !== 'copy-column'
+ ) || []),
+ ]);
+ }
- const loadAllFields = await getAllFields(sheets, sheet.slug);
- setSheetActions(sheet, [
- copyColumnValuesBlueprint(loadAllFields),
- ...(sheet.config.actions?.filter(
- (a) =>
- a.operation !== 'copy-column'
- ) || []),
- ]);
- })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sheets.forEach(async (sheet) => { | |
| const loadAllFields = await getAllFields(sheets, sheet.slug); | |
| setSheetActions(sheet, [ | |
| copyColumnValuesBlueprint(loadAllFields), | |
| ...(sheet.config.actions?.filter( | |
| (a) => | |
| a.operation !== 'copy-column' | |
| ) || []), | |
| ]); | |
| }) | |
| for (const sheet of sheets) { | |
| const loadAllFields = await getAllFields(sheets, sheet.slug); | |
| await setSheetActions(sheet, [ | |
| copyColumnValuesBlueprint(loadAllFields), | |
| ...(sheet.config.actions?.filter( | |
| (a) => | |
| a.operation !== 'copy-column' | |
| ) || []), | |
| ]); | |
| } |
| const { key_copy_from, key_paste_to } = job.input | ||
| const patch = records.map(copyColumnValues(key_copy_from, key_paste_to)) | ||
|
|
There was a problem hiding this comment.
Validate input keys before copying column values
There is no validation to ensure that key_copy_from and key_paste_to are valid and not the same. Adding validation will prevent potential errors and unintended data overwrites.
Apply this diff to add validation:
const { key_copy_from, key_paste_to } = job.input
+ if (!key_copy_from || !key_paste_to || key_copy_from === key_paste_to) {
+ throw new Error('Invalid input: Source and destination columns must be specified and cannot be the same.')
+ }
const patch = records.map(copyColumnValues(key_copy_from, key_paste_to))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { key_copy_from, key_paste_to } = job.input | |
| const patch = records.map(copyColumnValues(key_copy_from, key_paste_to)) | |
| const { key_copy_from, key_paste_to } = job.input | |
| if (!key_copy_from || !key_paste_to || key_copy_from === key_paste_to) { | |
| throw new Error('Invalid input: Source and destination columns must be specified and cannot be the same.') | |
| } | |
| const patch = records.map(copyColumnValues(key_copy_from, key_paste_to)) |
Please explain how to summarize this PR for the Changelog:
This plugin allows users to copy all the values from one column to another within a sheet.
You'll need to add the action to your sheet like below:
And add a listener like below:
listener.use(copyColumn());