fix: stop click propagation on attribute modal option selection#1027
fix: stop click propagation on attribute modal option selection#1027Smeet23 wants to merge 1 commit intogetmaxun:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughFixed a click event propagation issue in the BrowserWindow component where the attribute selection popup would not close properly. Added event parameter capture and Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/run/RunsTable.tsx (1)
465-481:⚠️ Potential issue | 🟡 MinorMissing dependencies in
useCallbackforrenderTableRows.The
renderTableRowscallback usesrerunHandler,handleRowExpand,expandedRows, andgetPaginationStatebut only some of these are in the dependency array. This could cause stale closure issues where the callback uses outdated versions of these functions/values.🐛 Proposed fix for dependency array
- }, [paginationStates, runId, runningRecordingName, currentInterpretationLog, abortRunHandler, handleDelete, accordionSortConfigs]); + }, [paginationStates, runId, runningRecordingName, currentInterpretationLog, abortRunHandler, rerunHandler, handleDelete, accordionSortConfigs, expandedRows, handleRowExpand, getPaginationState, urlRunId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/run/RunsTable.tsx` around lines 465 - 481, The renderTableRows useCallback is missing dependencies causing stale closures; update the dependency array for renderTableRows to include rerunHandler, handleRowExpand, expandedRows, and getPaginationState (in addition to the existing paginationStates, runId, runningRecordingName, currentInterpretationLog, abortRunHandler, handleDelete, accordionSortConfigs) so the callback always sees current handlers and state when mapping sortedData and slicing via getPaginationState.
🧹 Nitpick comments (3)
public/locales/ja.json (1)
1-653: Consider separating formatting changes from functional fixes.This file shows formatting/whitespace changes across all 653 lines, but according to the AI summary, no translation keys or values were modified. While consistent formatting is valuable, bundling mass reformatting with an unrelated bug fix (click propagation in BrowserWindow.tsx) can make PR reviews more difficult and complicate git history.
For better maintainability, consider:
- Applying formatting changes in a dedicated PR separate from functional bug fixes
- Ensuring all locale files follow the same formatting standard consistently
- Using automated tooling (e.g., Prettier) with a committed configuration to prevent future formatting drift
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/locales/ja.json` around lines 1 - 653, The PR mixes large formatting-only changes in public/locales/ja.json with an unrelated functional fix (click propagation in BrowserWindow.tsx); revert or remove the wholesale whitespace/format-only edits from this PR so only the functional change in BrowserWindow.tsx remains, and move the ja.json reformatting to a separate commit/PR (or re-run Prettier with a committed config across all locale files) to ensure consistent formatting without polluting the bug-fix PR.src/pages/MainPage.tsx (1)
229-265: Consider addingconnect_erroranddisconnectsocket handlers for consistency.The existing
handleRunRecordingfunction (lines 173-201) includes handlers forconnect_erroranddisconnectevents on the socket. The newhandleReRunRecordingonly listens torun-completed. For consistency and better error handling/debugging, consider adding these handlers.♻️ Proposed addition of error handlers
socket.on('run-completed', (data) => { setRerenderRuns(true); invalidateRuns(); if (data.status === 'success') { notify('success', t('main_page.notifications.interpretation_success', { name: robotName })); } else { notify('error', t('main_page.notifications.interpretation_failed', { name: robotName })); } }); + + socket.on('connect_error', (error) => { + console.log('error', `Failed to connect to browser ${browserId}: ${error}`); + }); + + socket.on('disconnect', (reason) => { + console.log('warn', `Disconnected from browser ${browserId}: ${reason}`); + }); notify('info', t('main_page.notifications.run_started', { name: robotName }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/MainPage.tsx` around lines 229 - 265, In handleReRunRecording, add the same socket lifecycle handlers used in handleRunRecording: attach socket.on('connect_error', ...) to notify/log connection errors and socket.on('disconnect', ...) to notify/log disconnections (use the existing notify and console.error patterns), and ensure any necessary socket cleanup is consistent with how sockets are managed via setSockets; locate the socket creation in createAndRunRecording's then-block (the io(...) call and setSockets update) and add the two handlers there mirroring handleRunRecording.src/components/run/ColapsibleRow.tsx (1)
214-227: Consider adding a tooltip for the rerun button for better UX.The delete and settings buttons in this component have clear labels via their modals, but the rerun button only has an
aria-label. A tooltip would improve discoverability and user experience, especially since the column header uses a translated label that users may want to confirm matches the action.♻️ Proposed addition of tooltip
+import { Tooltip } from "@mui/material"; // ... in the render: case 'rerun': return ( <TableCell key={column.id} align={column.align}> {['success', 'failed', 'aborted'].includes(row.status) && ( + <Tooltip title={t('runs_table.rerun')}> <IconButton aria-label="rerun" size="small" onClick={() => rerunHandler(row.robotMetaId, row.name, row.interpreterSettings)} > <Replay /> </IconButton> + </Tooltip> )} </TableCell> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/run/ColapsibleRow.tsx` around lines 214 - 227, Wrap the rerun IconButton in ColapsibleRow.tsx (the 'rerun' case where IconButton calls rerunHandler and renders <Replay />) with a Tooltip that shows a clear label (preferably the same translated string used for the column header, e.g., via the t(...) translation function), ensuring the Tooltip's title matches accessibility needs while keeping the existing aria-label on the IconButton; this will improve discoverability without changing rerunHandler or button behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/MainPage.tsx`:
- Around line 236-238: The notification for the queued case uses the wrong
translation key; instead of calling notify('info',
t('main_page.notifications.run_started', { name: robotName })) when queued is
true, change it to indicate the run is queued (use the existing queued message
pattern from handleRunRecording or a translation like
'main_page.notifications.run_queued') so the message reads "Run queued: <name>"
or uses t('main_page.notifications.run_queued', { name: robotName }); update the
notify call in the if (queued) branch and keep setQueuedRuns as-is.
---
Outside diff comments:
In `@src/components/run/RunsTable.tsx`:
- Around line 465-481: The renderTableRows useCallback is missing dependencies
causing stale closures; update the dependency array for renderTableRows to
include rerunHandler, handleRowExpand, expandedRows, and getPaginationState (in
addition to the existing paginationStates, runId, runningRecordingName,
currentInterpretationLog, abortRunHandler, handleDelete, accordionSortConfigs)
so the callback always sees current handlers and state when mapping sortedData
and slicing via getPaginationState.
---
Nitpick comments:
In `@public/locales/ja.json`:
- Around line 1-653: The PR mixes large formatting-only changes in
public/locales/ja.json with an unrelated functional fix (click propagation in
BrowserWindow.tsx); revert or remove the wholesale whitespace/format-only edits
from this PR so only the functional change in BrowserWindow.tsx remains, and
move the ja.json reformatting to a separate commit/PR (or re-run Prettier with a
committed config across all locale files) to ensure consistent formatting
without polluting the bug-fix PR.
In `@src/components/run/ColapsibleRow.tsx`:
- Around line 214-227: Wrap the rerun IconButton in ColapsibleRow.tsx (the
'rerun' case where IconButton calls rerunHandler and renders <Replay />) with a
Tooltip that shows a clear label (preferably the same translated string used for
the column header, e.g., via the t(...) translation function), ensuring the
Tooltip's title matches accessibility needs while keeping the existing
aria-label on the IconButton; this will improve discoverability without changing
rerunHandler or button behavior.
In `@src/pages/MainPage.tsx`:
- Around line 229-265: In handleReRunRecording, add the same socket lifecycle
handlers used in handleRunRecording: attach socket.on('connect_error', ...) to
notify/log connection errors and socket.on('disconnect', ...) to notify/log
disconnections (use the existing notify and console.error patterns), and ensure
any necessary socket cleanup is consistent with how sockets are managed via
setSockets; locate the socket creation in createAndRunRecording's then-block
(the io(...) call and setSockets update) and add the two handlers there
mirroring handleRunRecording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46691927-d410-4b70-babd-a2caeba35f4f
📒 Files selected for processing (11)
public/locales/de.jsonpublic/locales/en.jsonpublic/locales/es.jsonpublic/locales/ja.jsonpublic/locales/tr.jsonpublic/locales/zh.jsonsrc/components/browser/BrowserWindow.tsxsrc/components/run/ColapsibleRow.tsxsrc/components/run/Runs.tsxsrc/components/run/RunsTable.tsxsrc/pages/MainPage.tsx
Clicking an option in the Select Attribute modal was bubbling up to the parent BrowserWindow div's handleClick handler, which could re-trigger element selection logic and require a second click to actually close the modal. Adding e.stopPropagation() to the button's onClick prevents the event from reaching the parent handler. Fixes getmaxun#623
Smeet23
force-pushed
the
fix/attribute-modal-close
branch
from
0790259 to
0756570
Compare
|
Hi team — just a gentle ping on this PR. All checks are passing. Would appreciate a review when you get a chance. Happy to address any feedback. Thanks! |
2 participants
Summary
The Select Attribute popup sometimes required two clicks to close after selecting a text/link option.
Root cause: The option
<Button>click event was bubbling up to the parent<div id="browser-window">which has anonClick={handleClick}handler. This re-triggered element selection logic after the modal had already started closing, causing inconsistent behavior.Fix: Add
e.stopPropagation()to the button'sonClickin the attribute modal — one line change inBrowserWindow.tsx.Fixes #623
Test plan
Summary by CodeRabbit