feat(desktop): configurable link-click behavior in v2#3600
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds preference-driven link-action mappings (plain/meta/meta+shift), persisted v2UserPreferences, hooks to resolve inline/terminal link actions, settings UI to edit mappings, and wiring to control link clicks across MarkdownEditor, TerminalPane, sidebar reveal, and external editor/browser flows. Changes
sequenceDiagram
participant User
participant Renderer as "Markdown/Terminal"
participant LinkActions as "LinkActionsApi"
participant Prefs as "PreferencesStore"
participant App as "AppHandler"
participant External as "ExternalAgent"
User->>Renderer: click link (with modifiers)
Renderer->>LinkActions: getFileAction(event) / getUrlAction(event)
LinkActions->>Prefs: read fileLinks / urlLinks
Prefs-->>LinkActions: return LinkTierMap
LinkActions-->>Renderer: return LinkAction ("pane" / "external" / null)
alt action == "external"
Renderer->>App: request external open (editor/browser)
App->>External: electronTrpcClient / system open
else action == "pane"
Renderer->>App: reveal/open in-app (may pass { isDirectory: true })
App->>Renderer: reveal result / open pane
else action == null
Renderer->>User: show "Not bound · configure in Settings → Links" hint
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Greptile SummaryThis PR adds a configurable link-click behavior system for v2 of the desktop app. It introduces a Settings → Links page where users can map three click tiers (plain, Key changes:
Confidence Score: 4/5PR is safe to merge; one P2 logic nuance in MarkdownEditor and one minor callback memoization miss do not block the primary user path. The overall architecture is clean and well-layered (schema → collection → hook → UI). The tier-demotion logic, the isDirectory reveal hint, and the settings plumbing are all correct. Two non-critical issues were found: (1) the handleClickOn handler unconditionally consumes link-click events even when the action is null, which prevents cursor placement on link text in the editing surface; (2) the onPendingRevealConsumed callback is not memoized, causing FilesTab's reveal effect to re-run on every WorkspaceContent render. apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx — handleClickOn event-consumption logic; apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx — onPendingRevealConsumed memoization.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx | Adds handleClickOn to intercept link clicks; unconditionally consumes the event for all anchor clicks, preventing cursor placement even when action is null. |
| apps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.ts | New hook; upsert logic for the singleton preferences row is correct; uses TanStack DB live query and insert/update correctly. |
| apps/desktop/src/renderer/hooks/useV2UserPreferences/useLinkActions.ts | Clean 2-hook abstraction (terminal 3-tier / inline 2-tier); memoized callbacks key on stable preference object references. |
| apps/desktop/src/renderer/hooks/useV2UserPreferences/tiers.ts | Tier-resolution helpers look correct; actionFor is exported but not used internally (harmless). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | pendingRevealDirectory state threads the isDirectory hint correctly; onPendingRevealConsumed callback is not memoized causing needless FilesTab effect re-runs. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx | pendingRevealDirectory propagation and reveal-with-isDirectory logic is sound; async onPendingRevealConsumed call after reveal resolves is safe. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | File and URL link dispatch correctly routes through tier map; folder clicks remain hardcoded as intended; getFileAction/getUrlAction added to deps array. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | New schema definitions (LinkAction, LinkTier, LinkTierMap, V2UserPreferencesRow) are well-structured with Zod defaults and a singleton id literal. |
| apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx | Demotion logic (demote conflicting tier to null rather than swap) is correct; Select/Label IDs are unique via idPrefix. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx | Always-visible Open in editor button added correctly; existing FileViewToggle is now conditionally rendered instead of early-returning null. |
| apps/desktop/src/renderer/hooks/host-service/useFileTree/useFileTree.ts | reveal signature extended with optional isDirectory hint; logic correctly falls back to entriesByPath lookup when the hint is not provided. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User clicks link]) --> B{Surface?}
B --> C[TerminalPane]
B --> D[MarkdownEditor]
C --> E{Link type?}
E --> F[Folder]
E --> G[File]
E --> H[URL]
F --> F1{Modifier?}
F1 -->|none| F2[Show hint tooltip]
F1 -->|⌘| F3[revealPath + sidebar expand]
F1 -->|⌘⇧| F4[External editor]
G --> G1[getFileAction via useTerminalLinkActions]
G1 -->|null| G2[Show hint tooltip]
G1 -->|pane| G3[Open in FilePane]
G1 -->|external| G4[Open in external editor]
H --> H1[getUrlAction via useTerminalLinkActions]
H1 -->|null| H2[Show hint tooltip]
H1 -->|pane| H3[Open in BrowserPane]
H1 -->|external| H4[Open in system browser]
D --> I[getUrlAction via useInlineLinkActions plain or meta tier only]
I -->|null| I1[Do nothing - cursor placement blocked]
I -->|pane or external| I2[Open in system browser no pane context]
subgraph Settings
J[Settings Links page] --> K[LinkTierMapper File links]
J --> L[LinkTierMapper URL links]
K --> M[(v2UserPreferences localStorage)]
L --> M
end
M --> G1
M --> H1
M --> I
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx
Line: 281-296
Comment:
**`return true` prevents cursor placement on links regardless of action**
`handleClickOn` always calls `event.preventDefault()` and returns `true` for any anchor element with an `href`. When `getUrlAction(event)` returns `null` ("do nothing"), the event is fully consumed — TipTap won't place the cursor at the clicked position and the browser won't follow the link. This means users can no longer single-click on link text to position their cursor there for editing.
In an editing surface (task detail, chat input), cursor-on-link is often needed to select or modify the link. With the current code, keyboard navigation or approaching from adjacent text becomes the only option.
A minimal fix is to skip `preventDefault` and return `false` (let TipTap handle normally) when `getUrlAction` is null:
```
handleClickOn: (_view, _pos, _node, _nodePos, event) => {
const target = event.target as HTMLElement | null;
const anchor = target?.closest?.("a") as HTMLAnchorElement | null;
if (!anchor) return false;
const href = anchor.getAttribute("href");
if (!href) return false;
const action = getUrlAction(event);
if (action === null) return false; // let TipTap place the cursor
event.preventDefault();
electronTrpcClient.external.openUrl.mutate(href).catch((error) => {
console.error("[MarkdownEditor] Failed to open URL:", href, error);
});
return true;
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Line: 485
Comment:
**`onPendingRevealConsumed` not memoized — unnecessary effect re-runs**
The inline arrow function `() => setPendingRevealDirectory(null)` is recreated on every render of `WorkspaceContent`. Because `onPendingRevealConsumed` is in the dependency array of `FilesTab`'s reveal `useEffect`, that effect re-runs on every parent render. The guard `selectedFilePath !== prevSelectedRef.current` prevents a real reveal from firing twice, but the effect still executes and updates the ref on each render.
```suggestion
onPendingRevealConsumed={onPendingRevealConsumed}
```
And in the component body alongside `revealPath`:
```
const onPendingRevealConsumed = useCallback(
() => setPendingRevealDirectory(null),
[],
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): configurable link-click b..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+281
to
+296
| handleClickOn: (_view, _pos, _node, _nodePos, event) => { | ||
| const target = event.target as HTMLElement | null; | ||
| const anchor = target?.closest?.("a") as HTMLAnchorElement | null; | ||
| if (!anchor) return false; | ||
| const href = anchor.getAttribute("href"); | ||
| if (!href) return false; | ||
| event.preventDefault(); | ||
| // No pane context available here, so "pane" and "external" both | ||
| // route to the system browser. Null means do nothing. | ||
| if (getUrlAction(event) !== null) { | ||
| electronTrpcClient.external.openUrl.mutate(href).catch((error) => { | ||
| console.error("[MarkdownEditor] Failed to open URL:", href, error); | ||
| }); | ||
| } | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
return true prevents cursor placement on links regardless of action
handleClickOn always calls event.preventDefault() and returns true for any anchor element with an href. When getUrlAction(event) returns null ("do nothing"), the event is fully consumed — TipTap won't place the cursor at the clicked position and the browser won't follow the link. This means users can no longer single-click on link text to position their cursor there for editing.
In an editing surface (task detail, chat input), cursor-on-link is often needed to select or modify the link. With the current code, keyboard navigation or approaching from adjacent text becomes the only option.
A minimal fix is to skip preventDefault and return false (let TipTap handle normally) when getUrlAction is null:
handleClickOn: (_view, _pos, _node, _nodePos, event) => {
const target = event.target as HTMLElement | null;
const anchor = target?.closest?.("a") as HTMLAnchorElement | null;
if (!anchor) return false;
const href = anchor.getAttribute("href");
if (!href) return false;
const action = getUrlAction(event);
if (action === null) return false; // let TipTap place the cursor
event.preventDefault();
electronTrpcClient.external.openUrl.mutate(href).catch((error) => {
console.error("[MarkdownEditor] Failed to open URL:", href, error);
});
return true;
},
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx
Line: 281-296
Comment:
**`return true` prevents cursor placement on links regardless of action**
`handleClickOn` always calls `event.preventDefault()` and returns `true` for any anchor element with an `href`. When `getUrlAction(event)` returns `null` ("do nothing"), the event is fully consumed — TipTap won't place the cursor at the clicked position and the browser won't follow the link. This means users can no longer single-click on link text to position their cursor there for editing.
In an editing surface (task detail, chat input), cursor-on-link is often needed to select or modify the link. With the current code, keyboard navigation or approaching from adjacent text becomes the only option.
A minimal fix is to skip `preventDefault` and return `false` (let TipTap handle normally) when `getUrlAction` is null:
```
handleClickOn: (_view, _pos, _node, _nodePos, event) => {
const target = event.target as HTMLElement | null;
const anchor = target?.closest?.("a") as HTMLAnchorElement | null;
if (!anchor) return false;
const href = anchor.getAttribute("href");
if (!href) return false;
const action = getUrlAction(event);
if (action === null) return false; // let TipTap place the cursor
event.preventDefault();
electronTrpcClient.external.openUrl.mutate(href).catch((error) => {
console.error("[MarkdownEditor] Failed to open URL:", href, error);
});
return true;
},
```
How can I resolve this? If you propose a fix, please make it concise.| onSearch={handleQuickOpen} | ||
| selectedFilePath={selectedFilePath} | ||
| pendingRevealDirectory={pendingRevealDirectory} | ||
| onPendingRevealConsumed={() => setPendingRevealDirectory(null)} |
There was a problem hiding this comment.
onPendingRevealConsumed not memoized — unnecessary effect re-runs
The inline arrow function () => setPendingRevealDirectory(null) is recreated on every render of WorkspaceContent. Because onPendingRevealConsumed is in the dependency array of FilesTab's reveal useEffect, that effect re-runs on every parent render. The guard selectedFilePath !== prevSelectedRef.current prevents a real reveal from firing twice, but the effect still executes and updates the ref on each render.
Suggested change
| onPendingRevealConsumed={() => setPendingRevealDirectory(null)} | |
| onPendingRevealConsumed={onPendingRevealConsumed} |
And in the component body alongside revealPath:
const onPendingRevealConsumed = useCallback(
() => setPendingRevealDirectory(null),
[],
);
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Line: 485
Comment:
**`onPendingRevealConsumed` not memoized — unnecessary effect re-runs**
The inline arrow function `() => setPendingRevealDirectory(null)` is recreated on every render of `WorkspaceContent`. Because `onPendingRevealConsumed` is in the dependency array of `FilesTab`'s reveal `useEffect`, that effect re-runs on every parent render. The guard `selectedFilePath !== prevSelectedRef.current` prevents a real reveal from firing twice, but the effect still executes and updates the ref on each render.
```suggestion
onPendingRevealConsumed={onPendingRevealConsumed}
```
And in the component body alongside `revealPath`:
```
const onPendingRevealConsumed = useCallback(
() => setPendingRevealDirectory(null),
[],
);
```
How can I resolve this? If you propose a fix, please make it concise.![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 (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx (1)
18-24:⚠️ Potential issue | 🟡 MinorMake modifier-hover labels preference-aware.
With configurable link actions, Line 39 can still show hardcoded labels like “Open in pane” or “Open in external editor” even when the user bound that tier to another action or “Do nothing”. Please derive this label from the resolved file/URL action instead of
shiftalone.Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx around lines 18 - 24, The getLabel function is using hardcoded strings based solely on shift and info.kind; change it to derive the modifier-hover label from the resolved action for that link instead. Locate getLabel(LinkHoverInfo, shift) and replace the branch logic to first compute the effective action (e.g., call the existing resolver/util that maps a LinkHoverInfo + modifier to a File/URL action or "do nothing"), then return a label based on that resolved action (for example: resolvedAction === "openInExternalBrowser" -> "Open in external browser", "openInPane" -> "Open in pane", "openInExternalEditor" -> "Open in external editor", "revealInSidebar" -> "Reveal in sidebar", "doNothing" -> appropriate label). Ensure you still consider info.isDirectory for reveal vs open when the resolver returns a generic "open" action and include shift only as an input to the resolver, not as the final label source.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
35-39:⚠️ Potential issue | 🟡 MinorUpdate
UsePaneRegistryOptions.onRevealPathto accept the optional directory hint.
UsePaneRegistryOptions.onRevealPathatapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:143is typed as(path: string) => void, butTerminalPaneat line 177 calls it withonRevealPath(link.resolvedPath, { isDirectory: true }). Update the interface toonRevealPath: (path: string, options?: { isDirectory?: boolean }) => voidto align with the expected contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 35 - 39, Update the UsePaneRegistryOptions type so onRevealPath accepts an optional options object with the directory hint; change its signature from (path: string) => void to onRevealPath: (path: string, options?: { isDirectory?: boolean }) => void, so calls from TerminalPane (which invokes onRevealPath(link.resolvedPath, { isDirectory: true })) type-check correctly; locate and update the UsePaneRegistryOptions interface and any exported usages in usePaneRegistry to match the new signature and adjust consumers where necessary.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx (1)
40-49: Redundant guard in JSX.
shouldShowTogglealready includesactiveView !== null, so the extra&& activeViewon line 49 is redundant (though it does serve as a type narrower for TS). If the goal is narrowing, keeping it is fine; otherwise it can be simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx around lines 40 - 49, The JSX has a redundant runtime guard: shouldShowToggle already includes activeView !== null, so in FilePaneHeaderExtras.tsx update the conditional render expression in the return from "{shouldShowToggle && activeView && ( ... )}" to just "{shouldShowToggle && ( ... )}" to remove the duplicate check; if you intended TypeScript narrowing instead, alternatively ensure shouldShowToggle's type reflects non-null activeView so the extra "&& activeView" isn't needed.
🤖 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/MarkdownEditor/MarkdownEditor.tsx`:
- Around line 281-296: The handler handleClickOn currently calls
event.preventDefault() and always returns true even when getUrlAction(event) ===
null, which swallows clicks and prevents TipTap's default selection (cursor
placement) inside links; fix by checking getUrlAction(event) first, and only
call event.preventDefault() and invoke
electronTrpcClient.external.openUrl.mutate(href) when getUrlAction(event) !==
null, otherwise return false (do not preventDefault) so the click falls through
to TipTap's default behavior; update the handler logic around getUrlAction,
anchor, href, and the return value accordingly.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx:
- Around line 57-70: The icon-only button inside FilePaneHeaderExtras (the
button wrapped by TooltipTrigger that calls handleOpenExternal) lacks an
accessible name; add an explicit aria-label (e.g., aria-label="Open in editor")
or aria-labelledby on that button element so screen readers announce it,
ensuring the label matches the TooltipContent text and preserves existing
onClick/props and classes; update the button in the FilePaneHeaderExtras
component where Tooltip and TooltipTrigger are used to include this attribute.
- Around line 43-45: The toolbar currently always renders the external-editor
button and calls openInExternalEditor via handleOpenExternal, which causes a
toast error for remote workspaces; update the FilePaneHeaderExtras component to
detect workspace locality (use the existing workspace/locality flag or hook used
elsewhere, e.g., isLocalWorkspace or workspace.host comparison) and
conditionally disable or hide the button when the workspace is remote, and if
you keep it visible add a tooltip explaining why it’s disabled; ensure
handleOpenExternal and the dependency array ([filePath, openInExternalEditor])
remain but never invoked for remote workspaces so the toast error is avoided.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`:
- Around line 222-226: Add a .superRefine to linkTierMapSchema that enforces
uniqueness of non-null link actions across the three keys (plain, meta,
metaShift) so duplicates can't be saved outside the UI; inside the superRefine
callback iterate the entries of the parsed object (referencing linkTierMapSchema
and the keys plain/meta/metaShift), collect non-null actions using a stable
identity (e.g., an id field if present or JSON.stringify as a fallback), detect
any duplicate identities, and call ctx.addIssue for each offending key (or add a
single issue on the root) to fail validation when the same action appears in
more than one tier—this mirrors the uniqueness enforced in LinkTierMapper.tsx
and prevents invalid localStorage/imported state.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx:
- Around line 18-24: The getLabel function is using hardcoded strings based
solely on shift and info.kind; change it to derive the modifier-hover label from
the resolved action for that link instead. Locate getLabel(LinkHoverInfo, shift)
and replace the branch logic to first compute the effective action (e.g., call
the existing resolver/util that maps a LinkHoverInfo + modifier to a File/URL
action or "do nothing"), then return a label based on that resolved action (for
example: resolvedAction === "openInExternalBrowser" -> "Open in external
browser", "openInPane" -> "Open in pane", "openInExternalEditor" -> "Open in
external editor", "revealInSidebar" -> "Reveal in sidebar", "doNothing" ->
appropriate label). Ensure you still consider info.isDirectory for reveal vs
open when the resolver returns a generic "open" action and include shift only as
an input to the resolver, not as the final label source.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 35-39: Update the UsePaneRegistryOptions type so onRevealPath
accepts an optional options object with the directory hint; change its signature
from (path: string) => void to onRevealPath: (path: string, options?: {
isDirectory?: boolean }) => void, so calls from TerminalPane (which invokes
onRevealPath(link.resolvedPath, { isDirectory: true })) type-check correctly;
locate and update the UsePaneRegistryOptions interface and any exported usages
in usePaneRegistry to match the new signature and adjust consumers where
necessary.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx:
- Around line 40-49: The JSX has a redundant runtime guard: shouldShowToggle
already includes activeView !== null, so in FilePaneHeaderExtras.tsx update the
conditional render expression in the return from "{shouldShowToggle &&
activeView && ( ... )}" to just "{shouldShowToggle && ( ... )}" to remove the
duplicate check; if you intended TypeScript narrowing instead, alternatively
ensure shouldShowToggle's type reflects non-null activeView so the extra "&&
activeView" isn't needed.
🪄 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: c016cb8a-aa4e-425f-b88f-145cd6fbd204
📒 Files selected for processing (23)
apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsxapps/desktop/src/renderer/hooks/host-service/useFileTree/useFileTree.tsapps/desktop/src/renderer/hooks/useV2UserPreferences/index.tsapps/desktop/src/renderer/hooks/useV2UserPreferences/tiers.tsapps/desktop/src/renderer/hooks/useV2UserPreferences/useLinkActions.tsapps/desktop/src/renderer/hooks/useV2UserPreferences/useV2UserPreferences.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsxapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinksSettings/LinksSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinksSettings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/links/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/stores/settings-state.ts
Comment on lines
+43
to
+45
| const handleOpenExternal = useCallback(() => { | ||
| openInExternalEditor(filePath); | ||
| }, [filePath, openInExternalEditor]); |
Contributor
There was a problem hiding this comment.
Consider disabling the button on remote workspaces.
useOpenInExternalEditor short-circuits with a toast.error("Can't open remote workspace paths in an external editor") when the workspace host machine doesn't match the local machine. The toolbar button is always rendered, so users on remote workspaces will get an error toast every click instead of a clearly unavailable affordance. Consider hiding/disabling the button (or surfacing the reason via the tooltip) when the workspace is non-local. If this is intentional — e.g. locality isn't reliably known at render time and the toast is the designed fallback — feel free to ignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsx
around lines 43 - 45, The toolbar currently always renders the external-editor
button and calls openInExternalEditor via handleOpenExternal, which causes a
toast error for remote workspaces; update the FilePaneHeaderExtras component to
detect workspace locality (use the existing workspace/locality flag or hook used
elsewhere, e.g., isLocalWorkspace or workspace.host comparison) and
conditionally disable or hide the button when the workspace is remote, and if
you keep it visible add a tooltip explaining why it’s disabled; ensure
handleOpenExternal and the dependency array ([filePath, openInExternalEditor])
remain but never invoked for remote workspaces so the toast error is avoided.
Comment on lines
+222
to
+226
| const linkTierMapSchema = z.object({ | ||
| plain: linkActionSchema.nullable(), | ||
| meta: linkActionSchema.nullable(), | ||
| metaShift: linkActionSchema.nullable(), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Zod object-level validation is already available/used in the repo.
rg -n -C2 --type ts --type tsx '\.superRefine\s*\('
rg -n -C2 --type json '"zod"\s*:'Repository: superset-sh/superset
Length of output: 4812
🏁 Script executed:
# Read the schema file to see the full context around lines 222-226
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | head -260 | tail -70Repository: superset-sh/superset
Length of output: 3101
🏁 Script executed:
# Search for LinkAction and LinkTier type definitions
rg -n -A3 'type LinkAction|interface LinkAction|export.*LinkAction' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/Repository: superset-sh/superset
Length of output: 653
🏁 Script executed:
# Search for LinkTierMapper to understand the deduplication logic
rg -n -A10 'LinkTierMapper|demote.*duplicate' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if .superRefine is used anywhere in the codebase (with corrected regex)
rg -n '\.superRefine' apps/desktop/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for LinkTierMapper more broadly (not just in that directory)
rg -n 'LinkTierMapper' apps/desktop/Repository: superset-sh/superset
Length of output: 1241
🏁 Script executed:
# Search for files that might import or use linkTierMapSchema
rg -n 'linkTierMapSchema|LinkTierMap' apps/desktop/src/renderer/ -t ts -t tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check for any existing .superRefine usage anywhere in apps/desktop
rg -n 'superRefine' apps/desktop/ -t ts -t tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look for any validation related to duplicate actions or link tier deduplication
rg -n 'duplicate|demote|unique.*action' apps/desktop/src/renderer/ -t ts -t tsx -iRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Read the LinkTierMapper component to understand the deduplication logic
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx | head -150Repository: superset-sh/superset
Length of output: 3832
🏁 Script executed:
# Search for .superRefine usage without specifying file type
find apps/desktop -name "*.ts" -o -name "*.tsx" | xargs grep -l "superRefine" 2>/dev/nullRepository: superset-sh/superset
Length of output: 48
🏁 Script executed:
# Look for any validation logic or deduplication in the LinkTierMapper context
grep -r "duplicate\|demote" apps/desktop/src/renderer/routes/_authenticated/settings/links/ 2>/dev/nullRepository: superset-sh/superset
Length of output: 246
🏁 Script executed:
# Check what imports linkTierMapSchema uses it
grep -r "linkTierMapSchema\|fileLinks\|urlLinks" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/ --include="*.ts" --include="*.tsx" 2>/dev/nullRepository: superset-sh/superset
Length of output: 1192
Add schema validation to prevent duplicate link actions across tiers.
The LinkTierMapper component enforces action uniqueness in the UI (lines 59–68 of LinkTierMapper.tsx), demoting any duplicate actions to null when a user interacts with the settings. However, the linkTierMapSchema has no validation, so data written directly to localStorage, imported, or passed via non-UI paths can bypass this constraint and persist duplicate actions.
Add a .superRefine check to the schema to own this invariant at the boundary:
Suggested validation
-const linkTierMapSchema = z.object({
+const linkTierMapSchema = z
+ .object({
plain: linkActionSchema.nullable(),
meta: linkActionSchema.nullable(),
metaShift: linkActionSchema.nullable(),
-});
+ })
+ .superRefine((map, ctx) => {
+ const seen = new Map<LinkAction, LinkTier>();
+ for (const tier of ["plain", "meta", "metaShift"] as const) {
+ const action = map[tier];
+ if (action === null) continue;
+ const previousTier = seen.get(action);
+ if (previousTier) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: [tier],
+ message: `${action} is already assigned to ${previousTier}`,
+ });
+ continue;
+ }
+ seen.set(action, tier);
+ }
+ });📝 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
| const linkTierMapSchema = z.object({ | |
| plain: linkActionSchema.nullable(), | |
| meta: linkActionSchema.nullable(), | |
| metaShift: linkActionSchema.nullable(), | |
| }); | |
| const linkTierMapSchema = z | |
| .object({ | |
| plain: linkActionSchema.nullable(), | |
| meta: linkActionSchema.nullable(), | |
| metaShift: linkActionSchema.nullable(), | |
| }) | |
| .superRefine((map, ctx) => { | |
| const seen = new Map<LinkAction, LinkTier>(); | |
| for (const tier of ["plain", "meta", "metaShift"] as const) { | |
| const action = map[tier]; | |
| if (action === null) continue; | |
| const previousTier = seen.get(action); | |
| if (previousTier) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: [tier], | |
| message: `${action} is already assigned to ${previousTier}`, | |
| }); | |
| continue; | |
| } | |
| seen.set(action, tier); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`
around lines 222 - 226, Add a .superRefine to linkTierMapSchema that enforces
uniqueness of non-null link actions across the three keys (plain, meta,
metaShift) so duplicates can't be saved outside the UI; inside the superRefine
callback iterate the entries of the parsed object (referencing linkTierMapSchema
and the keys plain/meta/metaShift), collect non-null actions using a stable
identity (e.g., an id field if present or JSON.stringify as a fallback), detect
any duplicate identities, and call ctx.addIssue for each offending key (or add a
single issue on the root) to fail validation when the same action appears in
more than one tier—this mirrors the uniqueness enforced in LinkTierMapper.tsx
and prevents invalid localStorage/imported state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 23 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/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:484">
P2: Directory reveal can be skipped when the Files tab mounts in the same update.</violation>
</file>
<file name="apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/MarkdownEditor/MarkdownEditor.tsx:287">
P1: When `getUrlAction(event)` returns `null` ("do nothing"), this handler still calls `event.preventDefault()` and returns `true`, which prevents TipTap/ProseMirror from placing the cursor at the click position. Users will be unable to click on link text to position their cursor for editing. Move `event.preventDefault()` after the null check and return `false` when the action is null to let ProseMirror handle selection normally.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Adds Settings → Links with two 3-tier mappers (File / URL) letting users bind "Click", "⌘-click", and "⌘⇧-click" to one of: Do nothing, in-app pane (File viewer / In-app browser), or external (editor / browser). Picking an action already bound to another tier demotes that other tier to "Do nothing" rather than swapping, so the three tiers stay distinct without silently inheriting a meaning the user didn't choose. Preferences live in a new `v2_user_preferences` localStorage collection keyed by organizationId — a v2-native store, not the v1 `settings` table. Three hooks expose the data: `useV2UserPreferences`, `useTerminalLinkActions` (3-tier, terminal), and `useInlineLinkActions` (2-tier, chat / markdown). Wired consumers: - v2 TerminalPane — file + URL clicks dispatch through the tier map; folder clicks stay hardcoded (⌘ reveals, ⌘⇧ external) since there's only one sensible primary for folders. - TaskMarkdownRenderer — URLs previously inert now dispatch; tasks have no pane context so "pane" and "external" both open the system browser. - FilePaneHeaderExtras — new "Open in editor" toolbar button as an always-available escape hatch. Keyboard settings moved to the Editor & Workflow group. Folder reveal threads an `isDirectory: true` hint from TerminalPane through page.tsx → WorkspaceSidebar → FilesTab → `useFileTree.reveal` so a known-directory target gets expanded even when the terminal's resolved path and listDirectory's child paths don't byte-match (case differences, symlink resolution). Click-hint tooltip updated from the hardcoded "Hold ⌘ to open · ⌘⇧ for external" to "Not bound · configure in Settings → Links" so it reflects user-configurable reality instead of defaults.
saddlepaddle
force-pushed
the
settings-switch-links
branch
from
ad75065 to
bf3c52a
Compare
`openPane` calls `replacePane` whenever an existing unpinned pane of the same kind is present in the active tab. `replacePane` was only updating `activePaneId` if the *replaced* pane was already the active one — otherwise it preserved the existing active pane. In practice that meant user-initiated actions that reused a preview pane (⌘-click a terminal file link when a FilePane was already open, for example) would swap the file in but keep focus on the terminal. URL clicks happen to escape this by taking the `splitPane` branch when no browser pane exists yet — but the same bug would surface the second time a different URL was clicked. replacePane is internal to the panes store and only called by openPane, whose contract is "the user asked for this, make it visible and focused." Always activate the new pane. Existing tests still pass.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
`pendingRevealDirectory` was tracked as a single string, so a second
⌘-click on a collapsed folder — same path as the prior reveal — didn't
re-run the FilesTab reveal effect, leaving the folder collapsed. Switch
to a `{ path, isDirectory, nonce }` shape; the nonce bumps on every
reveal request, and the effect depends on the nonce so repeat reveals
always re-expand and re-scroll. Active file pane switches bump the
nonce too so file-pane changes still drive the tree into view.
Terminal hover tooltip read hardcoded labels from the old default
mapping (⌘ = pane, ⌘⇧ = external). Rebinding external to plain left
⌘⇧ hover still saying "Open in external editor". Now the tooltip reads
the user's live fileLinks / urlLinks mapping and derives the label
from whichever action is bound at the hovered tier. Unbound tiers
return null, so no tooltip renders. Folder hover stays hardcoded
(⌘ = Reveal in sidebar, ⌘⇧ = Open in editor) since folder behavior
itself isn't settings-driven.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:
- Around line 329-335: The async reveal in fileTree.reveal can complete
out-of-order and cause a stale scroll; tie the reveal/scroll to a pendingReveal
token and guard the scheduled scroll by verifying the token is still current
before calling querySelector/scrollIntoView. Specifically, when starting the
reveal (in the same block that calls fileTree.reveal), capture the current
pendingReveal identifier (e.g., pendingReveal or pendingRevealRef.current), and
in the .then and inside requestAnimationFrame check that pendingReveal still
equals that captured value (and that the component is mounted) before running
scrollContainerRef.current?.querySelector(...)? .scrollIntoView(...); also clear
or update pendingReveal on cleanup/unmount to prevent stale resolves from
acting.
🪄 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: b284e7e0-2adf-42e4-8607-1943414286e4
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/LinkHoverTooltip/LinkHoverTooltip.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:329">
P2: This async reveal has no cancellation guard — if a new `pendingReveal` nonce fires while a previous `fileTree.reveal()` is still in flight, the stale promise callback will scroll the sidebar back to the old path. Return a cleanup function from the effect that sets a `cancelled` flag, and check it before the `requestAnimationFrame` scroll.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:336">
P1: The reveal effect depends on the whole `fileTree` object, which is recreated each render, so a single pending reveal can retrigger continuously. Depend on a stable function reference instead (e.g. `fileTree.reveal`) to avoid repeated reveal/scroll cycles.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- FilesTab reveal effect: drop fileTree/rootPath from deps. fileTree's identity changes every render, so the nonce-keyed effect was firing every render → setState → re-render → infinite loop. Closure captures the latest values via useFileTree's internal refs. - UrlLinkProvider.handleActivation: forward every activation instead of gating on ⌘/ctrl internally. v2's settings layer maps "plain click" to an action, so the hardcoded modifier gate was eating clicks before v2 could see them. v1 consumer (Terminal/helpers.ts) now enforces its own gate + preventDefault so v1 behavior is unchanged. - LinkTierMapper: drop demote-on-conflict. Each tier picks independently; duplicates are fine (e.g. all three tiers bound to "external editor").
Contributor
There was a problem hiding this comment.
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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
86-86:⚠️ Potential issue | 🟠 MajorForward the mouse event to the URL preference handler before legacy gating.
Line 194 drops plain-click URL activations, so a configured
"plain"URL action can never fire. Also, Line 198 only passesuri, which prevents the v2 handler from distinguishingmetavsmetaShift.🐛 Proposed fix
- onUrlClickRef?: { current: ((url: string) => void) | undefined }; + onUrlClickRef?: { current: ((event: MouseEvent, url: string) => void) | undefined };onUrlClick: (event, uri) => { - if (!event.metaKey && !event.ctrlKey) return; - event.preventDefault(); const handler = urlClickRef?.current; if (handler) { - handler(uri); + event.preventDefault(); + handler(event, uri); return; } + if (!event.metaKey && !event.ctrlKey) return; + event.preventDefault(); trpcClient.external.openUrl.mutate(uri).catch((error) => {Also applies to: 193-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts` at line 86, The URL click logic currently drops plain-click activations and only forwards uri, so update the handlers to invoke onUrlClickRef.current (when defined) with the full mouse event before executing the legacy gating logic and fallback behavior; pass both the MouseEvent and the uri so the v2 handler can distinguish modifiers (e.g., meta vs meta+shift). Apply this change for the plain-click branch and the other URL-activation branch (both places referencing onUrlClickRef) and ensure you short-circuit only if the v2 handler returns a consumed result or indicates it handled the event.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx (1)
327-338:⚠️ Potential issue | 🟡 MinorStale reveal can overwrite a newer scroll.
The async
fileTree.reveal(...).then(...)isn't guarded, so if a secondpendingRevealarrives before the first resolves, the older resolution can runscrollIntoViewafter the newer one, landing the sidebar on the stale path. Also, a rejectedrevealpromise is swallowed silently. Capture the current nonce (or use ancancelledflag in the effect cleanup) before scheduling the scroll, and attach a.catchto surface failures.Proposed fix
useEffect(() => { prevSelectedRef.current = selectedFilePath; if (!pendingReveal || !rootPath) return; const { path, isDirectory } = pendingReveal; - void fileTree.reveal(path, { isDirectory }).then(() => { - requestAnimationFrame(() => { - scrollContainerRef.current - ?.querySelector(`[data-filepath="${CSS.escape(path)}"]`) - ?.scrollIntoView({ block: "center" }); - }); - }); + let cancelled = false; + void fileTree + .reveal(path, { isDirectory }) + .then(() => { + if (cancelled) return; + requestAnimationFrame(() => { + if (cancelled) return; + scrollContainerRef.current + ?.querySelector(`[data-filepath="${CSS.escape(path)}"]`) + ?.scrollIntoView({ block: "center" }); + }); + }) + .catch((error) => { + if (cancelled) return; + toast.error("Couldn't reveal path", { + description: error instanceof Error ? error.message : String(error), + }); + }); + return () => { + cancelled = true; + }; }, [pendingReveal?.nonce]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx around lines 327 - 338, The reveal effect can run out-of-order and swallow errors; to fix it, capture the current pendingReveal nonce at the top of the useEffect (e.g. const currentNonce = pendingReveal?.nonce) and use that value inside the promise chain (or set a cancelled flag in cleanup) to ignore any resolution whose nonce no longer matches before calling scrollIntoView; also append a .catch to the fileTree.reveal(...) chain and log or surface the error via the existing logger instead of silently swallowing it. Ensure you still reference pendingReveal, fileTree.reveal, scrollContainerRef, CSS.escape and prevSelectedRef as needed and keep the dependency on pendingReveal?.nonce so new reveals cancel/ignore prior completions.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx (1)
296-296: DeadprevSelectedRef— remove it.Per the AI summary, the prior “track previous selection” logic was removed in favor of the nonce-driven trigger.
prevSelectedRefis still declared at line 296 and written at line 328, but it's never read anywhere in the file. Drop the ref and the in-effect assignment to avoid confusing future readers.Proposed fix
- const prevSelectedRef = useRef(selectedFilePath); @@ useEffect(() => { - prevSelectedRef.current = selectedFilePath; if (!pendingReveal || !rootPath) return;Also applies to: 328-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx at line 296, Remove the unused ref prevSelectedRef and its assignment; specifically delete the declaration "const prevSelectedRef = useRef(selectedFilePath);" and the later in-effect write that sets prevSelectedRef.current = selectedFilePath inside the FilesTab component so only the nonce-driven trigger remains (leave selectedFilePath and all other logic intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Line 86: The URL click logic currently drops plain-click activations and only
forwards uri, so update the handlers to invoke onUrlClickRef.current (when
defined) with the full mouse event before executing the legacy gating logic and
fallback behavior; pass both the MouseEvent and the uri so the v2 handler can
distinguish modifiers (e.g., meta vs meta+shift). Apply this change for the
plain-click branch and the other URL-activation branch (both places referencing
onUrlClickRef) and ensure you short-circuit only if the v2 handler returns a
consumed result or indicates it handled the event.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:
- Around line 327-338: The reveal effect can run out-of-order and swallow
errors; to fix it, capture the current pendingReveal nonce at the top of the
useEffect (e.g. const currentNonce = pendingReveal?.nonce) and use that value
inside the promise chain (or set a cancelled flag in cleanup) to ignore any
resolution whose nonce no longer matches before calling scrollIntoView; also
append a .catch to the fileTree.reveal(...) chain and log or surface the error
via the existing logger instead of silently swallowing it. Ensure you still
reference pendingReveal, fileTree.reveal, scrollContainerRef, CSS.escape and
prevSelectedRef as needed and keep the dependency on pendingReveal?.nonce so new
reveals cancel/ignore prior completions.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:
- Line 296: Remove the unused ref prevSelectedRef and its assignment;
specifically delete the declaration "const prevSelectedRef =
useRef(selectedFilePath);" and the later in-effect write that sets
prevSelectedRef.current = selectedFilePath inside the FilesTab component so only
the nonce-driven trigger remains (leave selectedFilePath and all other logic
intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13fa04bd-361a-4701-b6f2-05d7638b1ee6
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsxapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/link-providers/url-link-provider.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/link-providers/url-link-provider.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/link-providers/url-link-provider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx
Contributor
There was a problem hiding this comment.
3 issues found across 5 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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:338">
P2: Removing `rootPath` from this effect’s dependency list can drop reveal requests that fire before the workspace root path is loaded.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx:58">
P1: Re-adding a tier to an action no longer demotes the previous tier to Do nothing, so duplicate bindings are possible.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts:194">
P1: Plain URL clicks are now ignored before the configured handler runs, so terminal links no longer open on a normal click.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| (tier: LinkTier, nextSlot: SlotValue) => { | ||
| const nextAction = fromSlot(nextSlot); | ||
| if (value[tier] === nextAction) return; | ||
| onChange({ ...value, [tier]: nextAction }); |
Contributor
There was a problem hiding this comment.
P1: Re-adding a tier to an action no longer demotes the previous tier to Do nothing, so duplicate bindings are possible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsx, line 58:
<comment>Re-adding a tier to an action no longer demotes the previous tier to Do nothing, so duplicate bindings are possible.</comment>
<file context>
@@ -55,19 +55,7 @@ export function LinkTierMapper({
- }
- }
- onChange(nextMap);
+ onChange({ ...value, [tier]: nextAction });
},
[value, onChange],
</file context>
Suggested change
| onChange({ ...value, [tier]: nextAction }); | |
| const nextMap: LinkTierMap = { ...value, [tier]: nextAction }; | |
| if (nextAction !== null) { | |
| for (const other of Object.keys(value) as LinkTier[]) { | |
| if (other !== tier && value[other] === nextAction) { | |
| nextMap[other] = null; | |
| } | |
| } | |
| } | |
| onChange(nextMap); |
| }, | ||
| onUrlClick: (_event, uri) => { | ||
| onUrlClick: (event, uri) => { | ||
| if (!event.metaKey && !event.ctrlKey) return; |
Contributor
There was a problem hiding this comment.
P1: Plain URL clicks are now ignored before the configured handler runs, so terminal links no longer open on a normal click.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts, line 194:
<comment>Plain URL clicks are now ignored before the configured handler runs, so terminal links no longer open on a normal click.</comment>
<file context>
@@ -190,7 +190,9 @@ export function createTerminalInWrapper(options: CreateTerminalOptions = {}): {
},
- onUrlClick: (_event, uri) => {
+ onUrlClick: (event, uri) => {
+ if (!event.metaKey && !event.ctrlKey) return;
+ event.preventDefault();
const handler = urlClickRef?.current;
</file context>
- MarkdownEditor handleClickOn: when the URL tier resolves to null ("do
nothing"), return false without preventing default. Previously the
handler swallowed every anchor click, so users couldn't click into a
link to position the cursor for editing.
- FilePaneHeaderExtras: add aria-label to the icon-only "Open in editor"
button so screen readers announce it.
- FilesTab reveal effect: add a cancellation guard so a stale reveal
promise doesn't scroll the sidebar back to an outdated path when a
newer nonce fires mid-flight. Put rootPath back in deps so a reveal
fired before the worktree path loads still runs once it arrives;
fileTree stays out of deps to keep the loop fix.
Every setPendingReveal call already produces a new object reference, so keying the FilesTab effect on the whole pendingReveal object re-runs it for repeat reveals of the same path — no counter needed. Also drops the dead prevSelectedRef that lingered from the old selectedFilePath guard.
13 tasks
Kitenite
added a commit
that referenced
this pull request
May 4, 2026
Add v2 project setup section (#3566, #3605, #3606, #3592, #3626, #3632), scheduled agent runs (#3576), Opus 4.7 (#3579), v1 review comments in pane (#3596), configurable v2 link-click (#3600), Copy Branch Name (#3635), safer terminal preset defaults (#3546), and /pricing page (#3639). Expand bug fixes with v2 git correctness, cross-fork PR misattribution, terminal paste/Unicode/Shift+Enter, and security bumps.
Kitenite
added a commit
that referenced
this pull request
May 6, 2026
…-27) (#3792) * docs: generate weekly changelog 2026-04-27 * docs: reframe weekly changelog around v2 public beta Lead with v2 public beta + Settings → Experimental enable, restructure around the v1→v2 migration story, sidebar overhaul, cross-workspace terminals, and v2 chat. Pull in ~30 v2 PRs the bot missed and demote non-v2 items (Hosts page, marketing menu) to a brief "Also this week". * docs: pull in missed v2 features and bug fixes Add v2 project setup section (#3566, #3605, #3606, #3592, #3626, #3632), scheduled agent runs (#3576), Opus 4.7 (#3579), v1 review comments in pane (#3596), configurable v2 link-click (#3600), Copy Branch Name (#3635), safer terminal preset defaults (#3546), and /pricing page (#3639). Expand bug fixes with v2 git correctness, cross-fork PR misattribution, terminal paste/Unicode/Shift+Enter, and security bumps. * docs(changelog): add v2 public beta hero screenshot * docs(changelog): add Settings → Experimental screenshot, compress hero pngquant compression: v2-public-beta.png 704KB → 166KB (76%), v2-enable-flag.png 160KB → 36KB (78%). No visible quality loss. * docs(changelog): tighten v2 launch prose, condense bullet groups * docs(changelog): reframe cloud-first pillar as remote workspaces * docs(changelog): cut parallel-agents and honest-state pillars, fold into sub-sections * docs(changelog): tweak title and lead phrasing * docs(changelog): rewrite v2 launch lede around Twitter narrative Pull the launch story (physical limits, 3 ex-CTOs, cloud workspaces) into the lede, restructure pillars around Remote workspaces, Reimagined diff view, and Superset CLI, and add v2-remote-workspaces and v2-changes-pane screenshots to back the new sections. * docs(changelog): add CLI install snippet and docs link * docs(changelog): cut narrative lede, match standard changelog tone --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Kiet Ho <hoakiet98@gmail.com>
saddlepaddle
pushed a commit
that referenced
this pull request
May 6, 2026
…-27) (#3792) * docs: generate weekly changelog 2026-04-27 * docs: reframe weekly changelog around v2 public beta Lead with v2 public beta + Settings → Experimental enable, restructure around the v1→v2 migration story, sidebar overhaul, cross-workspace terminals, and v2 chat. Pull in ~30 v2 PRs the bot missed and demote non-v2 items (Hosts page, marketing menu) to a brief "Also this week". * docs: pull in missed v2 features and bug fixes Add v2 project setup section (#3566, #3605, #3606, #3592, #3626, #3632), scheduled agent runs (#3576), Opus 4.7 (#3579), v1 review comments in pane (#3596), configurable v2 link-click (#3600), Copy Branch Name (#3635), safer terminal preset defaults (#3546), and /pricing page (#3639). Expand bug fixes with v2 git correctness, cross-fork PR misattribution, terminal paste/Unicode/Shift+Enter, and security bumps. * docs(changelog): add v2 public beta hero screenshot * docs(changelog): add Settings → Experimental screenshot, compress hero pngquant compression: v2-public-beta.png 704KB → 166KB (76%), v2-enable-flag.png 160KB → 36KB (78%). No visible quality loss. * docs(changelog): tighten v2 launch prose, condense bullet groups * docs(changelog): reframe cloud-first pillar as remote workspaces * docs(changelog): cut parallel-agents and honest-state pillars, fold into sub-sections * docs(changelog): tweak title and lead phrasing * docs(changelog): rewrite v2 launch lede around Twitter narrative Pull the launch story (physical limits, 3 ex-CTOs, cloud workspaces) into the lede, restructure pillars around Remote workspaces, Reimagined diff view, and Superset CLI, and add v2-remote-workspaces and v2-changes-pane screenshots to back the new sections. * docs(changelog): add CLI install snippet and docs link * docs(changelog): cut narrative lede, match standard changelog tone --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Kiet Ho <hoakiet98@gmail.com>
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
Click,⌘-click,⌘⇧-click) binds toDo nothing, in-app pane (File viewer / In-app browser), or external (editor / system browser). Picking an action already bound to another tier demotes that tier toDo nothing— no silent swapping.v2_user_preferencesTanStack DBlocalStorageCollectionkeyed byorganizationId. Consumers go throughuseV2UserPreferences,useTerminalLinkActions(3-tier), anduseInlineLinkActions(2-tier).handleClickOn; previously URLs in task markdown were inert. FilePane toolbar gained an always-available "Open in editor" icon button.isDirectory: truehint fromTerminalPanethroughrevealPath→WorkspaceSidebar→FilesTab→useFileTree.revealso known-directory targets get expanded even when the terminal'sstatPathresolved path andlistDirectory's child paths don't byte-match (case differences / symlink resolution)."Hold ⌘ to open · ⌘⇧ for external"to"Not bound · configure in Settings → Links"so it reflects user-configurable reality.Test plan
"Changes saved"fires on each change; values persist across a window reload.Click = Do nothing→ hint tooltip.metabinding; if both tiers areDo nothingthe link doesn't open.bun run --filter @superset/desktop typecheckandbun run lint:fixclean.Summary by cubic
Adds a new Settings → Links page to configure what Click, ⌘-click, and ⌘⇧-click do for file paths and URLs across v2. Terminal, task markdown, and panes honor these settings; folders still use ⌘ to reveal and ⌘⇧ to open externally.
New Features
v2_user_preferences; hooks:useV2UserPreferences,useTerminalLinkActions(3-tier),useInlineLinkActions(2-tier).Bug Fixes
useFileTree.reveal(path, { isDirectory })expands known directories even when path forms differ (case/symlink), and the effect cancels stale runs and waits for the worktree path.openPanenow activates panes replaced via reuse, so reused File/Browser panes get focus after link clicks.Written for commit dcb1a66. Summary will update on new commits.
Summary by CodeRabbit
New Features
Settings
UX