Skip to content

[WEB-2710] fix: issue widget modal rendering #5896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pushya22 merged 1 commit into from
Oct 23, 2024
Merged

[WEB-2710] fix: issue widget modal rendering #5896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions web/core/components/issues/issue-detail-widgets/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ type Props = {
projectId: string;
issueId: string;
disabled: boolean;
renderWidgetModals?: boolean;
};

export const IssueDetailWidgets: FC<Props> = (props) => {
const { workspaceSlug, projectId, issueId, disabled } = props;
const { workspaceSlug, projectId, issueId, disabled, renderWidgetModals = true } = props;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.tsx
  • web/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 setting renderWidgetModals={false}
  • web/core/components/issues/issue-detail/main-content.tsx: Should explicitly set renderWidgetModals={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

return (
<>
<div className="flex flex-col gap-5">
Expand All @@ -32,7 +33,9 @@ export const IssueDetailWidgets: FC<Props> = (props) => {
disabled={disabled}
/>
</div>
<IssueDetailWidgetModals workspaceSlug={workspaceSlug} projectId={projectId} issueId={issueId} />
{renderWidgetModals && (
<IssueDetailWidgetModals workspaceSlug={workspaceSlug} projectId={projectId} issueId={issueId} />
)}
</>
);
};
4 changes: 4 additions & 0 deletions web/core/components/issues/issue-detail/main-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export const IssueMainContent: React.FC<Props> = observer((props) => {
const { data: currentUser } = useUser();
const {
issue: { getIssueById },
peekIssue,
} = useIssueDetail();
const { setShowAlert } = useReloadConfirmations(isSubmitting === "submitting");

Expand All @@ -53,6 +54,8 @@ export const IssueMainContent: React.FC<Props> = observer((props) => {
const issue = issueId ? getIssueById(issueId) : undefined;
if (!issue || !issue.project_id) return <></>;

const isPeekModeActive = Boolean(peekIssue);

Comment on lines +57 to +58
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

return (
<>
<div className="rounded-lg space-y-4">
Expand Down Expand Up @@ -110,6 +113,7 @@ export const IssueMainContent: React.FC<Props> = observer((props) => {
projectId={projectId}
issueId={issueId}
disabled={!isEditable || isArchived}
renderWidgetModals={!isPeekModeActive}
/>

{windowSize[0] < 768 && (
Expand Down