[WEB-2710] fix: issue widget modal rendering#5896
Conversation
WalkthroughThe changes introduce a new optional property, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/core/components/issues/issue-detail-widgets/root.tsx (2 hunks)
- web/core/components/issues/issue-detail/main-content.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
web/core/components/issues/issue-detail-widgets/root.tsx (1)
15-15: LGTM! Clean type definition.The optional boolean prop is well-defined and follows TypeScript best practices.
web/core/components/issues/issue-detail/main-content.tsx (2)
43-43: LGTM: Clean hook usageThe addition of
peekIssuefrom theuseIssueDetailhook is clean and follows React patterns.
116-116: LGTM: Effectively prevents double modal renderingThe
renderWidgetModalsprop correctly prevents widget modal rendering during peek mode, addressing the original issue.Let's verify that this is the only instance where widget modals are rendered:
✅ Verification successful
Let me try a different search strategy to verify the widget modal rendering implementation.
This will help us:
- Find all usages of IssueDetailWidgets component and its props
- Find any other widget modal related code that might be relevant
Verification complete: Widget modal rendering control is properly implemented
The search results confirm that the widget modal rendering is correctly controlled:
- The
renderWidgetModalsprop is defined inIssueDetailWidgetscomponent with a default value oftrue- The prop is properly used in the component to conditionally render
IssueDetailWidgetModalsusing{renderWidgetModals && (...)}- The only place where this prop is explicitly set to prevent rendering is in the issue detail main content with
!isPeekModeActive- No other instances of widget modal rendering were found that could cause double rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential widget modal rendering locations rg -l "IssueDetailWidgets.*renderWidgetModals"Length of output: 48
Script:
#!/bin/bash # Search for IssueDetailWidgets component usage rg "IssueDetailWidgets" -A 3 # Also search for any widget modal related code rg -i "widgetmodal|widget.*modal" -A 2Length of output: 4102
|
|
||
| export const IssueDetailWidgets: FC<Props> = (props) => { | ||
| const { workspaceSlug, projectId, issueId, disabled } = props; | ||
| const { workspaceSlug, projectId, issueId, disabled, renderWidgetModals = true } = props; |
There was a problem hiding this comment.
💡 Codebase verification
Component usage needs attention - renderWidgetModals prop is not being passed
The search results show that IssueDetailWidgets is being used in three locations:
web/core/components/issues/issue-detail/main-content.tsxweb/core/components/issues/peek-overview/view.tsx(two instances)
However, none of these usages are passing the renderWidgetModals prop, which could lead to unexpected behavior. While the prop has a default value of true, the peek overview component might benefit from explicitly setting this prop to false to prevent modal rendering in peek mode.
Locations needing attention:
web/core/components/issues/peek-overview/view.tsx: Both instances should consider settingrenderWidgetModals={false}web/core/components/issues/issue-detail/main-content.tsx: Should explicitly setrenderWidgetModals={true}for clarity
🔗 Analysis chain
LGTM! Clean implementation of conditional rendering.
The implementation effectively solves the dual modal issue while maintaining backward compatibility through the default true value. The conditional rendering is clean and straightforward.
Let's verify that this component is properly used with the new prop in the parent components:
Also applies to: 36-38
Let me try a different approach to verify the usage of this component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of IssueDetailWidgets with renderWidgetModals prop
# Expected: Should find usage in IssueMainContent where renderWidgetModals is set based on peek mode
# Search for IssueDetailWidgets usage
ast-grep --pattern 'IssueDetailWidgets $$$renderWidgetModals={$_}$$$'
Length of output: 71
Script:
#!/bin/bash
# Let's first find all files that import or use IssueDetailWidgets
rg "IssueDetailWidgets" -l
# Then let's see the actual usage context
rg "IssueDetailWidgets" -A 3 -B 3
Length of output: 3720
| const isPeekModeActive = Boolean(peekIssue); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the peek mode state
While the current implementation is correct, consider memoizing isPeekModeActive if peekIssue changes frequently to prevent unnecessary re-renders.
- const isPeekModeActive = Boolean(peekIssue);
+ const isPeekModeActive = useMemo(() => Boolean(peekIssue), [peekIssue]);Committable suggestion was skipped due to low confidence.
3 participants
Changes:
This PR resolves a bug related to the widget modal rendering. The issue occurred when a user opened a sub-issue in peek overview mode from the detail page and then tried to open a widget modal. This action triggered the widget modal for both the peek overview and the issue detail page, causing two modals to open simultaneously.
To fix this, I have added a render condition for the issue detail widget modal. Now, when peek overview mode is active, the issue detail widget modal will not render, preventing two modals from opening at the same time.
Reference:
[WEB-2710]
Summary by CodeRabbit
New Features
renderWidgetModalsfor conditional rendering of modals in theIssueDetailWidgetscomponent.IssueMainContentcomponent, allowing for dynamic display of widget modals based on the active state.Bug Fixes
IssueMainContentcomponent.