Conversation
WalkthroughThis update introduces enhanced file handling and processing functionalities designed for memory-efficient large dataset operations. It includes a new plugin for auto-updating schema changes across live workbooks, simplification of record manipulation with new helper functions, and an additional utility for controlled concurrent asynchronous operations. Changes
Sequence Diagram(s)Auto-Update Schema PluginsequenceDiagram
participant Admin
participant DeploymentService
participant RolloutPlugin
participant FlatfileServer
Admin->>DeploymentService: Deploy new code
DeploymentService->>RolloutPlugin: Trigger rollout
RolloutPlugin->>FlatfileServer: Retrieve current space and workbooks
FlatfileServer-->>RolloutPlugin: Return space and workbooks
RolloutPlugin->>FlatfileServer: Update workbooks' schemas based on new configuration
FlatfileServer-->>RolloutPlugin: Confirm updates
RolloutPlugin->>Admin: Notify completion
Simplified Record HandlingsequenceDiagram
participant Developer
participant Simplified
participant FlatfileAPI
Developer->>Simplified: Get all records by sheet ID
Simplified->>FlatfileAPI: Fetch records
FlatfileAPI-->>Simplified: Return records
Simplified->>Developer: Transform records to SimpleRecord format
Developer->>Simplified: Update records
Simplified->>FlatfileAPI: Send update records request
FlatfileAPI-->>Simplified: Confirm updates
Simplified->>Developer: Notify completion
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (2)
Files selected for processing (8)
Files skipped from review due to trivial changes (2)
Additional context usedMarkdownlint
Biome
Additional comments not posted (9)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const token = | ||
| CrossEnvConfig.get('FLATFILE_BEARER_TOKEN') || | ||
| CrossEnvConfig.get('FLATFILE_API_KEY') | ||
| const { baseUrl, token } = getCredentials() |
There was a problem hiding this comment.
Improve error handling in getRecordsRaw.
Currently, errors from the HTTP response are only logged and then an exception is thrown with a generic message. Consider enhancing error handling by including more details from the HTTP response in the thrown exception.
- console.log(await httpResponse?.text())
- throw new Error(`Creating records failed.`)
+ const errorText = await httpResponse.text();
+ throw new Error(`Creating records failed: ${errorText}`);Also applies to: 120-123
| function getCredentials() { | ||
| const baseUrl = | ||
| CrossEnvConfig.get('AGENT_INTERNAL_URL') || | ||
| CrossEnvConfig.get('FLATFILE_API_URL') || | ||
| 'https://platform.flatfile.com/api' | ||
| const token = | ||
| CrossEnvConfig.get('FLATFILE_BEARER_TOKEN') || | ||
| CrossEnvConfig.get('FLATFILE_API_KEY') | ||
|
|
||
| return { baseUrl, token } | ||
| } |
There was a problem hiding this comment.
Function getCredentials reviewed.
This function effectively retrieves credentials from environment variables with appropriate fallbacks. Consider adding error checking to ensure that necessary credentials are not missing.
+ if (!token) {
+ throw new Error('API token is missing');
+ }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.
| function getCredentials() { | |
| const baseUrl = | |
| CrossEnvConfig.get('AGENT_INTERNAL_URL') || | |
| CrossEnvConfig.get('FLATFILE_API_URL') || | |
| 'https://platform.flatfile.com/api' | |
| const token = | |
| CrossEnvConfig.get('FLATFILE_BEARER_TOKEN') || | |
| CrossEnvConfig.get('FLATFILE_API_KEY') | |
| return { baseUrl, token } | |
| } | |
| function getCredentials() { | |
| const baseUrl = | |
| CrossEnvConfig.get('AGENT_INTERNAL_URL') || | |
| CrossEnvConfig.get('FLATFILE_API_URL') || | |
| 'https://platform.flatfile.com/api' | |
| const token = | |
| CrossEnvConfig.get('FLATFILE_BEARER_TOKEN') || | |
| CrossEnvConfig.get('FLATFILE_API_KEY') | |
| if (!token) { | |
| throw new Error('API token is missing'); | |
| } | |
| return { baseUrl, token } | |
| } |
| export async function asyncLimitSeries<T>( | ||
| limit: number, | ||
| fn: (i: number) => Promise<T> | ||
| ): Promise<T[]> { | ||
| let response = [] | ||
|
|
||
| for (let i = 0; i < limit; i++) { | ||
| const res = await fn(i) | ||
| response.push(res) | ||
| } | ||
|
|
||
| return response |
There was a problem hiding this comment.
Consider adding error handling in asyncLimitSeries.
The function currently does not handle errors that might occur during the execution of fn(i). Adding try-catch blocks could improve robustness.
for (let i = 0; i < limit; i++) {
+ try {
+ const res = await fn(i)
+ response.push(res)
+ } catch (e) {
+ console.error(`Error processing task ${i}:`, e)
+ // Handle error appropriately, e.g., continue, break, or modify response
+ }
- const res = await fn(i)
- response.push(res)
}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.
| export async function asyncLimitSeries<T>( | |
| limit: number, | |
| fn: (i: number) => Promise<T> | |
| ): Promise<T[]> { | |
| let response = [] | |
| for (let i = 0; i < limit; i++) { | |
| const res = await fn(i) | |
| response.push(res) | |
| } | |
| return response | |
| export async function asyncLimitSeries<T>( | |
| limit: number, | |
| fn: (i: number) => Promise<T> | |
| ): Promise<T[]> { | |
| let response = [] | |
| for (let i = 0; i < limit; i++) { | |
| try { | |
| const res = await fn(i) | |
| response.push(res) | |
| } catch (e) { | |
| console.error(`Error processing task ${i}:`, e) | |
| // Handle error appropriately, e.g., continue, break, or modify response | |
| } | |
| } | |
| return response | |
| } |
| export class Simplified { | ||
| /** | ||
| * Return all records for a sheet | ||
| * | ||
| * @param sheetId | ||
| * @param options | ||
| * @param tick | ||
| */ | ||
| static async getAllRecords( | ||
| sheetId: string, | ||
| options: Flatfile.records.GetRecordsRequest = {}, | ||
| tick?: TickHelper | ||
| ): Promise<SimpleRecord[]> { | ||
| const recordCount = await getSheetLength(sheetId) | ||
| const pageCount = Math.ceil(recordCount / PAGE_SIZE) | ||
|
|
||
| const recordPages = await asyncLimitSeries(pageCount, async (i: number) => { | ||
| await tick?.((i + 1) / pageCount, i + 1, pageCount).catch(console.log) | ||
| const res = await getRecordsRaw(sheetId, { | ||
| ...options, | ||
| pageNumber: i + 1, | ||
| }) | ||
| return res.map(Simplified.toSimpleRecord) | ||
| }) | ||
| return recordPages.flat(1) | ||
| } | ||
|
|
||
| static async findRecordsLimit( | ||
| sheetId: string, | ||
| options: Flatfile.records.GetRecordsRequest, | ||
| limit = 100 | ||
| ) { | ||
| const records = await getRecordsRaw(sheetId, { | ||
| ...options, | ||
| pageSize: limit, | ||
| pageNumber: 1, | ||
| }) | ||
| if (Array.isArray(records)) { | ||
| return records.map(Simplified.toSimpleRecord) | ||
| } | ||
| return [] | ||
| } | ||
|
|
||
| /** | ||
| * { foo: bar } => { foo : {value: bar}} | ||
| * @param obj | ||
| */ | ||
| static toRecordValues(obj: SimpleRecord): Flatfile.RecordData { | ||
| return Object.fromEntries( | ||
| Object.entries(obj) | ||
| .filter(([key]) => key !== '_id' && key !== '_metadata') | ||
| .map(([key, value]) => [key, { value: value as Primitive }]) | ||
| ) | ||
| } | ||
|
|
||
| static toStandardRecord(obj: SimpleRecord) { | ||
| return { | ||
| id: obj._id as string, | ||
| metadata: obj._metadata as any, | ||
| values: Simplified.toRecordValues(obj), | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| * @param r | ||
| */ | ||
| static toSimpleRecord(r: Flatfile.Record_): SimpleRecord { | ||
| const obj = Object.fromEntries( | ||
| Object.entries(r.values).map( | ||
| ([key, value]) => [key, value.value] as [string, any] | ||
| ) | ||
| ) | ||
| obj.id = r.id | ||
| return obj as SimpleRecord | ||
| } | ||
|
|
||
| static updateAllRecords( | ||
| sheetId: string, | ||
| records: SimpleRecord[], | ||
| tick?: TickHelper | ||
| ): Promise<void> { | ||
| return updateAllRecords( | ||
| sheetId, | ||
| records.map(Simplified.toStandardRecord), | ||
| tick | ||
| ) | ||
| } | ||
|
|
||
| static async createAllRecords( | ||
| sheetId: string, | ||
| records: SimpleRecord[], | ||
| tick?: TickHelper | ||
| ): Promise<void> { | ||
| await createAllRecords( | ||
| sheetId, | ||
| records.map(Simplified.toRecordValues), | ||
| tick | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the Simplified class to use functions instead of static methods.
Using a class with only static members can be less efficient and less modular than using standalone functions. This also aligns with the static analysis recommendation.
- export class Simplified {
- static getAllRecords(sheetId: string, options: Flatfile.records.GetRecordsRequest = {}, tick?: TickHelper): Promise<SimpleRecord[]> {
+ export function getAllRecords(sheetId: string, options: Flatfile.records.GetRecordsRequest = {}, tick?: TickHelper): Promise<SimpleRecord[]> {
...
- static updateAllRecords(sheetId: string, records: SimpleRecord[], tick?: TickHelper): Promise<void> {
+ export function updateAllRecords(sheetId: string, records: SimpleRecord[], tick?: TickHelper): Promise<void> {
...
- }Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 13-113: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| export function rollout(config: { | ||
| namespace: string | ||
| dev: boolean | ||
| updater: ( | ||
| space: Flatfile.Space, | ||
| workbooks: Flatfile.Workbook[] | ||
| ) => Promise<undefined | Flatfile.Workbook[]> | ||
| }) { | ||
| async function prepareUpdate(dev: boolean = false) { | ||
| const { data: spaces } = await api.spaces.list({ | ||
| archived: false, | ||
| namespace: config.namespace.split(':')[1], | ||
| pageSize: 1000, | ||
| }) | ||
|
|
||
| console.log(`Fetched ${spaces.length} spaces`) | ||
| const spacesWithSecrets = await asyncMap( | ||
| spaces, | ||
| async (space) => { | ||
| const { data: secrets } = await api.secrets.list({ spaceId: space.id }) | ||
| return { space, secrets } | ||
| }, | ||
| 10 | ||
| ) | ||
| console.log(`Hydrated secrets for ${spacesWithSecrets.length} spaces`) | ||
|
|
||
| const prodUpdate = spacesWithSecrets.filter(({ secrets }) => { | ||
| return secrets.some( | ||
| (s) => s.name === 'FF_AUTO_UPDATE' && s.value === 'true' | ||
| ) | ||
| }) | ||
|
|
||
| const devUpdate = spacesWithSecrets.filter(({ secrets }) => { | ||
| return secrets.some( | ||
| (s) => s.name === 'FF_AUTO_UPDATE_DEV' && s.value === 'true' | ||
| ) | ||
| }) | ||
|
|
||
| const updatedList = await asyncMap( | ||
| dev ? devUpdate : prodUpdate, | ||
| ({ space }) => { | ||
| return triggerUpdateJobForSpace(space) | ||
| } | ||
| ) | ||
|
|
||
| // loop through each and determine if it needs to be updated by checking secrets | ||
| console.log(`Triggered schema for ${updatedList.length} spaces`) | ||
| } | ||
|
|
||
| async function triggerUpdateJobForSpace(space: Flatfile.Space) { | ||
| await api.jobs.create({ | ||
| type: 'space', | ||
| source: space.id, | ||
| operation: 'auto-update', | ||
| trigger: 'immediate', | ||
| managed: true, | ||
| mode: 'foreground', | ||
| environmentId: space.environmentId, | ||
| }) | ||
| } | ||
|
|
||
| const cb = (listener: FlatfileListener) => { | ||
| const isLambda = !!process.env.LAMBDA_TASK_ROOT | ||
|
|
||
| if (!isLambda && config.dev) { | ||
| prepareUpdate(true).then(() => { | ||
| console.log( | ||
| 'running local dev refresh based update of spaces, suppress this with dev=false in the autoUpdate plugin config' | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| const triggerHooks = async (sheetId: string) => { | ||
| // loop through all records and increment metadata somehow triggering a new version | ||
| // this will trigger the auto update to run | ||
| const uuid = randomUUID() | ||
| console.log('Hooks updating =>', sheetId) | ||
|
|
||
| const records = await Simplified.getAllRecords(sheetId) | ||
| const updatedRecords = records.map((record) => { | ||
| record.metadata = { | ||
| ...((record.metadata as any) || {}), | ||
| _autoUpdateKey: uuid, | ||
| } | ||
| return record | ||
| }) | ||
|
|
||
| await Simplified.updateAllRecords(sheetId, updatedRecords) | ||
| console.log('Hooks updated =>', sheetId) | ||
| } | ||
|
|
||
| listener.use( | ||
| jobHandler('space:auto-update', async (e) => { | ||
| const { data: space } = await api.spaces.get(e.context.spaceId) | ||
| // which workbook to update | ||
| const { data: workbooks } = await api.workbooks.list({ | ||
| spaceId: space.id, | ||
| }) | ||
| const updated = await config.updater( | ||
| space, | ||
| workbooks.filter((wb) => !wb.name.includes('[file]')) | ||
| ) | ||
| if (updated?.length) { | ||
| await asyncMap(updated, async (wb) => { | ||
| console.log('workbook schema updated, triggering hooks', wb.id) | ||
| await asyncMap( | ||
| wb.sheets, | ||
| (sheet) => { | ||
| return triggerHooks(sheet.id) | ||
| }, | ||
| 10 | ||
| ) | ||
| }) | ||
| } | ||
| return { info: 'Successfully updated schema' } | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| cb.root = (listener: FlatfileListener) => { | ||
| listener.on('agent:created', () => prepareUpdate(false)) | ||
| listener.on('agent:updated', () => prepareUpdate(false)) | ||
| } | ||
|
|
||
| return cb | ||
| } |
There was a problem hiding this comment.
Enhance error handling and modularize the rollout function.
The rollout function is complex and handles multiple operations. Consider breaking it down into smaller, more manageable functions. Also, add error handling to improve robustness, especially for API calls.
+ try {
async function prepareUpdate(dev: boolean = false) {
const { data: spaces } = await api.spaces.list({
...
- }
+ } catch (error) {
+ console.error('Failed to prepare update:', error)
+ }Committable suggestion was skipped due to low confidence.
1 participant
Introduces Simplified record helpers for memory efficient processing of large datasets and launches a rollout plugin for helping with schema updating.