feat: allow training multiple capture actions in one recording session #562
Conversation
RohitR311
added
Type: Feature
|
""" WalkthroughThis update implements a comprehensive refactor across the robot training, data interpretation, and output handling flows to support multiple capture actions (text, list, screenshot) within a single robot training session. State management for capture actions is centralized and made explicit, with UI components updated to allow and display multiple data types. The backend processing, output categorization, and integration with external services (Airtable, Google Sheets) are restructured to handle and merge multiple categorized outputs. Localization files are updated to reflect new UI elements and messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RightSidePanel
participant ActionContext
participant InterpretationLog
participant Backend/Interpreter
participant OutputHandler
User->>RightSidePanel: Initiate capture action (text/list/screenshot)
RightSidePanel->>ActionContext: startAction(type)
ActionContext-->>RightSidePanel: Update activeAction state
RightSidePanel->>Backend/Interpreter: Emit capture event
Backend/Interpreter->>Backend/Interpreter: Track action type, process action
Backend/Interpreter->>OutputHandler: Categorize output by action type
OutputHandler-->>InterpretationLog: Send categorized data (text/list/screenshot)
InterpretationLog-->>User: Display data in tabbed/paginated UI
User->>RightSidePanel: Finish or discard action
RightSidePanel->>ActionContext: finishAction(type)
ActionContext-->>RightSidePanel: Reset state, emit end event
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (2)
src/components/recorder/RightSidePanel.tsx (2)
55-70: Context over-destructuring can cause avoidable re-rendersEvery value pulled out of
useActionContext()is now a hard dependency for this component; when any of them changes React will re-renderRightSidePanel. Most of these values (e.g.workflow,customLimit,showLimitOptions) are unrelated to the very first paint and will never be read again during a given action.To minimise reconciliation work you can adopt a selector-based pattern:
// Pseudo-code const paginationCtx = useActionContext(ctx => ({ paginationType : ctx.paginationType, updatePaginationType : ctx.updatePaginationType, })); const actionStatusCtx = useActionContext(ctx => ({ activeAction : ctx.activeAction, startAction : ctx.startAction, finishAction : ctx.finishAction, }));The component will then re-render only when the specific slice it cares about changes.
76-77: Replace magic string'none'with a typed enumComparing against a raw string makes typos undetectable and complicates refactors:
export type ActiveAction = 'none' | 'text' | 'list' | 'screenshot';Better yet, elevate this to an
enum/const object and expose it from the context:export const ActiveActions = { None: 'none', Text: 'text', List: 'list', Screenshot: 'screenshot', } as const; type ActiveAction = typeof ActiveActions[keyof typeof ActiveActions]; const isAnyActionActive = activeAction !== ActiveActions.None;This tightens the type system and prevents silent bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/RightSidePanel.tsx(15 hunks)
🔇 Additional comments (1)
src/components/recorder/RightSidePanel.tsx (1)
150-153: Is the action actually marked as started?
handleStartGetScreenshottriggersstartGetScreenshot()but never callsstartAction('screenshot'), whereas other flows rely onactiveActionto gate UI.Please verify that
startGetScreenshot()internally sets theactiveActionto'screenshot'.
If it does not, the capture buttons can reappear too early becauseisAnyActionActivewill stayfalse.
| onClick={() => { | ||
| stopGetScreenshot(); | ||
| setActiveAction('none'); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Call finishAction('screenshot') when discarding to keep global state consistent
The discard handler manually resets activeAction but bypasses other cleanup that finishAction performs (e.g. logging, telemetry, timers).
- onClick={() => {
- stopGetScreenshot();
- setActiveAction('none');
- }}
+ onClick={() => {
+ stopGetScreenshot();
+ finishAction('screenshot'); // single source of truth for cleanup
+ }}This keeps the action lifecycle symmetrical with the confirmation path.
📝 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.
| onClick={() => { | |
| stopGetScreenshot(); | |
| setActiveAction('none'); | |
| }} | |
| onClick={() => { | |
| stopGetScreenshot(); | |
| finishAction('screenshot'); // single source of truth for cleanup | |
| }} |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
maxun-core/src/interpret.ts (1)
48-48: Consider using a more specific function type forsetActionTypeThe
Functiontype is a generic type that accepts any function-like value, which can lead to type-safety issues. Using a more specific function signature would provide better type checking and documentation.- setActionType: Function, + setActionType: (actionType: string) => void,🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
maxun-core/src/interpret.ts(9 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
maxun-core/src/interpret.ts
[error] 48-48: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
maxun-core/src/interpret.ts (2)
381-383: Action type reporting correctly implemented for all custom workflow actionsAll custom workflow actions in
wawActionsnow properly report their action type at the beginning of execution, using optional chaining to safely access the callback.Also applies to: 391-393, 421-423, 432-434, 478-480, 493-495, 507-509, 520-522, 532-534
566-568: Non-custom actions now also report their action typesThis change completes the action reporting system by handling all actions that aren't part of the
wawActionsobject. The general action execution now reports the string representation of the action name.
maxun-core/src/interpret.ts
Outdated
| @@ -45,6 +45,7 @@ interface InterpreterOptions { | |||
| debugChannel: Partial<{ | |||
| activeId: Function, | |||
| debugMessage: Function, | |||
| setActionType: Function, | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the UI correctly handles action state throughout the capture flow
These changes support the action state management in the UI layer. Let's ensure that the setActionType callback integrates correctly with the UI's action lifecycle management.
Also applies to: 381-383, 391-393, 421-423, 432-434, 478-480, 493-495, 507-509, 520-522, 532-534, 566-568
🏁 Script executed:
#!/bin/bash
# Check how the UI components use the new setActionType callback
# Find the implementation of the action state management in the UI
rg -A 3 -B 3 "startAction|finishAction" --type=tsx
# Check for imports or usage of the setActionType in React components
rg "setActionType" --type=tsxLength of output: 138
🏁 Script executed:
#!/bin/bash
# Correct search for UI usage of action lifecycle callbacks in TSX/TS files
# Search for startAction in TSX files
rg -n 'startAction' -g '*.tsx'
# Search for finishAction in TSX files
rg -n 'finishAction' -g '*.tsx'
# Search for setActionType in TSX files
rg -n 'setActionType' -g '*.tsx'
# Also check for setActionType usage in plain TS files, in case UI logic is separated
rg -n 'setActionType' -g '*.ts'Length of output: 2760
Ensure the UI wires up the new setActionType callback
We found calls to debugChannel.setActionType in maxun-core/src/interpret.ts but no corresponding handling in the UI. The recorder panel only subscribes to startAction/finishAction, so your new action-type updates won’t propagate unless you explicitly pass and handle setActionType in your React components.
Locations to update:
- src/components/recorder/RightSidePanel.tsx
• IncludesetActionTypein the debug channel props alongsidestartAction/finishAction.
• Add logic (e.g., a state setter or effect) to respond tosetActionType(type)calls and update the UI accordingly.
Without these changes, the UI won’t reflect the action‐type changes emitted by interpret.ts.
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/src/pgboss-worker.ts (4)
266-270: Prefer explicit typing & null-coalescing forcategorizedOutputThe three branches are always expected to be objects; using “fallback‐to‐empty‐object” with
|| {}hides accidentalnull/undefinedcoming from the interpreter (silent data loss).
A typed alias also avoids pervasiveanylater.- const categorizedOutput = { - scrapeSchema: interpretationInfo.scrapeSchemaOutput || {}, - scrapeList: interpretationInfo.scrapeListOutput || {}, - other: interpretationInfo.otherOutput || {} - }; + const categorizedOutput: Record<'scrapeSchema' | 'scrapeList' | 'other', Record<string, any>> = { + scrapeSchema: interpretationInfo.scrapeSchemaOutput ?? {}, + scrapeList: interpretationInfo.scrapeListOutput ?? {}, + other: interpretationInfo.otherOutput ?? {}, + };
287-314: Counting logic can be simplified & de-duplicatedThe three nearly identical loops could be condensed into a helper, improving readability and reducing the risk of future divergence.
- let totalSchemaItemsExtracted = 0; - let totalListItemsExtracted = 0; - ... - if (categorizedOutput.scrapeSchema) { - Object.values(categorizedOutput.scrapeSchema).forEach((schemaResult: any) => { - ... - }); - } - ... - if (categorizedOutput.scrapeList) { - Object.values(categorizedOutput.scrapeList).forEach((listResult: any) => { - ... - }); - } + const countItems = (obj: Record<string, any>) => + Object.values(obj).reduce((acc, v) => acc + (Array.isArray(v) ? v.length : 1), 0); + + const totalSchemaItemsExtracted = countItems(categorizedOutput.scrapeSchema); + const totalListItemsExtracted = countItems(categorizedOutput.scrapeList);This also makes the subsequent
if (categorizedOutput.scrapeSchema)guards unnecessary.
482-487: DefaultcategorizedOutputconstant shadowed later
categorizedOutputis re-assigned a new object a few lines below. Declaring the placeholder withletis fine, but you can avoid shadowing entirely by initialising with the helper:let categorizedOutput = collectDataByType([]);Minor, but it keeps the variable semantics tighter.
566-583: Removeanyand promote generic helpersStrong typing here cascades to every consumer and guards against accidental shape changes.
-function collectDataByType(dataArray: any[]): Record<string, any> { - return dataArray.reduce((result: Record<string, any>, item, index) => { - result[`item-${index}`] = item; - return result; - }, {}); -} +function collectDataByType<T>(dataArray: T[]): Record<string, T> { + return dataArray.reduce<Record<string, T>>((result, item, index) => { + result[`item-${index}`] = item; + return result; + }, {}); +} ... -function collectBinaryData(binaryDataArray: { mimetype: string, data: string, type?: string }[]): Record<string, any> { - return binaryDataArray.reduce((result: Record<string, any>, item, index) => { - result[`item-${index}`] = item; - return result; - }, {}); -} + +interface BinaryData { + mimetype: string; + data: string; + type?: string; +} + +function collectBinaryData(binaryDataArray: BinaryData[]): Record<string, BinaryData> { + return binaryDataArray.reduce<Record<string, BinaryData>>((result, item, index) => { + result[`item-${index}`] = item; + return result; + }, {}); +}This keeps compile-time safety without affecting runtime behaviour.
server/src/pgboss-worker.ts
Outdated
| serializableOutput: { | ||
| scrapeSchema: Object.values(categorizedOutput.scrapeSchema), | ||
| scrapeList: Object.values(categorizedOutput.scrapeList), | ||
| other: Object.values(categorizedOutput.other), | ||
| }, | ||
| binaryOutput: uploadedBinaryOutput, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Storing Object.values(...) drops the item-index mapping
collectDataByType builds an object keyed as item-{index}, but persisting only Object.values(...) removes those keys, making it impossible to correlate back to the original action that produced each element.
If consumers do not need the mapping you can remove the intermediate object and store the raw array directly; otherwise, persist the keyed object.
- serializableOutput: {
- scrapeSchema: Object.values(categorizedOutput.scrapeSchema),
- scrapeList: Object.values(categorizedOutput.scrapeList),
- other: Object.values(categorizedOutput.other),
- },
+ // Keep the keyed structure to preserve traceability
+ serializableOutput: categorizedOutput,Are downstream readers (e.g. analytics, UI) prepared for this schema change? Please double-check before merging.
I’d like to inspect how categorizedOutput is produced and where serializableOutput flows downstream. Let’s gather references and context around these identifiers:
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for collectDataByType usage/definition..."
rg "collectDataByType" -n .
echo ""
echo "Searching for categorizedOutput references..."
rg "categorizedOutput" -n .
echo ""
echo "Searching for serializableOutput usages..."
rg "serializableOutput" -n .
echo ""
echo "Displaying lines 250–340 in server/src/pgboss-worker.ts for context..."
sed -n '250,340p' server/src/pgboss-worker.tsLength of output: 9378
I want to confirm how items are keyed and how the UI expects serializableOutput to be shaped. Let’s inspect the collectDataByType definition and the UI code that reads serializableOutput:
🏁 Script executed:
#!/bin/bash
set -e
echo "1) collectDataByType definition (around line 566):"
sed -n '550,620p' server/src/pgboss-worker.ts
echo ""
echo "2) RunContent.tsx usage (first 200 lines):"
sed -n '1,200p' src/components/run/RunContent.tsxLength of output: 10556
Review serializableOutput schema and downstream compatibility
Changing serializableOutput to a different shape will break several consumers that today expect either arrays under category keys or top-level indexed keys. You must update them in lockstep or stick with the current structure.
Affected locations:
• server/src/pgboss-worker.ts (lines 278–283): where you build and persist serializableOutput
• src/components/run/RunContent.tsx: assumes row.serializableOutput[firstKey] is an Array
• server/src/workflow-management/integrations/gsheet.ts & airtable.ts: look up plainRun.serializableOutput['item-0'] at top level
• server/src/routes/storage.ts & server/src/api/record.ts: similar top-level “item-#” access
• server/src/models/Run.ts: typed as Record<string, any[]>
Next steps:
- Decide on a single output schema (e.g. categorized arrays vs. indexed objects).
- Update each consumer to traverse that schema (UI: iterate categories or flatten arrays; integrations: drill into nested objects; API/storage: adjust key lookups).
- Align the
Runmodel type to match the final shape.
Until all of these are updated, please do not swap in categorizedOutput directly.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/context/browserActions.tsx (2)
35-36: Excellent centralization of action lifecycle managementThe introduction of
startActionandfinishActionmethods centralizes the logic for beginning and completing actions. This is a significant improvement that:
- Creates a consistent pattern for action management
- Reduces code duplication
- Makes it easier to maintain the action lifecycle
One suggestion for consideration - consider using the ActionType in the function signatures instead of repeating the union type:
-const startAction = (action: 'text' | 'list' | 'screenshot') => { +const startAction = (action: ActionType) => {-const finishAction = (action: 'text' | 'list' | 'screenshot') => { +const finishAction = (action: ActionType) => {Also applies to: 76-113
135-158: Effectively refactored individual action handlersThe refactoring of individual action start/stop functions to use the centralized
startActionmethod reduces code duplication and ensures consistent behavior. The consistent call tosetActiveAction('none')in all stop methods ensures proper cleanup.However, there appears to be redundancy between
stopGetListandfinishActionfor list actions - both reset the same state values. Consider refactoringstopGetListto usefinishActionto avoid potential maintenance issues:const stopGetList = () => { - setGetList(false); - setPaginationType(''); - setLimitType(''); - setCustomLimit(''); - setCaptureStage('complete'); - setActiveAction('none'); + finishAction('list'); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/context/browserActions.tsx(7 hunks)
🔇 Additional comments (7)
src/context/browserActions.tsx (7)
9-9: Good addition of ActionType for better type safetyThe introduction of
ActionTypeenum provides clear type definitions for the kinds of actions available in the system. This makes the code more self-documenting and helps ensure consistency throughout the codebase.
24-30: Well-structured state management for tracking actionsThe addition of
actionsInWorkflowandactiveActioncreates a clear state model for tracking both the active and completed actions. This supports the PR objective of allowing multiple capture actions in a single recording session.
67-72: Good state initialization with meaningful defaultsThe state initialization for tracking actions is well-structured with appropriate default values. Setting
activeActionto 'none' initially and all action flags inactionsInWorkflowtofalseensures a clean starting state.
174-176: Well-organized Context Provider value structureThe Context Provider value is well-organized with the new state and functions properly included. Grouping related properties and functions together makes the interface more intuitive for consumers of the context.
Also applies to: 181-182
77-78: Good defensive programming with guard clauseThe check to prevent starting a new action when one is already active is a good safeguard. This prevents potential state inconsistencies that could occur if multiple actions were mistakenly started simultaneously.
93-94: Important validation in finishAction implementationThe validation in
finishActionensures that only the currently active action can be finished. This guard clause prevents incorrect state transitions and potential bugs.
95-98: Elegant usage of state updater function patternUsing the functional state update pattern (
prev => ({ ...prev, [action]: true })) is the correct approach for updating an object state based on its previous value. This ensures that updates don't accidentally overwrite other properties in the state object.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/recorder/RightSidePanel.tsx (2)
345-349:⚠️ Potential issueIncorrect key used when checking unconfirmed list fields
hasUnconfirmedListTextFieldsindexesconfirmedListTextFieldswithfield.id, but the confirmation map is keyed byfieldKey(the object key instep.fields).
This results in a perpetual "unconfirmed" state, disabling the Confirm button.-const hasUnconfirmedListTextFields = browserSteps.some(step => - step.type === 'list' && Object.values(step.fields).some(field => - !confirmedListTextFields[step.id]?.[field.id] - ) -); +const hasUnconfirmedListTextFields = browserSteps.some(step => + step.type === 'list' && + Object.entries(step.fields).some(([fieldKey]) => + !confirmedListTextFields[step.id]?.[fieldKey] + ) +);
715-718:⚠️ Potential issueCall
finishAction('screenshot')when discarding to keep global state consistentThe discard handler manually resets
activeActionbut bypasses other cleanup thatfinishActionperforms (e.g. logging, telemetry, timers).- onClick={() => { - stopGetScreenshot(); - setActiveAction('none'); - }} + onClick={() => { + stopGetScreenshot(); + finishAction('screenshot'); // single source of truth for cleanup + }}
🧹 Nitpick comments (1)
src/components/recorder/RightSidePanel.tsx (1)
127-129: Consider conditional rendering based on workflow contentThese lines unconditionally set all capture actions to be shown, ignoring the previous logic that used the workflow content to determine which capture actions to display. While this change supports multiple actions in one session, consider whether you still want conditional UI based on the presence of specific action types in the workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/RightSidePanel.tsx(15 hunks)
🔇 Additional comments (5)
src/components/recorder/RightSidePanel.tsx (5)
56-70: Good integration of centralized action state managementThe component now properly leverages
useActionContextto access action state and handlers, with a clear derived state (isAnyActionActive) to control UI rendering. This is a well-structured approach that centralizes action lifecycle management.Also applies to: 76-76
487-516: Well-structured conditional rendering for capture buttonsThe capture buttons are now only displayed when no action is active (
!isAnyActionActive), which improves the user experience by preventing multiple simultaneous actions. This elegantly supports the PR's goal of allowing multiple capture actions in one recording session.
281-282: Consistent action lifecycle management across capture typesAll capture flows (text, list, screenshot) now properly call
finishActionbefore invokingonFinishCapture(), ensuring consistent state cleanup and event tracking. This is essential for reliable action state management when supporting multiple sequential actions.Also applies to: 340-343, 463-465
562-670: Good organization of pagination and limit controlsMoving the pagination and limit options inside the list capture block with conditional rendering creates a cleaner, more focused UI that guides the user through the capture process step by step.
639-654: Proper validation for numeric inputThe validation logic for the custom limit input ensures only positive integers are accepted, improving data quality and preventing errors. This is implemented both on change and via keyboard event prevention.
| notify('error', t('right_panel.errors.capture_text_discarded')); | ||
| }, [browserSteps, stopGetText, deleteBrowserStep]); | ||
| }, [browserSteps, stopGetText, deleteBrowserStep, notify, t]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using finishAction in discard handlers
Similar to the screenshot discard issue, the text and list discard handlers should also call finishAction instead of manually handling state resets, to ensure consistent cleanup across all action types.
-discardGetText = useCallback(() => {
+discardGetText = useCallback(() => {
stopGetText();
browserSteps.forEach(step => {
if (step.type === 'text') {
deleteBrowserStep(step.id);
}
});
setTextLabels({});
setErrors({});
setConfirmedTextSteps({});
setIsCaptureTextConfirmed(false);
notify('error', t('right_panel.errors.capture_text_discarded'));
+ finishAction('text');
-}, [browserSteps, stopGetText, deleteBrowserStep, notify, t]);
+}, [browserSteps, stopGetText, deleteBrowserStep, notify, t, finishAction]);And similarly for discardGetList.
Also applies to: 448-449
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/action/ActionDescriptionBox.tsx (2)
145-219: Consider extracting the repeated checkbox styling into a reusable component.The checkbox styling code is duplicated three times (for text, list, and screenshot actions). This creates unnecessary repetition in the codebase.
+ const WorkflowActionCheckbox = ({ + isDarkMode, + label + }: { + isDarkMode: boolean; + label: string + }) => ( + <FormControlLabel + control={ + <Checkbox + checked={true} + disabled + sx={{ + color: isDarkMode ? 'white' : 'default', + '&.Mui-checked': { + color: '#ff33cc', + }, + }} + /> + } + label={ + <Typography variant="body2" gutterBottom color={isDarkMode ? 'white' : 'textPrimary'}> + {label} + </Typography> + } + /> + ); {actionsInWorkflow.text && ( - <FormControlLabel - control={ - <Checkbox - checked={true} - disabled - sx={{ - color: isDarkMode ? 'white' : 'default', - '&.Mui-checked': { - color: '#ff33cc', - }, - }} - /> - } - label={ - <Typography variant="body2" gutterBottom color={isDarkMode ? 'white' : 'textPrimary'}> - {t('action_description.actions.text')} - </Typography> - } - /> + <WorkflowActionCheckbox + isDarkMode={isDarkMode} + label={t('action_description.actions.text')} + /> )}This would make the code more maintainable and reduce duplication.
131-219: Enhance accessibility for screen readers.While using disabled checkboxes is appropriate for this UI pattern, consider adding ARIA attributes to improve the experience for screen reader users, clarifying that these are status indicators rather than interactive controls.
<Checkbox checked={true} disabled + aria-label={`${t('action_description.actions.text')} action included in workflow`} sx={{ color: isDarkMode ? 'white' : 'default', '&.Mui-checked': { color: '#ff33cc', }, }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/action/ActionDescriptionBox.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/action/ActionDescriptionBox.tsx (1)
src/context/browserActions.tsx (1)
useActionContext(202-208)
🔇 Additional comments (3)
src/components/action/ActionDescriptionBox.tsx (3)
54-70: Well-structured context implementation with proper type definitions.The component now extracts
actionsInWorkflowfrom the action context, with proper TypeScript type definitions. This provides strong typing support for tracking which actions are included in the current workflow.
131-144: Good implementation of conditional rendering based on workflow state.The code now intelligently determines what to display based on the number of actions in the workflow. The approach to count active actions using
Object.values(actionsInWorkflow).filter(Boolean).lengthis clean and effective.
54-219: Great implementation supporting multiple capture actions workflow.The changes to this component effectively support the PR's main objective of allowing users to train multiple capture actions in one recording session. The UI now provides clear visual feedback about which actions are part of the current workflow, enhancing the user experience.
| const actionsInWorkflowCount = Object.values(actionsInWorkflow).filter(Boolean).length; | ||
|
|
||
| return ( | ||
| <> | ||
| <Typography variant="subtitle2" gutterBottom>{t('action_description.default.title')}</Typography> | ||
| <Typography variant="body2" gutterBottom>{t('action_description.default.description')}</Typography> | ||
| <Typography variant="subtitle2" gutterBottom> | ||
| {actionsInWorkflowCount === 0 | ||
| ? t('action_description.default.title') | ||
| : t('action_description.workflow_progress.title')} | ||
| </Typography> | ||
|
|
||
| {actionsInWorkflowCount === 0 ? ( | ||
| <Typography variant="body2" gutterBottom> | ||
| {t('action_description.default.description')} | ||
| </Typography> | ||
| ) : ( | ||
| <> | ||
| <Typography variant="body2" gutterBottom> | ||
| {t('action_description.workflow_actions.description')} | ||
| </Typography> | ||
|
|
||
| <Box mt={2}> | ||
| {actionsInWorkflow.text && ( | ||
| <FormControlLabel | ||
| control={ | ||
| <Checkbox | ||
| checked={true} | ||
| disabled | ||
| sx={{ | ||
| color: isDarkMode ? 'white' : 'default', | ||
| '&.Mui-checked': { | ||
| color: '#ff33cc', | ||
| }, | ||
| }} | ||
| /> | ||
| } | ||
| label={ | ||
| <Typography variant="body2" gutterBottom color={isDarkMode ? 'white' : 'textPrimary'}> | ||
| {t('action_description.actions.text')} | ||
| </Typography> | ||
| } | ||
| /> | ||
| )} | ||
|
|
||
| {actionsInWorkflow.list && ( | ||
| <FormControlLabel | ||
| control={ | ||
| <Checkbox | ||
| checked={true} | ||
| disabled | ||
| sx={{ | ||
| color: isDarkMode ? 'white' : 'default', | ||
| '&.Mui-checked': { | ||
| color: '#ff33cc', | ||
| }, | ||
| }} | ||
| /> | ||
| } | ||
| label={ | ||
| <Typography variant="body2" gutterBottom color={isDarkMode ? 'white' : 'textPrimary'}> | ||
| {t('action_description.actions.list')} | ||
| </Typography> | ||
| } | ||
| /> | ||
| )} | ||
|
|
||
| {actionsInWorkflow.screenshot && ( | ||
| <FormControlLabel | ||
| control={ | ||
| <Checkbox | ||
| checked={true} | ||
| disabled | ||
| sx={{ | ||
| color: isDarkMode ? 'white' : 'default', | ||
| '&.Mui-checked': { | ||
| color: '#ff33cc', | ||
| }, | ||
| }} | ||
| /> | ||
| } | ||
| label={ | ||
| <Typography variant="body2" gutterBottom color={isDarkMode ? 'white' : 'textPrimary'}> | ||
| {t('action_description.actions.screenshot')} | ||
| </Typography> | ||
| } | ||
| /> | ||
| )} | ||
| </Box> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the availability of new translation keys.
The implementation uses several new translation keys such as action_description.workflow_progress.title, action_description.workflow_actions.description, and action_description.actions.text/list/screenshot. Ensure these are defined in all language files to avoid missing translations.
Run the following script to check for the presence of these translation keys:
🏁 Script executed:
#!/bin/bash
# Check for translation keys in language files
# Find language files
echo "Looking for translation files..."
find . -name "*.json" -path "**/locales/**" | grep -v "node_modules"
# Extract and inspect one of the English language files
echo -e "\nChecking for required translation keys..."
find . -name "en.json" -path "**/locales/**" | grep -v "node_modules" | head -1 | xargs cat | grep -E "workflow_progress|workflow_actions|actions\.text|actions\.list|actions\.screenshot"Length of output: 552
🏁 Script executed:
#!/bin/bash
# Verify presence of required translation keys in en.json
# Locate the English locale file
file=$(find public/locales -type f -name en.json | head -1)
if [ -z "$file" ]; then
echo "Error: en.json not found under public/locales."
exit 1
fi
echo "Using locale file: $file"
# Use Python to check each nested key
python3 <<EOF
import json
file = "$file"
data = json.load(open(file, 'r', encoding='utf-8'))
keys_to_check = [
"action_description.workflow_progress.title",
"action_description.workflow_actions.description",
"action_description.actions.text",
"action_description.actions.list",
"action_description.actions.screenshot"
]
missing = []
for full_key in keys_to_check:
parts = full_key.split('.')
node = data
for part in parts:
if isinstance(node, dict) and part in node:
node = node[part]
else:
missing.append(full_key)
break
else:
print(f"{full_key}: {node}")
if missing:
print("\nMissing translation keys:")
for k in missing:
print(" -", k)
exit(1)
EOFLength of output: 464
Add missing translation keys to locale files
The following keys used in src/components/action/ActionDescriptionBox.tsx are absent from public/locales/en.json (and need to be mirrored in all other language files):
- action_description.workflow_progress.title
- action_description.workflow_actions.description
- action_description.actions.text
- action_description.actions.list
- action_description.actions.screenshot
Please add these under the action_description namespace in each locale (e.g. public/locales/en.json, de.json, es.json, ja.json, zh.json) to prevent missing-translation errors at runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/src/workflow-management/classes/Interpreter.ts (2)
90-100: Introduce a typed record instead of loosely–typedany[]buckets
serializableDataByTypeis always expected to contain homogeneous payloads for three well-known capture types.
Declaring the fields asany[]forfeits compile-time safety and invites accidental misuse (e.g., pushing a string intoscrapeSchema). A small interface brings clarity and IDE completion at zero runtime cost.- public serializableDataByType: { - scrapeSchema: any[], - scrapeList: any[], - other: any[] - } = { + interface SerializableBuckets { + scrapeSchema: unknown[]; + scrapeList: unknown[]; + other: unknown[]; + } + + public serializableDataByType: SerializableBuckets = { scrapeSchema: [], scrapeList: [], other: [] };(You can use a more specific union instead of
unknownonce the payload shapes are known.)
380-409: Replace O(n²) object spreads inside.reducewith a linear accumulatorBiome is right: spreading an ever-growing object each iteration degrades to quadratic time and memory churn.
A simple loop (orforEach) mutating a fresh object is clearer and faster.-const scrapeSchemaOutput = Object.keys(mergedScrapeSchema).length > 0 - ? { "schema-merged": [mergedScrapeSchema] } - : this.serializableDataByType.scrapeSchema.reduce((acc, item, idx) => { - return { [`schema-${idx}`]: item, ...acc }; - }, {}); +const scrapeSchemaOutput: Record<string, unknown> = {}; +if (Object.keys(mergedScrapeSchema).length > 0) { + scrapeSchemaOutput["schema-merged"] = [mergedScrapeSchema]; +} else { + this.serializableDataByType.scrapeSchema.forEach((item, idx) => { + scrapeSchemaOutput[`schema-${idx}`] = item; + }); +}Apply the same pattern to
scrapeListOutput,otherOutput, andbinaryOutput.🧰 Tools
🪛 Biome (1.9.4)
[error] 388-388: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 394-394: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 400-400: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 406-406: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/classes/Interpreter.ts(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/src/workflow-management/classes/Interpreter.ts (1)
maxun-core/src/interpret.ts (1)
Interpreter(55-1233)
🪛 Biome (1.9.4)
server/src/workflow-management/classes/Interpreter.ts
[error] 388-388: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 394-394: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (1)
server/src/workflow-management/classes/Interpreter.ts (1)
103-106:currentActionTypemay race in asynchronous steps – verify thread-safety
currentActionTypeis mutated by everysetActionTypecall and is relied on immediately afterwards byserializableCallback.
If the interpreter ever executes two workflow steps in parallel (e.g., throughPromise.allor Playwright’spage.waitFor*callbacks), the variable could be overwritten before the correspondingserializableCallbackfires, leading to mis-categorised data.Please verify that
maxun-coreguarantees sequential execution, or consider capturing the type in a closure:serializableCallback: (data: any, action = this.currentActionType) => { … }
| if (this.currentActionType === 'scrapeSchema') { | ||
| if (Array.isArray(data) && data.length > 0) { | ||
| mergedScrapeSchema = { ...mergedScrapeSchema, ...data[0] }; | ||
| this.serializableDataByType.scrapeSchema.push(data); | ||
| } else { | ||
| mergedScrapeSchema = { ...mergedScrapeSchema, ...data }; | ||
| this.serializableDataByType.scrapeSchema.push([data]); | ||
| } | ||
| } else if (this.currentActionType === 'scrapeList') { | ||
| this.serializableDataByType.scrapeList.push(data); | ||
| } else { | ||
| this.serializableDataByType.other.push(data); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
scrapeSchema merge logic ignores multi-element arrays
When data is [{…}, {…}] you only merge the first element (data[0]).
Either iterate or merge every item:
- mergedScrapeSchema = { ...mergedScrapeSchema, ...data[0] };
+ data.forEach(obj => Object.assign(mergedScrapeSchema, obj));Failing to do so silently drops captured fields.
📝 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.
| if (this.currentActionType === 'scrapeSchema') { | |
| if (Array.isArray(data) && data.length > 0) { | |
| mergedScrapeSchema = { ...mergedScrapeSchema, ...data[0] }; | |
| this.serializableDataByType.scrapeSchema.push(data); | |
| } else { | |
| mergedScrapeSchema = { ...mergedScrapeSchema, ...data }; | |
| this.serializableDataByType.scrapeSchema.push([data]); | |
| } | |
| } else if (this.currentActionType === 'scrapeList') { | |
| this.serializableDataByType.scrapeList.push(data); | |
| } else { | |
| this.serializableDataByType.other.push(data); | |
| } | |
| if (this.currentActionType === 'scrapeSchema') { | |
| if (Array.isArray(data) && data.length > 0) { | |
| data.forEach(obj => Object.assign(mergedScrapeSchema, obj)); | |
| this.serializableDataByType.scrapeSchema.push(data); | |
| } else { | |
| mergedScrapeSchema = { ...mergedScrapeSchema, ...data }; | |
| this.serializableDataByType.scrapeSchema.push([data]); | |
| } | |
| } else if (this.currentActionType === 'scrapeList') { | |
| this.serializableDataByType.scrapeList.push(data); | |
| } else { | |
| this.serializableDataByType.other.push(data); | |
| } |
| serializableCallback: (data: any) => { | ||
| this.socket.emit('serializableCallback', data); | ||
| if (this.currentActionType === 'scrapeSchema') { | ||
| if (Array.isArray(data) && data.length > 0) { | ||
| this.socket.emit('serializableCallback', { | ||
| type: 'captureText', | ||
| data | ||
| }); | ||
| } else { | ||
| this.socket.emit('serializableCallback', { | ||
| type: 'captureText', | ||
| data : [data] | ||
| }); | ||
| } | ||
| } else if (this.currentActionType === 'scrapeList') { | ||
| this.socket.emit('serializableCallback', { | ||
| type: 'captureList', | ||
| data | ||
| }); | ||
| } else { | ||
| this.socket.emit('serializableCallback', { | ||
| type: 'other', | ||
| data | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent event type and missing server-side storage
-
The event emitted for a
scrapeSchemaaction is tagged as"captureText", whereas the runtime concept is “schema capture”. Down-stream consumers (e.g., the newRightSidePanel) expect"captureText","captureList", and"captureScreenshot".
• If the UI indeed uses"captureText"for schema, leave a comment clarifying this naming choice.
• Otherwise, rename to"captureSchema"(or similar) to prevent future confusion. -
Unlike
InterpretRecording, this branch never pushes the received payload intoserializableDataByType, so an editor-side session loses the data after the socket emission. If later features (e.g., re-running the last capture) rely on the bucket, this will fail silently.
if (this.currentActionType === 'scrapeSchema') {
…
- this.socket.emit('serializableCallback', { type: 'captureText', data })
+ this.serializableDataByType.scrapeSchema.push(Array.isArray(data) ? data : [data]);
+ this.socket.emit('serializableCallback', { type: 'captureSchema', data })Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/components/run/InterpretationLog.tsx (3)
116-132: Large base64 screenshots can bloat memoryEvery screenshot is stored as an in-memory base64 string; long sessions may exhaust browser memory.
Options:
- Revoke previous URL objects / discard images once “Next” is pressed.
- Stream them to
indexedDB/localStorageand keep only the active image in state.- Compress server-side before emitting.
Consider at least imposing a max-screenshot cap or warning the user.
166-186: ExtractgetAvailableTabsoutside render path
getAvailableTabsis re-created on every render, and its reference is part ofhandleSerializableCallback’s dependency list, causing additional re-creations of that callback and potentially excess re-renders.Move it outside the component or memoise with
useMemo:-const getAvailableTabs = useCallback(() => { +const getAvailableTabs = useMemo(() => { const tabs = []; ... return tabs; -}, [captureListData.length, ...]); +}, [captureListData.length, captureTextData.length, screenshotData.length, otherData.length]);This keeps a stable reference while still updating when the relevant lengths change.
428-460: Row keys are plain indexes – introduce stable identifiersUsing
idxforkeycan cause React to mis-identify rows if sorting or pagination changes. If each row has a natural unique column (e.g.,id,url) use that; otherwise, compute a hash of the row contents.-<TableRow key={idx}> +<TableRow key={row.id ?? idx}>This prevents subtle UI glitches and improves reconciliation performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/run/InterpretationLog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/run/InterpretationLog.tsx
[error] 347-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 521-542: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| if (type === 'captureList' && Array.isArray(data)) { | ||
| setCaptureListData(prev => [...prev, data]); | ||
| if (captureListData.length === 0) { | ||
| const availableTabs = getAvailableTabs(); | ||
| const tabIndex = availableTabs.findIndex(tab => tab.id === 'captureList'); | ||
| if (tabIndex !== -1) setActiveTab(tabIndex); | ||
| } | ||
| } else if (type === 'captureText') { | ||
| if (Array.isArray(data)) { | ||
| setCaptureTextData(data); | ||
| } else { | ||
| setCaptureTextData([data]); | ||
| } | ||
| if (captureTextData.length === 0) { | ||
| const availableTabs = getAvailableTabs(); | ||
| const tabIndex = availableTabs.findIndex(tab => tab.id === 'captureText'); | ||
| if (tabIndex !== -1) setActiveTab(tabIndex); | ||
| } | ||
| } else if (type === 'other') { |
There was a problem hiding this comment.
captureText branch overwrites previous data – likely unintended
Unlike captureList & other, the captureText branch replaces existing state instead of appending, so only the last capture survives:
if (Array.isArray(data)) {
setCaptureTextData(data); // <-- overwrite
} else {
setCaptureTextData([data]); // <-- overwrite
}If multiple text captures are expected per session, append instead:
-if (Array.isArray(data)) {
- setCaptureTextData(data);
-} else {
- setCaptureTextData([data]);
-}
+setCaptureTextData(prev =>
+ Array.isArray(data) ? [...prev, ...data] : [...prev, data]
+);Doing so aligns behaviour with the other categories and fixes silently-lost captures.
| const [captureListData, setCaptureListData] = useState<any[]>([]); | ||
| const [captureTextData, setCaptureTextData] = useState<any[]>([]); | ||
| const [screenshotData, setScreenshotData] = useState<string[]>([]); | ||
| const [otherData, setOtherData] = useState<any[]>([]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Give the capture-state hooks proper typing
All four pieces of state hold well-defined structures, yet they are typed any[], forfeiting type-safety and IntelliSense. Consider explicit generics:
-const [captureListData, setCaptureListData] = useState<any[]>([]);
-const [captureTextData, setCaptureTextData] = useState<any[]>([]);
-const [screenshotData, setScreenshotData] = useState<string[]>([]);
-const [otherData, setOtherData] = useState<any[]>([]);
+type Row = Record<string, unknown>;
+
+const [captureListData, setCaptureListData] = useState<Row[][]>([]);
+const [captureTextData, setCaptureTextData] = useState<Row[]>([]);
+const [screenshotData, setScreenshotData] = useState<string[]>([]);
+const [otherData, setOtherData] = useState<Row[][]>([]);This prevents accidental misuse (e.g., pushing plain strings into captureListData).
📝 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 [captureListData, setCaptureListData] = useState<any[]>([]); | |
| const [captureTextData, setCaptureTextData] = useState<any[]>([]); | |
| const [screenshotData, setScreenshotData] = useState<string[]>([]); | |
| const [otherData, setOtherData] = useState<any[]>([]); | |
| type Row = Record<string, unknown>; | |
| const [captureListData, setCaptureListData] = useState<Row[][]>([]); | |
| const [captureTextData, setCaptureTextData] = useState<Row[]>([]); | |
| const [screenshotData, setScreenshotData] = useState<string[]>([]); | |
| const [otherData, setOtherData] = useState<Row[][]>([]); |
| @@ -1,7 +1,7 @@ | |||
| import * as React from 'react'; | |||
| import SwipeableDrawer from '@mui/material/SwipeableDrawer'; | |||
| import Typography from '@mui/material/Typography'; | |||
| import { Button, Grid } from '@mui/material'; | |||
| import { Button, Grid, Tabs, Tab, Box } from '@mui/material'; | |||
There was a problem hiding this comment.
Remove unused Tabs & Tab imports
Tabs and Tab are imported but never referenced after the refactor – only custom Box elements are used for the tab bar. Dropping the dead imports keeps the bundle leaner and prevents eslint warnings.
-import { Button, Grid, Tabs, Tab, Box } from '@mui/material';
+import { Button, Grid, Box } from '@mui/material';📝 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.
| import { Button, Grid, Tabs, Tab, Box } from '@mui/material'; | |
| -import { Button, Grid, Tabs, Tab, Box } from '@mui/material'; | |
| +import { Button, Grid, Box } from '@mui/material'; |
| {captureListData[captureListPage] && | ||
| captureListData[captureListPage].map((row: any, idx: any) => ( | ||
| <TableRow | ||
| key={idx} | ||
| sx={{ | ||
| borderBottom: '1px solid', | ||
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | ||
| }} | ||
| > | ||
| {Object.keys(row).map((column) => ( | ||
| <TableCell | ||
| key={column} | ||
| sx={{ | ||
| borderBottom: 'none', | ||
| py: 2 | ||
| }} | ||
| > | ||
| {row[column]} | ||
| </TableCell> | ||
| ))} | ||
| </TableRow> | ||
| ))} |
There was a problem hiding this comment.
Leverage optional chaining to silence potential undefined access
Static analysis rightly flags captureListData[captureListPage].map(...) because the index lookup can be undefined. Optional chaining avoids runtime errors and removes linter noise:
-{captureListData[captureListPage] &&
- captureListData[captureListPage].map((row, idx) => (
+{captureListData[captureListPage]?.map((row, idx) => ((Repeat for the surrounding header construction.)
📝 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.
| {captureListData[captureListPage] && | |
| captureListData[captureListPage].map((row: any, idx: any) => ( | |
| <TableRow | |
| key={idx} | |
| sx={{ | |
| borderBottom: '1px solid', | |
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | |
| }} | |
| > | |
| {Object.keys(row).map((column) => ( | |
| <TableCell | |
| key={column} | |
| sx={{ | |
| borderBottom: 'none', | |
| py: 2 | |
| }} | |
| > | |
| {row[column]} | |
| </TableCell> | |
| ))} | |
| </TableRow> | |
| ))} | |
| {captureListData[captureListPage]?.map((row: any, idx: any) => ( | |
| <TableRow | |
| key={idx} | |
| sx={{ | |
| borderBottom: '1px solid', | |
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | |
| }} | |
| > | |
| {Object.keys(row).map((column) => ( | |
| <TableCell | |
| key={column} | |
| sx={{ | |
| borderBottom: 'none', | |
| py: 2 | |
| }} | |
| > | |
| {row[column]} | |
| </TableCell> | |
| ))} | |
| </TableRow> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 347-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| {otherData[otherPage] && | ||
| otherData[otherPage].map((row: any, idx: any) => ( | ||
| <TableRow | ||
| key={idx} | ||
| sx={{ | ||
| borderBottom: '1px solid', | ||
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | ||
| }} | ||
| > | ||
| {Object.keys(row).map((column) => ( | ||
| <TableCell | ||
| key={column} | ||
| sx={{ | ||
| borderBottom: 'none', | ||
| py: 2 | ||
| }} | ||
| > | ||
| {row[column]} | ||
| </TableCell> | ||
| ))} | ||
| </TableRow> | ||
| ))} |
There was a problem hiding this comment.
Same optional-chain issue within “other” dataset rendering
Adopt identical pattern for the other section:
-{otherData[otherPage] &&
- otherData[otherPage].map((row, idx) => (
+{otherData[otherPage]?.map((row, idx) => (This keeps the table empty instead of crashing when the page index is temporarily out of range after reset.
📝 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.
| {otherData[otherPage] && | |
| otherData[otherPage].map((row: any, idx: any) => ( | |
| <TableRow | |
| key={idx} | |
| sx={{ | |
| borderBottom: '1px solid', | |
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | |
| }} | |
| > | |
| {Object.keys(row).map((column) => ( | |
| <TableCell | |
| key={column} | |
| sx={{ | |
| borderBottom: 'none', | |
| py: 2 | |
| }} | |
| > | |
| {row[column]} | |
| </TableCell> | |
| ))} | |
| </TableRow> | |
| ))} | |
| {otherData[otherPage]?.map((row: any, idx: any) => ( | |
| <TableRow | |
| key={idx} | |
| sx={{ | |
| borderBottom: '1px solid', | |
| borderColor: darkMode ? '#3a4453' : '#dee2e6' | |
| }} | |
| > | |
| {Object.keys(row).map((column) => ( | |
| <TableCell | |
| key={column} | |
| sx={{ | |
| borderBottom: 'none', | |
| py: 2 | |
| }} | |
| > | |
| {row[column]} | |
| </TableCell> | |
| ))} | |
| </TableRow> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 521-542: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| {activeTab === availableTabs.findIndex(tab => tab.id === 'captureList') && captureListData.length > 0 && ( | ||
| <Box> | ||
| <Box sx={{ | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| alignItems: 'center', | ||
| mb: 2, | ||
| mt: 2 | ||
| }}> | ||
| <Typography variant="body2"> | ||
| {`Table ${captureListPage + 1} of ${captureListData.length}`} | ||
| </Typography> | ||
| <Box> | ||
| <Button | ||
| onClick={() => setCaptureListPage(prev => Math.max(0, prev - 1))} | ||
| disabled={captureListPage === 0} | ||
| size="small" | ||
| > | ||
| Previous | ||
| </Button> | ||
| <Button | ||
| onClick={() => setCaptureListPage(prev => Math.min(captureListData.length - 1, prev + 1))} | ||
| disabled={captureListPage >= captureListData.length - 1} | ||
| size="small" | ||
| sx={{ ml: 1 }} | ||
| > | ||
| Next | ||
| </Button> | ||
| </Box> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pagination buttons can desync with data length
When a dataset is reset (e.g., shouldResetInterpretationLog becomes true) the current page indices remain, but the underlying arrays are emptied first, triggering “index out of bounds” until the indices are reset later in the same effect. Reset the page indices before clearing the arrays to avoid the transient mismatch:
-setCaptureListData([]);
-setCaptureTextData([]);
-...
-setCaptureListPage(0);
+setCaptureListPage(0);
+setScreenshotPage(0);
+setOtherPage(0);
+setCaptureListData([]);
+setCaptureTextData([]);
+...Alternatively, guard the useEffect cleanup with batching (React.startTransition) or move pagination index checks into the render.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/browser/BrowserContent.tsx (1)
81-103: 💡 Verification agent🧩 Analysis chain
Guard against invalid URLs and avoid unnecessary re-renders
new URL(url)will throw for non-absolute or malformed URLs (e.g. “about:blank”, empty string, plain text). A single bad navigation event will crash the whole component.- The dependency array
[tabs, tabIndex]causeshandleUrlChangedto be recreated on every tab switch or title update, negating the memoisation win ofuseCallback.tabsis an array and its reference changes on everysetTabscall, so the function will almost always be regenerated.const handleUrlChanged = useCallback((url: string) => { - const parsedUrl = new URL(url); + let parsedUrl: URL | null = null; + try { + parsedUrl = new URL(url); + } catch (_) { + // Ignore invalid URLs (e.g. user typing) + return; + } ... - }, [tabs, tabIndex]); + // tabs is derived state, tabIndex is primitive – exclude them to keep a + // stable reference; use functional setState to read latest tabs instead. + }, [tabIndex]);Using the functional form of
setTabsinside the callback will always provide the latesttabs, making it safe to drop the array from the deps.
🏁 Script executed:
#!/bin/bash rg -n "handleUrlChanged" -n src/components/browser/BrowserContent.tsx || true rg -n "useCallback" -n src/components/browser/BrowserContent.tsx | sed -n '1,20p' rg -n "setTabs" -n src/components/browser/BrowserContent.tsx | sed -n '1,20p'Length of output: 1041
Guard against invalid URLs and stabilize handleUrlChanged
Wrapping the URL constructor in a try/catch prevents a malformed or non-absolute URL (e.g. “about:blank”, an empty string, or user input) from crashing your component. You can also lean fully on the functional updater for
tabs, remove thetabsarray from the dependency list, and only depend on the primitivetabIndex.File:
src/components/browser/BrowserContent.tsx
Lines: 81–103- const handleUrlChanged = useCallback((url: string) => { - const parsedUrl = new URL(url); - if (parsedUrl.hostname) { - const host = parsedUrl.hostname - .match(/\b(?!www\.)[a-zA-Z0-9]+/g) - ?.join("."); - if (host && host !== tabs[tabIndex]) { - setTabs((prevState) => [ - ...prevState.slice(0, tabIndex), - host, - ...prevState.slice(tabIndex + 1), - ]); - } - } else { - if (tabs[tabIndex] !== "new tab") { - setTabs((prevState) => [ - ...prevState.slice(0, tabIndex), - "new tab", - ...prevState.slice(tabIndex + 1), - ]); - } - } - }, [tabs, tabIndex]); + const handleUrlChanged = useCallback((url: string) => { + let parsedUrl: URL; + try { + parsedUrl = new URL(url); + } catch { + // Invalid or relative URL – ignore + return; + } + + setTabs((prevTabs) => { + const current = prevTabs[tabIndex]; + const host = parsedUrl.hostname + .match(/\b(?!www\.)[a-zA-Z0-9]+/g) + ?.join("."); + + if (host) { + if (host !== current) { + return [ + ...prevTabs.slice(0, tabIndex), + host, + ...prevTabs.slice(tabIndex + 1), + ]; + } + } else if (current !== "new tab") { + return [ + ...prevTabs.slice(0, tabIndex), + "new tab", + ...prevTabs.slice(tabIndex + 1), + ]; + } + + return prevTabs; + }); + }, [tabIndex]);
- We catch parse errors to avoid crashes on invalid URLs.
- We only list
tabIndexin the deps, because all access totabsnow happens inside the functional updater.
🧹 Nitpick comments (4)
maxun-core/src/interpret.ts (2)
381-384: DRY up repetitivesetActionTypecallsThe same 3-line check is copy-pasted ten times. Repetition is error-prone and clutters the core logic.
+ private setCurrentAction(type: string) { + this.options.debugChannel?.setActionType?.(type); + } ... - if (this.options.debugChannel?.setActionType) { - this.options.debugChannel.setActionType('screenshot'); - } + this.setCurrentAction('screenshot');Benefits: easier maintenance, consistent behaviour, and smaller diff surface for future additions.
Also applies to: 391-394, 421-424, 432-435, 462-465, 477-480, 491-494, 504-507, 516-519, 550-553
440-458: Potential data race when mergingscrapeSchemaresults
this.cumulativeResults[0]is mutated in place and then passed by reference toserializableCallback.
If twoscrapeSchemaactions run concurrently (e.g. viaenqueueLinks), parallel writes can interleave and corrupt the object.Consider deep-cloning before mutation or guarding the whole block with the existing
Concurrencyhelper.- const mergedResult = this.cumulativeResults[0]; + const mergedResult = { ...this.cumulativeResults[0] }; // clone ... this.cumulativeResults[0] = mergedResult;Alternatively, move this logic into a mutex/queue inside
Concurrency.src/components/run/RunContent.tsx (2)
195-202: CSV download leaks Blob URLs
downloadCSVcreates an object URL but never revokes it, unlikedownloadJSON.const downloadCSV = (data: any[], columns: string[], filename: string) => { const csvContent = convertToCSV(data, columns); const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); const url = URL.createObjectURL(blob); ... document.body.removeChild(link); + setTimeout(() => URL.revokeObjectURL(url), 100); };Without revocation, every click accumulates an in-memory entry.
217-233: Use a single helper for file downloads
downloadCSV,downloadJSON, anddownloadAllJSONduplicate link/URL logic.
Extracting asaveBlob(blob: Blob, filename: string)utility would consolidate the behaviour and ensure all callers revoke URLs consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
maxun-core/src/interpret.ts(8 hunks)src/components/browser/BrowserContent.tsx(3 hunks)src/components/run/RunContent.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
maxun-core/src/interpret.ts
[error] 48-48: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
src/components/run/RunContent.tsx (1)
258-264: Edge-case: navigating beyond list bounds
navigateListTable('next')and'prev'rely on stalecurrentListIndexbecause they close over the old value.
Use the functional updater form to guarantee correctness when users click quickly:setCurrentListIndex(idx => Math.min(idx + 1, listData.length - 1));Same for the
'prev'branch.
feat: runs ui v2
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/run/RunContent.tsx (4)
57-59: Redundant state update inuseEffect
setTab(tab)inside the effect does not change the state (it is set to the same value) but still triggers a re-render on everyinterpretationInProgresschange.
Simply remove the setter call – the component already keeps the current tab in state.- useEffect(() => { - setTab(tab); - }, [interpretationInProgress]); + // No need to mirror `interpretationInProgress` into `tab`
483-490: Call site passes empty data/columns intorenderDataTableThe
listDatablock deliberately passes[]fordataandcolumns, relying on the internal pagination logic.
Besides producing empty downloads (see previous comment) this obscures intent and violates the component contract (“supply me the data I need to render”). Consider passing the flattened list (or the first table) plus a flag, or splitting the component into two dedicated variants.
496-580: Unused React fragmentThe fragment
<> ... </>around the singleAccordionelement is unnecessary and flagged by Biome.
Simply return theAccordionnode.- <> <Accordion defaultExpanded sx={{ mb: 2 }}> … </Accordion> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 497-579: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
241-248: Index navigation can underflow/overflow if keys length changesIf
row.binaryOutputchanges while the user is viewing screenshots (e.g., live updates during a running session),currentScreenshotIndexmight now be out of bounds.
Add a guard in theuseEffectthat setscurrentScreenshotIndex(0)wheneverscreenshotKeys.lengthshrinks below the current index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.json(0 hunks)public/locales/de.json(1 hunks)public/locales/en.json(2 hunks)public/locales/es.json(1 hunks)public/locales/ja.json(1 hunks)public/locales/zh.json(1 hunks)src/components/run/RunContent.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- public/locales/de.json
- public/locales/es.json
- public/locales/ja.json
- public/locales/zh.json
- public/locales/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/run/RunContent.tsx
[error] 497-579: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
| const processLegacyData = (legacyOutput: Record<string, any>) => { | ||
| let allData: any[] = []; | ||
|
|
||
| Object.keys(legacyOutput).forEach(key => { | ||
| const data = legacyOutput[key]; | ||
| if (Array.isArray(data)) { | ||
| // Filter out completely empty rows | ||
| const filteredData = data.filter(row => | ||
| Object.values(row).some(value => value !== undefined && value !== "") | ||
| ); | ||
| setTableData(filteredData); | ||
| if (filteredData.length > 0) { | ||
| setColumns(Object.keys(filteredData[0])); | ||
| } | ||
| allData = [...allData, ...filteredData]; | ||
| } | ||
| }); | ||
|
|
||
| if (allData.length > 0) { | ||
| const allColumns = new Set<string>(); | ||
| allData.forEach(item => { | ||
| Object.keys(item).forEach(key => allColumns.add(key)); | ||
| }); | ||
|
|
||
| setLegacyData(allData); | ||
| setLegacyColumns(Array.from(allColumns)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially stale legacy state when switching runs
processLegacyData only updates legacyData / legacyColumns when it finds non-empty data, but does not reset the state when the next run has no legacy output.
Add an else branch or clear the state at the beginning of the function to avoid displaying stale data from a previous run.
| // Function to download a specific dataset as CSV | ||
| const downloadCSV = (data: any[], columns: string[], filename: string) => { | ||
| const csvContent = convertToCSV(data, columns); | ||
| const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); | ||
| const url = URL.createObjectURL(blob); | ||
|
|
||
| const link = document.createElement("a"); | ||
| link.href = url; | ||
| link.setAttribute("download", "data.csv"); | ||
| link.setAttribute("download", filename); | ||
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); | ||
| }; |
There was a problem hiding this comment.
Missing URL.revokeObjectURL after CSV download
downloadCSV creates an object URL but never revokes it, leaking a browser resource every time the user clicks “Download as CSV”.
Mirror the clean-up strategy you already use in downloadJSON.
const downloadCSV = (data: any[], columns: string[], filename: string) => {
const csvContent = convertToCSV(data, columns);
const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' });
const url = URL.createObjectURL(blob);
@@
document.body.removeChild(link);
+
+ // Free the object URL – consistent with downloadJSON
+ setTimeout(() => {
+ URL.revokeObjectURL(url);
+ }, 100);
};📝 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 to download a specific dataset as CSV | |
| const downloadCSV = (data: any[], columns: string[], filename: string) => { | |
| const csvContent = convertToCSV(data, columns); | |
| const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.setAttribute("download", "data.csv"); | |
| link.setAttribute("download", filename); | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| }; | |
| // Function to download a specific dataset as CSV | |
| const downloadCSV = (data: any[], columns: string[], filename: string) => { | |
| const csvContent = convertToCSV(data, columns); | |
| const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.setAttribute("download", filename); | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| // Free the object URL – consistent with downloadJSON | |
| setTimeout(() => { | |
| URL.revokeObjectURL(url); | |
| }, 100); | |
| }; |
| <Box> | ||
| <Button | ||
| component="a" | ||
| onClick={() => downloadJSON(data, jsonFilename)} | ||
| sx={{ | ||
| color: '#FF00C3', | ||
| textTransform: 'none', | ||
| mr: 2, | ||
| p: 0, | ||
| minWidth: 'auto', | ||
| backgroundColor: 'transparent', | ||
| '&:hover': { | ||
| backgroundColor: 'transparent', | ||
| textDecoration: 'underline' | ||
| } | ||
| }} | ||
| > | ||
| Download as JSON | ||
| </Button> | ||
|
|
||
| <Button | ||
| component="a" | ||
| onClick={() => downloadCSV(data, columns, csvFilename)} | ||
| sx={{ | ||
| color: '#FF00C3', | ||
| textTransform: 'none', | ||
| p: 0, | ||
| minWidth: 'auto', | ||
| backgroundColor: 'transparent', | ||
| '&:hover': { | ||
| backgroundColor: 'transparent', | ||
| textDecoration: 'underline' | ||
| } | ||
| }} | ||
| > | ||
| Download as CSV | ||
| </Button> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Downloads of paginated list data are always empty
When isPaginatedList is true, renderDataTable is invoked with data and columns set to empty arrays (see call site on lines 484-490).
Consequently, both “Download as JSON” and “Download as CSV” buttons produce empty files.
Two straightforward fixes:
- onClick={() => downloadJSON(data, jsonFilename)}
+ onClick={() => downloadJSON(
+ isPaginatedList ? listData.flat() : data,
+ jsonFilename
+ )}
- onClick={() => downloadCSV(data, columns, csvFilename)}
+ onClick={() => downloadCSV(
+ isPaginatedList ? listData.flat() : data,
+ isPaginatedList ? Array.from(new Set(listColumns.flat())) : columns,
+ csvFilename
+ )}Alternatively, move the download buttons outside renderDataTable and generate the files directly where the full dataset is known.
📝 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.
| <Box> | |
| <Button | |
| component="a" | |
| onClick={() => downloadJSON(data, jsonFilename)} | |
| sx={{ | |
| color: '#FF00C3', | |
| textTransform: 'none', | |
| mr: 2, | |
| p: 0, | |
| minWidth: 'auto', | |
| backgroundColor: 'transparent', | |
| '&:hover': { | |
| backgroundColor: 'transparent', | |
| textDecoration: 'underline' | |
| } | |
| }} | |
| > | |
| Download as JSON | |
| </Button> | |
| <Button | |
| component="a" | |
| onClick={() => downloadCSV(data, columns, csvFilename)} | |
| sx={{ | |
| color: '#FF00C3', | |
| textTransform: 'none', | |
| p: 0, | |
| minWidth: 'auto', | |
| backgroundColor: 'transparent', | |
| '&:hover': { | |
| backgroundColor: 'transparent', | |
| textDecoration: 'underline' | |
| } | |
| }} | |
| > | |
| Download as CSV | |
| </Button> | |
| </Box> | |
| <Box> | |
| <Button | |
| component="a" | |
| - onClick={() => downloadJSON(data, jsonFilename)} | |
| + onClick={() => downloadJSON( | |
| + isPaginatedList ? listData.flat() : data, | |
| + jsonFilename | |
| + )} | |
| sx={{ | |
| color: '#FF00C3', | |
| textTransform: 'none', | |
| mr: 2, | |
| p: 0, | |
| minWidth: 'auto', | |
| backgroundColor: 'transparent', | |
| '&:hover': { | |
| backgroundColor: 'transparent', | |
| textDecoration: 'underline' | |
| } | |
| }} | |
| > | |
| Download as JSON | |
| </Button> | |
| <Button | |
| component="a" | |
| - onClick={() => downloadCSV(data, columns, csvFilename)} | |
| + onClick={() => downloadCSV( | |
| + isPaginatedList ? listData.flat() : data, | |
| + isPaginatedList ? Array.from(new Set(listColumns.flat())) : columns, | |
| + csvFilename | |
| + )} | |
| sx={{ | |
| color: '#FF00C3', | |
| textTransform: 'none', | |
| p: 0, | |
| minWidth: 'auto', | |
| backgroundColor: 'transparent', | |
| '&:hover': { | |
| backgroundColor: 'transparent', | |
| textDecoration: 'underline' | |
| } | |
| }} | |
| > | |
| Download as CSV | |
| </Button> | |
| </Box> |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/run/RunContent.tsx (3)
91-113:⚠️ Potential issueReset legacy data state to avoid displaying stale data.
processLegacyDataonly updates state when it finds non-empty data, but does not clear previous data when a new run has empty output. This can lead to displaying stale data from a previous run.Add an explicit state reset at the beginning of the function:
const processLegacyData = (legacyOutput: Record<string, any>) => { + // Reset legacy data to avoid showing stale data from previous runs + setLegacyData([]); + setLegacyColumns([]); let allData: any[] = []; Object.keys(legacyOutput).forEach(key => { const data = legacyOutput[key];
202-214:⚠️ Potential issueMissing
URL.revokeObjectURLafter CSV download.
downloadCSVcreates an object URL but never revokes it, leaking a browser resource every time the user clicks "Download as CSV".Mirror the clean-up strategy you already use in
downloadJSON:const downloadCSV = (data: any[], columns: string[], filename: string) => { const csvContent = convertToCSV(data, columns); const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); const url = URL.createObjectURL(blob); const link = document.createElement("a"); link.href = url; link.setAttribute("download", filename); document.body.appendChild(link); link.click(); document.body.removeChild(link); + + // Free the object URL – consistent with downloadJSON + setTimeout(() => { + URL.revokeObjectURL(url); + }, 100); };
280-317:⚠️ Potential issueDownloads of paginated list data are always empty.
When
isPaginatedListis true,renderDataTableis invoked withdataandcolumnsset to empty arrays (see call site on lines 484-490). Consequently, both "Download as JSON" and "Download as CSV" buttons produce empty files.Update the download handlers to use the correct data source:
- onClick={() => downloadJSON(data, jsonFilename)} + onClick={() => downloadJSON( + isPaginatedList ? listData.flat() : data, + jsonFilename + )} - onClick={() => downloadCSV(data, columns, csvFilename)} + onClick={() => downloadCSV( + isPaginatedList ? listData.flat() : data, + isPaginatedList ? Array.from(new Set(listColumns.flat())) : columns, + csvFilename + )}
🧹 Nitpick comments (5)
src/components/run/RunContent.tsx (5)
115-141: Refactor to reduce code duplication with legacy data processing.
processDataCategoryandprocessLegacyDatahave almost identical logic. Consider consolidating them into a single reusable function to improve maintainability.You could define a common utility function:
const processDataArray = ( dataObj: Record<string, any>, setData: React.Dispatch<React.SetStateAction<any[]>>, setColumns: React.Dispatch<React.SetStateAction<string[]>> ) => { // Reset state to avoid stale data setData([]); setColumns([]); let allData: any[] = []; Object.keys(dataObj).forEach(key => { const data = dataObj[key]; if (Array.isArray(data)) { const filteredData = data.filter(row => Object.values(row).some(value => value !== undefined && value !== "") ); allData = [...allData, ...filteredData]; } }); if (allData.length > 0) { const allColumns = new Set<string>(); allData.forEach(item => { Object.keys(item).forEach(key => allColumns.add(key)); }); setData(allData); setColumns(Array.from(allColumns)); } };Then reuse it:
const processLegacyData = (legacyOutput: Record<string, any>) => { processDataArray(legacyOutput, setLegacyData, setLegacyColumns); }; const processDataCategory = ( categoryData: Record<string, any>, setData: React.Dispatch<React.SetStateAction<any[]>>, setColumns: React.Dispatch<React.SetStateAction<string[]>> ) => { processDataArray(categoryData, setData, setColumns); };
497-593: Remove unnecessary Fragment wrapper.The Fragment wrapper around the Accordion is redundant since it contains only a single child component.
{hasScreenshots && ( - <> <Accordion defaultExpanded sx={{ mb: 2 }}> {/* Accordion content */} </Accordion> - </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 497-592: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
514-528: Add user feedback for screenshot download failures.The screenshot download process catches errors but only logs them to the console. Consider adding user feedback for failed downloads.
.catch(err => { console.error('Download failed:', err); + // Add user feedback + // You could use a toast notification or other feedback mechanism + // Example with Material-UI Snackbar: + setError(t('run_content.download_failed', 'Download failed')); });Consider implementing a feedback system (like a Snackbar component) to notify users of errors, which would be reusable across your application.
143-191: Optimize list processing for better performance and readability.
processScrapeListcontains nearly identical logic branches for array and object handling. Consider refactoring to reduce duplication and improve readability.const processScrapeList = (scrapeListData: any) => { const tablesList: any[][] = []; const columnsList: string[][] = []; - if (Array.isArray(scrapeListData)) { - scrapeListData.forEach(tableData => { - if (Array.isArray(tableData) && tableData.length > 0) { - const filteredData = tableData.filter(row => - Object.values(row).some(value => value !== undefined && value !== "") - ); - - if (filteredData.length > 0) { - tablesList.push(filteredData); - - const tableColumns = new Set<string>(); - filteredData.forEach(item => { - Object.keys(item).forEach(key => tableColumns.add(key)); - }); - - columnsList.push(Array.from(tableColumns)); - } - } - }); - } else if (typeof scrapeListData === 'object') { - Object.keys(scrapeListData).forEach(key => { - const tableData = scrapeListData[key]; - if (Array.isArray(tableData) && tableData.length > 0) { - const filteredData = tableData.filter(row => - Object.values(row).some(value => value !== undefined && value !== "") - ); - - if (filteredData.length > 0) { - tablesList.push(filteredData); - - const tableColumns = new Set<string>(); - filteredData.forEach(item => { - Object.keys(item).forEach(key => tableColumns.add(key)); - }); - - columnsList.push(Array.from(tableColumns)); - } - } - }); - } + // Process tables from either array or object format + const processTable = (tableData: any) => { + if (Array.isArray(tableData) && tableData.length > 0) { + const filteredData = tableData.filter(row => + Object.values(row).some(value => value !== undefined && value !== "") + ); + + if (filteredData.length > 0) { + tablesList.push(filteredData); + + const tableColumns = new Set<string>(); + filteredData.forEach(item => { + Object.keys(item).forEach(key => tableColumns.add(key)); + }); + + columnsList.push(Array.from(tableColumns)); + } + } + }; + + // Handle both array and object formats + if (Array.isArray(scrapeListData)) { + scrapeListData.forEach(processTable); + } else if (typeof scrapeListData === 'object') { + Object.values(scrapeListData).forEach(processTable); + } setListData(tablesList); setListColumns(columnsList); setCurrentListIndex(0); };
61-82: Consider structuring data processing in a single useEffect.The current implementation separates data processing into multiple useEffect hooks, which could lead to race conditions or unnecessary re-renders. Consider consolidating related data processing logic.
useEffect(() => { + // Reset all states to avoid stale data + setSchemaData([]); + setSchemaColumns([]); + setListData([]); + setListColumns([]); + setLegacyData([]); + setLegacyColumns([]); + setIsLegacyData(false); + if (!row.serializableOutput) return; if (!row.serializableOutput.scrapeSchema && !row.serializableOutput.scrapeList && Object.keys(row.serializableOutput).length > 0) { setIsLegacyData(true); processLegacyData(row.serializableOutput); return; } setIsLegacyData(false); if (row.serializableOutput.scrapeSchema && Object.keys(row.serializableOutput.scrapeSchema).length > 0) { processDataCategory(row.serializableOutput.scrapeSchema, setSchemaData, setSchemaColumns); } if (row.serializableOutput.scrapeList) { processScrapeList(row.serializableOutput.scrapeList); } + // Process screenshots in the same effect to ensure consistent state updates + if (row.binaryOutput && Object.keys(row.binaryOutput).length > 0) { + setScreenshotKeys(Object.keys(row.binaryOutput)); + setCurrentScreenshotIndex(0); + } else { + setScreenshotKeys([]); + setCurrentScreenshotIndex(0); + } -}, [row.serializableOutput]); +}, [row.serializableOutput, row.binaryOutput]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/run/RunContent.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/run/RunContent.tsx
[error] 497-592: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
|
|
||
| // Function to convert table data to CSV format | ||
| const convertToCSV = (data: any[], columns: string[]): string => { | ||
| const header = columns.join(','); | ||
| const rows = data.map(row => | ||
| columns.map(col => JSON.stringify(row[col], null, 2)).join(',') | ||
| columns.map(col => JSON.stringify(row[col] || "", null, 2)).join(',') |
There was a problem hiding this comment.
Fix CSV formatting issue.
Using JSON.stringify(row[col] || "", null, 2) for CSV values is inappropriate and will corrupt the CSV format with unnecessary whitespace and quotes. The pretty-printing parameters null, 2 are meant for JSON formatting, not CSV.
- columns.map(col => JSON.stringify(row[col] || "", null, 2)).join(',')
+ columns.map(col => {
+ const value = row[col] === undefined ? "" : row[col];
+ // Properly escape values for CSV format
+ return typeof value === 'string' ?
+ `"${value.replace(/"/g, '""')}"` :
+ String(value);
+ }).join(',')📝 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.
| columns.map(col => JSON.stringify(row[col] || "", null, 2)).join(',') | |
| columns.map(col => { | |
| const value = row[col] === undefined ? "" : row[col]; | |
| // Properly escape values for CSV format | |
| return typeof value === 'string' | |
| ? `"${value.replace(/"/g, '""')}"` | |
| : String(value); | |
| }).join(',') |
| useEffect(() => { | ||
| if (row.binaryOutput && Object.keys(row.binaryOutput).length > 0) { | ||
| setScreenshotKeys(Object.keys(row.binaryOutput)); | ||
| setCurrentScreenshotIndex(0); | ||
| } | ||
| }, [row.binaryOutput]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset screenshot state when no screenshots are available.
Similar to the legacy data issue, the component should reset screenshot state when a new run has no screenshots to avoid displaying stale data.
useEffect(() => {
if (row.binaryOutput && Object.keys(row.binaryOutput).length > 0) {
setScreenshotKeys(Object.keys(row.binaryOutput));
setCurrentScreenshotIndex(0);
+ } else {
+ // Reset state to avoid showing stale screenshots
+ setScreenshotKeys([]);
+ setCurrentScreenshotIndex(0);
}
}, [row.binaryOutput]);📝 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.
| useEffect(() => { | |
| if (row.binaryOutput && Object.keys(row.binaryOutput).length > 0) { | |
| setScreenshotKeys(Object.keys(row.binaryOutput)); | |
| setCurrentScreenshotIndex(0); | |
| } | |
| }, [row.binaryOutput]); | |
| useEffect(() => { | |
| if (row.binaryOutput && Object.keys(row.binaryOutput).length > 0) { | |
| setScreenshotKeys(Object.keys(row.binaryOutput)); | |
| setCurrentScreenshotIndex(0); | |
| } else { | |
| // Reset state to avoid showing stale screenshots | |
| setScreenshotKeys([]); | |
| setCurrentScreenshotIndex(0); | |
| } | |
| }, [row.binaryOutput]); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/components/run/InterpretationLog.tsx (4)
4-4: Remove unused importsTabsandTabThese components are imported but never used in the implementation. The tabs are instead built using custom
Boxcomponents.-import { Button, Grid, Tabs, Tab, Box } from '@mui/material'; +import { Button, Grid, Box } from '@mui/material';
33-37: Use proper typing for state variables instead ofany[]All four state variables could benefit from more specific type definitions to improve type safety and developer experience.
-const [captureListData, setCaptureListData] = useState<any[]>([]); -const [captureTextData, setCaptureTextData] = useState<any[]>([]); -const [screenshotData, setScreenshotData] = useState<string[]>([]); -const [otherData, setOtherData] = useState<any[]>([]); +type Row = Record<string, unknown>; + +const [captureListData, setCaptureListData] = useState<Row[][]>([]); +const [captureTextData, setCaptureTextData] = useState<Row[]>([]); +const [screenshotData, setScreenshotData] = useState<string[]>([]); +const [otherData, setOtherData] = useState<Row[][]>([]);
87-97: FixcaptureTextbranch overwriting previous dataUnlike
captureListand screenshot data, thecaptureTextbranch replaces existing state instead of appending, which means only the last capture will survive in a multi-capture session.-if (Array.isArray(data)) { - setCaptureTextData(data); -} else { - setCaptureTextData([data]); -} +setCaptureTextData(prev => + Array.isArray(data) ? [...prev, ...data] : [...prev, data] +);
330-351: Leverage optional chaining for array accessStatic analysis rightly flags potential undefined access with
captureListData[captureListPage].map. Optional chaining is cleaner and safer.-{captureListData[captureListPage] && -captureListData[captureListPage].map((row: any, idx: any) => ( +{captureListData[captureListPage]?.map((row: any, idx: any) => (🧰 Tools
🪛 Biome (1.9.4)
[error] 330-351: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (4)
src/components/run/InterpretationLog.tsx (4)
101-101: Optimize dependency array inhandleSerializableCallbackThe current dependency array causes the callback to be recreated whenever the length of the arrays changes. Consider using refs to access these values instead of including them in the dependency array.
-}, [captureListData.length, captureTextData.length, otherData.length, t]); +}, [t]);You could then use refs to check the length:
const captureListDataRef = useRef<typeof captureListData>([]); useEffect(() => { captureListDataRef.current = captureListData; }, [captureListData]); // Then in the callback if (captureListDataRef.current.length === 0) { // ... }
123-123: Optimize dependency array inhandleBinaryCallbackSimilar to the previous comment, the callback is recreated whenever
screenshotData.lengthchanges.-}, [screenshotData.length, t]); +}, [t]);And use a ref approach as suggested above.
189-189: Add null/undefined check before accessingcaptureTextData[0]The current implementation assumes that
captureTextData[0]exists whencaptureTextData.length > 0, which is generally true but could be improved with more explicit checks.-const getCaptureTextColumns = captureTextData.length > 0 ? Object.keys(captureTextData[0]) : []; +const getCaptureTextColumns = captureTextData.length > 0 && captureTextData[0] ? Object.keys(captureTextData[0]) : [];
313-326: Use optional chaining for table header construction tooSimilar to the previous comment, the table header construction could be simplified with optional chaining.
-{captureListData[captureListPage] && captureListData[captureListPage].length > 0 && - Object.keys(captureListData[captureListPage][0]).map((column) => ( +{captureListData[captureListPage]?.[0] && + Object.keys(captureListData[captureListPage][0]).map((column) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/run/InterpretationLog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/run/InterpretationLog.tsx
[error] 330-351: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/run/InterpretationLog.tsx (2)
341-362: Use optional chaining for safer array access.The conditional check before accessing
captureListData[captureListPage]can be replaced with optional chaining for cleaner code and to avoid potential runtime errors if the index is out of bounds.-{captureListData[captureListPage] && -captureListData[captureListPage].map((row: any, idx: any) => ( +{captureListData[captureListPage]?.map((row: any, idx: any) => (🧰 Tools
🪛 Biome (1.9.4)
[error] 341-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
86-96:⚠️ Potential issueFix
captureTextdata handling to append instead of overwrite previous data.Unlike
captureListand screenshots which append new data, thecaptureTextbranch replaces existing state. This means only the last capture will be retained if multiple text captures occur in one session.-if (Array.isArray(data)) { - setCaptureTextData(data); -} else { - setCaptureTextData([data]); -} +setCaptureTextData(prev => + Array.isArray(data) ? [...prev, ...data] : [...prev, data] +);
🧹 Nitpick comments (5)
src/components/run/InterpretationLog.tsx (5)
4-4: Remove unused imports.The
TabsandTabcomponents are imported but never used in this component. The tabs are implemented with customBoxcomponents instead.-import { Button, Grid, Tabs, Tab, Box } from '@mui/material'; +import { Button, Grid, Box } from '@mui/material';
33-35: Add proper typing to state variables.Using
any[]for state variables sacrifices type safety and IntelliSense benefits. Consider defining proper types for these data structures.-const [captureListData, setCaptureListData] = useState<any[]>([]); -const [captureTextData, setCaptureTextData] = useState<any[]>([]); +type CaptureRow = Record<string, unknown>; + +const [captureListData, setCaptureListData] = useState<CaptureRow[][]>([]); +const [captureTextData, setCaptureTextData] = useState<CaptureRow[]>([]);
324-337: Use optional chaining for table header rendering.Similar to the table body, the conditional check before accessing array data can be simplified with optional chaining.
-{captureListData[captureListPage] && captureListData[captureListPage].length > 0 && - Object.keys(captureListData[captureListPage][0]).map((column) => ( +{captureListData[captureListPage]?.[0] && + Object.keys(captureListData[captureListPage][0]).map((column) => (
261-285: Add accessibility attributes to custom tab implementation.The custom tabs implementation using Box components lacks proper accessibility attributes that would be automatically included with MUI's Tab components. Consider adding appropriate ARIA attributes.
<Box key={tab.id} onClick={() => setActiveTab(index)} + role="tab" + aria-selected={activeTab === index} + aria-controls={`tabpanel-${tab.id}`} + id={`tab-${tab.id}`} + tabIndex={activeTab === index ? 0 : -1} sx={{ px: 4, py: 2, cursor: 'pointer', borderBottom: activeTab === index ? '2px solid' : 'none', borderColor: activeTab === index ? (darkMode ? '#ff00c3' : '#ff00c3') : 'transparent', backgroundColor: activeTab === index ? (darkMode ? '#34404d' : '#e9ecef') : 'transparent', color: darkMode ? 'white' : 'black', fontWeight: activeTab === index ? 500 : 400, textAlign: 'center', position: 'relative', '&:hover': { backgroundColor: activeTab !== index ? (darkMode ? '#303b49' : '#e2e6ea') : undefined } }} > <Typography variant="body1"> {tab.label} </Typography> </Box>Also wrap the tab list in a container with proper role attributes:
<Box + role="tablist" + aria-orientation="horizontal" sx={{ display: 'flex', borderBottom: '1px solid', borderColor: darkMode ? '#3a4453' : '#dee2e6', backgroundColor: darkMode ? '#2a3441' : '#f8f9fa' }} >
289-461: Add ARIA attributes to tab content panels.For proper accessibility, each tab panel should have correct ARIA attributes that connect it to its tab.
Add the following attributes to each tab panel content box:
{(activeTab === availableTabs.findIndex(tab => tab.id === 'captureList') || singleContentType === 'captureList') && captureListData.length > 0 && ( - <Box> + <Box + role="tabpanel" + id={`tabpanel-captureList`} + aria-labelledby={`tab-captureList`} + hidden={activeTab !== availableTabs.findIndex(tab => tab.id === 'captureList') && singleContentType !== 'captureList'} + >Apply similar changes to the other tab panels (captureScreenshot and captureText).
🧰 Tools
🪛 Biome (1.9.4)
[error] 341-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/run/InterpretationLog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/run/InterpretationLog.tsx
[error] 341-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| if (type === 'captureList') { | ||
| setCaptureListData(prev => [...prev, data]); | ||
| if (captureListData.length === 0) { | ||
| const availableTabs = getAvailableTabs(); | ||
| const tabIndex = availableTabs.findIndex(tab => tab.id === 'captureList'); | ||
| if (tabIndex !== -1) setActiveTab(tabIndex); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition when setting the active tab.
The code checks captureListData.length === 0 but the state is being updated in the same function. React state updates are asynchronous, so this check may not reflect the actual updated state.
if (type === 'captureList') {
setCaptureListData(prev => [...prev, data]);
- if (captureListData.length === 0) {
+ if (captureListData.length === 0) {
const availableTabs = getAvailableTabs();
const tabIndex = availableTabs.findIndex(tab => tab.id === 'captureList');
if (tabIndex !== -1) setActiveTab(tabIndex);
}
}For a more reliable solution, consider:
if (type === 'captureList') {
setCaptureListData(prev => {
const isFirstData = prev.length === 0;
const newData = [...prev, data];
if (isFirstData) {
// Use setTimeout to ensure this runs after state updates
setTimeout(() => {
const availableTabs = getAvailableTabs();
const tabIndex = availableTabs.findIndex(tab => tab.id === 'captureList');
if (tabIndex !== -1) setActiveTab(tabIndex);
}, 0);
}
return newData;
});
}| scrollLogToBottom(); | ||
| }, [log, scrollLogToBottom, t]); | ||
|
|
||
| }, [screenshotData.length, t]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update dependency array in handleBinaryCallback.
Similar to the previous issue, handleBinaryCallback includes screenshotData.length in its dependency array, which could cause unnecessary recreations of the callback.
-}, [screenshotData.length, t]);
+}, [t]);📝 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.
| }, [screenshotData.length, t]); | |
| }, [t]); |
| }, [log, scrollLogToBottom, t]); | ||
|
|
||
| const handleBinaryCallback = useCallback(({ data, mimetype }: any) => { | ||
| }, [captureListData.length, captureTextData.length, t]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update dependency array in useCallback.
The handleSerializableCallback useCallback's dependency array includes captureListData.length and captureTextData.length which will cause the callback to be recreated whenever these data arrays change. Since this callback is handling socket events, frequent recreations could lead to multiple socket event bindings or missed events.
-}, [captureListData.length, captureTextData.length, t]);
+}, [t]);Instead of relying on the current array lengths in the callback, consider using functional state updates to reference the previous state.
📝 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.
| }, [captureListData.length, captureTextData.length, t]); | |
| }, [t]); |
What this PR does?
Brings in the functionality to train and execute multiple capture actions in a single robot run.
Closes: #549
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor