fix(desktop): adjust new workspace modal styling#2205
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors the NewWorkspaceModal from a Dialog-based UI with centralized Zustand store state management to a CommandDialog-based UI with local component state. The store is simplified to only manage modal open/close state and pre-selection, while individual components (PromptGroup, BranchesGroup, IssuesGroup, PullRequestsGroup) now maintain their own state and handle mutations locally. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
saddlepaddle
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/PromptGroup.tsx`:
- Around line 143-148: The existing useEffect that resets picker state on
projectId change (calls setBaseBranch, setBaseBranchOpen, setBranchSearch) must
also clear the manual branch override state so the new project's default/prefix
behavior is applied: add calls to reset branchName (e.g., setBranchName("")),
set branchNameEdited to false (setBranchNameEdited(false)), and restore
applyPrefix to its default (setApplyPrefix(true) or the appropriate default
setter) inside the same effect so all branch-related state is cleared when
projectId changes.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`:
- Around line 35-44: The effect that initializes the default project when the
modal opens only depends on isOpen so if recentProjects arrives asynchronously
after getRecents resolves the effect never reruns; update the effect used in
NewWorkspaceModal (the useEffect that calls setSelectedProjectId) to also depend
on recentProjects (and optionally preSelectedProjectId/selectedProjectId) and
ensure it only runs when isOpen is true, so when recentProjects changes while
the modal is open it will set selectedProjectId to preSelectedProjectId or to
recentProjects[0].id as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6edbbcb7-17f5-4fda-9ee2-b4a096b845db
📒 Files selected for processing (7)
apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/BranchesGroup/BranchesGroup.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/IssuesGroup/IssuesGroup.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/PromptGroup.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/components/PromptGroupAdvancedOptions/PromptGroupAdvancedOptions.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/PullRequestsGroup/PullRequestsGroup.tsxapps/desktop/src/renderer/stores/new-workspace-modal.ts
Comment on lines
+143
to
+148
| // biome-ignore lint/correctness/useExhaustiveDependencies: intentionally reset advanced branch state when project changes | ||
| useEffect(() => { | ||
| if (previousProjectIdRef.current === projectId) { | ||
| return; | ||
| } | ||
| previousProjectIdRef.current = projectId; | ||
| setBaseBranch(null); | ||
| setBaseBranchOpen(false); | ||
| setBranchSearch(""); | ||
| }, [projectId, setBaseBranch, setBranchSearch]); | ||
| }, [projectId]); |
Contributor
There was a problem hiding this comment.
Reset the manual branch override when the project changes.
This effect clears the base-branch picker state, but it leaves branchName and branchNameEdited intact. If a user customizes the branch for project A and then switches to project B, the old branch name carries over and applyPrefix stays false, so the new project's prefix/default-branch behavior is skipped accidentally.
Suggested fix
// biome-ignore lint/correctness/useExhaustiveDependencies: intentionally reset advanced branch state when project changes
useEffect(() => {
+ setBranchName("");
+ setBranchNameEdited(false);
setBaseBranch(null);
setBaseBranchOpen(false);
setBranchSearch("");
}, [projectId]);📝 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.
Suggested change
| // biome-ignore lint/correctness/useExhaustiveDependencies: intentionally reset advanced branch state when project changes | |
| useEffect(() => { | |
| if (previousProjectIdRef.current === projectId) { | |
| return; | |
| } | |
| previousProjectIdRef.current = projectId; | |
| setBaseBranch(null); | |
| setBaseBranchOpen(false); | |
| setBranchSearch(""); | |
| }, [projectId, setBaseBranch, setBranchSearch]); | |
| }, [projectId]); | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: intentionally reset advanced branch state when project changes | |
| useEffect(() => { | |
| setBranchName(""); | |
| setBranchNameEdited(false); | |
| setBaseBranch(null); | |
| setBaseBranchOpen(false); | |
| setBranchSearch(""); | |
| }, [projectId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/PromptGroup.tsx`
around lines 143 - 148, The existing useEffect that resets picker state on
projectId change (calls setBaseBranch, setBaseBranchOpen, setBranchSearch) must
also clear the manual branch override state so the new project's default/prefix
behavior is applied: add calls to reset branchName (e.g., setBranchName("")),
set branchNameEdited to false (setBranchNameEdited(false)), and restore
applyPrefix to its default (setApplyPrefix(true) or the appropriate default
setter) inside the same effect so all branch-related state is cleared when
projectId changes.
Comment on lines
+35
to
+44
| // Sync pre-selected project when modal opens | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: reset on modal open | ||
| useEffect(() => { | ||
| if (!isOpen) return; | ||
| const hasSelectedProject = recentProjects.some( | ||
| (project) => project.id === selectedProjectId, | ||
| ); | ||
| if (!hasSelectedProject) { | ||
| setSelectedProjectId(recentProjects[0]?.id ?? null); | ||
| if (preSelectedProjectId) { | ||
| setSelectedProjectId(preSelectedProjectId); | ||
| } else if (recentProjects.length > 0 && !selectedProjectId) { | ||
| setSelectedProjectId(recentProjects[0].id); | ||
| } | ||
| }, [isOpen, recentProjects, selectedProjectId, setSelectedProjectId]); | ||
| }, [isOpen]); |
Contributor
There was a problem hiding this comment.
Let the default-project initialization react to async recents.
If the modal opens before getRecents resolves, recentProjects is still empty here, so selectedProjectId stays null. Because the effect only depends on isOpen, it never reruns when the recents query finishes, and the modal opens without a default project until the user picks one manually.
Suggested fix
- // Sync pre-selected project when modal opens
- // biome-ignore lint/correctness/useExhaustiveDependencies: reset on modal open
useEffect(() => {
if (!isOpen) return;
if (preSelectedProjectId) {
setSelectedProjectId(preSelectedProjectId);
- } else if (recentProjects.length > 0 && !selectedProjectId) {
- setSelectedProjectId(recentProjects[0].id);
+ return;
}
- }, [isOpen]);
+ if (recentProjects.length > 0) {
+ setSelectedProjectId((current) => current ?? recentProjects[0].id);
+ }
+ }, [isOpen, preSelectedProjectId, recentProjects]);📝 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.
Suggested change
| // Sync pre-selected project when modal opens | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: reset on modal open | |
| useEffect(() => { | |
| if (!isOpen) return; | |
| const hasSelectedProject = recentProjects.some( | |
| (project) => project.id === selectedProjectId, | |
| ); | |
| if (!hasSelectedProject) { | |
| setSelectedProjectId(recentProjects[0]?.id ?? null); | |
| if (preSelectedProjectId) { | |
| setSelectedProjectId(preSelectedProjectId); | |
| } else if (recentProjects.length > 0 && !selectedProjectId) { | |
| setSelectedProjectId(recentProjects[0].id); | |
| } | |
| }, [isOpen, recentProjects, selectedProjectId, setSelectedProjectId]); | |
| }, [isOpen]); | |
| useEffect(() => { | |
| if (!isOpen) return; | |
| if (preSelectedProjectId) { | |
| setSelectedProjectId(preSelectedProjectId); | |
| return; | |
| } | |
| if (recentProjects.length > 0) { | |
| setSelectedProjectId((current) => current ?? recentProjects[0].id); | |
| } | |
| }, [isOpen, preSelectedProjectId, recentProjects]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`
around lines 35 - 44, The effect that initializes the default project when the
modal opens only depends on isOpen so if recentProjects arrives asynchronously
after getRecents resolves the effect never reruns; update the effect used in
NewWorkspaceModal (the useEffect that calls setSelectedProjectId) to also depend
on recentProjects (and optionally preSelectedProjectId/selectedProjectId) and
ensure it only runs when isOpen is true, so when recentProjects changes while
the modal is open it will set selectedProjectId to preSelectedProjectId or to
recentProjects[0].id as before.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
639c133bf,adc7a16d7,6228e8ccf) that broke the New Workspace modal styling, restoring all modal files to Satya's revamp state (2210a060a)PromptGroupAdvancedOptionscomponent (branch name, base branch, setup script) with correct stylingTest plan
Summary by cubic
Restores the New Workspace modal to the revamp design and brings back advanced options (branch name, base branch, setup script) with correct styling and placement. Also simplifies modal state and creation flows for stability across all tabs.
Bug Fixes
CommandDialogfrom@superset/ui/commandand fix header/tabs, search input, and scrolling.PromptGroupAdvancedOptions, place it between the prompt input and Create button, and remove the duplicate branch preview.Refactors
isOpenandpreSelectedProjectId.Written for commit 68132ec. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes