Context Menu Positioning Lane Chat#682
Conversation
Menus now measure and clamp to the viewport so they stay visible in resized windows, and lane right-click menus can open the Work tab new-chat composer with the lane preselected. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThis PR introduces a shared ChangesClamped context menu positioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Start chat in lane action
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/hooks/useClampedFixedPosition.ts (1)
30-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpreading
depsinto the effect's dependency array is fragile.
[anchor?.x, anchor?.y, ...deps]has react-hooks/exhaustive-deps implications: the dependency array's size must stay stable across renders for a given call site. Sincedepsis an opaque caller-supplied array, nothing in the hook's API prevents a future caller from passing a variable-length array (e.g., conditionally including/excluding an item), which would silently break dependency comparisons. Current callers happen to use fixed-length arrays, but this is baked into the hook's contract.Consider deriving a single stable key (e.g.,
deps.join("|")) or documenting/enforcing a fixed-length contract fordeps.♻️ Possible mitigation
export function useClampedFixedPosition( anchor: FixedAnchor | null, - deps: unknown[] = [], + depsKey: string | number = "", ): { ref: MutableRefObject<HTMLDivElement | null>; position: ClampedFixedPosition | null; } { const ref = useRef<HTMLDivElement | null>(null); const [position, setPosition] = useState<ClampedFixedPosition | null>(null); useLayoutEffect(() => { if (!anchor || !ref.current) { setPosition(null); return; } const rect = ref.current.getBoundingClientRect(); setPosition(clampFixedPosition(anchor, { width: rect.width, height: rect.height })); - }, [anchor?.x, anchor?.y, ...deps]); + }, [anchor?.x, anchor?.y, depsKey]);🤖 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/hooks/useClampedFixedPosition.ts` around lines 30 - 47, The useClampedFixedPosition hook’s effect dependency array is unstable because it spreads the caller-supplied deps array, which can change length across renders and break hook comparison semantics. Update the hook to avoid spreading deps directly in useLayoutEffect; instead derive a single stable dependency value from deps (or otherwise enforce/document a fixed-length contract) and keep the effect dependencies shape consistent alongside anchor?.x and anchor?.y. Locate the fix in useClampedFixedPosition and preserve the existing clampFixedPosition behavior.
🤖 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/lanes/LanesPage.tsx`:
- Around line 1282-1295: The start-chat flow is duplicated in LanesPage and
useWorkLaneContextMenu, so extract the shared logic into a common helper/hook
such as useStartChatInLane. Move the current guard, startChatDraftPatch
construction, and selectLane/navigate sequence into that shared function, then
have both call sites pass their own projectRoot, setWorkViewState, selectLane,
and navigate implementations. Keep the existing behavior identical while
centralizing the callback so future changes only happen in one place.
---
Nitpick comments:
In `@apps/desktop/src/renderer/hooks/useClampedFixedPosition.ts`:
- Around line 30-47: The useClampedFixedPosition hook’s effect dependency array
is unstable because it spreads the caller-supplied deps array, which can change
length across renders and break hook comparison semantics. Update the hook to
avoid spreading deps directly in useLayoutEffect; instead derive a single stable
dependency value from deps (or otherwise enforce/document a fixed-length
contract) and keep the effect dependencies shape consistent alongside anchor?.x
and anchor?.y. Locate the fix in useClampedFixedPosition and preserve the
existing clampFixedPosition behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23c7dcda-64f8-4d04-95e9-751a8a6124a5
📒 Files selected for processing (12)
apps/desktop/src/renderer/components/app/TabNav.tsxapps/desktop/src/renderer/components/files/v2/ContextMenu.tsxapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/useWorkLaneContextMenu.test.tsxapps/desktop/src/renderer/components/terminals/useWorkLaneContextMenu.tsxapps/desktop/src/renderer/hooks/useClampedFixedPosition.test.tsapps/desktop/src/renderer/hooks/useClampedFixedPosition.tsapps/desktop/src/renderer/lib/workDraft.test.tsapps/desktop/src/renderer/lib/workDraft.ts
| const startChatInLane = useCallback( | ||
| (laneId: string) => { | ||
| if (activeProjectRoot) { | ||
| setWorkViewState(activeProjectRoot, (prev) => ({ | ||
| ...prev, | ||
| ...startChatDraftPatch(laneId), | ||
| })); | ||
| } | ||
| selectLane(laneId); | ||
| void navigate("/work"); | ||
| }, | ||
| [activeProjectRoot, navigate, selectLane, setWorkViewState], | ||
| ); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate startChatInLane logic — extract to a shared hook/helper.
This callback is functionally identical to startChatInLane in useWorkLaneContextMenu.tsx (same guard, same patch construction, same selectLane/navigate sequence). Keeping two independent copies risks the two call sites silently diverging as this flow evolves (e.g., future guards against deleting lanes, or telemetry).
Consider extracting a small shared hook (e.g. useStartChatInLane) that both LanesPage and useWorkLaneContextMenu call with their own navigate/selectLane/setWorkViewState/project-root values.
♻️ Proposed shared hook
// apps/desktop/src/renderer/lib/workDraft.ts (or a new hook file)
export function useStartChatInLane(
projectRoot: string | null,
setWorkViewState: (projectRoot: string, updater: (prev: WorkProjectViewState) => WorkProjectViewState) => void,
selectLane: (laneId: string) => void,
navigate: (path: string) => void | Promise<void>,
) {
return useCallback(
(laneId: string) => {
if (projectRoot) {
setWorkViewState(projectRoot, (prev) => ({ ...prev, ...startChatDraftPatch(laneId) }));
}
selectLane(laneId);
void navigate("/work");
},
[projectRoot, setWorkViewState, selectLane, navigate],
);
}🤖 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/components/lanes/LanesPage.tsx` around lines 1282 -
1295, The start-chat flow is duplicated in LanesPage and useWorkLaneContextMenu,
so extract the shared logic into a common helper/hook such as
useStartChatInLane. Move the current guard, startChatDraftPatch construction,
and selectLane/navigate sequence into that shared function, then have both call
sites pass their own projectRoot, setWorkViewState, selectLane, and navigate
implementations. Keep the existing behavior identical while centralizing the
callback so future changes only happen in one place.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const startChatInLane = useCallback( | ||
| (laneId: string) => { | ||
| if (projectRoot) { | ||
| setWorkViewState(projectRoot, (prev) => ({ | ||
| ...prev, | ||
| ...startChatDraftPatch(laneId), | ||
| })); | ||
| } | ||
| selectLane(laneId); | ||
| void navigate("/work"); | ||
| }, | ||
| [navigate, projectRoot, selectLane, setWorkViewState], | ||
| ); |
There was a problem hiding this comment.
projectRoot is null skips draft setup
When projectRoot is null, setWorkViewState is skipped but selectLane and navigate("/work") still execute, taking the user to the Work view without a pre-configured chat draft. The user would see the Work view in its previous state rather than a new chat draft for the selected lane, which is silent unexpected behavior. The same pattern is duplicated in LanesPage.tsx (startChatInLane). Since lanes are only visible with an active project, this state is unlikely in practice, but an early return (or at minimum a guard before the navigate) would make the failure mode explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/terminals/useWorkLaneContextMenu.tsx
Line: 63-75
Comment:
**Silent navigation when `projectRoot` is null skips draft setup**
When `projectRoot` is null, `setWorkViewState` is skipped but `selectLane` and `navigate("/work")` still execute, taking the user to the Work view without a pre-configured chat draft. The user would see the Work view in its previous state rather than a new chat draft for the selected lane, which is silent unexpected behavior. The same pattern is duplicated in `LanesPage.tsx` (`startChatInLane`). Since lanes are only visible with an active project, this state is unlikely in practice, but an early return (or at minimum a guard before the navigate) would make the failure mode explicit.
How can I resolve this? If you propose a fix, please make it concise.
Extract useStartChatInLane, guard when no project root, and replace spread deps in useClampedFixedPosition. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Greptile Summary
This PR adds "Start chat in lane" to lane context menus and fixes context menus appearing off-screen across multiple views (terminal, graph, lane, tab) by replacing ad-hoc layout math with a shared
useClampedFixedPositionhook.useClampedFixedPositionhook measures the rendered element viagetBoundingClientRectin auseLayoutEffect, hides the menu withvisibility: "hidden"on first render, then reveals it at the correct clamped position — ensuring menus never paint off-screen even once.useStartChatInLane(shared betweenLanesPageanduseWorkLaneContextMenu) andstartChatDraftPatch, with unit tests covering both the patch helper and the full navigation flow for remote projects.Confidence Score: 5/5
Safe to merge — all context menu sites migrate cleanly to the shared hook, and the new chat-in-lane feature is well-guarded and tested.
The positioning refactor is a straightforward extraction with identical semantics (the visibility:hidden pattern is correct and useLayoutEffect fires before paint). The new start-chat feature has unit tests covering the remote-project root path, the patch shape, and the full navigation sequence. No pre-existing guards were removed and no new unguarded code paths were introduced.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Right-click on lane / tab / terminal / graph node"] --> B["setState: anchor = {x, y}"] B --> C["Render menu div\nvisibility: hidden\nposition = null"] C --> D["useLayoutEffect fires\nanchor != null && ref.current != null"] D --> E["getBoundingClientRect()\nmeasure actual width/height"] E --> F["clampFixedPosition(anchor, size, padding=8)"] F --> G["setPosition({left, top})"] G --> H["Re-render\nvisibility: visible\nposition = clamped coords"] H --> I["Menu displayed within viewport"] J["User clicks 'Start chat in lane'"] --> K["onClose() - close menu"] K --> L["useStartChatInLane callback"] L --> M{projectRoot null?} M -- yes --> N["early return (no-op)"] M -- no --> O["setWorkViewState with startChatDraftPatch\ndraftKind=chat, draftLaneId, orchestratorEnabled=false"] O --> P["selectLane(laneId)"] P --> Q["navigate('/work')"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["Right-click on lane / tab / terminal / graph node"] --> B["setState: anchor = {x, y}"] B --> C["Render menu div\nvisibility: hidden\nposition = null"] C --> D["useLayoutEffect fires\nanchor != null && ref.current != null"] D --> E["getBoundingClientRect()\nmeasure actual width/height"] E --> F["clampFixedPosition(anchor, size, padding=8)"] F --> G["setPosition({left, top})"] G --> H["Re-render\nvisibility: visible\nposition = clamped coords"] H --> I["Menu displayed within viewport"] J["User clicks 'Start chat in lane'"] --> K["onClose() - close menu"] K --> L["useStartChatInLane callback"] L --> M{projectRoot null?} M -- yes --> N["early return (no-op)"] M -- no --> O["setWorkViewState with startChatDraftPatch\ndraftKind=chat, draftLaneId, orchestratorEnabled=false"] O --> P["selectLane(laneId)"] P --> Q["navigate('/work')"]Reviews (2): Last reviewed commit: "Address review feedback for start-chat a..." | Re-trigger Greptile