polish v2 project settings + follow-up review feedback from #4665#4675
Merged
Conversation
Restructures the v2 project settings page to match the canonical SettingsRow pattern used in OrganizationSettings: flat list of rows with a uniform w-96 control column, no inner borders or card chrome. - Move project icon thumbnail into the page header beside the title; clicking the thumbnail opens a dropdown menu (Upload / Use GitHub icon / Remove) instead of inline buttons. - Host scope chip rendered inline in the header (single-host hidden). - Repository and Location each render a single w-96 field with a ghost icon button absolutely positioned inside it: FaGithub + tooltip "Open in GitHub" for Repository, LuFolderOpen + tooltip "Change location" for Location. - Location path display uses overflow-x-auto whitespace-nowrap so long paths scroll horizontally inside the field instead of breaking mid-word; ClickablePath gains an opt-in truncate prop (kept for v1 callers). - Scripts editor uses underline-style tabs (no pill chrome, no gap, px-3 per trigger, hover transition), a skeleton-shaped loading state matching the rendered layout, and a tucked-in "Import" button inside the textarea corner. - All inputs normalized to text-sm; mono fields keep font-mono. - Delete project row aligned to py-2.5 / gap-8 rhythm without a separator above.
- cli/projects list: gracefully degrade when the host service can't be reached and the user did not explicitly pass --host/--local. Falls back to showing organization projects with setUp="?" and path="-" instead of throwing, restoring the previously API-only listing behavior. Still throws when --host or --local was requested explicitly, so the user gets a clear error in that case. (P1 flagged by greptile-apps and cubic-dev-ai.) - cli/projects setup: reject "Project ID specified twice" whenever both --project and the positional argument are passed, even when the values match. (Flagged by coderabbitai.) - v2 project settings: add `select-text cursor-text` to both AlertDialogDescription blocks in ProjectLocationSection so users can copy the conflict message and the From/To paths into bug reports. Aligns with the renderer-wide "error text must be selectable" rule in apps/desktop/AGENTS.md. (Flagged by coderabbitai.)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors V2 project settings UI for multi-host selection, adds path truncation and icon/scripts UI updates, introduces SetupProjectModal and RemotePathPicker, adds host filesystem browse RPC, and hardens CLI project commands' host handling and validation. ChangesDesktop V2 Project Settings UI Refactoring
CLI Command Validation and Error Handling
Sequence Diagram(s)sequenceDiagram
participant V2ProjectSettings
participant useWorkspaceHostOptions
participant HostSelect
participant Router
V2ProjectSettings->>useWorkspaceHostOptions: request host options
useWorkspaceHostOptions-->>V2ProjectSettings: hostOptions, localHostId, currentDeviceName
V2ProjectSettings->>HostSelect: render selector when multiple hosts
HostSelect->>Router: navigate with search.hostId on change
Router-->>V2ProjectSettings: new route triggers re-render & host-targeted data fetch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit 78f75f8 on May 17, 2026 8:34pm UTC. |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx (1)
364-466: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove
ScriptFieldto its own component file.
V2ScriptsEditor.tsxnow includes multiple components in the changed area. Please extractScriptFieldinto a dedicated file and import it.Suggested refactor
+import { ScriptField } from "./ScriptField"; ... -interface ScriptFieldProps { - placeholder: string; - value: string; - onChange: (value: string) => void; - onFocus: () => void; - onBlur: () => void; -} - -function ScriptField({ ... }: ScriptFieldProps) { - ... -}As per coding guidelines
**/components/**/*.tsx: "Create one component per file - no multi-component files".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx around lines 364 - 466, Extract the ScriptField component into its own file: create a new component file that exports ScriptField (keeping the ScriptFieldProps interface), move the internal implementation (useState, useRef, useCallback, importFirstFile, JSX including Textarea, HiDocumentArrowUp, cn, and the hidden file input) into it, add the necessary imports (React, useState, useRef, useCallback, Textarea, HiDocumentArrowUp, cn), and ensure it is exported as default or named export; then remove the ScriptField definition from V2ScriptsEditor.tsx and import the new ScriptField symbol into that file, preserving props usage (placeholder, value, onChange, onFocus, onBlur) and behavior exactly as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx`:
- Around line 65-67: The span that renders the path inside the ClickablePath
component is a direct flex child and needs min-w-0 to allow the truncate
behavior to work; update the className generation in ClickablePath (the span
that conditionally uses the truncate boolean) to include "min-w-0" whenever
"truncate" is true (ensure the classes array for that span includes "min-w-0"
alongside "truncate" and any existing font/spacing classes).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/ProjectLocationSection/ProjectLocationSection.tsx:
- Around line 271-288: The "Change location" button should only render for local
targets; update the conditional around the Tooltip/Button (currently
"currentPath && (...)") to also check that the project/target is local (e.g.,
"!isRemoteHost" or "host?.type === 'local'") so the button and its handleChange
directory dialog are not shown for remote hosts; use whatever existing
prop/state (project, host, target, isRemote, etc.) is available in this
component to detect remote targets and disable/hide the control (also ensure the
disabled logic still respects selectDirectory.isPending and isSubmitting).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx:
- Around line 39-61: Move the inline SettingsRow component into its own file and
import it back into V2ProjectSettings: create a new component file (e.g.,
SettingsRow.tsx) exporting the SettingsRow function component (same props:
label, hint, htmlFor, children) and keep the exact JSX structure and classNames;
then remove the SettingsRow declaration from V2ProjectSettings.tsx and add an
import for SettingsRow at the top of that file so V2ProjectSettings uses the
extracted component. Ensure the export/import use the correct component name
SettingsRow and preserve prop typing and ReactNode imports.
In `@packages/cli/src/commands/projects/list/command.ts`:
- Around line 42-44: The catch block currently swallows every error when
hostExplicit is false; change it to only suppress known
host-unreachable/host-selection errors and rethrow everything else: inside the
catch(err) in command.ts, keep the existing if (hostExplicit) throw err; but
after that inspect err (e.g., err.name or instanceof against your
HostUnreachableError/HostSelectionError types, or match error.message for "host
unreachable" / "host context") and only return the downgraded setUp/path
placeholders for those specific errors; for any other error rethrow it. Ensure
you reference the same catch scope that uses hostExplicit so unrelated
exceptions are not masked.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx:
- Around line 364-466: Extract the ScriptField component into its own file:
create a new component file that exports ScriptField (keeping the
ScriptFieldProps interface), move the internal implementation (useState, useRef,
useCallback, importFirstFile, JSX including Textarea, HiDocumentArrowUp, cn, and
the hidden file input) into it, add the necessary imports (React, useState,
useRef, useCallback, Textarea, HiDocumentArrowUp, cn), and ensure it is exported
as default or named export; then remove the ScriptField definition from
V2ScriptsEditor.tsx and import the new ScriptField symbol into that file,
preserving props usage (placeholder, value, onChange, onFocus, onBlur) and
behavior exactly as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de63b2d-8032-421d-b3bc-e73e758cbb6b
📒 Files selected for processing (10)
apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/DeleteProjectSection/DeleteProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/IconUploadField/IconUploadField.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/ProjectLocationSection/ProjectLocationSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsxpackages/cli/src/commands/projects/list/command.tspackages/cli/src/commands/projects/setup/command.ts
Comment on lines
+39
to
+61
| function SettingsRow({ | ||
| label, | ||
| hint, | ||
| htmlFor, | ||
| children, | ||
| }: { | ||
| label: string; | ||
| hint?: ReactNode; | ||
| htmlFor?: string; | ||
| children: ReactNode; | ||
| }) { | ||
| return ( | ||
| <div className="flex items-center justify-between gap-8 py-2.5"> | ||
| <div className="min-w-0 flex-1"> | ||
| <Label htmlFor={htmlFor} className="text-sm font-medium"> | ||
| {label} | ||
| </Label> | ||
| {hint && <p className="mt-0.5 text-xs text-muted-foreground">{hint}</p>} | ||
| </div> | ||
| <div className="shrink-0">{children}</div> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract SettingsRow into its own component file.
Line 39 introduces a second component in this component file. Please move SettingsRow to a separate component file and import it here.
Suggested refactor
+import { SettingsRow } from "./components/SettingsRow";
...
-interface ProjectSettingsHostOption {
+interface ProjectSettingsHostOption {
id: string;
name: string;
isLocal: boolean;
isOnline: boolean;
}
-
-function SettingsRow({
- label,
- hint,
- htmlFor,
- children,
-}: {
- label: string;
- hint?: ReactNode;
- htmlFor?: string;
- children: ReactNode;
-}) {
- return (
- <div className="flex items-center justify-between gap-8 py-2.5">
- <div className="min-w-0 flex-1">
- <Label htmlFor={htmlFor} className="text-sm font-medium">
- {label}
- </Label>
- {hint && <p className="mt-0.5 text-xs text-muted-foreground">{hint}</p>}
- </div>
- <div className="shrink-0">{children}</div>
- </div>
- );
-}As per coding guidelines **/components/**/*.tsx: "Create one component per file - no multi-component files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx
around lines 39 - 61, Move the inline SettingsRow component into its own file
and import it back into V2ProjectSettings: create a new component file (e.g.,
SettingsRow.tsx) exporting the SettingsRow function component (same props:
label, hint, htmlFor, children) and keep the exact JSX structure and classNames;
then remove the SettingsRow declaration from V2ProjectSettings.tsx and add an
import for SettingsRow at the top of that file so V2ProjectSettings uses the
extracted component. Ensure the export/import use the correct component name
SettingsRow and preserve prop typing and ReactNode imports.
Comment on lines
+42
to
+44
| } catch (err) { | ||
| if (hostExplicit) throw err; | ||
| } |
Contributor
There was a problem hiding this comment.
Narrow error suppression to host-unreachable failures only.
At Line 42, the catch currently suppresses all failures when host selection wasn’t explicit. This can mask unrelated defects and return misleading setUp/path placeholders. Please only downgrade known host-unreachable/host-context errors and rethrow everything else.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/projects/list/command.ts` around lines 42 - 44, The
catch block currently swallows every error when hostExplicit is false; change it
to only suppress known host-unreachable/host-selection errors and rethrow
everything else: inside the catch(err) in command.ts, keep the existing if
(hostExplicit) throw err; but after that inspect err (e.g., err.name or
instanceof against your HostUnreachableError/HostSelectionError types, or match
error.message for "host unreachable" / "host context") and only return the
downgraded setUp/path placeholders for those specific errors; for any other
error rethrow it. Ensure you reference the same catch scope that uses
hostExplicit so unrelated exceptions are not masked.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx:435">
P2: The inline absolute-positioned Import button can overlap textarea content because no space is reserved inside the textarea for it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Contributor
Greptile SummaryThis PR polishes the v2 project settings page to a Linear-style flat layout (inline
Confidence Score: 4/5Safe to merge; the CLI fixes and UI layout changes are all correct, with two informational strings removed that are worth restoring before shipping. All functional behavior is correct — the CLI graceful-degradation logic, the duplicate-ID guard, the icon dropdown, and the multi-host selector work as described. The only concerns are presentational: the 'local clones are not affected' safety hint was dropped from the Delete row, and the docs link plus per-tab descriptions were removed from the Scripts editor, leaving first-time users without guidance. Neither affects runtime correctness. DeleteProjectSection.tsx (missing safety hint) and V2ScriptsEditor.tsx (missing docs link and tab descriptions).
|
| Filename | Overview |
|---|---|
| packages/cli/src/commands/projects/list/command.ts | Adds graceful degradation: wraps host resolution in try/catch and falls back to setUp="?"/ path="-" when the host service is unreachable and no explicit --host/--local flag was passed. Logic is correct. |
| packages/cli/src/commands/projects/setup/command.ts | Moves the "Project ID specified twice" check before the projectId extraction so it fires even when both values are equal. Correct fix. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx | Major layout refactor: replaces SettingsSection/ProjectSettingsHeader with inline SettingsRow pattern, adds multi-host selector in header, and wires hostId into location navigation. Logic and host-option building look correct. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/ProjectLocationSection/ProjectLocationSection.tsx | Restyled location display to input-shaped container with absolute icon button; added hostId prop for correct navigation after conflict resolution; both AlertDialogDescriptions gain select-text/cursor-text. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx | Switches to underline-style tabs, adds skeleton loading state, moves Import into the textarea corner. The "Docs" link and all per-tab descriptions have been removed. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/DeleteProjectSection/DeleteProjectSection.tsx | Adds py-2.5 padding to match SettingsRow style, but removes the description text clarifying that local clones are unaffected by deletion. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/IconUploadField/IconUploadField.tsx | Replaces three separate buttons with a dropdown menu triggered from the thumbnail; direct click still works when no secondary actions are available. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[superset projects list] --> B{options.host defined\nor options.local === true?}
B -- yes --> C[hostExplicit = true]
B -- no --> D[hostExplicit = false]
C & D --> E[resolveHostTarget + fetch host projects]
E --> F{Error thrown?}
F -- no --> G[Build hostProjectById map]
F -- yes + hostExplicit --> H[re-throw — user sees error]
F -- yes + not hostExplicit --> I[hostProjectById = null]
G --> J[Map: setUp=yes/no, path=repoPath/-]
I --> K[Map: setUp=?, path=-]
J & K --> L[Display table]
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx, line 293-360 (link)Docs link and per-tab descriptions removed without replacement
The refactor dropped both the "Docs" button (linking to
EXTERNAL_LINKS.SETUP_TEARDOWN_SCRIPTS) and the per-tab description strings ("Runs when a new workspace is created. Multiple lines run as one chain — failures short-circuit.", "Runs when a workspace is deleted.", "Runs from the workspace Run button."). Users who are setting up scripts for the first time have no hint about the semantics of each tab, and no path to the documentation. Was this intentional?Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx Line: 293-360 Comment: **Docs link and per-tab descriptions removed without replacement** The refactor dropped both the "Docs" button (linking to `EXTERNAL_LINKS.SETUP_TEARDOWN_SCRIPTS`) and the per-tab description strings ("Runs when a new workspace is created. Multiple lines run as one chain — failures short-circuit.", "Runs when a workspace is deleted.", "Runs from the workspace Run button."). Users who are setting up scripts for the first time have no hint about the semantics of each tab, and no path to the documentation. Was this intentional? How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/V2ScriptsEditor/V2ScriptsEditor.tsx:293-360
**Docs link and per-tab descriptions removed without replacement**
The refactor dropped both the "Docs" button (linking to `EXTERNAL_LINKS.SETUP_TEARDOWN_SCRIPTS`) and the per-tab description strings ("Runs when a new workspace is created. Multiple lines run as one chain — failures short-circuit.", "Runs when a workspace is deleted.", "Runs from the workspace Run button."). Users who are setting up scripts for the first time have no hint about the semantics of each tab, and no path to the documentation. Was this intentional?
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/DeleteProjectSection/DeleteProjectSection.tsx:56-60
The row now shows only "Delete project" with no hint. The alert dialog covers "removes from organization" and "cannot be undone," but neither the row nor the dialog any longer mentions that **local clones are unaffected**. That detail matters to users who are nervous about losing files on disk — it's the one sentence that lets them proceed confidently.
```suggestion
return (
<div className="flex items-center justify-between gap-8 py-2.5">
<div className="min-w-0 flex-1">
<div className="text-sm font-medium">Delete project</div>
<p className="text-xs text-muted-foreground mt-0.5">
Local clones on any host are not affected.
</p>
</div>
```
Reviews (1): Last reviewed commit: "fix: address review feedback on PR #4665" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
56
to
60
| return ( | ||
| <div className="flex items-center justify-between gap-8"> | ||
| <div className="flex items-center justify-between gap-8 py-2.5"> | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="text-sm font-medium">Delete project</div> | ||
| <p className="text-xs text-muted-foreground mt-0.5"> | ||
| Removes the project from the organization. Workspaces and local clones | ||
| on any host are not affected. | ||
| </p> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Suggested change
| return ( | |
| <div className="flex items-center justify-between gap-8"> | |
| <div className="flex items-center justify-between gap-8 py-2.5"> | |
| <div className="min-w-0 flex-1"> | |
| <div className="text-sm font-medium">Delete project</div> | |
| <p className="text-xs text-muted-foreground mt-0.5"> | |
| Removes the project from the organization. Workspaces and local clones | |
| on any host are not affected. | |
| </p> | |
| </div> | |
| return ( | |
| <div className="flex items-center justify-between gap-8 py-2.5"> | |
| <div className="min-w-0 flex-1"> | |
| <div className="text-sm font-medium">Delete project</div> | |
| <p className="text-xs text-muted-foreground mt-0.5"> | |
| Local clones on any host are not affected. | |
| </p> | |
| </div> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/DeleteProjectSection/DeleteProjectSection.tsx
Line: 56-60
Comment:
The row now shows only "Delete project" with no hint. The alert dialog covers "removes from organization" and "cannot be undone," but neither the row nor the dialog any longer mentions that **local clones are unaffected**. That detail matters to users who are nervous about losing files on disk — it's the one sentence that lets them proceed confidently.
```suggestion
return (
<div className="flex items-center justify-between gap-8 py-2.5">
<div className="min-w-0 flex-1">
<div className="text-sm font-medium">Delete project</div>
<p className="text-xs text-muted-foreground mt-0.5">
Local clones on any host are not affected.
</p>
</div>
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
The remote empty state previously rendered three side-by-side blocks (a decorative "Not set up on X" input chrome, plus two stacked input+button rows for Import and Clone) — busy and repeated the host name three times. Replace with a single inline form: mode Select (Clone / Import) + path input + "Set up" button. Mode defaults to Clone when a GitHub remote is linked, otherwise Import. The Clone option in the dropdown disables and shows a tooltip when no repo is linked. Local empty state drops the decorative left container too and just shows the two outline buttons that trigger the native folder picker. No behavior change: handleImport / handleClone still drive the same setup logic and conflict precheck.
Replace the inline empty-state setup form (mode select + path input +
button) with a single "Set up project…" button that opens a new
SetupProjectModal. The empty Location row now reads:
Not set up on {host}. [Set up project…]
The modal has Clone / Import existing tabs, a Repository readout on
the Clone tab, and a path input. For local-host setup the input is
paired with a browse button (native folder picker); for remote-host
setup the input is text-only with a hostName-qualified placeholder.
Conflict precheck is preserved: on import, if the path is already
linked to another project, the modal closes and the existing
"Repository already linked" AlertDialog opens via the parent.
ProjectLocationSection drops the now-unused remoteImportPath /
remoteCloneParentDir / remoteMode state and the inline submit
handlers; runSetup / runClone live in the modal. The Change… flow
and the Relocate confirmation dialog are untouched.
When setting up a project on a remote host you couldn't previously
pick a directory — only type the absolute path blind. Add a reusable
folder browser that talks to the host service over tRPC.
Backend:
- New filesystem.browseHost protected query on the host-service
router. Takes an optional absolute or ~-prefixed path (defaults to
homedir()), normalizes it, and returns { path, parentPath, homePath,
entries[] } with entries sorted dotfiles-last and folders-first.
Hidden files filtered by default; pass includeHidden:true to see
them. Unlike the existing listDirectory endpoint, this is not
scoped to a workspace — used for setup flows where no workspace
exists yet on the host. Path must be absolute or start with ~ for
safety; relative paths are rejected.
Frontend (renderer/components/RemotePathPicker):
- Reusable component that opens a Dialog with a path input
(~-aware), Up / Home / Refresh icon buttons, and a scrollable list
of subfolders. Single-click highlights a folder in the path input;
double-click descends into it. "Use this folder" picks the current
path and closes. Reusable across any future flow that needs to
pick a path on a host (workspace settings, script paths, etc.).
Integration:
- SetupProjectModal: the browse button now opens RemotePathPicker
for remote hosts and the existing native Electron picker for local
hosts, behind the same LuFolderOpen icon. Picked path fills the
Clone parent-directory or Import existing-repo input.
Replace the four-button toolbar + editable path input with a clean breadcrumb-driven path picker: - Breadcrumb at top doubles as path display and navigation. Clicking any earlier segment jumps to that level. Last segment is bold. If the path lives under $HOME it starts with a "Home" segment; long paths collapse middle segments with BreadcrumbEllipsis. - Folders below in a borderless scroll area. Single click on a folder descends into it (replaces the earlier double-click pattern, which was easy to miss). Hover state on each row, no outer border. - Refresh moves into a quiet icon button next to the breadcrumb. - Symlink indicator: small LuExternalLink at the row end instead of a text "link" badge. - Empty state shows a muted folder glyph plus "Empty folder" / "No subfolders" message. - Loading state renders skeleton rows that match the rendered row layout (icon + label). - Borders only where they carry weight: under the breadcrumb row and above the footer. The dialog itself drops outer padding and gains per-section padding so the borders span the full dialog width. - Drops the Up / Home / Edit-path toolbar buttons; up-navigation happens via breadcrumb segments and home-navigation via the first "Home" segment. The current path is always visible in the breadcrumb, so no separate read-only path readout is needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/RemotePathPicker/RemotePathPicker.tsx`:
- Around line 116-120: The confirm handler handlePick is using query.data?.path
?? currentPath so it ignores the single-clicked child stored in pathDraft;
change handlePick to prefer the clicked folder by using pathDraft ??
query.data?.path ?? currentPath when calling onPick and onOpenChange, and apply
the same replacement for the other confirmation block around lines 208-213 so
single-click + "Use this folder" returns the clicked child rather than the
parent.
In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts`:
- Around line 95-101: The catch-all mapping of stat failures to NOT_FOUND is
incorrect: in the block around stat(targetPath) you should inspect the thrown
error (from stat) and only throw TRPCError with code "NOT_FOUND" when the error
indicates a missing file (e.g., err.code === "ENOENT"); for permission or other
errors throw a different TRPCError (e.g., code "FORBIDDEN" for EACCES/EPERM or
"INTERNAL_SERVER_ERROR" otherwise) and include the original error message for
debugging. Update the try/catch around stat(targetPath) and the TRPCError
creation so callers can distinguish missing paths from access failures; keep
references to stat, targetPath and TRPCError when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b4b3b40-aeb0-40f8-9f94-1b5551176d3e
📒 Files selected for processing (4)
apps/desktop/src/renderer/components/RemotePathPicker/RemotePathPicker.tsxapps/desktop/src/renderer/components/RemotePathPicker/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/SetupProjectModal/SetupProjectModal.tsxpackages/host-service/src/trpc/router/filesystem/filesystem.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/RemotePathPicker/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/SetupProjectModal/SetupProjectModal.tsx
Comment on lines
+95
to
+101
| try { | ||
| stats = await stat(targetPath); | ||
| } catch { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: `Path not found: ${targetPath}`, | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't report every browse failure as NOT_FOUND.
Line 97 maps all stat failures to NOT_FOUND. That makes permission-denied or other access failures look like missing paths, and the renderer toasts that message verbatim. Please reserve NOT_FOUND for actual missing-path cases and surface access errors separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts` around lines
95 - 101, The catch-all mapping of stat failures to NOT_FOUND is incorrect: in
the block around stat(targetPath) you should inspect the thrown error (from
stat) and only throw TRPCError with code "NOT_FOUND" when the error indicates a
missing file (e.g., err.code === "ENOENT"); for permission or other errors throw
a different TRPCError (e.g., code "FORBIDDEN" for EACCES/EPERM or
"INTERNAL_SERVER_ERROR" otherwise) and include the original error message for
debugging. Update the try/catch around stat(targetPath) and the TRPCError
creation so callers can distinguish missing paths from access failures; keep
references to stat, targetPath and TRPCError when making the change.
Contributor
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/filesystem/filesystem.ts">
<violation number="1" location="packages/host-service/src/trpc/router/filesystem/filesystem.ts:97">
P2: `browseHost` maps every `stat()` error to `NOT_FOUND`, which misreports permission/IO failures as missing paths. Only `ENOENT` should become `NOT_FOUND`; other errors should preserve/access-denied semantics.</violation>
</file>
<file name="apps/desktop/src/renderer/components/RemotePathPicker/RemotePathPicker.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/RemotePathPicker/RemotePathPicker.tsx:59">
P2: This path parsing is POSIX-only. Normalize separators before splitting so Windows host paths are handled correctly in breadcrumb navigation.
(Based on your team's feedback about using cross-platform path utilities instead of manual path splitting.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let stats: Awaited<ReturnType<typeof stat>>; | ||
| try { | ||
| stats = await stat(targetPath); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: browseHost maps every stat() error to NOT_FOUND, which misreports permission/IO failures as missing paths. Only ENOENT should become NOT_FOUND; other errors should preserve/access-denied semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/filesystem/filesystem.ts, line 97:
<comment>`browseHost` maps every `stat()` error to `NOT_FOUND`, which misreports permission/IO failures as missing paths. Only `ENOENT` should become `NOT_FOUND`; other errors should preserve/access-denied semantics.</comment>
<file context>
@@ -51,6 +70,87 @@ const writeFileContentSchema = z.union([
+ let stats: Awaited<ReturnType<typeof stat>>;
+ try {
+ stats = await stat(targetPath);
+ } catch {
+ throw new TRPCError({
+ code: "NOT_FOUND",
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
|
|
||
| const MAX_VISIBLE_SEGMENTS = 4; | ||
|
|
||
| function pathToSegments(path: string, homePath: string | null): Segment[] { |
Contributor
There was a problem hiding this comment.
P2: This path parsing is POSIX-only. Normalize separators before splitting so Windows host paths are handled correctly in breadcrumb navigation.
(Based on your team's feedback about using cross-platform path utilities instead of manual path splitting.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/components/RemotePathPicker/RemotePathPicker.tsx, line 59:
<comment>This path parsing is POSIX-only. Normalize separators before splitting so Windows host paths are handled correctly in breadcrumb navigation.
(Based on your team's feedback about using cross-platform path utilities instead of manual path splitting.) </comment>
<file context>
@@ -0,0 +1,289 @@
+
+const MAX_VISIBLE_SEGMENTS = 4;
+
+function pathToSegments(path: string, homePath: string | null): Segment[] {
+ const segments: Segment[] = [];
+ if (homePath && (path === homePath || path === `${homePath}/`)) {
</file context>
Functional fixes: - ProjectLocationSection: the "Change location" pencil button used to open the local native folder picker even when the project was set up on a remote host, which would have produced an invalid relocation path. Now: on local targets it still opens the native picker; on remote targets it opens the RemotePathPicker against the remote host's filesystem. The relocate flow (conflict precheck + relocate confirm dialog) is unchanged. - V2ScriptsEditor/ScriptField: add `pb-8 pr-20` to the textarea so the absolute-positioned Import button at the bottom-right can no longer overlap typed content. - ClickablePath: the truncate span needs `min-w-0` when its parent is a flex container, otherwise the ellipsis never kicks in. Structural fixes (one-component-per-file rule from AGENTS.md): - Extract `SettingsRow` from V2ProjectSettings.tsx into its own SettingsRow/ folder with an index re-export. V2ProjectSettings.tsx imports it. - Extract `ScriptField` from V2ScriptsEditor.tsx into a co-located components/ScriptField/ folder. V2ScriptsEditor.tsx imports it. (Co-located under V2ScriptsEditor since it's only used by that editor.)
sazabi Bot
pushed a commit
that referenced
this pull request
May 20, 2026
) * feat(desktop): polish v2 project settings to Linear-style layout Restructures the v2 project settings page to match the canonical SettingsRow pattern used in OrganizationSettings: flat list of rows with a uniform w-96 control column, no inner borders or card chrome. - Move project icon thumbnail into the page header beside the title; clicking the thumbnail opens a dropdown menu (Upload / Use GitHub icon / Remove) instead of inline buttons. - Host scope chip rendered inline in the header (single-host hidden). - Repository and Location each render a single w-96 field with a ghost icon button absolutely positioned inside it: FaGithub + tooltip "Open in GitHub" for Repository, LuFolderOpen + tooltip "Change location" for Location. - Location path display uses overflow-x-auto whitespace-nowrap so long paths scroll horizontally inside the field instead of breaking mid-word; ClickablePath gains an opt-in truncate prop (kept for v1 callers). - Scripts editor uses underline-style tabs (no pill chrome, no gap, px-3 per trigger, hover transition), a skeleton-shaped loading state matching the rendered layout, and a tucked-in "Import" button inside the textarea corner. - All inputs normalized to text-sm; mono fields keep font-mono. - Delete project row aligned to py-2.5 / gap-8 rhythm without a separator above. * fix: address review feedback on PR #4665 - cli/projects list: gracefully degrade when the host service can't be reached and the user did not explicitly pass --host/--local. Falls back to showing organization projects with setUp="?" and path="-" instead of throwing, restoring the previously API-only listing behavior. Still throws when --host or --local was requested explicitly, so the user gets a clear error in that case. (P1 flagged by greptile-apps and cubic-dev-ai.) - cli/projects setup: reject "Project ID specified twice" whenever both --project and the positional argument are passed, even when the values match. (Flagged by coderabbitai.) - v2 project settings: add `select-text cursor-text` to both AlertDialogDescription blocks in ProjectLocationSection so users can copy the conflict message and the From/To paths into bug reports. Aligns with the renderer-wide "error text must be selectable" rule in apps/desktop/AGENTS.md. (Flagged by coderabbitai.) * refactor(desktop): tighten v2 Location empty state UI The remote empty state previously rendered three side-by-side blocks (a decorative "Not set up on X" input chrome, plus two stacked input+button rows for Import and Clone) — busy and repeated the host name three times. Replace with a single inline form: mode Select (Clone / Import) + path input + "Set up" button. Mode defaults to Clone when a GitHub remote is linked, otherwise Import. The Clone option in the dropdown disables and shows a tooltip when no repo is linked. Local empty state drops the decorative left container too and just shows the two outline buttons that trigger the native folder picker. No behavior change: handleImport / handleClone still drive the same setup logic and conflict precheck. * feat(desktop): move v2 project setup form into a modal Replace the inline empty-state setup form (mode select + path input + button) with a single "Set up project…" button that opens a new SetupProjectModal. The empty Location row now reads: Not set up on {host}. [Set up project…] The modal has Clone / Import existing tabs, a Repository readout on the Clone tab, and a path input. For local-host setup the input is paired with a browse button (native folder picker); for remote-host setup the input is text-only with a hostName-qualified placeholder. Conflict precheck is preserved: on import, if the path is already linked to another project, the modal closes and the existing "Repository already linked" AlertDialog opens via the parent. ProjectLocationSection drops the now-unused remoteImportPath / remoteCloneParentDir / remoteMode state and the inline submit handlers; runSetup / runClone live in the modal. The Change… flow and the Relocate confirmation dialog are untouched. * feat(desktop): add RemotePathPicker for browsing host filesystem When setting up a project on a remote host you couldn't previously pick a directory — only type the absolute path blind. Add a reusable folder browser that talks to the host service over tRPC. Backend: - New filesystem.browseHost protected query on the host-service router. Takes an optional absolute or ~-prefixed path (defaults to homedir()), normalizes it, and returns { path, parentPath, homePath, entries[] } with entries sorted dotfiles-last and folders-first. Hidden files filtered by default; pass includeHidden:true to see them. Unlike the existing listDirectory endpoint, this is not scoped to a workspace — used for setup flows where no workspace exists yet on the host. Path must be absolute or start with ~ for safety; relative paths are rejected. Frontend (renderer/components/RemotePathPicker): - Reusable component that opens a Dialog with a path input (~-aware), Up / Home / Refresh icon buttons, and a scrollable list of subfolders. Single-click highlights a folder in the path input; double-click descends into it. "Use this folder" picks the current path and closes. Reusable across any future flow that needs to pick a path on a host (workspace settings, script paths, etc.). Integration: - SetupProjectModal: the browse button now opens RemotePathPicker for remote hosts and the existing native Electron picker for local hosts, behind the same LuFolderOpen icon. Picked path fills the Clone parent-directory or Import existing-repo input. * refactor(desktop): redesign RemotePathPicker with Linear design Replace the four-button toolbar + editable path input with a clean breadcrumb-driven path picker: - Breadcrumb at top doubles as path display and navigation. Clicking any earlier segment jumps to that level. Last segment is bold. If the path lives under $HOME it starts with a "Home" segment; long paths collapse middle segments with BreadcrumbEllipsis. - Folders below in a borderless scroll area. Single click on a folder descends into it (replaces the earlier double-click pattern, which was easy to miss). Hover state on each row, no outer border. - Refresh moves into a quiet icon button next to the breadcrumb. - Symlink indicator: small LuExternalLink at the row end instead of a text "link" badge. - Empty state shows a muted folder glyph plus "Empty folder" / "No subfolders" message. - Loading state renders skeleton rows that match the rendered row layout (icon + label). - Borders only where they carry weight: under the breadcrumb row and above the footer. The dialog itself drops outer padding and gains per-section padding so the borders span the full dialog width. - Drops the Up / Home / Edit-path toolbar buttons; up-navigation happens via breadcrumb segments and home-navigation via the first "Home" segment. The current path is always visible in the breadcrumb, so no separate read-only path readout is needed. * fix(desktop): address review feedback on PR #4675 Functional fixes: - ProjectLocationSection: the "Change location" pencil button used to open the local native folder picker even when the project was set up on a remote host, which would have produced an invalid relocation path. Now: on local targets it still opens the native picker; on remote targets it opens the RemotePathPicker against the remote host's filesystem. The relocate flow (conflict precheck + relocate confirm dialog) is unchanged. - V2ScriptsEditor/ScriptField: add `pb-8 pr-20` to the textarea so the absolute-positioned Import button at the bottom-right can no longer overlap typed content. - ClickablePath: the truncate span needs `min-w-0` when its parent is a flex container, otherwise the ellipsis never kicks in. Structural fixes (one-component-per-file rule from AGENTS.md): - Extract `SettingsRow` from V2ProjectSettings.tsx into its own SettingsRow/ folder with an index re-export. V2ProjectSettings.tsx imports it. - Extract `ScriptField` from V2ScriptsEditor.tsx into a co-located components/ScriptField/ folder. V2ScriptsEditor.tsx imports it. (Co-located under V2ScriptsEditor since it's only used by that editor.)
This was referenced May 20, 2026
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 25, 2026
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
Stacked on top of #4665 (merged as
ca7ad95e5). Two commits:feat(desktop): polish v2 project settings to Linear-style layout— restructures the v2 project settings page to match the canonicalSettingsRowpattern used inOrganizationSettings: flat list offlex items-center justify-between gap-8 py-2.5rows, uniformw-96controls, no inner borders or card chrome. Project icon moves into the page header (click thumbnail → dropdown menu for Upload / Use GitHub icon / Remove). Host scope chip rendered in the header. Repository and Location each render a singlew-96field with a ghost icon button absolutely positioned inside (GitHub icon for Open in GitHub, folder icon for Change location). Location path usesoverflow-x-auto whitespace-nowrapso long paths scroll horizontally inside the field. Scripts editor uses underline-style tabs (no pill chrome, hover transition), a skeleton-shaped loading state, and an inline "Import" button tucked into the textarea corner. All input text normalized totext-sm.fix: address review feedback on PR #4665— three items flagged bygreptile-apps,cubic-dev-ai, andcoderabbitaion the original PR:cli/projects list: gracefully degrade when the host service can't be reached and the user did not pass--host/--local. Falls back tosetUp="?"/path="-"instead of throwing, restoring the previously API-only listing behavior. Still throws when--hostor--localwas passed explicitly. (P1 from greptile-apps + cubic-dev-ai.)cli/projects setup: reject "Project ID specified twice" whenever both--projectand the positional argument are present, even when the values match. (coderabbitai.)ProjectLocationSection.tsx: addselect-text cursor-textto bothAlertDialogDescriptionblocks so users can copy the conflict project name and the From/To paths into bug reports. Aligns with the renderer-wide "error text must be selectable" rule fromapps/desktop/AGENTS.md. (coderabbitai.)Test plan
Clone here…/Import existing…buttons (and the remote-host variant with the two input rows).?hostId=…updates.superset projects listwith no host service running and no--host/--localflag — lists projects withsetUp="?"/path="-"instead of erroring.superset projects list --localwith no host service running — errors with the resolveHostTarget message (expected).superset projects setup <id> --project <id>— errors with "Project ID specified twice" even though the values are identical.Summary by cubic
Polished the v2 Project Settings with a flatter layout, a header icon menu, a multi-host selector, a first-time setup modal, and a breadcrumb-based remote folder picker. Also addressed review feedback with fixes to remote “Change location,” scripts editor UI, and CLI edge cases.
New Features
SettingsRowlayout with uniform w-96 controls.?hostId.ClickablePathsupports truncation.RemotePathPickerwith breadcrumb navigation (Home-aware, ellipsis for long paths, symlink indicator, refresh, skeleton rows), backed byhost-servicefilesystem.browseHost(absolute or~paths; hidden files filtered).Bug Fixes
RemotePathPicker(no more local picker on remote targets).ClickablePathtruncation works reliably in flex layouts.projects list: when the host service is unreachable and no--host/--localis passed, degrade tosetUp="?"andpath="-"; still errors when a host is explicitly requested.projects setup: now errors when both a positional ID and--projectare provided, even if they match.Written for commit 78f75f8. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
UI