Repository navigation
Fix Escape propagation when command palette is visible - #847
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes introduce per-window command palette state tracking with escape suppression, pending open markers, and request lifecycle helpers in AppDelegate, alongside extensive test coverage for escape key consumption, IME marked text handling, and command palette visibility routing across windows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as AppDelegate
participant Palette as Command Palette
participant Window as NSWindow
User->>App: Press Escape key (keyCode 53)
activate App
App->>App: Check if Escape should be consumed
App->>Window: Query command palette visibility
activate Window
Window-->>App: Is palette visible/pending/overlay?
deactivate Window
alt Palette should be dismissed
App->>Palette: Dismiss command palette
activate Palette
Palette-->>App: Palette dismissed
deactivate Palette
App->>App: Begin escape suppression
App->>App: Clear pending open state
App-->>User: Consume event
else Palette has IME marked text
App-->>User: Pass through to IME
else Palette not visible
App-->>User: Don't consume event
end
User->>App: Release Escape key (key-up)
App->>App: End escape suppression
deactivate App
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 |
Contributor
Greptile SummaryFixed Escape key propagation bug where pressing Escape to dismiss the command palette would leak the key event to the underlying terminal or browser.
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: cfed61f |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfed61ffbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
![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 (3)
Sources/AppDelegate.swift (1)
3800-3810: Avoid full view-tree scans on every keyDown in the shortcut hot path.Line 6038 and Line 6042 currently probe overlay/responder state on every keyDown.
commandPaletteOverlayContainer(in:)(Line 3800) walks the entire view hierarchy, which is expensive for high-frequency typing paths.⚡ Suggested optimization (lazy probing)
let commandPaletteVisibleInTargetWindow = commandPaletteTargetWindow.map { isCommandPaletteVisible(for: $0) } ?? false let commandPalettePendingOpenInTargetWindow = commandPaletteTargetWindow.map { isCommandPalettePendingOpen(for: $0) } ?? false -let commandPaletteOverlayVisibleInTargetWindow = commandPaletteTargetWindow.map { - isCommandPaletteOverlayPresented(in: $0) -} ?? false -let commandPaletteResponderActiveInTargetWindow = commandPaletteTargetWindow.map { - isCommandPaletteResponderActive(in: $0) -} ?? false +let shouldProbeOverlayOrResponder = + !commandPaletteVisibleInTargetWindow && !commandPalettePendingOpenInTargetWindow +let commandPaletteOverlayVisibleInTargetWindow = shouldProbeOverlayOrResponder + ? (commandPaletteTargetWindow.map { isCommandPaletteOverlayPresented(in: $0) } ?? false) + : false +let commandPaletteResponderActiveInTargetWindow = shouldProbeOverlayOrResponder + ? (commandPaletteTargetWindow.map { isCommandPaletteResponderActive(in: $0) } ?? false) + : falseAlso applies to: 6038-6043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3800 - 3810, The current hot path calls commandPaletteOverlayContainer(in:) (which walks the full view tree) on every keyDown; avoid that by caching the container lookup per window: add a weak cached reference keyed to the NSWindow (e.g., an associated object or a small WeakBox stored on the NSWindow) and change the keyDown code to first check the cache and return immediately if it points to a live view, only falling back to calling commandPaletteOverlayContainer(in:) on cache miss; when you discover the container, store the weak reference in the cache, and invalidate the cache whenever the window's contentView changes or overlays are added/removed (observe relevant notifications or override add/remove subview via a lightweight observer) so the expensive full-tree scan runs rarely instead of on every keystroke.cmuxTests/AppDelegateShortcutRoutingTests.swift (2)
728-758: Usedeferfor visibility cleanup to prevent cross-test state leakage.Line 728 and Line 786 set shared command-palette visibility to
true, but reset happens later on the success path. If the test exits early, state can leak into following tests.♻️ Suggested hardening
@@ - appDelegate.setCommandPaletteVisible(true, for: window) + appDelegate.setCommandPaletteVisible(true, for: window) + defer { + appDelegate.setCommandPaletteVisible(false, for: window) + } @@ - // Simulate the palette overlay synchronizing to closed state while the Escape key is still held. - appDelegate.setCommandPaletteVisible(false, for: window) + // Simulate the palette overlay synchronizing to closed state while the Escape key is still held. + appDelegate.setCommandPaletteVisible(false, for: window) @@ - appDelegate.setCommandPaletteVisible(true, for: window) + appDelegate.setCommandPaletteVisible(true, for: window) + defer { + appDelegate.setCommandPaletteVisible(false, for: window) + } @@ - // Simulate the palette overlay synchronizing to closed state before Escape key-up arrives. - appDelegate.setCommandPaletteVisible(false, for: window) + // Simulate the palette overlay synchronizing to closed state before Escape key-up arrives. + appDelegate.setCommandPaletteVisible(false, for: window)Also applies to: 786-817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 728 - 758, The test sets shared command-palette visibility via appDelegate.setCommandPaletteVisible(true, for: window) but only resets it later, risking cross-test leakage on early returns; wrap the visibility cleanup in a defer immediately after setting visible so appDelegate.setCommandPaletteVisible(false, for: window) always runs, and do the same for the other occurrence (around lines that setVisible true before calling debugHandleCustomShortcut) to guarantee deterministic cleanup regardless of test exits.
576-577: Avoid real-time sleeps in unit tests for stale-state timing checks.Line 576 and Line 634 add 1.25s/6.25s wall-clock waits, which will slow the suite and can become timing-sensitive under load. Prefer a test hook to inject/advance the pending-open age instead of sleeping.
Also applies to: 633-635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 576 - 577, The tests use RunLoop.main.run(...) sleeps (RunLoop.main.run calls) to wait for a "pending-open age" to elapse; replace these real-time sleeps by adding a test hook to control time: introduce a DateProvider/Clock abstraction or a test-only setter on the component that tracks pending-open timestamps (e.g., inject a DateProvider into the AppDelegate/shortcut routing component or add setPendingOpenDate(_:)/advancePendingOpenAge(by:) methods) and update AppDelegateShortcutRoutingTests to use that hook to simulate advancing the pending-open age instead of calling RunLoop.main.run; update any code paths that compute "pending open" age to use the injected DateProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3330-3333: The pending-open flag is treated as unbounded and can
be left true forever; modify the logic so pending-open is transient by storing a
timestamp or expiry alongside commandPalettePendingOpenByWindowId and update
isCommandPalettePendingOpen(for:) to check expiry (use mainWindowId(for:) to
locate the entry), and ensure code paths that set visible = false (and anywhere
that re-sets visible) clear or invalidate the pending-open entry (or rely on
expiry) so a dropped/failed open request cannot leave the palette effectively
visible and continue capturing shortcuts; update all places that set or read
commandPalettePendingOpenByWindowId (e.g., set/clear sites referenced in the
review) to use the new expiry-aware semantics.
---
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 728-758: The test sets shared command-palette visibility via
appDelegate.setCommandPaletteVisible(true, for: window) but only resets it
later, risking cross-test leakage on early returns; wrap the visibility cleanup
in a defer immediately after setting visible so
appDelegate.setCommandPaletteVisible(false, for: window) always runs, and do the
same for the other occurrence (around lines that setVisible true before calling
debugHandleCustomShortcut) to guarantee deterministic cleanup regardless of test
exits.
- Around line 576-577: The tests use RunLoop.main.run(...) sleeps
(RunLoop.main.run calls) to wait for a "pending-open age" to elapse; replace
these real-time sleeps by adding a test hook to control time: introduce a
DateProvider/Clock abstraction or a test-only setter on the component that
tracks pending-open timestamps (e.g., inject a DateProvider into the
AppDelegate/shortcut routing component or add
setPendingOpenDate(_:)/advancePendingOpenAge(by:) methods) and update
AppDelegateShortcutRoutingTests to use that hook to simulate advancing the
pending-open age instead of calling RunLoop.main.run; update any code paths that
compute "pending open" age to use the injected DateProvider.
In `@Sources/AppDelegate.swift`:
- Around line 3800-3810: The current hot path calls
commandPaletteOverlayContainer(in:) (which walks the full view tree) on every
keyDown; avoid that by caching the container lookup per window: add a weak
cached reference keyed to the NSWindow (e.g., an associated object or a small
WeakBox stored on the NSWindow) and change the keyDown code to first check the
cache and return immediately if it points to a live view, only falling back to
calling commandPaletteOverlayContainer(in:) on cache miss; when you discover the
container, store the weak reference in the cache, and invalidate the cache
whenever the window's contentView changes or overlays are added/removed (observe
relevant notifications or override add/remove subview via a lightweight
observer) so the expensive full-tree scan runs rarely instead of on every
keystroke.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5af2c22-9faf-4f35-aa30-91434c286f7c
📒 Files selected for processing (3)
Sources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fa731e334
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 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="cmuxTests/AppDelegateShortcutRoutingTests.swift">
<violation number="1" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:634">
P3: Avoid long fixed run-loop sleeps in unit tests; they significantly slow the suite and increase flakiness.</violation>
<violation number="2" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:705">
P2: This test does not verify the dismissal it claims in its name; it only checks key consumption, so regressions in actual palette dismissal can slip through.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:3415">
P2: Clear the recent command-palette request timestamp when visibility state resolves; otherwise Escape can be incorrectly swallowed for up to 1.25s after the palette is already closed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2126cc5884
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Comment on lines
+3388
to
+3389
| if ProcessInfo.processInfo.systemUptime - startedAt <= 0.35 { | ||
| return true |
There was a problem hiding this comment.
The new suppression check returns true for any Escape event within 350ms of palette dismiss, not just repeats/key-up, so a deliberate second Escape tap right after closing the palette is swallowed before it reaches the terminal. This is introduced in shouldConsumeSuppressedEscape because the time-window branch has no event-type guard; in practice, users who dismiss the palette and immediately press Escape again (e.g., to cancel an in-terminal mode) will lose that second key press.
Useful? React with 👍 / 👎.
0xble
added a commit
to 0xble/cmux
that referenced
this pull request
Mar 7, 2026
This was referenced Mar 14, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Fix command palette Escape propagation and add regressions * Respect IME marked text for command palette Escape * Harden command palette escape pending-open routing
This was referenced May 7, 2026
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 28, 2026
The dogfood tour could never have covered this. Its clickAt step is a plain click with no modifiers, and handleCommandClickRelease returns at its first guard unless Command is held, so every frame the tour captured was of a click that did nothing. A green tour run said only that nothing threw. The cmd-click harness already in this file drives the real path: it synthesizes the flagsChanged transition and the button press ghostty sees, and the coordinator's open-URL capture sink records where a link would have gone. Printing a reference token instead of a filename is the only thing it was missing, so add that line format and cover the four cases that matter: a bare manaflow-ai#847 resolved against the pane's remote, a slug-qualified reference that needs no remote at all, an abbreviated SHA behind an ssh remote, and an ordinary word that must cost nothing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 28, 2026
Three fixes from review, all about tests that could pass without the feature working. The fixture remote was `manaflow-ai/cmux`, the same repository CI runs from. If a pane ever resolved to the checkout instead of the fixture directory, the opened URL would have been identical and the test would have passed anyway. The fixture now claims `manaflow-ai/cmux-reference- fixture`, a slug that can only come from the fixture. The ordinary-word negative asserted only that nothing opened, so it stayed green with the feature deleted. It now first proves the click harness ran, resolved a point on the token, and reached the terminal view. Its comment also claimed to cover a late repository lookup, which is not true: the detector rejects `nothing-at-all` outright and no lookup is ever started. The comment now says what the test really is, a false-positive guard. That left the late-lookup path uncovered, so there is a new test for it. `manaflow-ai#847` against a GitLab remote is a reference, so it does take the resolve-the-repository path, and it must still open nothing when the slug comes back empty. That is the case an over-eager fallback would break. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 28, 2026
The first run of this tour looked like nothing happened. The frames from run 36410500117 say otherwise: the clicks did hold Command, and Chrome for Testing was launched, which only the link open explains. The captures could not show it because `shot` is app-scoped and an external browser is not in cmux's window. Two fixes. Every after-click capture is now screen-wide, so a link that opens outside cmux is visible instead of looking like a no-op. Each pane now starts in a directory with a GitHub remote. Before, the panes sat in the home directory, so a bare `manaflow-ai#847` and a bare commit SHA had no remote to resolve against and correctly opened nothing. That made three of the four cases prove only that they do nothing. The repository is written as files rather than with `git init`. cmux never shells out to git here: it walks up for a `.git` directory and parses `<gitDirectory>/config` itself, so files exercise the actual path and do not depend on a git binary, a global user.name, or init.defaultBranch. The empty `objects` and `refs/heads` directories are there so the real `git` accepts the directory too, for anyone who opens that pane and types a git command. Also adds the `paths` globs the media selector needs to pick this tour up when the link-routing code changes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 28, 2026
The tour's clicks never landed on a token. Frames from main's own modifier-clicks-tour (run 36435029937) settle the geometry: the window shots are 960x487, the first row of output sits at y=0.068, rows are 0.0185 apart, and a character is 0.0057 wide. That tour clicks at x=0.4, y=0.10, which is (384, 49) in the frame, while its URL ends at x=340 and sits on the row at y=33. It misses on both axes, so its green result says nothing, and this tour's x=0.05, y=0.14 was no better founded. Click at x=0.01, y=0.40 instead, against 60 rows that each start at column 0 with the token, so the point is inside the first token of about row 18. Give each pane its own outcome, so a frame identifies which path produced it: a GitLab remote with a valid manaflow-ai#847 opens nothing, a GitHub fixture remote with manaflow-ai#847 opens cmux-reference-fixture/issues/847, a directory with no .git at all with manaflow-ai#847 opens cmux/issues/847, and the fixture with 73396e6 opens that commit. Before, two panes both opened cmux/issues/847 and the screenshots could not tell them apart. Use manaflow-ai/cmux-reference-fixture, the slug the tests use, instead of manaflow-ai/cmux. A pane that resolved to the CI checkout by mistake now shows a visibly wrong URL rather than a correct-looking one. Put the negative first, before any browser window exists, so "nothing opened" is not a claim about a window left over from an earlier pane. Drop the cmd-hover shots. The affordance is a cursor shape, and shot() calls window.screenshot() or XCUIScreen.main.screenshot(), neither of which draws the cursor, so those frames could never show it. The hover XCTests cover it. Keep exactly four named shots and call the end card 99-final. pick_key_shots shows a reviewer MAX_KEY_SHOTS=4 evenly spaced shots and skips 99-final, so the four after-click frames are now the four a reviewer sees; before, three of them were dropped. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 28, 2026
Two flaws in the tour's own evidence, both visible in run 36451867703. The bare-ref and commit-ref panes pointed at manaflow-ai/cmux-reference-fixture, which does not exist, so both frames showed GitHub's 404 page. The URL bar proved the reference resolved correctly, but a reviewer skimming the frames reads two 404s as a broken feature. Point both at manaflow-ai/cmux so the pages render, and give the bare-ref pane this PR's own number so it stays distinct from the slug pane's manaflow-ai#847. The tour also captured its own shot named 99-final. The harness prefixes a tour's shots with their step index, so it landed as 31-99-final, which is not in pr_media.SKIP_AS_KEY and took a key-shot slot. With five candidates for four slots pick_key_shots dropped 22-03-slug-ref, the one frame showing a fully rendered page. The harness already captures 99-final and 99-final-screen, so drop ours. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
An agent writing a sidebar metadata block writes `#847`, not `[#847](https://...)`, so today the reference renders as text with nothing to click even though the row already knows how to open a link. `GitHubReferenceTextScanner` finds every reference in a run of plain text and reports the range each one occupies, which is the opposite of what `TerminalGitHubReferenceDetector` answers (the token under a pointer) and what rendered text needs. `GitHubReferenceAttributedStringLinkifier` walks parsed markdown and attaches the resolved URL, skipping runs the author already linked and runs the parser marked as a code span or a code block. It runs after the parser, never over the markdown source, so fences and spans are the parser's call rather than a second half-parser's guess. A reference a style change cuts in half, as in `owner/repo#84**7**`, reaches the scanner truncated and would resolve to a different issue than the one on screen. Those are dropped: a link the reader was not shown is worse than no link. Linkifying sets attributes only and never touches characters, so the height stability `SidebarMetadataMarkdownRenderer` exists for is unaffected, and it happens inside the memoized parse so a row re-evaluating its body does no extra work. Only `owner/repo#123` resolves in the sidebar for now, because that is the one form naming its own repository. A bare `#123`, a `GH-123` and a commit SHA all need the repository the row is about, and the sidebar snapshot carries no slug: `PullRequestProbeService` resolves one per workspace on every probe cycle and drops it at the package boundary. Bringing it through is a separate change, and a test pins the current boundary so it is a deliberate one. ## Changelog Added: GitHub issue and pull request references written as `owner/repo#123` in sidebar metadata are now clickable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
An agent writing a sidebar metadata block writes `#847`, not `[#847](https://...)`, so today the reference renders as text with nothing to click even though the row already knows how to open a link. `GitHubReferenceTextScanner` finds every reference in a run of plain text and reports the range each one occupies, which is the opposite of what `TerminalGitHubReferenceDetector` answers (the token under a pointer) and what rendered text needs. `GitHubReferenceAttributedStringLinkifier` walks parsed markdown and attaches the resolved URL, skipping runs the author already linked and runs the parser marked as a code span or a code block. It runs after the parser, never over the markdown source, so fences and spans are the parser's call rather than a second half-parser's guess. A reference a style change cuts in half, as in `owner/repo#84**7**`, reaches the scanner truncated and would resolve to a different issue than the one on screen. Those are dropped: a link the reader was not shown is worse than no link. Linkifying sets attributes only and never touches characters, so the height stability `SidebarMetadataMarkdownRenderer` exists for is unaffected, and it happens inside the memoized parse so a row re-evaluating its body does no extra work. Only `owner/repo#123` resolves in the sidebar for now, because that is the one form naming its own repository. A bare `#123`, a `GH-123` and a commit SHA all need the repository the row is about, and the sidebar snapshot carries no slug: `PullRequestProbeService` resolves one per workspace on every probe cycle and drops it at the package boundary. Bringing it through is a separate change, and a test pins the current boundary so it is a deliberate one. ## Changelog Added: GitHub issue and pull request references written as `owner/repo#123` in sidebar metadata are now clickable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
`isTruncated` asks whether the character on the far side of a run boundary is whitespace. That answers a different question than the one it is standing in for. `see **#847**.` has an unbroken reference inside the bold run and an ordinary sentence period in the next run, and the check calls it truncated, so a common line of agent prose loses its link with no sign of why. Parentheses around a bold reference fail the same way from the other side. It is also too weak in the direction it was written for. `#84.**7**` reaches the scanner as `#84.`, whose trailing period the detector trims, so the hit ends before the run does and the edge check sees nothing wrong. The link points at issue 84 while the reader was shown 847. These four tests fail as written; the following commit replaces the heuristic. The several-references test now also asserts where each link points, since its spans were correct in isolation from their destinations. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
The two bounds in `sidebarBoundedDisplayString` cut in different places and the marker cannot mean the same thing for both. The character bound can stop in the middle of a token; the line bound breaks on `\n` before appending it, so it always ends on a whole line and a whole token. These cover both. A reference cut by the character bound must come back unparseable, or the row offers a link to an issue nobody wrote. A reference the line bound left intact must stay its own whitespace token, or a correct link is thrown away to guard against a cut that did not happen. Red before the fix: a line-bound cut ends `#847...` with the marker attached. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
…-parse (#15893) * test(sidebar): pin the truncation marker to a single ellipsis character Sidebar text is bounded before it is scanned for GitHub references, and the reference parser trims trailing `.` before reading a number. The ASCII `...` marker is therefore invisible to it: a row cut mid-number reads `owner/repo#84...`, parses as `owner/repo#84`, and offers a link to a different issue than the one on screen (#15882). These fail on the current marker. They cover both break paths, since a fix that reaches only the character bound leaves multi-line metadata blocks, which is most of them, still cutting into a parseable reference. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(sidebar): cut with an ellipsis character so a reference cannot re-parse `sidebarBoundedDisplayString` appended `...`. Every `.` in that marker is trimmed by the GitHub reference parser before it reads a number, so a row cut mid-number read `owner/repo#84...`, parsed as `owner/repo#84`, and offered a link to an issue the author never wrote (#15882). A wrong destination is the one outcome the linkifier avoids everywhere it can see the cut, and it cannot see this one: by the time it runs, the marker is ordinary text in the same run as the number. `…` is not trimmed, so the same cut fails to parse and stays plain text. It is also already the marker for every other user-visible truncation in the app (fork conversation menu titles, upload failure details); the remaining `...` sites are event-bus payloads and debug logs, which no one reads as prose. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(sidebar): pin the marker a bounded cut earns The two bounds in `sidebarBoundedDisplayString` cut in different places and the marker cannot mean the same thing for both. The character bound can stop in the middle of a token; the line bound breaks on `\n` before appending it, so it always ends on a whole line and a whole token. These cover both. A reference cut by the character bound must come back unparseable, or the row offers a link to an issue nobody wrote. A reference the line bound left intact must stay its own whitespace token, or a correct link is thrown away to guard against a cut that did not happen. Red before the fix: a line-bound cut ends `#847...` with the marker attached. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(sidebar): separate the truncation marker the line bound cannot justify Attaching the marker to the last token is what stops a mid-number cut from re-parsing, and only the character bound can produce one. The line bound breaks on `\n` before appending it, so the text it keeps ends with a whole reference; attaching the marker there unlinks something complete and correct instead of guarding anything. Give the line-bound cut a space before its marker so the reference stays its own whitespace-delimited token, and keep the marker attached on the character-bound path where the token may be cut. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
The first pass rewrote the markdown source before handing it to the parser, and a regular expression cannot stand in for CommonMark's grammar. It wrote `[#847](https://github.com/...)` into double-backtick code spans, `~~~` fenced blocks, indented code blocks, code spans that wrap a line, and link reference definitions, so a reader saw markdown source inside their own code. A stray ``` in prose read as an unterminated fence and silently stopped linking everything after it. Linking now happens in a remark plugin that walks the parsed document's text nodes. The parser has already decided which characters are code, which are an existing link and which are prose, so every one of those cases follows from the tree instead of being re-derived. Four other things came out of the same review: - A number that is still arriving is a prefix of the number the agent means, so `#8471` passed through `#847` on its way in and offered a link to a different issue. The token at the end of a streaming message is left as text until something follows it. - `memo` on the transcript's markdown did not hold: the repository slug was read through a context whose value is a fresh object each render, so every message re-parsed on every streamed token. The slug now has its own context holding a string. - The `git` call that reads the remote passed its timeout into `gitOutput`'s byte limit, so it used the 10 s default on the path that starts a session. It gets 2 s and a 4 KB limit. - The slug cache never expired and never shrank. Entries expire, a miss expires sooner so `git init` in a session directory is picked up, the map is bounded, and concurrent checks share one `git` run. An SSH remote written `git@GitHub.com:` is the same host as one written in lower case, and now reads the same way. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 30, 2026
* test(agent-chat): cover GitHub references in the transcript An agent writes `#847`, `owner/repo#847`, `GH-1234` and abbreviated commit SHAs constantly, and the terminal makes all four clickable. The chat transcript does not: remark-gfm autolinks URLs and nothing else, so the same sentence is a link in one pane and plain text in the other. These tests state what the transcript should do, and they fail because the module they call does not exist yet. The rules are deliberately the same as TerminalGitHubReferenceDetector, including the parts that say no: a bare number needs a known repository, a hex run needs both a digit and a letter, and checksum lengths are not commits. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(agent-chat): make GitHub references in the transcript clickable remark-gfm autolinks URLs and nothing else, so `#847`, `owner/repo#847`, `GH-1234` and `a360a95` were plain text in the chat transcript while the terminal pane made all four clickable. The same sentence behaved two different ways depending on which pane you read it in. The reading rules are the TypeScript half of TerminalGitHubReferenceDetector, refusals included: a bare number or a commit SHA needs a known repository, a hex run needs both a digit and a letter so `deadbeef` and `12345678` stay text, and checksum lengths are not commits. Only github.com remotes produce a slug, because a self-hosted host spells its issue URLs against its own domain. References become ordinary markdown links before the transcript parses the source, so the renderer is untouched and a reference gets the same styling and click handling as any other link. Code spans, fenced blocks, existing links and autolinks are left exactly as written. The repository comes from the working-directory check the session already runs, so there is no new round trip; Chat asks for the check when a session arrives without one, which is what happens to a session started outside the composer or restored on a reconnect. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(agent-chat): link GitHub references from the parsed document The first pass rewrote the markdown source before handing it to the parser, and a regular expression cannot stand in for CommonMark's grammar. It wrote `[#847](https://github.com/...)` into double-backtick code spans, `~~~` fenced blocks, indented code blocks, code spans that wrap a line, and link reference definitions, so a reader saw markdown source inside their own code. A stray ``` in prose read as an unterminated fence and silently stopped linking everything after it. Linking now happens in a remark plugin that walks the parsed document's text nodes. The parser has already decided which characters are code, which are an existing link and which are prose, so every one of those cases follows from the tree instead of being re-derived. Four other things came out of the same review: - A number that is still arriving is a prefix of the number the agent means, so `#8471` passed through `#847` on its way in and offered a link to a different issue. The token at the end of a streaming message is left as text until something follows it. - `memo` on the transcript's markdown did not hold: the repository slug was read through a context whose value is a fresh object each render, so every message re-parsed on every streamed token. The slug now has its own context holding a string. - The `git` call that reads the remote passed its timeout into `gitOutput`'s byte limit, so it used the 10 s default on the path that starts a session. It gets 2 s and a 4 KB limit. - The slug cache never expired and never shrank. Entries expire, a miss expires sooner so `git init` in a session directory is picked up, the map is bounded, and concurrent checks share one `git` run. An SSH remote written `git@GitHub.com:` is the same host as one written in lower case, and now reads the same way. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(agent-chat): add GitHub references gallery fixture Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rajinsyed
pushed a commit
to rajinsyed/supermux
that referenced
this pull request
Oct 3, 2026
…-parse (#15893) * test(sidebar): pin the truncation marker to a single ellipsis character Sidebar text is bounded before it is scanned for GitHub references, and the reference parser trims trailing `.` before reading a number. The ASCII `...` marker is therefore invisible to it: a row cut mid-number reads `owner/repo#84...`, parses as `owner/repo#84`, and offers a link to a different issue than the one on screen (#15882). These fail on the current marker. They cover both break paths, since a fix that reaches only the character bound leaves multi-line metadata blocks, which is most of them, still cutting into a parseable reference. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(sidebar): cut with an ellipsis character so a reference cannot re-parse `sidebarBoundedDisplayString` appended `...`. Every `.` in that marker is trimmed by the GitHub reference parser before it reads a number, so a row cut mid-number read `owner/repo#84...`, parsed as `owner/repo#84`, and offered a link to an issue the author never wrote (#15882). A wrong destination is the one outcome the linkifier avoids everywhere it can see the cut, and it cannot see this one: by the time it runs, the marker is ordinary text in the same run as the number. `…` is not trimmed, so the same cut fails to parse and stays plain text. It is also already the marker for every other user-visible truncation in the app (fork conversation menu titles, upload failure details); the remaining `...` sites are event-bus payloads and debug logs, which no one reads as prose. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(sidebar): pin the marker a bounded cut earns The two bounds in `sidebarBoundedDisplayString` cut in different places and the marker cannot mean the same thing for both. The character bound can stop in the middle of a token; the line bound breaks on `\n` before appending it, so it always ends on a whole line and a whole token. These cover both. A reference cut by the character bound must come back unparseable, or the row offers a link to an issue nobody wrote. A reference the line bound left intact must stay its own whitespace token, or a correct link is thrown away to guard against a cut that did not happen. Red before the fix: a line-bound cut ends `manaflow-ai/cmux#847...` with the marker attached. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(sidebar): separate the truncation marker the line bound cannot justify Attaching the marker to the last token is what stops a mid-number cut from re-parsing, and only the character bound can produce one. The line bound breaks on `\n` before appending it, so the text it keeps ends with a whole reference; attaching the marker there unlinks something complete and correct instead of guarding anything. Give the line-bound cut a space before its marker so the reference stays its own whitespace-delimited token, and keep the marker attached on the character-bound path where the token may be cut. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
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
Escapewhile command palette is visible so the key does not propagate to the terminal/browser belowEscapeTesting
xcodebuild -quiet -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-escape-prop-fail -only-testing:cmuxTests/CommandPaletteOpenShortcutConsumptionTests/testConsumesEscapeWhenPaletteIsVisible test(fails before fix withXCTAssertTrue failed, then passes after fix)xcodebuild -quiet -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-escape-prop-fail -only-testing:cmuxTests/CommandPaletteOpenShortcutConsumptionTests -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testEscapeDismissesVisibleCommandPaletteAndIsConsumed test(passes)codex --dangerously-bypass-approvals-and-sandbox --model gpt-5.3-codex -c model_reasoning_effort="xhigh" --search review --uncommitted(final pass: no actionable findings)Issues
Summary by cubic
Stops Escape from leaking to the terminal/browser when the command palette is open, pending open, or just dismissed. Escape cleanly dismisses the palette and is consumed, while respecting IME input.
Written for commit 2126cc5. Summary will update on new commits.
Summary by CodeRabbit