Add diff panel keyboard toggle and shortcut hint#93
Merged
juliusmarminge merged 4 commits intomainfrom Feb 27, 2026
Merged
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Desktop sidebar toggle shortcut unnecessarily broken
- Removed the erroneous
if (!isMobile) returnguard that disabled the Cmd/Ctrl+B sidebar toggle shortcut on desktop, since the shortcut key 'b' never conflicts with the 'mod+d' diff toggle.
- Removed the erroneous
Or push these changes by commenting:
@cursor push 919f3b34a0
Preview (919f3b34a0)
diff --git a/apps/web/src/components/ui/sidebar.tsx b/apps/web/src/components/ui/sidebar.tsx
--- a/apps/web/src/components/ui/sidebar.tsx
+++ b/apps/web/src/components/ui/sidebar.tsx
@@ -132,7 +132,6 @@
// Adds a keyboard shortcut to toggle the sidebar.
React.useEffect(() => {
const handleKeyDown = (event: KeyboardEvent) => {
- if (!isMobile) return;
if (event.key === SIDEBAR_KEYBOARD_SHORTCUT && (event.metaKey || event.ctrlKey)) {
event.preventDefault();
toggleSidebar();
@@ -141,7 +140,7 @@
window.addEventListener("keydown", handleKeyDown);
return () => window.removeEventListener("keydown", handleKeyDown);
- }, [isMobile, toggleSidebar]);
+ }, [toggleSidebar]);
// We add a state so that we can do data-state="expanded" or "collapsed".
// This makes it easier to style the sidebar with Tailwind classes.| // Adds a keyboard shortcut to toggle the sidebar. | ||
| React.useEffect(() => { | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (!isMobile) return; |
Contributor
There was a problem hiding this comment.
Desktop sidebar toggle shortcut unnecessarily broken
High Severity
The early return if (!isMobile) return; disables the sidebar keyboard shortcut (Cmd/Ctrl+B) on desktop. The PR description states this prevents desktop mod+d from being intercepted, but SIDEBAR_KEYBOARD_SHORTCUT is "b", not "d" — the sidebar handler would never intercept mod+d. This is a regression that removes a working desktop feature for no benefit.
![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
🤖 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/web/src/components/ChatView.tsx`:
- Around line 622-631: The onToggleDiff handler uses the captured diffOpen
variable which can become stale on rapid toggles; inside the navigate call use
the search callback's previous value to decide current diff state instead of the
outer diffOpen. In the search: (previous) => { const rest =
stripDiffSearchParams(previous); const currentlyOpen =
Boolean(getDiffParamFrom(previous) /*or check previous.diff*/); return
currentlyOpen ? rest : { ...rest, diff: "1" }; } so replace references to
diffOpen with a computation based on previous (use existing
stripDiffSearchParams / parse previous.search for the diff param) in the
onToggleDiff function to ensure reliable toggling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/keybindings.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/keybindings.test.tsapps/web/src/keybindings.tspackages/contracts/src/keybindings.test.tspackages/contracts/src/keybindings.ts
Comment on lines
+622
to
+631
| const onToggleDiff = useCallback(() => { | ||
| void navigate({ | ||
| to: "/$threadId", | ||
| params: { threadId }, | ||
| search: (previous) => { | ||
| const rest = stripDiffSearchParams(previous); | ||
| return diffOpen ? rest : { ...rest, diff: "1" }; | ||
| }, | ||
| }); | ||
| }, [diffOpen, navigate, threadId]); |
There was a problem hiding this comment.
Use previous search state for diff toggle decisions.
Line [628] relies on captured diffOpen, which can produce missed toggles on rapid repeated shortcut/button invocations.
Proposed fix
const onToggleDiff = useCallback(() => {
void navigate({
to: "/$threadId",
params: { threadId },
search: (previous) => {
const rest = stripDiffSearchParams(previous);
- return diffOpen ? rest : { ...rest, diff: "1" };
+ const previousDiffOpen =
+ parseDiffRouteSearch(previous as Record<string, unknown>).diff === "1";
+ return previousDiffOpen ? rest : { ...rest, diff: "1" };
},
});
- }, [diffOpen, navigate, threadId]);
+ }, [navigate, threadId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ChatView.tsx` around lines 622 - 631, The
onToggleDiff handler uses the captured diffOpen variable which can become stale
on rapid toggles; inside the navigate call use the search callback's previous
value to decide current diff state instead of the outer diffOpen. In the search:
(previous) => { const rest = stripDiffSearchParams(previous); const
currentlyOpen = Boolean(getDiffParamFrom(previous) /*or check previous.diff*/);
return currentlyOpen ? rest : { ...rest, diff: "1" }; } so replace references to
diffOpen with a computation based on previous (use existing
stripDiffSearchParams / parse previous.search for the diff param) in the
onToggleDiff function to ensure reliable toggling.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/src/components/ChatView.tsx (1)
622-632:⚠️ Potential issue | 🟡 MinorAvoid stale
diffOpencapture inonToggleDiff.Line [629] bases toggle behavior on the outer
diffOpen, which can be stale under rapid repeated toggles. Compute frompreviousinside thesearchupdater instead.Proposed fix
const onToggleDiff = useCallback(() => { void navigate({ to: "/$threadId", params: { threadId }, replace: true, search: (previous) => { const rest = stripDiffSearchParams(previous); - return diffOpen ? rest : { ...rest, diff: "1" }; + const previousDiffOpen = + parseDiffRouteSearch(previous as Record<string, unknown>).diff === "1"; + return previousDiffOpen ? rest : { ...rest, diff: "1" }; }, }); - }, [diffOpen, navigate, threadId]); + }, [navigate, threadId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 622 - 632, The onToggleDiff callback captures outer diffOpen which can be stale; instead derive whether diff is open from the previous search params inside the search updater passed to navigate. Modify onToggleDiff (the useCallback that calls navigate with search: (previous) => { ... }) to inspect previous (use stripDiffSearchParams or parse previous.search/diff param) to decide whether to include diff: "1" or not, rather than referencing the outer diffOpen variable; keep threadId and replace behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 622-632: The onToggleDiff callback captures outer diffOpen which
can be stale; instead derive whether diff is open from the previous search
params inside the search updater passed to navigate. Modify onToggleDiff (the
useCallback that calls navigate with search: (previous) => { ... }) to inspect
previous (use stripDiffSearchParams or parse previous.search/diff param) to
decide whether to include diff: "1" or not, rather than referencing the outer
diffOpen variable; keep threadId and replace behavior unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/ChatView.tsx (1)
2116-2136: Consider using Popover for interactive element tooltip.Per the coding guidelines,
Tooltipshould not be used when the trigger is an interactive element like a button. TheTogglehere is interactive, and the guideline recommends usingPopoverwithtooltipStyleinstead.That said, since the
Togglealready hasaria-label="Toggle diff panel"for screen-reader accessibility and the tooltip is purely supplementary (showing the shortcut hint), this is not a blocker. Based on learnings: "For Tooltip vs Popover in Base UI: NEVER useTooltipwhen the trigger is an interactive element... UsePopoverwithtooltipStyleinstead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 2116 - 2136, The Tooltip is being used with an interactive trigger (the Toggle component); replace the Tooltip wrapper with a Popover configured for tooltip behavior: use Popover (instead of Tooltip) with a PopoverTrigger that renders the existing Toggle (preserving props: className="shrink-0", pressed={diffOpen}, onPressedChange={onToggleDiff}, aria-label="Toggle diff panel", variant="outline", size="xs" and the DiffIcon child) and a PopoverContent (or Popup) styled via tooltipStyle to show the same text computed from diffToggleShortcutLabel (`Toggle diff panel (${diffToggleShortcutLabel})` or fallback). Keep the accessibility label on Toggle and ensure the popover is positioned side="bottom" and behaves as a non-focus-trapping tooltip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 2116-2136: The Tooltip is being used with an interactive trigger
(the Toggle component); replace the Tooltip wrapper with a Popover configured
for tooltip behavior: use Popover (instead of Tooltip) with a PopoverTrigger
that renders the existing Toggle (preserving props: className="shrink-0",
pressed={diffOpen}, onPressedChange={onToggleDiff}, aria-label="Toggle diff
panel", variant="outline", size="xs" and the DiffIcon child) and a
PopoverContent (or Popup) styled via tooltipStyle to show the same text computed
from diffToggleShortcutLabel (`Toggle diff panel (${diffToggleShortcutLabel})`
or fallback). Keep the accessibility label on Toggle and ensure the popover is
positioned side="bottom" and behaves as a non-focus-trapping tooltip.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Added
replace: truesilently changes existing toggle button behavior- Added
replace: truetocloseDiff,openDiff, andonOpenTurnDiffnavigate calls to make all diff-related navigation consistent withonToggleDiff, eliminating the inconsistent browser history behavior.
- Added
Or push these changes by commenting:
@cursor push 1c78f72156
Preview (1c78f72156)
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -1641,6 +1641,7 @@
void navigate({
to: "/$threadId",
params: { threadId },
+ replace: true,
search: (previous) => {
const rest = stripDiffSearchParams(previous);
return filePath
diff --git a/apps/web/src/routes/_chat.$threadId.tsx b/apps/web/src/routes/_chat.$threadId.tsx
--- a/apps/web/src/routes/_chat.$threadId.tsx
+++ b/apps/web/src/routes/_chat.$threadId.tsx
@@ -145,6 +145,7 @@
void navigate({
to: "/$threadId",
params: { threadId },
+ replace: true,
search: (previous) => {
return stripDiffSearchParams(previous);
},
@@ -154,6 +155,7 @@
void navigate({
to: "/$threadId",
params: { threadId },
+ replace: true,
search: (previous) => {
const rest = stripDiffSearchParams(previous);
return { ...rest, diff: "1" };| return diffOpen ? rest : { ...rest, diff: "1" }; | ||
| }, | ||
| }); | ||
| }, [diffOpen, navigate, threadId]); |
Contributor
There was a problem hiding this comment.
Added replace: true silently changes existing toggle button behavior
Low Severity
The refactored onToggleDiff adds replace: true to the navigate call, which was absent from the original. This changes the existing diff toggle button's behavior — it now replaces the current history entry rather than pushing a new one. This means users can no longer use browser back to undo a toggle. It also creates an inconsistency with closeDiff and onOpenTurnDiff, which don't use replace: true, leading to inconsistent history behavior depending on how the diff panel is opened or closed.
- Bind `mod+d` to `diff.toggle` when terminal is not focused - Wire ChatView to handle `diff.toggle` and show its shortcut in the diff button tooltip - Limit sidebar `mod+b` listener to mobile to avoid desktop shortcut conflicts - Extend web/contracts keybinding tests and command list for `diff.toggle`
- Add `replace: true` to thread navigation in `ChatView` - Prevents extra browser history entries when opening/closing diff mode
- Wrap diff panel toggle in shared tooltip components - Display shortcut hint in tooltip popup instead of title attribute
- Remove global keydown listener from `SidebarProvider` - Keep sidebar toggling to explicit UI interactions only
juliusmarminge
force-pushed
the
codething/62a0cc86
branch
from
e48b6cc to
9a2fc6f
Compare
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
diff.togglekeybinding command and include it in shared keybinding contracts.mod+dto toggle the diff panel when terminal is not focused (while keeping terminal split onmod+dwhen terminal is focused).diff.toggleinChatViewkeyboard dispatch and surface its shortcut as a tooltip on the diff toggle button.isDiffToggleShortcutin web keybinding helpers and add coverage for command matching/label formatting.mod+dis not intercepted.Testing
apps/web/src/keybindings.test.tsfor:shortcutLabelForCommand(..., "diff.toggle")label resolution (Ctrl+Don Linux).isDiffToggleShortcutmatching outside terminal focus and not matching inside terminal focus.packages/contracts/src/keybindings.test.tsto verifydiff.toggleparses as a valid keybinding command.Note
Medium Risk
Updates shared keybinding contracts and default bindings, plus global keyboard handling in
ChatView, which could introduce shortcut conflicts or regressions in key dispatch/navigation behavior.Overview
Adds a new
diff.togglekeybinding command to the shared contracts and server defaults, bindingmod+dto toggle the diff panel when!terminalFocus(while keepingmod+dforterminal.splitwhen focused).Updates
ChatViewto handle thediff.togglecommand in the global keydown dispatcher and to show a tooltip on the diff toggle button that includes the resolved shortcut label.Removes the sidebar’s global keyboard shortcut handler, and adds unit tests covering
diff.toggleparsing, command matching, and shortcut label formatting.Written by Cursor Bugbot for commit 9a2fc6f. This will update automatically on new commits. Configure here.
Note
Add
mod+dkeybinding to emitdiff.toggleand show a tooltip with the shortcut inChatHeaderfor toggling the diff panelIntroduce
diff.toggleas a recognized command, bindmod+dwhen!terminalFocus, handle the shortcut inChatView, and display a tooltip with the shortcut label inChatHeader; remove globalmod+bsidebar toggle.📍Where to Start
Start with the global keybinding handling and shortcut label in
ChatViewin ChatView.tsx.Macroscope summarized 9a2fc6f.
Summary by CodeRabbit
New Features
Improvements
Tests