-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/admin forth/1239/make bulk ai flow run unlimite #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/admin forth/1239/make bulk ai flow run unlimite #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements concurrent bulk AI flow processing with configurable record selection modes. It refactors the frontend state management from array-based tracking to Map-based storage, adds support for filtering-based record selection (in addition to manual checkbox selection), and implements concurrency control using the p-limit library.
Changes:
- Added
concurrencyLimitandrecordSelectorconfiguration options to control concurrent processing and record selection behavior - Refactored frontend state management from multiple synchronized arrays to a centralized Map-based approach with RecordState objects
- Implemented per-record concurrent processing using p-limit instead of sequential batch processing
- Added backend endpoints for fetching individual record data and filtered record IDs
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| types.ts | Adds new plugin options: concurrencyLimit (default 10) and recordSelector ('checkbox' or 'filtered') |
| index.ts | Adds get_old_data and get_filtered_ids endpoints; fixes recordId validation logic |
| custom/package.json | Adds p-limit dependency for concurrency control |
| custom/package-lock.json | Lockfile update for p-limit and yocto-queue dependencies |
| custom/VisionTable.vue | Refactors to use record objects from parent instead of managing separate arrays; simplifies prop interface |
| custom/VisionAction.vue | Major refactoring to Map-based state management with per-record concurrent processing; implements filtered record selection |
Files not reviewed (1)
- custom/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getOrCreateRecord(recordId: string | number): RecordState { | ||
| const key = String(recordId); | ||
| let record = recordsById.get(key); | ||
| if (!record) { | ||
| record = createEmptyRecord(recordId); | ||
| record.isChecked = !uncheckedRecordIds.has(key); | ||
| recordsById.set(key, record); | ||
| } | ||
| return record; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recordsById Map uses string keys (via String(recordId)), but record IDs from the backend could be numbers. While this normalization is consistent throughout the code, there's a risk if the backend returns mixed types or if string-based IDs contain leading zeros which would be lost in number conversion. The code at line 377 checks !record.isChecked but this flag can become stale if users interact with checkboxes during processing. Consider adding validation to ensure ID type consistency or document the expected ID type.
| const saveResults = await Promise.all(saveTasks); | ||
| const failedResult = saveResults.find(res => res?.ok === false || res?.error); | ||
|
|
||
| if (!failedResult) { | ||
| confirmDialog.value.close(); | ||
| props.updateList(); | ||
| props.clearCheckboxes(); | ||
| } else if (res.ok === false) { | ||
| props.clearCheckboxes?.(); | ||
| } else if (failedResult.ok === false) { | ||
| adminforth.alert({ | ||
| message: res.error, | ||
| message: failedResult.error, | ||
| variant: 'danger', | ||
| timeout: 'unlimited', | ||
| }); | ||
| isError.value = true; | ||
| errorMessage.value = t(`Failed to save data. You are not allowed to save.`); | ||
| } else { | ||
| console.error('Error saving data:', res); | ||
| console.error('Error saving data:', failedResult); | ||
| isError.value = true; | ||
| errorMessage.value = t(`Failed to save data. Please, try to re-run the action.`); | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When saving records concurrently, if any save operation fails, only the first failed result is reported to the user via adminforth.alert. Other failed records are silently ignored. Users won't know which records failed to save if there are multiple failures. Consider collecting all failures and reporting them together, or implementing retry logic for failed saves, so users can identify which records need to be re-processed.
| async function regenerateCell(recordInfo: any) { | ||
| if (coreStore.isInternetError) { | ||
| adminforth.alert({ | ||
| message: t('Cannot regenerate column while internet connection is lost. Please check your connection and try again.'), | ||
| variant: 'danger', | ||
| timeout: 'unlimited', | ||
| }); | ||
| return; | ||
| } | ||
| if (!regeneratingFieldsStatus.value[recordInfo.recordId]) { | ||
| regeneratingFieldsStatus.value[recordInfo.recordId] = {}; | ||
| const record = recordsById.get(String(recordInfo.recordId)); | ||
| if (!record) { | ||
| return; | ||
| } | ||
| if (!record.regeneratingFieldsStatus[recordInfo.fieldName]) { | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| } | ||
| regeneratingFieldsStatus.value[recordInfo.recordId][recordInfo.fieldName] = true; | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = true; | ||
| touchRecords(); | ||
| const actionType = props.meta.outputFieldsForAnalizeFromImages?.includes(recordInfo.fieldName) | ||
| ? 'analyze' | ||
| : props.meta.outputPlainFields?.includes(recordInfo.fieldName) | ||
| ? 'analyze_no_images' | ||
| : null; | ||
| if (!actionType) { | ||
| console.error(`Field ${recordInfo.fieldName} is not configured for analysis.`); | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| touchRecords(); | ||
| return; | ||
| } | ||
|
|
||
| let generationPromptsForField = {}; | ||
| if (actionType === 'analyze') { | ||
| generationPromptsForField = generationPrompts.value.imageFieldsPrompts || {}; | ||
| isAnalizingFields.value = true; | ||
| record.aiStatus.analyzedImages = false; | ||
| } else if (actionType === 'analyze_no_images') { | ||
| generationPromptsForField = generationPrompts.value.plainFieldsPrompts || {}; | ||
| isAnalizingImages.value = true; | ||
| record.aiStatus.analyzedNoImages = false; | ||
| } | ||
|
|
||
| let jobId; | ||
| let res; | ||
| try { | ||
| res = await callAdminForthApi({ | ||
| path: `/plugin/${props.meta.pluginInstanceId}/create-job`, | ||
| method: 'POST', | ||
| body: { | ||
| fieldToRegenerate: recordInfo.fieldName, | ||
| recordId: recordInfo.recordId, | ||
| action: actionType, | ||
| actionType: "regenerate_cell", | ||
| prompt: generationPromptsForField[recordInfo.fieldName] || null, | ||
| }, | ||
| silentError: true, | ||
| }); | ||
| } catch (e) { | ||
| regeneratingFieldsStatus.value[recordInfo.recordId][recordInfo.fieldName] = false; | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| console.error(`Error during cell regeneration for record ${recordInfo.recordId}, field ${recordInfo.fieldName}:`, e); | ||
| } | ||
| if ( res.ok === false) { | ||
| adminforth.alert({ | ||
| message: res.error, | ||
| variant: 'danger', | ||
| }); | ||
| isError.value = true; | ||
| errorMessage.value = t(`Failed to regenerate field`); | ||
| regeneratingFieldsStatus.value[recordInfo.recordId][recordInfo.fieldName] = false; | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| return; | ||
| } | ||
| jobId = res.jobId; | ||
| res = {} | ||
| do { | ||
| res = await callAdminForthApi({ | ||
| path: `/plugin/${props.meta.pluginInstanceId}/get-job-status`, | ||
| method: 'POST', | ||
| body: { jobId }, | ||
| silentError: true, | ||
| }); | ||
| if (actionType === 'analyze') { | ||
| await new Promise(resolve => setTimeout(resolve, props.meta.refreshRates?.fillFieldsFromImages)); | ||
| } else if (actionType === 'analyze_no_images') { | ||
| await new Promise(resolve => setTimeout(resolve, props.meta.refreshRates?.fillPlainFields)); | ||
| } | ||
| } while (res.job?.status === 'in_progress'); | ||
| if (res.job?.status === 'failed' || !res.ok || !res) { | ||
| adminforth.alert({ | ||
| message: t(`Regeneration action failed for record: ${recordInfo.recordId}. Error: ${res.job?.error || 'Unknown error'}`), | ||
| variant: 'danger', | ||
| timeout: 'unlimited', | ||
| }); | ||
| if (actionType === 'analyze') { | ||
| imageToTextErrorMessages.value[recordInfo.recordId][recordInfo.fieldName] = res.job?.error || 'Unknown error'; | ||
| isAnalizingFields.value = false; | ||
| record.imageToTextErrorMessages[recordInfo.fieldName] = res.job?.error || 'Unknown error'; | ||
| record.aiStatus.analyzedImages = true; | ||
| } else if (actionType === 'analyze_no_images') { | ||
| textToTextErrorMessages.value[recordInfo.recordId][recordInfo.fieldName] = res.job?.error || 'Unknown error'; | ||
| isAnalizingImages.value = false; | ||
| record.textToTextErrorMessages[recordInfo.fieldName] = res.job?.error || 'Unknown error'; | ||
| record.aiStatus.analyzedNoImages = true; | ||
| } | ||
| regeneratingFieldsStatus.value[recordInfo.recordId][recordInfo.fieldName] = false; | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| touchRecords(); | ||
| return; | ||
| } else if (res.job?.status === 'completed') { | ||
| const index = selected.value.findIndex(item => String(item[primaryKey]) === String(recordInfo.recordId)); | ||
|
|
||
| const pk = selected.value[index]?.[primaryKey]; | ||
| if (pk) { | ||
| selected.value[index] = { | ||
| ...selected.value[index], | ||
| ...res.job.result, | ||
| isChecked: true, | ||
| [primaryKey]: pk, | ||
| }; | ||
| } | ||
| record.data = { | ||
| ...record.data, | ||
| ...res.job.result, | ||
| }; | ||
| if (actionType === 'analyze') { | ||
| if (imageToTextErrorMessages.value[index]) { | ||
| imageToTextErrorMessages.value[index][recordInfo.fieldName] = ''; | ||
| } | ||
| isAnalizingFields.value = false; | ||
| record.imageToTextErrorMessages[recordInfo.fieldName] = ''; | ||
| record.aiStatus.analyzedImages = true; | ||
| } else if (actionType === 'analyze_no_images') { | ||
| if (textToTextErrorMessages.value[index]) { | ||
| textToTextErrorMessages.value[index][recordInfo.fieldName] = ''; | ||
| } | ||
| isAnalizingImages.value = false; | ||
| record.textToTextErrorMessages[recordInfo.fieldName] = ''; | ||
| record.aiStatus.analyzedNoImages = true; | ||
| } | ||
| regeneratingFieldsStatus.value[recordInfo.recordId][recordInfo.fieldName] = false; | ||
| record.regeneratingFieldsStatus[recordInfo.fieldName] = false; | ||
| touchRecords(); | ||
| } | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the regenerateCell function, the polling logic doesn't use the reusable pollJob function and instead implements its own polling with a simple setTimeout. This creates code duplication and inconsistency - the polling logic differs from the main processing flow. The regenerateCell polling also lacks the checkIfDialogOpen() safety check that pollJob has, potentially continuing to poll even after the dialog is closed. Consider refactoring to use the shared pollJob function or at minimum add dialog-open checks.
| concurrencyLimit?: number; | ||
|
|
||
| /** | ||
| * Defines the way how records are selected for the action. |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trailing space after the period on line 129 which creates inconsistent spacing in the JSDoc comment.
| * Defines the way how records are selected for the action. | |
| * Defines the way how records are selected for the action. |
| <template #cell:checkboxes="{ item }"> | ||
| <div class="max-w-[100px] flex items-center justify-center"> | ||
| <Checkbox | ||
| v-model="selected[tableColumnsIndexes.findIndex(el => el[primaryKey] === item[primaryKey])].isChecked" | ||
| v-model="item.isChecked" | ||
| /> | ||
| </div> | ||
| </template> |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When recordSelector is set to 'filtered', checkboxes are still displayed in the table and users can interact with them. However, there's no mechanism to synchronize checkbox state changes with the uncheckedRecordIds Set. According to the documentation, 'filtered' mode should apply actions to all records matching filters "without showing any checkboxes". Consider either hiding the checkbox column entirely when in 'filtered' mode, or if checkboxes should remain visible for manual deselection, implement proper synchronization between item.isChecked and uncheckedRecordIds.
| async function runAiActions() { | ||
| popupMode.value = 'generation'; | ||
|
|
||
| if (props.meta.isImageGeneration) { | ||
| isGeneratingImages.value = true; | ||
| runAiAction({ | ||
| endpoint: 'initial_image_generate', | ||
| actionType: 'generate_images', | ||
| responseFlag: isAiResponseReceivedImage, | ||
| }); | ||
| } | ||
| if (props.meta.isFieldsForAnalizeFromImages) { | ||
| isAnalizingImages.value = true; | ||
| runAiAction({ | ||
| endpoint: 'analyze', | ||
| actionType: 'analyze', | ||
| responseFlag: isAiResponseReceivedAnalizeImage, | ||
| }); | ||
| } | ||
| if (props.meta.isFieldsForAnalizePlain) { | ||
| isAnalizingFields.value = true; | ||
| runAiAction({ | ||
| endpoint: 'analyze_no_images', | ||
| actionType: 'analyze_no_images', | ||
| responseFlag: isAiResponseReceivedAnalizeNoImage, | ||
| }); | ||
| if (!await checkRateLimits()) { | ||
| return; | ||
| } | ||
| const limit = pLimit(props.meta.concurrencyLimit || 10); | ||
| const tasks = recordIds.value | ||
| .filter(id => !uncheckedRecordIds.has(String(id))) | ||
| .map(id => limit(() => processOneRecord(String(id)))); | ||
| await Promise.all(tasks); | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the dialog is closed during active processing (via checkIfDialogOpen checks), concurrent operations are abandoned mid-flight but their associated jobs continue running on the backend. The job polling stops, but the backend jobs don't get cancelled. This can lead to wasted server resources and rate limit consumption for operations whose results will never be used. Consider adding a cleanup mechanism to cancel pending backend jobs when the dialog is closed, or at minimum document this behavior for operators.
| * 'checkbox' means that user will select records manually by checkboxes, | ||
| * | ||
| * 'filtered' means that action will be applied to all records matching current | ||
| * filters without showing any checkboxes (use with caution). |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trailing space after the period on line 134 which creates inconsistent spacing in the JSDoc comment.
| * filters without showing any checkboxes (use with caution). | |
| * filters without showing any checkboxes (use with caution). |
| //return { error: "Missing action type" }; | ||
| } | ||
| else if (!recordId) { | ||
| else if (!recordId && typeof recordId !== 'number') { |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition typeof recordId !== 'number' will always be true when combined with !recordId using AND operator. If recordId is 0 (a valid number), it would be falsy but pass the type check. This creates a logic error where recordId = 0 would incorrectly fail validation. Consider changing to (recordId === undefined || recordId === null) to properly handle numeric IDs including 0.
| else if (!recordId && typeof recordId !== 'number') { | |
| else if (recordId === undefined || recordId === null || typeof recordId !== 'number') { |
| const paginatedRecords = computed(() => props.records.slice(pagination.offset, pagination.offset + pagination.limit)); | ||
|
|
||
| const tableDataProvider = async ({ offset, limit }) => { | ||
| pagination.offset = offset; | ||
| pagination.limit = limit; | ||
| return { | ||
| data: paginatedRecords.value, | ||
| total: props.records.length, | ||
| }; | ||
| }; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pagination logic computes paginatedRecords by slicing the records array, but the tableDataProvider function is async and returns the already-computed paginatedRecords.value. This creates unnecessary complexity - the function doesn't need to be async since it performs no async operations. Additionally, the Table component likely expects the data provider to fetch data based on offset/limit, but here the entire records array is kept in memory and sliced. For large datasets (which is the point of unlimited bulk operations), this could cause memory issues. Consider whether server-side pagination would be more appropriate.
No description provided.