[WEB-3357 | WEB-3363 | WEB-3370] chore: command-k enhancement and fixes #6600
Conversation
…vale for workspace toggle updated
WalkthroughThis pull request refactors several components within the command palette. The primary change is replacing the usage of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CM as CommandModal
participant SE as State Effect
participant UI as UI Elements
U->>CM: Open command palette
CM->>SE: Check if issueDetails exist
SE-->>CM: Set searchInIssue to true
U->>UI: View issue details with close (X) button
U->>CM: Click close (X) button
CM->>UI: Set searchInIssue to false
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (7)
web/core/components/command-palette/actions/issue-actions/change-priority.tsx (1)
26-33: Enhance error handling in submitChanges.The current error handling only logs to console. Consider adding user feedback for failed operations.
const submitChanges = async (formData: Partial<TIssue>) => { if (!workspaceSlug || !projectId || !issue) return; const payload = { ...formData }; - await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload).catch((e) => { - console.error(e); - }); + try { + await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload); + } catch (error) { + console.error(error); + setToast({ type: TOAST_TYPE.ERROR, title: "Failed to update priority" }); + } };web/core/components/command-palette/actions/issue-actions/change-state.tsx (1)
25-32: Enhance error handling in submitChanges.The current error handling only logs to console. Consider adding user feedback for failed operations.
const submitChanges = async (formData: Partial<TIssue>) => { if (!workspaceSlug || !projectId || !issue) return; const payload = { ...formData }; - await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload).catch((e) => { - console.error(e); - }); + try { + await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload); + } catch (error) { + console.error(error); + setToast({ type: TOAST_TYPE.ERROR, title: "Failed to update state" }); + } };web/core/components/command-palette/actions/issue-actions/change-assignee.tsx (1)
58-65: Enhance error handling in handleUpdateIssue.The current error handling only logs to console. Consider adding user feedback for failed operations.
const handleUpdateIssue = async (formData: Partial<TIssue>) => { if (!workspaceSlug || !projectId || !issue) return; const payload = { ...formData }; - await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload).catch((e) => { - console.error(e); - }); + try { + await updateIssue(workspaceSlug.toString(), projectId.toString(), issue.id, payload); + } catch (error) { + console.error(error); + setToast({ type: TOAST_TYPE.ERROR, title: "Failed to update assignees" }); + } };web/core/components/command-palette/actions/issue-actions/actions-list.tsx (1)
33-40: Enhance error handling in handleUpdateIssue.The current error handling only logs to console. Consider adding user feedback for failed operations.
const handleUpdateIssue = async (formData: Partial<TIssue>) => { if (!workspaceSlug || !projectId || !issueDetails) return; const payload = { ...formData }; - await updateIssue(workspaceSlug.toString(), projectId.toString(), issueDetails.id, payload).catch((e) => { - console.error(e); - }); + try { + await updateIssue(workspaceSlug.toString(), projectId.toString(), issueDetails.id, payload); + } catch (error) { + console.error(error); + setToast({ type: TOAST_TYPE.ERROR, title: "Failed to update issue" }); + } };web/core/components/command-palette/command-modal.tsx (3)
59-64: Consider using TypeScript type for the results state.The results state initialization could benefit from explicit TypeScript typing to improve type safety and code maintainability.
- const [results, setResults] = useState<IWorkspaceSearchResults>({ - results: { workspace: [], project: [], issue: [], cycle: [], module: [], issue_view: [], page: [] }, - }); + const [results, setResults] = useState<IWorkspaceSearchResults>(() => ({ + results: { + workspace: [] as const, + project: [] as const, + issue: [] as const, + cycle: [] as const, + module: [] as const, + issue_view: [] as const, + page: [] as const, + }, + }));
93-97: Consider cleanup in useEffect.The useEffect hook should clean up its side effects when the component unmounts or when dependencies change.
useEffect(() => { if (issueDetails && isCommandPaletteOpen) { setSearchInIssue(true); } + return () => { + setSearchInIssue(false); + }; }, [issueDetails, isCommandPaletteOpen]);
238-267: Consider extracting the issue context indicator to a separate component.The issue context indicator UI logic is complex enough to warrant its own component, which would improve maintainability and reusability.
+ // IssueContextIndicator.tsx + interface IssueContextIndicatorProps { + issueDetails: any; // Replace with proper type + onClose: () => void; + } + + const IssueContextIndicator: React.FC<IssueContextIndicatorProps> = ({ issueDetails, onClose }) => ( + <> + <span className="flex items-center text-sm">Update in:</span> + <span className="flex items-center gap-1 rounded px-1.5 py-1 text-sm bg-custom-primary-100/10 "> + {issueDetails.project_id && ( + <IssueIdentifier + issueId={issueDetails.id} + projectId={issueDetails.project_id} + textContainerClassName="text-sm text-custom-primary-200" + /> + )} + <X + size={12} + strokeWidth={2} + className="flex-shrink-0 cursor-pointer" + onClick={onClose} + /> + </span> + </> + ); // In CommandModal component - {searchInIssue && issueDetails && ( - <> - <span className="flex items-center text-sm">Update in:</span> - ... - </> - )} + {searchInIssue && issueDetails && ( + <IssueContextIndicator + issueDetails={issueDetails} + onClose={() => setSearchInIssue(false)} + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/core/components/command-palette/actions/issue-actions/actions-list.tsx(3 hunks)web/core/components/command-palette/actions/issue-actions/change-assignee.tsx(1 hunks)web/core/components/command-palette/actions/issue-actions/change-priority.tsx(1 hunks)web/core/components/command-palette/actions/issue-actions/change-state.tsx(1 hunks)web/core/components/command-palette/command-modal.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
web/core/components/command-palette/actions/issue-actions/change-priority.tsx (1)
8-8: LGTM! Clean hook refactoring.The switch from
useIssuestouseIssueDetailmakes the code more focused and direct.Also applies to: 13-13
web/core/components/command-palette/actions/issue-actions/change-state.tsx (1)
10-10: LGTM! Clean hook refactoring.The switch from
useIssuestouseIssueDetailmakes the code more focused and direct.web/core/components/command-palette/actions/issue-actions/change-assignee.tsx (1)
14-14: LGTM! Clean hook refactoring.The switch from
useIssuestouseIssueDetailmakes the code more focused and direct.Also applies to: 23-23
web/core/components/command-palette/actions/issue-actions/actions-list.tsx (2)
13-13: LGTM! Clean hook refactoring.The switch from
useIssuestouseIssueDetailmakes the code more focused and direct.Also applies to: 29-29
65-65: LGTM! Clean toast message handling.The toast messages have been simplified while maintaining clarity.
Also applies to: 68-68
web/core/components/command-palette/command-modal.tsx (3)
8-8: LGTM! Imports are properly organized.The new imports for
Xicon andcnutility function are correctly added and align with the component's enhanced functionality.Also applies to: 15-15
217-217: LGTM! Smooth scrolling behavior added.The addition of smooth scrolling behavior improves the user experience when navigating through items.
269-271: LGTM! Improved className handling with cn utility.The use of the
cnutility function for className composition is a good practice for managing conditional classes.
| <Dialog | ||
| as="div" | ||
| className="relative z-30" | ||
| onClose={() => { | ||
| closePalette(); | ||
| if (searchInIssue) { | ||
| setSearchInIssue(true); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
Fix potential infinite loop in Dialog onClose.
The onClose handler sets searchInIssue to true when it's already true, which could lead to unnecessary re-renders.
<Dialog
as="div"
className="relative z-30"
onClose={() => {
closePalette();
- if (searchInIssue) {
- setSearchInIssue(true);
- }
}}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Dialog | |
| as="div" | |
| className="relative z-30" | |
| onClose={() => { | |
| closePalette(); | |
| if (searchInIssue) { | |
| setSearchInIssue(true); | |
| } | |
| }} | |
| > | |
| <Dialog | |
| as="div" | |
| className="relative z-30" | |
| onClose={() => { | |
| closePalette(); | |
| }} | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/core/components/command-palette/command-modal.tsx (4)
60-64: State initialization can be simplified.The results state initialization can be further simplified using object shorthand notation since the property name matches the value name.
- const [results, setResults] = useState<IWorkspaceSearchResults>({ - results: { workspace: [], project: [], issue: [], cycle: [], module: [], issue_view: [], page: [] }, - }); + const [results, setResults] = useState<IWorkspaceSearchResults>({ + results: { workspace: [], project: [], issue: [], cycle: [], module: [], issue_view: [], page: [] } + });
93-97: Consider adding cleanup for searchInIssue state.The useEffect hook sets searchInIssue but doesn't clean up when the modal closes.
useEffect(() => { if (issueDetails && isCommandPaletteOpen) { setSearchInIssue(true); } + return () => { + if (!isCommandPaletteOpen) { + setSearchInIssue(false); + } + }; }, [issueDetails, isCommandPaletteOpen]);
99-105: Simplify isWorkspaceLevel logic.The isWorkspaceLevel useEffect can be simplified to a single condition.
useEffect(() => { - if (!projectId && !isWorkspaceLevel) { - setIsWorkspaceLevel(true); - } else { - setIsWorkspaceLevel(false); - } + setIsWorkspaceLevel(!projectId); }, [projectId]);
246-275: Consider extracting issue context indicator to a separate component.The issue context indicator UI logic is complex enough to warrant its own component for better maintainability.
+const IssueContextIndicator: React.FC<{ + issueDetails: any; + onClose: () => void; +}> = ({ issueDetails, onClose }) => ( + <> + <span className="flex items-center text-sm">Update in:</span> + <span className="flex items-center gap-1 rounded px-1.5 py-1 text-sm bg-custom-primary-100/10 "> + {issueDetails.project_id && ( + <IssueIdentifier + issueId={issueDetails.id} + projectId={issueDetails.project_id} + textContainerClassName="text-sm text-custom-primary-200" + /> + )} + <X + size={12} + strokeWidth={2} + className="flex-shrink-0 cursor-pointer" + onClick={onClose} + /> + </span> + </> +); // Then in the main component: {searchInIssue && issueDetails && ( <IssueContextIndicator issueDetails={issueDetails} onClose={() => setSearchInIssue(false)} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/components/command-palette/command-modal.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
web/core/components/command-palette/command-modal.tsx (2)
155-164: Fix potential infinite loop in Dialog onClose.The onClose handler sets
searchInIssueto true when it's already true, which could lead to unnecessary re-renders.
277-279: LGTM: Improved className handling.Good use of the
cnutility for className concatenation.
…es (#6600) * fix: command-k work item actions * chore: command k work item context indicator improvement and default vale for workspace toggle updated * chore: code refactor
Description
This PR includes following changes:
Type of Change
Media
Test Scenarios
References
[WEB-3357 | WEB-3363 | WEB-3370]
Summary by CodeRabbit
New Features
Refactor