hotfix: action overlay not closing bug #144
Conversation
|
WalkthroughThe changes in this pull request primarily involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/__shared__/ActionsOverlay/styled.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 1e17f5939f56398619bbce4089359cd3133cf994 and b9b7e52. 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (4)packages/components/modules/comments/Comments/__tests__/Comments.cy.tsx (3)
The test case effectively verifies that regular comments cannot be deleted by checking for the absence of the delete button. Line range hint The test effectively verifies that:
Line range hint The test thoroughly verifies the entire deletion flow:
packages/components/modules/comments/Comments/__tests__/__mocks__/requests.ts (1)
The 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
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: 0
🧹 Outside diff range and nitpick comments (3)
packages/components/modules/__shared__/ActionsOverlay/types.ts (1)
32-35: Document the offset props usageThe new
ActionOverlayTooltipContainerPropsinterface introduces positioning control throughoffsetTopandoffsetRight. Consider adding JSDoc comments to explain:
- The purpose of these offset properties
- Their expected values and units
- How they affect tooltip positioning
+/** + * Props for controlling the positioning of the actions overlay tooltip container + * @property offsetTop - Distance from the top edge (in pixels) + * @property offsetRight - Distance from the right edge (in pixels) + */ export interface ActionOverlayTooltipContainerProps extends BoxProps { offsetTop?: number offsetRight?: number }packages/components/CHANGELOG.md (1)
3-8: Enhance changelog entry clarityWhile the changelog entry correctly documents the fix, consider making it more specific to better align with the PR objectives and provide clearer context:
## 0.0.22 ### Patch Changes -Fixed overlay not closing issue by forcing hover state to 'false' when exiting the delete dialog +Fixed ActionsOverlay not closing after delete dialog is dismissed by resetting hover stateThis revision:
- Specifically identifies the
ActionsOverlaycomponent- Clarifies the sequence of events (dialog dismissal → overlay closure)
- Maintains technical detail about the hover state reset
packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
Line range hint
174-209: Consider simplifying the conditional rendering logic.While the rename implementation is correct, the nested conditions (
deviceHasHover && isHoveringItem) and the empty div fallback could be simplified.Consider this more concise approach:
- if (deviceHasHover && isHoveringItem) { - return ( - <ActionOverlayTooltipContainer - // ... props - > - {/* ... content */} - </ActionOverlayTooltipContainer> - ) - } - return <div /> + return deviceHasHover && isHoveringItem ? ( + <ActionOverlayTooltipContainer + // ... props + > + {/* ... content */} + </ActionOverlayTooltipContainer> + ) : null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(6 hunks)packages/components/modules/__shared__/ActionsOverlay/styled.tsx(2 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(2 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🔇 Additional comments (11)
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx (2)
26-26: LGTM! Improved container width handling.
The change from fixed width to max-content allows the overlay to adapt to its content size, which is a better approach for handling varying content lengths.
Line range hint 8-11: Verify component rename impact.
The component in the meta configuration has been changed from ActionsOverlay to ActionsOverlayOnButton. This structural change might affect other stories or tests.
packages/components/modules/__shared__/ActionsOverlay/types.ts (1)
Line range hint 21-30: Verify tooltip behavior with delete dialog interaction
The interface now extends ActionOverlayTooltipContainerProps instead of the previous container props. Since this PR fixes an overlay closing issue, ensure that the tooltip positioning and visibility states are properly handled when the delete dialog is opened and closed.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Tooltip and delete dialog interaction is properly handled
The code shows that:
- The
closeOnClickproperty is correctly implemented in both tooltip and drawer views, properly closing the overlay when actions are clicked - Delete handling is properly integrated with overlay state management:
handleDeleteItemis called and immediately followed byhandleLongPressItemOptionsClose()setIsHoveringItem(false)is set after delete operation
The interface extension with ActionOverlayTooltipContainerProps is justified as the component properly utilizes the tooltip positioning props (offsetTop, offsetRight) while maintaining correct overlay closing behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tooltip-related state management in the component
ast-grep --pattern 'const [$_] = useState<boolean>($$$)' packages/components/modules/__shared__/ActionsOverlay/*.tsx
# Look for delete dialog and tooltip interaction handling
rg -A 5 'handleDeleteItem|closeOnClick' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 8181
packages/components/modules/__shared__/ActionsOverlay/styled.tsx (3)
4-4: LGTM: Import statement correctly updated
The import statement has been properly updated to reflect the new type name.
6-6: Verify all usages of the renamed component
The component has been renamed from ActionOverlayContainer to ActionOverlayTooltipContainer. Let's verify all imports and usages have been updated accordingly.
✅ Verification successful
Let me check the actual content of these files to verify the usage and ensure the rename is consistent.
Component rename has been properly implemented
The verification shows that the rename from ActionOverlayContainer to ActionOverlayTooltipContainer has been consistently implemented across all relevant files:
- The component is properly exported in
styled.tsx - It's correctly imported and used in
index.tsx - The types are properly defined in
types.ts - No references to the old name
ActionOverlayContainerremain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old component name
# Search for old component name references
echo "Checking for old ActionOverlayContainer references:"
rg "ActionOverlayContainer" -l
# Search for new component name references to verify usage
echo -e "\nVerifying new ActionOverlayTooltipContainer references:"
rg "ActionOverlayTooltipContainer" -l
Length of output: 500
Script:
#!/bin/bash
# Check the content of files for context around the component usage
echo "Content of styled.tsx:"
rg -A 3 "ActionOverlayTooltipContainer" packages/components/modules/__shared__/ActionsOverlay/styled.tsx
echo -e "\nContent of index.tsx:"
rg -A 3 "ActionOverlayTooltipContainer" packages/components/modules/__shared__/ActionsOverlay/index.tsx
echo -e "\nContent of types.ts:"
rg -A 3 "ActionOverlayTooltipContainer" packages/components/modules/__shared__/ActionsOverlay/types.ts
# Also check for any potential old references in git history of these files
echo -e "\nChecking recent git history for these files:"
git log -n 1 --oneline packages/components/modules/__shared__/ActionsOverlay/
Length of output: 1631
19-19: Verify z-index change impact on overlay visibility
The z-index has been intentionally lowered from 1500 (tooltip) to 1200 (drawer) to ensure proper stacking order with modals (1300). While this change makes sense for the delete dialog scenario, we should verify that the overlay remains visible in all other use cases.
Consider documenting this z-index relationship in a more central location (like theme configuration) to prevent future regressions.
✅ Verification successful
Based on the search results, I can now provide a final response:
Z-index change for ActionsOverlay is appropriate
The z-index modification from tooltip (1500) to drawer (1200) is verified to be safe because:
- The component documentation shows it's used in two modes: tooltip on web (hover) and drawer on mobile (long press)
- It's primarily used in CommentItem and ChatRoomItem components where it needs to stay below modals
- The component's behavior is well-documented and tested through stories
- No other components are found to depend on the overlay's z-index value
The change aligns with the component's design goals:
- On web: Shows actions in a tooltip on hover
- On mobile: Shows actions in a swipeable drawer on long press
- Must work correctly with modal dialogs (z-index: 1300)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other components that might be affected by this z-index change
# Search for other components using z-index in the same range
echo "Checking for components with similar z-index values:"
rg "zIndex\.(tooltip|modal|drawer)" -B2 -A2
# Search for potential overlay/modal interactions in stories or tests
echo -e "\nChecking for overlay/modal interaction tests:"
fd -e stories.tsx -e test.tsx -e spec.tsx | xargs rg "ActionOverlayTooltip.*Modal" -l
Length of output: 977
Script:
#!/bin/bash
# Let's search for any usage of ActionsOverlay component to understand its interactions
echo "Searching for ActionsOverlay usage:"
rg "ActionsOverlay" -B2 -A2
# Let's also check if there are any stories or tests for ActionsOverlay
echo -e "\nChecking for ActionsOverlay stories and tests:"
fd "ActionsOverlay" -e stories.tsx -e test.tsx -e spec.tsx
# Let's check for any z-index related issues or PRs
echo -e "\nChecking for z-index related issues in commit messages:"
git log --grep="z-index\|zIndex" --pretty=format:"%h %s" -n 5
Length of output: 14702
packages/components/modules/__shared__/ActionsOverlay/index.tsx (5)
14-14: LGTM! Improved naming clarity.
The rename from ActionOverlayContainer to ActionOverlayTooltipContainer better reflects the component's purpose.
82-82: LGTM! Comprehensive state cleanup.
Good addition of hover state reset when delete completes, ensuring consistent behavior across all exit paths.
99-99: LGTM! Improved code maintainability.
Good refactor to use the extracted method instead of inline function, ensuring consistent behavior and reducing code duplication.
70-74: LGTM! Core bug fix implementation.
The new handleDeleteDialogClose method properly handles both dialog state and hover state, fixing the reported issue where the overlay wasn't closing after dialog dismissal.
✅ Verification successful
State management in handleDeleteDialogClose is correctly implemented
The search results confirm that handleDeleteDialogClose is implementing the state management correctly:
setIsDeleteDialogOpenis used consistently for dialog state managementsetIsHoveringItemis properly managed in mouse events (enter/leave) and cleanup scenarios- The combined reset of both states in
handleDeleteDialogCloseis unique and appropriate for the dialog close use case - No other locations in the codebase manage these states together in a way that could conflict
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify this is the only place managing these states together
rg -A 2 "setIsDeleteDialogOpen.*setIsHoveringItem"
Length of output: 52
Script:
#!/bin/bash
# Let's search for each state setter separately and their usages
echo "=== Searching for setIsDeleteDialogOpen usage ==="
rg "setIsDeleteDialogOpen" -A 2
echo -e "\n=== Searching for setIsHoveringItem usage ==="
rg "setIsHoveringItem" -A 2
Length of output: 2747
220-220: Verify the positioning change impact.
The addition of position="relative" creates a new positioning context. Please ensure this doesn't affect the overlay positioning in different scenarios (e.g., nested containers, scrolling containers).
✅ Verification successful
Position style change is safe and necessary
The position="relative" addition is correct and necessary for the ActionsOverlay component as it:
- Creates a positioning context for the absolute-positioned action tooltips shown on hover (via
ActionOverlayTooltipContainer) - Is used consistently across both ChatRoomItem and CommentItem implementations
- Doesn't affect the component's external layout as it's contained within the component's own Box wrapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other positioning-related styles that might be affected
rg -A 2 "position: (relative|absolute|fixed)" --type css
Length of output: 58
Script:
#!/bin/bash
# Search for position-related props and styles in TypeScript/JavaScript files
rg -A 2 'position=["'\'']?(relative|absolute|fixed)' --type ts --type tsx --type jsx
# Search for styled-components or CSS-in-JS position styles
rg -A 2 'position:\s*(relative|absolute|fixed)' --type ts --type tsx --type jsx
# Look for ActionsOverlay component usage
ast-grep --pattern 'ActionsOverlay'
Length of output: 1914
Script:
#!/bin/bash
# Let's check the content of ActionsOverlay component and its stories to understand the positioning context
cat packages/components/modules/__shared__/ActionsOverlay/index.tsx
cat packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx
# Check the usage in ChatRoomItem and CommentItem to understand the context
cat packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
cat packages/components/modules/comments/CommentItem/index.tsx
Length of output: 19074
| right: 12 - offsetRight, | ||
| top: -12 - offsetTop, | ||
| zIndex: theme.zIndex.tooltip, | ||
| zIndex: theme.zIndex.drawer, // zIndex.modal is 1300, so using zIndex.drawer (1200) insead of zIndex.tooltip (1500) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/modules/comments/CommentsList/index.tsx (1)
42-42: Consider makingenableDeleteconfigurableThe
enableDeleteprop is hardcoded totrue, which means delete functionality is always enabled for all comments. Consider making this configurable by passing it through CommentsList props to support different use cases where delete functionality might need to be disabled.- enableDelete + enableDelete={CommentItemProps?.enableDelete ?? false}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 08906dc and ce31f5faaa09a952bcdea347e97eb8cd4a055205.
📒 Files selected for processing (3)
packages/components/modules/__shared__/ActionsOverlay/styled.tsx(2 hunks)packages/components/modules/comments/CommentItem/index.tsx(1 hunks)packages/components/modules/comments/CommentsList/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/modules/shared/ActionsOverlay/styled.tsx
🔇 Additional comments (2)
packages/components/modules/comments/CommentItem/index.tsx (2)
35-35: LGTM: Secure default for delete functionality
Setting enableDelete to false by default is a good security practice, ensuring delete functionality is explicitly opted into rather than being enabled by default.
Line range hint 116-122: Verify overlay closing after delete completion
The overlay might not be closing because the delete completion isn't being handled. Consider updating the handleDeleteComment function to explicitly close the overlay after the delete mutation completes.
const handleDeleteComment = () => {
- deleteComment({ variables: { id: comment.id } })
+ deleteComment({
+ variables: { id: comment.id },
+ onCompleted: () => {
+ // Close the overlay
+ resetCommentReply()
+ },
+ onError: (error) => {
+ console.error('Failed to delete comment:', error)
+ }
+ })
- resetCommentReply()
}Let's verify if the ActionsOverlay component has proper cleanup methods:
pt-tsl
force-pushed
the
BA-hotfix-overlay-not-closing-bug
branch
from
ce31f5f to
1e17f59
Compare
pt-tsl
force-pushed
the
BA-hotfix-overlay-not-closing-bug
branch
from
1e17f59 to
b9b7e52
Compare
|
Summary by CodeRabbit
Release Notes
New Features
enableDeleteproperty in the CommentsList component.Bug Fixes
Documentation
Chores