Conversation
- Update event param from BulkRecordHook to be mandatory
marcus-ff
changed the title
WalkthroughThis pull request introduces a standardized Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (5)
.changeset/slow-eggs-burn.md (1)
16-16: Consider enhancing the changeset description.The current description could be more informative. Consider adding details about the
TickFunctiontype signature and the motivation for standardizing it across plugins.-This release adds a type for the tick() function. +This release introduces a standardized TickFunction type (progress: number, message?: string) => Promise<Flatfile.JobResponse> across all plugins to improve type consistency and maintainability.plugins/merge-connection/src/poll.for.merge.sync.ts (1)
Line range hint
21-24: Consider normalizing progress calculationThe progress calculation is currently limited to 10-40 range:
Math.min(40, Math.round(10 + (30 * completedSyncs) / totalModels))Consider using a more standard 0-100 range for consistency with other progress indicators. This would make it easier to understand and maintain.
Example implementation:
-Math.min(40, Math.round(10 + (30 * completedSyncs) / totalModels)) +Math.round((completedSyncs / totalModels) * 100)plugins/merge-connection/src/sync.workbook.ts (1)
Line range hint
45-49: Consider simplifying the progress calculationThe progress calculation could be simplified using a helper function to make it more readable and maintainable.
Consider extracting the progress calculation:
-await tick( - Math.min(90, Math.round(40 + (50 * processedSheets) / sheets.length)), - `Synced ${sheet.config.name}` -) +const calculateProgress = (processed: number, total: number) => + Math.min(90, Math.round(40 + (50 * processed) / total)) +await tick(calculateProgress(processedSheets, sheets.length), + `Synced ${sheet.config.name}`)plugins/dedupe/src/dedupe.plugin.ts (1)
Line range hint
77-82: Consider adding a final 100% progress updateThe progress is capped at 99% during processing, but there's no explicit 100% progress update before completion.
Consider adding a final progress update:
await tick( progress, `Processing ${records.length} records on page ${pageNumber} of ${totalPageCount}` ) + if (pageNumber === totalPageCount) { + await tick(100, 'Processing complete') + }plugins/export-workbook/src/plugin.ts (1)
Line range hint
151-154: Consider improving progress calculation for sheet preparationThe current progress calculation
Math.round(((sheetIndex + 1) / sheets.length) * 70)could be simplified and made more precise.Consider this alternative:
-await tick( - Math.round(((sheetIndex + 1) / sheets.length) * 70), - `${sheet.name} Prepared` -) +const progress = 1 + Math.floor((sheetIndex + 1) * 69 / sheets.length) +await tick(progress, `${sheet.name} Prepared`)This ensures:
- Progress starts at 1% (initial tick)
- Evenly distributes remaining 69% across sheets
- Avoids potential rounding issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (18)
.changeset/slow-eggs-burn.md(1 hunks)plugins/dedupe/src/dedupe.plugin.ts(2 hunks)plugins/dedupe/src/index.ts(2 hunks)plugins/export-workbook/src/index.ts(2 hunks)plugins/export-workbook/src/plugin.ts(2 hunks)plugins/graphql-schema/src/index.ts(2 hunks)plugins/job-handler/src/job.handler.ts(2 hunks)plugins/json-schema/src/index.ts(2 hunks)plugins/merge-connection/src/create.workbook.ts(2 hunks)plugins/merge-connection/src/poll.for.merge.sync.ts(2 hunks)plugins/merge-connection/src/sync.workbook.ts(2 hunks)plugins/openapi-schema/src/index.ts(2 hunks)plugins/record-hook/src/index.ts(1 hunks)plugins/space-configure/src/space.configure.ts(2 hunks)plugins/sql-ddl-converter/src/index.ts(2 hunks)plugins/yaml-schema/README.md(1 hunks)plugins/yaml-schema/src/index.ts(2 hunks)plugins/zip-extractor/src/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/record-hook/src/index.ts
🔇 Additional comments (22)
plugins/sql-ddl-converter/src/index.ts (1)
2-2: LGTM! Type changes are properly implemented.
The changes correctly implement the standardized TickFunction type:
- Import is properly added from @flatfile/plugin-job-handler
- Type is correctly applied to the tick parameter
- Integration with configureSpace remains unchanged
Also applies to: 11-11
plugins/json-schema/src/index.ts (1)
2-2: LGTM! Consistent implementation of TickFunction type.
The changes maintain consistency with other plugins while properly implementing the standardized type:
- Import statement is correctly added
- Type is properly applied to the tick parameter
- Function signature remains consistent with other plugins
Also applies to: 11-11
plugins/graphql-schema/src/index.ts (1)
2-2: LGTM! Type standardization is properly implemented.
The changes correctly implement the TickFunction type while maintaining the unique aspects of the GraphQL implementation:
- Import is properly added
- Type is correctly applied to the tick parameter
- Event parameter usage in generateSetup is preserved
Let's verify the consistency of TickFunction implementation across all plugins:
Also applies to: 12-12
✅ Verification successful
TickFunction implementation is consistently used across all plugins
The verification results show perfect alignment:
- The
TickFunctionimport from '@flatfile/plugin-job-handler' is present in all relevant plugin files - The
tick: TickFunctiontype annotation is consistently used across all plugins - No legacy tick function signatures were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of TickFunction across plugins
# Check for consistent import statement
echo "Checking import consistency..."
rg -l "import.*TickFunction.*from.*@flatfile/plugin-job-handler" plugins/
# Check for consistent type usage
echo "Checking type usage consistency..."
rg -l "tick:\s*TickFunction" plugins/
# Check for any remaining old tick function signatures
echo "Checking for remaining old signatures..."
rg "tick:\s*\(.*progress.*message.*\)\s*=>\s*Promise<.*JobResponse>" plugins/
Length of output: 1593
plugins/openapi-schema/src/index.ts (1)
2-2: LGTM! Type changes are well-structured.
The introduction of the TickFunction type and its usage in the callback signature improves type consistency while maintaining the existing functionality.
Also applies to: 11-11
plugins/export-workbook/src/index.ts (1)
2-2: LGTM! Verify exportRecords implementation.
The TickFunction type is correctly imported and implemented in the plugin. The changes maintain the existing functionality while improving type consistency.
Let's verify that the exportRecords function in plugin.ts also uses the TickFunction type:
Also applies to: 15-16
✅ Verification successful
LGTM! The TickFunction type is correctly implemented
The exportRecords function in plugins/export-workbook/src/plugin.ts correctly uses the TickFunction type as the third parameter in its signature:
export const exportRecords = async (
event: FlatfileEvent,
options: PluginOptions,
tick: TickFunction
): Promise<void | Flatfile.JobCompleteDetails>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify exportRecords function signature in plugin.ts
ast-grep --pattern 'export async function exportRecords($_: FlatfileEvent, $_: PluginOptions, tick: TickFunction)'
Length of output: 116
Script:
#!/bin/bash
# Let's try to find the exportRecords function implementation with a broader pattern
ast-grep --pattern 'export async function exportRecords'
# Also search for any file containing exportRecords function
rg "export.*function.*exportRecords" -A 5
Length of output: 102
Script:
#!/bin/bash
# Let's find all files in the export-workbook plugin directory
fd . plugins/export-workbook
# Then let's look at the content of any TypeScript files in that directory
fd -e ts . plugins/export-workbook --exec cat
Length of output: 11342
plugins/dedupe/src/index.ts (1)
2-2: LGTM! Clean type signature update
The changes properly implement the new TickFunction type, simplifying the callback signature while maintaining the same functionality. This aligns well with the standardization goal.
Also applies to: 16-16
plugins/yaml-schema/src/index.ts (1)
2-2: LGTM! Consistent type implementation
The changes properly integrate the TickFunction type, maintaining consistency with other plugins while preserving the existing functionality.
Also applies to: 16-16
plugins/merge-connection/src/poll.for.merge.sync.ts (1)
1-1: LGTM! Type signature properly updated
The TickFunction type is correctly imported and implemented in the function signature.
Also applies to: 10-10
plugins/job-handler/src/job.handler.ts (2)
44-44: LGTM: Type usage in jobHandler
The TickFunction type is correctly applied to the handler parameter.
16-19: Consider standardizing parameter naming across codebase
The TickFunction type uses info as the parameter name, while some implementations use message. Consider standardizing this across the codebase for better consistency.
Let's verify the parameter naming across the codebase:
plugins/merge-connection/src/sync.workbook.ts (1)
13-13: LGTM: TickFunction integration
The TickFunction type is correctly imported and applied to the event handler.
plugins/dedupe/src/dedupe.plugin.ts (1)
31-31: LGTM: TickFunction integration
The TickFunction type is correctly imported and applied to the dedupe function.
plugins/zip-extractor/src/index.ts (2)
2-2: LGTM: Clean import of TickFunction type
The import is correctly placed and follows TypeScript best practices.
43-43: LGTM: Proper implementation of TickFunction type
The callback signature has been correctly updated to use the standardized TickFunction type. All existing tick function calls within the implementation maintain the correct parameter types (number for progress, optional string for message).
plugins/space-configure/src/space.configure.ts (2)
4-4: LGTM: Clean import with proper destructuring
The import correctly combines both the type and the jobHandler function using TypeScript's type import syntax.
31-31: LGTM: Proper type implementation in callback signature
The callback parameter's tick function has been correctly updated to use the standardized TickFunction type, maintaining consistency across plugins.
plugins/yaml-schema/README.md (1)
90-90: LGTM: Documentation accurately reflects the new type
The code example in the documentation has been properly updated to use the new TickFunction type, providing clear guidance for developers implementing the callback.
plugins/merge-connection/src/create.workbook.ts (3)
8-8: LGTM: Clean import of TickFunction type
The import statement is correctly placed and follows TypeScript best practices for type imports.
Line range hint 22-180: LGTM: Well-structured progress reporting
The tick function is used effectively throughout the code with:
- Well-distributed progress points (20%, 40%, 60%, 80%, 90%)
- Clear, descriptive messages for each progress update
- Proper error handling ensuring tick promises are awaited
22-22: LGTM: Function signature updated to use TickFunction type
The function signature has been properly updated to use the standardized TickFunction type, improving type consistency across plugins.
Let's verify the tick function usage pattern matches other plugins:
✅ Verification successful
Function signature matches the standard pattern across plugins
The verification confirms that the TickFunction type is consistently used across multiple plugins in the codebase, including zip-extractor, yaml-schema, space-configure, and others. The implementation in create.workbook.ts follows this established pattern perfectly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tick function parameter declarations across plugins
rg -t ts "tick:\s*(TickFunction|.*?=>.*?Promise<Flatfile\.JobResponse>)" plugins/
Length of output: 1197
plugins/export-workbook/src/plugin.ts (2)
3-3: LGTM: Clean import of TickFunction type
The import statement is correctly placed and follows TypeScript best practices for type imports.
45-46: LGTM: Function signature updated to use TickFunction type
The exportRecords function signature has been properly updated to use the standardized TickFunction type, maintaining consistency with other plugins.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
2 participants
This PR fixes the issue https://github.com/FlatFilers/configurator/issues/20
Please explain how to summarize this PR for the Changelog:
Declare the type
TickFunctionTell code reviewer how and what to test:
The app should work the same way