Anchor textbox autocomplete to the cursor#5021
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 Ctrl‑N/P first‑responder routing for terminal textboxes, refactors mention query/candidate shapes and trigger-specific insertion text, implements async ripgrep-backed file indexing with directory-skip/coalescing, redesigns controller staleness tracking, migrates mention UI to an NSPanel host, and expands tests covering these behaviors. ChangesMention Completion Workflow Overhaul
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant TextBoxInputTextView
participant TextBoxMentionCompletionController
participant TextBoxMentionIndexStore
participant NSPanel
User->>TextBoxInputTextView: Type trigger (@, /, $) or press Ctrl-N/Ctrl-P
TextBoxInputTextView->>AppDelegate: keyEquivalent handling
AppDelegate->>TextBoxInputTextView: forward Ctrl-N/P to first responder when routed
TextBoxInputTextView->>TextBoxMentionCompletionController: refresh(query, root)
TextBoxMentionCompletionController->>TextBoxMentionIndexStore: load candidates async
alt File indexing path
TextBoxMentionIndexStore->>TextBoxMentionIndexStore: spawn `rg --files` stream or enumerate filesystem
TextBoxMentionIndexStore->>TextBoxMentionIndexStore: apply shouldSkipIndexedDirectoryName and budgets
end
TextBoxMentionIndexStore-->>TextBoxMentionCompletionController: return candidates (with targetPath)
TextBoxMentionCompletionController-->>TextBoxInputTextView: update suggestions state (current/stale, selection)
TextBoxInputTextView->>NSPanel: create/update panel positioned at query anchor
NSPanel-->>User: show suggestions
User->>TextBoxInputTextView: navigate (Ctrl-N/P) or accept/submit
TextBoxInputTextView->>TextBoxMentionCompletionController: accept or submit based on trigger and state
TextBoxInputTextView->>NSPanel: dismiss panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 2 warnings)
✅ Passed checks (11 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR replaces the
Confidence Score: 4/5Safe to merge with one follow-up: the skill index scan runs synchronously on the actor executor on every cache miss, which can stall concurrent suggestion requests during warmup or after a root change. The file-index path is well-designed — detached tasks, coalesced refreshes, async git calls via TextBoxProcessTerminationStatus — but the skill-index path (skillIndex) takes a different shape: it calls scanSkillFiles (directory enumeration) and up to 800 String(contentsOf:) reads directly on the actor's executor with no detached task boundary. A user with many plugin skills or a slow volume will stall the actor for the duration of that scan, blocking any concurrent file-suggestion lookup until it completes. The rest of the refactor — panel lifecycle, keyboard routing, $ trigger, controller extraction — is sound. Sources/TextBoxMentionIndexStore.swift — specifically the skillIndex method at line 268 Important Files Changed
Sequence DiagramsequenceDiagram
participant TB as TextBoxInputTextView
participant MCC as TextBoxMentionCompletionController
participant IS as TextBoxMentionIndexStore (actor)
participant DT as Task.detached (utility)
participant FS as FileSystem / rg
TB->>MCC: refresh(for: query, rootDirectory:)
MCC->>IS: suggestions(for: query, rootDirectory:)
alt .file query with empty query (cache miss)
IS->>IS: scanRootFileSystemCandidates (async, await git check)
IS->>DT: fileIndexRefreshTask → scanFiles (detached)
DT->>FS: rg --files / FileManager.enumerator
FS-->>DT: paths
DT-->>IS: TextBoxMentionCandidateIndex (stored)
else .file query with non-empty query
IS->>DT: fileIndexRefreshTask → scanFiles (detached, coalesced)
DT-->>IS: TextBoxMentionCandidateIndex
IS-->>MCC: ranked suggestions
else .skill query (/ or $)
IS->>IS: skillIndex (SYNC on actor executor on cache miss)
IS->>FS: scanSkillFiles + skillName String(contentsOf:) [BLOCKING]
IS-->>MCC: ranked suggestions
end
MCC-->>TB: onStateChanged → syncMentionCompletionPopover
TB->>TB: show/reposition NSPanel
Reviews (19): Last reviewed commit: "Merge origin/main into textbox autocompl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1809,27 +1994,32 @@ actor TextBoxMentionIndexStore { | |||
| includingPropertiesForKeys: [.isDirectoryKey, .isRegularFileKey], | |||
| options: [.skipsHiddenFiles, .skipsPackageDescendants], | |||
| errorHandler: { _, _ in true } | |||
| ) else { | |||
| return [] | |||
| } | |||
| ) else { return result } | |||
|
|
|||
There was a problem hiding this comment.
scanFilesWithRipgrep
stdoutHandle.readData(ofLength: 64 * 1024) is a synchronous blocking I/O call, and process.waitUntilExit() blocks until the subprocess exits. Both run inside Task.detached(priority: .utility), which puts them on the Swift cooperative thread pool. Blocking a cooperative pool thread with synchronous process I/O stalls the thread until rg finishes — on a large repo this can be seconds — and can starve other concurrency work. On macOS, the runtime can promote blocked threads but there is no guarantee, and sustained blocking under keystroke pressure degrades the whole app.
The correct shape is to bridge the pipe to async using a completion-handler-to-async wrapper (e.g. FileHandle.bytes / AsyncBytes, DispatchIO, or a withCheckedContinuation wrapping terminationHandler) so the cooperative thread is yielded while waiting for I/O, matching the async caller at the call site.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| if query.query.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return Self.scanRootFiles(rootURL: URL(fileURLWithPath: rootDirectory, isDirectory: true)) | ||
| .prefix(Self.suggestionLimit) | ||
| .map { $0.suggestion(trigger: query.trigger) } | ||
| } |
There was a problem hiding this comment.
scanRootFiles synchronous I/O holds the actor executor
Self.scanRootFiles(...) calls FileManager.default.contentsOfDirectory synchronously without being wrapped in a detached task, unlike the non-empty-query path which uses Task.detached. Because fileSuggestions is an actor method, this call blocks the TextBoxMentionIndexStore actor's executor thread for the duration of the directory stat. While root scans are typically fast, the pattern is inconsistent with the rest of the method and could add noticeable latency if the root is on a mounted network volume or a slow path. Wrapping the call in a Task.detached(priority: .utility) (same as the full scan) keeps the actor free to service concurrent requests.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| private static func subsequenceMatchPositions(query: String, in text: String) -> [Int] { | ||
| let q = Array(query.lowercased()) | ||
| let t = Array(text.lowercased()) | ||
| guard !q.isEmpty, !t.isEmpty else { return [] } | ||
| var positions: [Int] = [] | ||
| var qIdx = 0 | ||
| for (tIdx, char) in t.enumerated() { | ||
| if qIdx < q.count, char == q[qIdx] { | ||
| positions.append(tIdx) | ||
| qIdx += 1 | ||
| if qIdx == q.count { return positions } | ||
| } | ||
| } | ||
| return qIdx == q.count ? positions : [] |
There was a problem hiding this comment.
subsequenceMatchPositions converts both the query and text to Array(text.lowercased()) for case-insensitive matching and returns indices into that array. Those indices are then passed to title.index(title.startIndex, offsetBy: position, ...) on the original, non-lowercased title. For characters where lowercased() changes the number of Unicode extended grapheme clusters (e.g., the German sharp-S ß lowercases to ss — two grapheme clusters instead of one), positions in the lowercased array diverge from positions in the original string, causing highlights to land on the wrong characters or crash with an out-of-bounds offset. File paths are overwhelmingly ASCII so this rarely triggers, but the invariant is broken. Matching and collecting Range<String.Index> directly in the original string (using String.compare with .caseInsensitive) avoids the mismatch entirely.
| private func shouldBypassHiddenMentionCompletionKeyboardInteraction() -> Bool { | ||
| guard let window else { return false } | ||
| guard NSApp.isActive, | ||
| window.isKeyWindow, | ||
| window.firstResponder === self, | ||
| mentionCompletionPanel?.isVisible == true else { | ||
| dismissMentionCompletions() | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private func shouldBypassMentionCompletionKeyboardAcceptance() -> Bool { | ||
| guard let query = mentionCompletionController.activeQuery, | ||
| query.kind == .skill, | ||
| query.query.isEmpty else { | ||
| return false | ||
| } | ||
| dismissMentionCompletions() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Both shouldBypassHiddenMentionCompletionKeyboardInteraction() and shouldBypassMentionCompletionKeyboardAcceptance() are named like pure predicates but call dismissMentionCompletions() inside their guard path. Any future caller added during a refactor (e.g., a new key or command handler) will trigger an unconditional dismissal simply by checking the condition, with no indication at the call site that state is being mutated. Separating the dismiss call to the call site (i.e., if shouldBypass... { dismissMentionCompletions(); return false }) makes the mutation visible and prevents accidental double-dismiss if a caller path changes.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 7348-7360: The test currently asserts the first suggestion is the
temp fixture which is environment-dependent; change the assertions to search the
suggestions array for an entry whose title equals "/nested-skill" (using
TextBoxMentionIndexStore.shared.suggestions result and TextBoxMentionQuery) and
then assert that that found suggestion's insertionText
hasPrefix("[/nested-skill]("); in other words, replace
XCTAssertEqual(suggestions.first?.title, "/nested-skill") and the XCTAssertTrue
on suggestions.first with a contains/first(where:) lookup for title ==
"/nested-skill" and assert properties on that found item.
In `@Sources/TextBoxInput.swift`:
- Around line 5241-5258: The new TextBoxMentionCompletionPanel is created
without a stable cmux.* window identifier; set a deterministic
NSUserInterfaceItemIdentifier on the panel after creation (e.g., assign
panel.identifier = NSUserInterfaceItemIdentifier("cmux.mentionCompletionPanel")
or another stable cmux.* name) so global window routing/ownership rules remain
deterministic—apply this change where TextBoxMentionCompletionPanel is
instantiated and assigned to mentionCompletionPanel.
- Around line 1904-1911: The ripgrep glob changes aren’t necessary—keep the
existing skip globs as-is—and add a stable NSPanel identifier when creating the
mention panel: inside makeMentionCompletionPanel(host:) (where the
TextBoxMentionCompletionPanel is instantiated) set panel.identifier =
NSUserInterfaceItemIdentifier("cmux.textbox.mentionCompletionPanel") so the
panel has a stable cmux.* identifier for window/panel management.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1220e732-67ea-44d1-8240-47eb1c4c3fcb
📒 Files selected for processing (4)
Sources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TextBoxInput.swift (1)
5252-5268:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a stable
cmux.*identifier to the mention completion panel.Line 5252 creates a user-visible
NSPanelwithoutpanel.identifier, which violates the window identity rule and can break shared window routing/ownership behavior.Suggested fix
let panel = TextBoxMentionCompletionPanel( contentRect: NSRect(origin: .zero, size: host.fittingSize), styleMask: [.borderless, .nonactivatingPanel], backing: .buffered, defer: false ) + panel.identifier = NSUserInterfaceItemIdentifier("cmux.textbox.mentionCompletionPanel") panel.isFloatingPanel = true panel.hidesOnDeactivate = trueAs per coding guidelines: “User-visible NSWindow, NSPanel, NSWindowController, SwiftUI Window, or SwiftUI WindowGroup must have a stable cmux.* window identifier.”
🤖 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 `@Sources/TextBoxInput.swift` around lines 5252 - 5268, The newly created TextBoxMentionCompletionPanel instance (assigned to local variable panel and stored in mentionCompletionPanel) needs a stable cmux.* window identifier; set panel.identifier to an NSUserInterfaceItemIdentifier with a constant string like "cmux.mentionCompletionPanel" (or another stable cmux.* name) immediately after creating the panel so the user-visible NSPanel has a persistent identity for shared window routing and ownership.
🤖 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.
Duplicate comments:
In `@Sources/TextBoxInput.swift`:
- Around line 5252-5268: The newly created TextBoxMentionCompletionPanel
instance (assigned to local variable panel and stored in mentionCompletionPanel)
needs a stable cmux.* window identifier; set panel.identifier to an
NSUserInterfaceItemIdentifier with a constant string like
"cmux.mentionCompletionPanel" (or another stable cmux.* name) immediately after
creating the panel so the user-visible NSPanel has a persistent identity for
shared window routing and ownership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba0564a5-c0ea-4972-b957-72461b216e97
📒 Files selected for processing (1)
Sources/TextBoxInput.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
7192-7194:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid asserting the first empty-query file suggestion.
This fixture now returns multiple valid empty-query candidates (
@Sources/,@README.md,@ZEmpty/, etc.), so pinningsuggestions.firstmakes the test brittle to ranking changes even when the behavior is still correct. Assert that@Sources/is present, then verify its metadata.Suggested change
- XCTAssertEqual(suggestions.first?.title, "`@Sources/`") - XCTAssertEqual(suggestions.first?.systemImageName, "folder") - XCTAssertTrue(suggestions.first?.insertionText.hasPrefix("[`@Sources/`](") == true) + let sourcesDirectory = suggestions.first { $0.title == "`@Sources/`" } + XCTAssertNotNil(sourcesDirectory) + XCTAssertEqual(sourcesDirectory?.systemImageName, "folder") + XCTAssertTrue(sourcesDirectory?.insertionText.hasPrefix("[`@Sources/`](") == true)🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 7192 - 7194, The test currently assumes suggestions.first is the "`@Sources/`" candidate which is brittle; instead locate the candidate by searching the suggestions array for an element whose title == "`@Sources/`" (e.g. using first(where: { $0.title == "`@Sources/`" })), assert that this found suggestion is non-nil, then verify its systemImageName == "folder" and that its insertionText.hasPrefix("[`@Sources/`](") is true; update assertions that reference suggestions.first to use the located suggestion variable.
🤖 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 `@Sources/TextBoxInput.swift`:
- Around line 2000-2004: The current refresh path does a second full tree walk
because scanFilesWithRipgrep performs its own BFS and git-ignore probes even
after scanDirectoryCandidateSeed produced directorySeed
(directorySeed.candidates and seenDirectoryRelativePaths); fix by reusing or
gating that seed: modify scanFilesWithRipgrep to accept an optional
directorySeed or directoryCandidates parameter and when a non-empty
directorySeed is provided, skip the separate BFS and git check-ignore phase and
instead derive directory rows directly from the ripgrep file stream (populate
directoryCandidates and update seenDirectoryRelativePaths from file paths
emitted by rg), or cache the computed directorySeed at a higher level and
early-return when it’s available so fileCandidates/reserveCapacity still happens
without a second traversal. Ensure changes reference scanFilesWithRipgrep,
scanDirectoryCandidateSeed, directorySeed, directoryCandidates, fileCandidates,
and seenDirectoryRelativePaths.
---
Duplicate comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 7192-7194: The test currently assumes suggestions.first is the
"`@Sources/`" candidate which is brittle; instead locate the candidate by
searching the suggestions array for an element whose title == "`@Sources/`" (e.g.
using first(where: { $0.title == "`@Sources/`" })), assert that this found
suggestion is non-nil, then verify its systemImageName == "folder" and that its
insertionText.hasPrefix("[`@Sources/`](") is true; update assertions that
reference suggestions.first to use the located suggestion variable.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cc9a9f87-f26f-4ac3-9dbc-a4cbecbdea2f
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
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 (2)
Sources/TextBoxInput.swift (2)
5468-5478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the bare
//$special-case scoped to Return.Line 5471 and Line 5506 route both Return and Tab through
shouldBypassMentionCompletionKeyboardAcceptance(), so an empty skill query now dismisses suggestions on Tab too. The PR only calls out Return as the submit path for bare/and$; Tab completion for root skill suggestions regresses here.Suggested fix
- private func shouldBypassMentionCompletionKeyboardAcceptance() -> Bool { + private func shouldBypassMentionCompletionReturnAcceptance() -> Bool { guard let query = mentionCompletionController.activeQuery, query.kind == .skill, query.query.isEmpty else { return false } dismissMentionCompletions() return true }case kVK_Return, kVK_ANSI_KeypadEnter: guard !flags.contains(.shift) else { return false } if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } + if shouldBypassMentionCompletionReturnAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion() case kVK_Tab: if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion()case `#selector`(NSResponder.insertNewline(_:)), - `#selector`(NSResponder.insertTab(_:)): + `#selector`(NSResponder.insertTab(_:)): + if commandSelector == `#selector`(NSResponder.insertNewline(_:)), + shouldBypassMentionCompletionReturnAcceptance() { + return false + } - if shouldBypassMentionCompletionKeyboardAcceptance() { return false } if mentionCompletionController.hasStaleSuggestions { return true } return acceptMentionCompletion()Also applies to: 5503-5508, 5531-5539
🤖 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 `@Sources/TextBoxInput.swift` around lines 5468 - 5478, The Tab case is incorrectly sharing the same "bare `/` / `$` submit" bypass logic as Return; update the kVK_Tab branch so it does not call shouldBypassMentionCompletionKeyboardAcceptance() (or otherwise apply the bare-query bypass) — keep that special-case scoped only to the kVK_Return / kVK_ANSI_KeypadEnter branch; in practice modify the kVK_Tab handling around mentionCompletionController.hasStaleSuggestions and acceptMentionCompletion() to omit the call to shouldBypassMentionCompletionKeyboardAcceptance() so Tab will not dismiss root-skill suggestions while Return still uses the special-case check in shouldBypassMentionCompletionKeyboardAcceptance().
2080-2326:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBound
rg/gitsubprocess lifetime.
scanFilesWithRipgrep,isGitWorkTree, andgitIgnoredRelativePathsrunProcessand then block on completion (process.waitUntilExit()/ stdout reads) with no timeout or cancellation path. SincefileIndexRefreshTaskstores a detached scan infileIndexRefreshTasks[rootDirectory]andstoreFileIndexclears it only afterscanTask.valuereturns, a hungrg/gitcan leave the in-flight entry stuck and cause subsequent refreshes that await the index to block indefinitely. Add a timeout (terminating theProcess) and ensurefileIndexRefreshTasks[rootDirectory]is cleared on timeout/failure.
🤖 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.
Outside diff comments:
In `@Sources/TextBoxInput.swift`:
- Around line 5468-5478: The Tab case is incorrectly sharing the same "bare `/`
/ `$` submit" bypass logic as Return; update the kVK_Tab branch so it does not
call shouldBypassMentionCompletionKeyboardAcceptance() (or otherwise apply the
bare-query bypass) — keep that special-case scoped only to the kVK_Return /
kVK_ANSI_KeypadEnter branch; in practice modify the kVK_Tab handling around
mentionCompletionController.hasStaleSuggestions and acceptMentionCompletion() to
omit the call to shouldBypassMentionCompletionKeyboardAcceptance() so Tab will
not dismiss root-skill suggestions while Return still uses the special-case
check in shouldBypassMentionCompletionKeyboardAcceptance().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2217da3-70b8-4900-8d1d-c6489cb2c7e2
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TextBoxInput.swift (1)
1730-1741: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the new mention-completion pipeline into separate Swift files.
This PR adds filesystem/git/
rgindexing plus controller state to a file that is already far past the repo's size limit. Please split the new index store/controller out ofTextBoxInput.swiftbefore merge.As per coding guidelines: “Do not add more than 250 lines to an existing production Swift file that is already over 800 lines” and “Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file.”
Also applies to: 2585-2603
🤖 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 `@Sources/TextBoxInput.swift` around lines 1730 - 1741, The TextBoxMentionIndexStore actor and its supporting types (e.g., TextBoxMentionIndexStore, CachedIndex, fileIndexTTL, maxCachedFileIndexes, directorySeedBatchSize and any helper methods used between the two highlighted ranges) must be extracted into one or more new Swift source files; move the entire actor, its nested structs/enums, constants, and any non-UI controller/state logic out of TextBoxInput.swift into a dedicated file (e.g., TextBoxMentionIndexStore.swift) and update visibility (public/internal/private) and imports so references from TextBoxInput.swift compile; ensure you also move or refactor any helper functions, extensions, and tests that reference these symbols, update import/module boundaries if needed, and run the build to fix any broken references.
🤖 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 `@Sources/TextBoxInput.swift`:
- Around line 5542-5545: The Escape handling currently dismisses mention
completions whenever mentionCompletionController.shouldShowPopover is true, but
this is also true during the loading-only state; change the check so Escape only
dismisses when actual suggestion rows exist. In the block that calls
shouldBypassHiddenMentionCompletionKeyboardInteraction(), guard
mentionCompletionController.shouldShowPopover AND a check for visible results
(e.g., mentionCompletionController.hasRows or
mentionCompletionController.visibleResultsCount > 0) before calling
dismissMentionCompletions(); apply the same change to the analogous block at the
other occurrence (the block around the second instance that currently mirrors
lines 5571-5575).
- Around line 1858-1867: The code can evict the looked-up rootDirectory during
pruneFileIndexCache(now:), so first check fileIndexesByRoot for rootDirectory
and, if a valid CachedIndex exists (now.timeIntervalSince(cached.createdAt) <
Self.fileIndexTTL), update its lastAccessedAt immediately (write back a new
CachedIndex with cached.index and cached.createdAt but lastAccessedAt: now)
before calling pruneFileIndexCache(now:); alternatively, reorder to update the
cache entry's lastAccessedAt for rootDirectory prior to invoking
pruneFileIndexCache(now:), ensuring pruneFileIndexCache cannot evict the very
entry you're refreshing (references: fileIndexesByRoot,
pruneFileIndexCache(now:), CachedIndex, fileIndexTTL, rootDirectory).
---
Outside diff comments:
In `@Sources/TextBoxInput.swift`:
- Around line 1730-1741: The TextBoxMentionIndexStore actor and its supporting
types (e.g., TextBoxMentionIndexStore, CachedIndex, fileIndexTTL,
maxCachedFileIndexes, directorySeedBatchSize and any helper methods used between
the two highlighted ranges) must be extracted into one or more new Swift source
files; move the entire actor, its nested structs/enums, constants, and any
non-UI controller/state logic out of TextBoxInput.swift into a dedicated file
(e.g., TextBoxMentionIndexStore.swift) and update visibility
(public/internal/private) and imports so references from TextBoxInput.swift
compile; ensure you also move or refactor any helper functions, extensions, and
tests that reference these symbols, update import/module boundaries if needed,
and run the build to fix any broken references.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 87f2a3e8-8cda-477a-aaa5-46edf0d4fe3d
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| if shouldBypassHiddenMentionCompletionKeyboardInteraction() { return false } | ||
| guard mentionCompletionController.shouldShowPopover else { return false } | ||
| dismissMentionCompletions() | ||
| return true |
There was a problem hiding this comment.
Don't let Escape dismiss while only the loading spinner is visible.
shouldShowPopover is also true during the loading-only state, so Esc/cancel now clears the active query before any suggestion rows arrive. The new behavior contract here says Escape should dismiss only when rows exist.
Suggested fix
- guard mentionCompletionController.shouldShowPopover else { return false }
+ guard mentionCompletionController.hasSuggestions else { return false }
dismissMentionCompletions()
return true
...
- guard mentionCompletionController.shouldShowPopover else { return false }
+ guard mentionCompletionController.hasSuggestions else { return false }
dismissMentionCompletions()
return trueAlso applies to: 5571-5575
🤖 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 `@Sources/TextBoxInput.swift` around lines 5542 - 5545, The Escape handling
currently dismisses mention completions whenever
mentionCompletionController.shouldShowPopover is true, but this is also true
during the loading-only state; change the check so Escape only dismisses when
actual suggestion rows exist. In the block that calls
shouldBypassHiddenMentionCompletionKeyboardInteraction(), guard
mentionCompletionController.shouldShowPopover AND a check for visible results
(e.g., mentionCompletionController.hasRows or
mentionCompletionController.visibleResultsCount > 0) before calling
dismissMentionCompletions(); apply the same change to the analogous block at the
other occurrence (the block around the second instance that currently mirrors
lines 5571-5575).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Stale automated review addressed by later commits; current CodeRabbit, Cursor, Greptile, and CI checks pass.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1bd3564. Configure here.
| guard mentionCompletionPanel?.isVisible == true, | ||
| !mentionCompletionRepositionIsScheduled else { | ||
| return | ||
| } |
There was a problem hiding this comment.
Completion panel hidden after refocus
Medium Severity
After the parent window loses key status, syncMentionCompletionPopover hides the mention panel but leaves completion active. Nothing calls sync again when the window becomes key, and reposition scheduling only runs while the panel is already visible. The popup can stay missing while @///$ completion is still active until the query changes enough to trigger a new lookup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1bd3564. Configure here.
1 participant
Summary
Testing
Not run: local XCTest/xcodebuild test actions per repo rule.
Note
Medium Risk
Touches global window key routing and async filesystem/git/rg indexing for completions; mistakes could affect textbox shortcuts or performance on large repos.
Overview
Reworks textbox @ / / $ mention completion so the UI is a cursor-anchored, non-activating child panel (replacing
NSPopover), with repositioning on window move/resize and a loading row when suggestions are still fetching.Keyboard and acceptance behavior is tightened: Ctrl-N / Ctrl-P (and related keys) are forwarded from
performKeyEquivalentonly for the textbox; navigation applies only when suggestions are current and visible; bare/and$skill triggers no longer auto-accept on Return;$inserts plain skill tokens while@//keep markdown links.Mention logic is split into dedicated modules with a heavier
TextBoxMentionIndexStore: longer-lived file index cache, coalesced background refreshes, optional ripgrep file listing, directory suggestions, git-ignore filtering, package-directory skips, index warmup on root change, and higher suggestion limits. The popover UI is simplified (title + query highlight, scroll cap).Adds
TextBoxMentionCompletionTestsand extends shortcut-routing tests; registers the completion panel as a non–Cmd+W auxiliary window.Reviewed by Cursor Bugbot for commit 1bd3564. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
$-triggered mention completion now accepts empty queries and inserts bare skill tokens;@//continue inserting link-style suggestionsImprovements
Tests