feat: allow training multiple capture actions in one recording session #562
Conversation
RohitR311
added
Type: Feature
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe The The The The API and scheduler components handling run execution results have been updated to store and process this categorized serializable output structure consistently. The The The The Changes
Assessment against linked issues
Suggested reviewers
Poem
🪧 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 | |
| }} |
| const hasUnconfirmedListTextFields = browserSteps.some(step => | ||
| step.type === 'list' && Object.values(step.fields).some(field => | ||
| !confirmedListTextFields[step.id]?.[field.id] | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Incorrect key used when checking unconfirmed list fields
hasUnconfirmedListTextFields indexes confirmedListTextFields with field.id, but the confirmation map is keyed by fieldKey (the object key in step.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]
+ )
+);This restores the intended behaviour.
📝 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 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] | |
| ) | |
| ); |
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.
| @@ -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.
| 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.
| setActionType: Function, | ||
| }> | ||
| } |
There was a problem hiding this comment.
Replace banned Function type with an explicit signature
The static-analysis warning is valid – Function defeats type-safety and autocompletion.
- debugChannel: Partial<{
- activeId: Function,
- debugMessage: Function,
- setActionType: Function,
- }>
+ debugChannel: Partial<{
+ activeId: (id: number) => void,
+ debugMessage: (msg: string) => void,
+ setActionType: (type: string) => void,
+ }>This change propagates helpful typings to every call-site and removes the biome error.
📝 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.
| setActionType: Function, | |
| }> | |
| } | |
| export interface InterpretOptions<Context> { | |
| debugChannel: Partial<{ | |
| activeId: (id: number) => void, | |
| debugMessage: (msg: string) => void, | |
| setActionType: (type: 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)
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
Refactor
Bug Fixes
Style