Stop propagation on the last cell of the TargetReleaseTable #168
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
zacharyblasczyk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1)
Line range hint
129-145: Consider accessibility improvements for the dropdown cell.While the implementation works, consider these accessibility enhancements:
- <TableCell onClick={(e) => e.stopPropagation()}> - <div className="flex justify-end"> + <TableCell> + <div + className="flex justify-end" + onClick={(e) => e.stopPropagation()} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.stopPropagation(); + } + }} + role="button" + tabIndex={0} + >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1 hunks)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1)
129-129: LGTM! Event propagation is correctly handled.The stopPropagation implementation effectively prevents the row's click handler from being triggered when interacting with the dropdown menu.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (1)
Line range hint
246-246: Remove unused importsetJobId.The
setJobIddestructured fromuseJobDraweris no longer used in the component.Apply this diff to remove the unused import:
- const { setJobId } = useJobDrawer();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (1 hunks)
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (2)
270-270: LGTM! Dialog auto-closes after redeploy.The addition of
setIsOpen(false)improves user experience by automatically closing the dialog after a successful redeploy operation.
Line range hint
246-273: Verify integration with TargetReleaseTable click handling.The changes here are part of a larger change set involving click propagation in TargetReleaseTable. Please ensure that:
- The dialog opens correctly when clicking the menu item
- The click doesn't propagate to the parent row
- The dialog properly closes without triggering row selection
✅ Verification successful
Based on the search results, I can now provide a final response since I have found clear evidence of click propagation handling in the codebase:
Click propagation is properly handled in the component.
The code shows that:
- The last cell in TargetReleaseTable has explicit stopPropagation:
<TableCell onClick={(e) => e.stopPropagation()}>- This cell contains the JobDropdownMenu component, ensuring clicks on the menu don't trigger the parent row's click handler
- The parent row has its own click handler to set the job ID:
onClick={() => setJobId(job.job.id)}This implementation correctly prevents click propagation from the dropdown menu to the row, while maintaining both functionalities:
- Row clicks will open the job details
- Menu clicks will be handled independently without triggering the row action
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TargetReleaseTable implementation to verify click handling rg -l "TargetReleaseTable.*onClick.*stopPropagation"Length of output: 54
Script:
#!/bin/bash # Let's search for TargetReleaseTable component implementation rg -l "TargetReleaseTable"Length of output: 280
Script:
#!/bin/bash # Search for any click handlers in the component rg "onClick" --type ts --type tsx -C 3Length of output: 66
Script:
#!/bin/bash # Search for stopPropagation usage in the codebase rg "stopPropagation" --type ts --type tsx -C 3Length of output: 74
Script:
#!/bin/bash # Let's examine the TargetReleaseTable implementation cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx"Length of output: 6327
Script:
#!/bin/bash # Search for click handlers without file type flags rg "onClick" -C 3Length of output: 164157
Script:
#!/bin/bash # Search for stopPropagation without file type flags rg "stopPropagation" -C 3Length of output: 11389
Summary by CodeRabbit
New Features
TargetReleaseTablecomponent to allow users to interact with the dropdown menu without triggering the row's click event, improving usability.RedeployReleaseDialogto automatically close upon completion of the redeploy action, enhancing user experience.Bug Fixes
ForceReleaseTargetDialogandRedeployReleaseDialogfor consistent management across dialogs.