Chat Mention Tags -> Primary - #1068
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds multi-word ChangesMulti-word composer mentions
Lane PR badge behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/desktop/src/renderer/lib/lanePrBadge.ts (1)
30-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrefer a valid timestamp to a missing timestamp.
Lines 33-38 treat one valid timestamp and one missing or invalid timestamp as equal. The higher PR number then wins. This can select a PR with no recency data over known newer work.
Rank timestamp validity before timestamp value. Use PR number only when both timestamps are unavailable. Add a named regression test such as
it("prefers a valid updatedAt over a missing updatedAt", ...)inapps/desktop/src/renderer/lib/lanePrBadge.test.ts.Proposed fix
const aUpdated = Date.parse(a.updatedAt ?? ""); const bUpdated = Date.parse(b.updatedAt ?? ""); - if (Number.isFinite(aUpdated) && Number.isFinite(bUpdated) && aUpdated !== bUpdated) { + const aHasUpdated = Number.isFinite(aUpdated); + const bHasUpdated = Number.isFinite(bUpdated); + if (aHasUpdated !== bHasUpdated) return aHasUpdated ? -1 : 1; + if (aHasUpdated && aUpdated !== bUpdated) { return bUpdated - aUpdated; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/lanePrBadge.ts` around lines 30 - 38, Update comparePrimaryPr to rank timestamp validity before comparing timestamp values: prefer the PR with a finite updatedAt when the other timestamp is missing or invalid, compare recency when both are valid, and use githubPrNumber only when both are unavailable. Add a regression test named “prefers a valid updatedAt over a missing updatedAt” in the lane badge tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/lanes/LanePrHoverCard.tsx`:
- Around line 69-78: Update the LanePrHoverCard useEffect scroll handler to
ignore events whose target is contained within panelRef, while still closing on
viewport and external scrolls. Add the named regression test in
LanePrBadge.test.tsx verifying the card remains open when its own panel scrolls.
- Around line 84-135: Update LanePrHoverCard’s trigger and portaled panel
keyboard handling so an explicit keyboard action opens the card and focuses its
first interactive PR row after rendering, allowing focus to enter the portaled
content before scheduleClose dismisses it. Add Escape handling on the card to
close it and restore focus to triggerRef, while preserving existing pointer and
focus behavior. Add a regression test named like “moves focus into the multi-PR
hover card from the trigger” covering this flow.
---
Outside diff comments:
In `@apps/desktop/src/renderer/lib/lanePrBadge.ts`:
- Around line 30-38: Update comparePrimaryPr to rank timestamp validity before
comparing timestamp values: prefer the PR with a finite updatedAt when the other
timestamp is missing or invalid, compare recency when both are valid, and use
githubPrNumber only when both are unavailable. Add a regression test named
“prefers a valid updatedAt over a missing updatedAt” in the lane badge tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ff1095f-3d81-44a5-97ce-7035d598814d
⛔ Files ignored due to path filters (4)
docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (12)
apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsxapps/desktop/src/renderer/components/lanes/LanePrHoverCard.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.tsxapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.tsapps/desktop/src/shared/composerTriggers.test.tsapps/desktop/src/shared/composerTriggers.tsapps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swiftapps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
dde7827 to
f8ef13d
Compare
|
@codex review |
![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: f8ef13debf
ℹ️ 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".
| const displayText = token.kind === "mention" | ||
| ? mentionLabelsRef.current.get(tokenText)?.trim() || tokenText | ||
| : tokenText; |
There was a problem hiding this comment.
In the default plain-text composer, the textarea becomes transparent and this overlay supplies every visible glyph, so replacing the serialized @chat:<id> text with an arbitrarily sized title makes the two layers wrap and position following text differently. After selecting a mention whose title differs in width from its token, the visible caret, selection, and subsequent prose can appear on a different character or line from where edits actually occur; preserve layout-equivalent text or render mentions without relying on a text-mirroring overlay.
Useful? React with 👍 / 👎.
| // Once a title is an exact prefix, keep it visible while the user continues | ||
| // ordinary prose after the mention. Exact longer titles still win above this | ||
| // fallback, so a real multi-word title is selected before a shorter prefix. | ||
| if (loweredQuery.startsWith(`${target} `)) return 1; |
There was a problem hiding this comment.
This helper is also used to score subtitles, so a query such as @Primary · codex please review keeps a chat whose subtitle is Primary · codex selectable. Selection narrowing, however, only compares the query with the item's title; it therefore leaves the trigger unnarrowed and replaces the entire query, deleting please review. Apply this keep-alive rule only to titles, or carry the actually matched label into replacement-span calculation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 431fa9921e
ℹ️ 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".
| const AT_TRIGGER_RE = /(?:^|\s)(@([^\s@]*))$/; | ||
| // (file paths) and spaces (chat names are commonly multi-word), but not | ||
| // another `@` or a newline, so emails and cross-line prose never trigger. | ||
| const AT_TRIGGER_RE = /(?:^|[ \t\r\n])(@([^@\r\n]*))$/; |
There was a problem hiding this comment.
When a user types an exact file path followed by prose, such as @src/foo.ts about this, this regex makes the entire src/foo.ts about this string the active query. Desktop, ADE Code, and iOS then pass that query unchanged to file quick-open, whose scorePath requires the whole query to occur in the path, so the file suggestion disappears and composerTriggerForSelection can never preserve the trailing prose. Keep an exact leading file match alive or separate the searchable label from the trailing prose before querying.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/ade-cli/src/tuiClient/app.tsx (1)
7344-7355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated trailing-prose prefix check.
The pattern
query.startsWith(\${target} `)appears twice in this file: inmatchesMentionQuery(title/label matching) and in the PR title filter. Extract a small shared helper (for examplematchesWithTrailingProse(target, query)`) so both call sites stay in sync if the trailing-prose rule changes.♻️ Suggested helper
+function matchesWithTrailingProse(target: string, loweredQuery: string): boolean { + return target.includes(loweredQuery) || loweredQuery.startsWith(`${target} `); +}Then use it in both
matchesMentionQueryand the PR title filter instead of repeating the inline check.Also applies to: 7458-7459
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 7344 - 7355, Extract a shared helper for the trailing-prose prefix rule currently expressed as query.startsWith(`${target} `), then replace the duplicated checks in matchesMentionQuery and the PR title filter with that helper. Preserve the existing matching behavior and normalization at both call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 1857-1862: Move mention-label persistence out of AgentChatComposer
and associate it with the draft/session, or hydrate labels from each mention ID
before rendering so remounted composers recover titles. Preserve canonical
`@chat`:<id> storage while keeping plain and rich rendering user-facing. Add a
named regression test covering persisted mention titles after composer remount
in both modes, then run the required desktop type check, tests, build, and lint.
In `@apps/desktop/src/shared/composerTriggers.ts`:
- Around line 17-27: Update FILE_QUERY_RE in
apps/desktop/src/shared/composerTriggers.ts (lines 17-27) to preserve
extensionless paths containing spaces when followed by prose; synchronize
fileQueryRegex in apps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swift (lines
155-167) with the same behavior. Add a named regression test in
apps/desktop/src/shared/composerTriggers.test.ts (lines 64-68) covering an
extensionless path with spaces followed by prose and asserting the complete path
label is selected.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 7344-7355: Extract a shared helper for the trailing-prose prefix
rule currently expressed as query.startsWith(`${target} `), then replace the
duplicated checks in matchesMentionQuery and the PR title filter with that
helper. Preserve the existing matching behavior and normalization at both call
sites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 333cb050-385e-4da6-93b0-b3bb5755fad4
⛔ Files ignored due to path filters (4)
docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (14)
apps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/ChatCommandMenu.tsxapps/desktop/src/renderer/components/lanes/LanePrHoverCard.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsxapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.tsapps/desktop/src/shared/chatMentions.test.tsapps/desktop/src/shared/chatMentions.tsapps/desktop/src/shared/composerTriggers.test.tsapps/desktop/src/shared/composerTriggers.tsapps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swiftapps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/lib/lanePrBadge.test.ts
- apps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
- apps/desktop/src/renderer/lib/lanePrBadge.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6ba626d20
ℹ️ 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".
| const byAttention = lanePrAttentionRank(b) - lanePrAttentionRank(a); | ||
| if (byAttention !== 0) return byAttention; | ||
| return comparePrimaryPr(a, b); | ||
| return pickPrimaryPr(candidates); |
There was a problem hiding this comment.
When a lane has no open or draft PR and contains both a merged and a closed PR, this newly selected helper compares lifecycle rank before updatedAt (merged ranks above closed), so an arbitrarily old merged PR always wins over a recently closed one. This makes the collapsed badge show stale history instead of the documented newest terminal activity; merged and closed PRs should share a terminal rank before applying the recency comparison.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1567fcd4c
ℹ️ 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".
| <div | ||
| ref={panelRef} |
There was a problem hiding this comment.
In both the LanesPage lane row and SessionCard, React events from this portal still bubble through the React tree to the enclosing row handler. Clicking non-interactive panel chrome such as the “Pull requests” header therefore selects the lane/session, even though interacting with the badge previously swallowed both click and mousedown events; stop those events on the portal panel itself while retaining the candidate-row handlers.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4610c48f
ℹ️ 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".
| onClick={(event) => event.stopPropagation()} | ||
| onMouseDown={(event) => event.stopPropagation()} |
There was a problem hiding this comment.
When a PR row inside this portal is clicked, its child handler navigates to /prs, but this bubble handler only stops propagation and leaves anchor set. App.tsx keeps both the Work and Lanes surfaces mounted while hiding them, so the portal attached to document.body escapes that hidden ancestor and remains visible over the destination PR screen; close the card when an action inside its content is activated or when its owning surface becomes inactive.
Useful? React with 👍 / 👎.
| // Prefer a longer path prefix when multiple indexed paths share the same | ||
| // beginning. The tiny fractional tie-break preserves existing score tiers. | ||
| best = Math.max(best, score + Math.min(prefix.length, 999) / 1000); |
There was a problem hiding this comment.
When an extensionless path is itself a prefix of another valid path containing spaces—for example files src/foo and src/foo bar with query src/foo bar—the full path receives the normal exact score of 1000, while the shorter file reaches this fallback and receives 1000.007. Sorting therefore places src/foo above the exact requested file, so pressing Enter in quick-open or the composer can attach the wrong file; keep fallback scores below an exact full-query match while using the fractional value only to break ties within the fallback tier.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db18fe5fa
ℹ️ 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".
| const AT_TRIGGER_RE = /(?:^|\s)(@([^\s@]*))$/; | ||
| // (file paths) and spaces (chat names are commonly multi-word), but not | ||
| // another `@` or a newline, so emails and cross-line prose never trigger. | ||
| const AT_TRIGGER_RE = /(?:^|[ \t\r\n])(@([^@\r\n]*))$/; |
There was a problem hiding this comment.
In the default plain-text composer, selecting a file inserts @src/foo.ts , but typing any following prose makes this regex detect the confirmed attachment again with a query such as src/foo.ts fix it. ChatCommandMenu reduces that back to src/foo.ts, immediately restores the same cached file row, and the composer then consumes Enter by selecting the file instead of sending the message. Treat confirmed file/mention tokens followed by whitespace as terminated triggers so users can continue the same line normally.
Useful? React with 👍 / 👎.
| // ordinary prose after the mention. This is intentionally title-only: a | ||
| // subtitle prefix is not a confirmed label, so it must not widen the | ||
| // replacement span and consume the prose that follows it. | ||
| if (allowTrailingProse && loweredQuery.startsWith(`${target} `)) return 1; |
There was a problem hiding this comment.
When entity titles overlap, trailing-prose matching gives every matching prefix the same score. For example, with titles Foo and Foo Bar, the query @Foo Bar please scores both as 1, so recency can place Foo first; pressing Enter then selects that entity and leaves Bar please as prose despite the user having typed the longer title exactly. Rank the longest confirmed title prefix ahead of shorter prefixes before applying the recency tie-break.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/shared/composerTriggers.ts`:
- Around line 75-87: Update the composer trigger candidate-matching helper
around addCandidateLabel and composerFileSearchQuery to accept or derive the
valid file-search match prefix from the selected result, so extensionless
path-prefix queries like “src/my review this” preserve trailing prose instead of
returning the original trigger. Add a named regression test covering canonical
selection from an extensionless path prefix, or document an exact equivalent
verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a675b425-06cb-4122-a84d-ea2125f35b0f
📒 Files selected for processing (14)
apps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/main/services/files/fileSearchIndexService.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/lanes/LanePrHoverCard.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsxapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.tsapps/desktop/src/shared/composerTriggers.test.tsapps/desktop/src/shared/composerTriggers.tsapps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swiftapps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsx
- apps/desktop/src/shared/composerTriggers.test.ts
- apps/ade-cli/src/tuiClient/app.tsx
- apps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swift
- apps/desktop/src/renderer/lib/lanePrBadge.test.ts
- apps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
- apps/desktop/src/renderer/lib/lanePrBadge.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed0084130
ℹ️ 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".
| } else { | ||
| const next = replaceComposerTriggerSpan(draft, commandMenuTrigger, `@${item.path} `); | ||
| const trigger = composerTriggerForSelection(commandMenuTrigger, item.path); | ||
| const next = replaceComposerTriggerSpan(draft, trigger, `@${item.path} `); |
There was a problem hiding this comment.
When the selected path contains spaces, this inserts it verbatim (for example, @src/my folder ), but findConfirmedComposerTokens and composerTriggerHasConfirmedPrefix only read the first \S+ body (src/my) while attachedPaths contains the full path. Consequently the plain composer neither renders the attachment as a chip nor recognizes it as terminated, and typing prose reopens the @ menu so Enter can be intercepted. Fresh evidence beyond the earlier confirmed-token report is that the newly added spaced-path search and selection flow now deliberately produces these tokens; use a shared encoding/parser that can round-trip whitespace-containing paths.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484b9622f7
ℹ️ 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".
| } | ||
|
|
||
| function composerPathPrefixForSelection(query: string, selectedLabel: string): string { | ||
| if (!/[\\/]/.test(selectedLabel)) return ""; |
There was a problem hiding this comment.
When the new root-level fallback returns a file whose name continues beyond the typed prefix—for example, root file my file for @my review this—this guard rejects the accepted prefix solely because the selected path has no slash. None of the remaining candidate labels matches the query, so selecting the row replaces the entire trigger and deletes review this. Allow root filenames to use the same accepted-prefix boundary, and mirror the fix in Swift.
Useful? React with 👍 / 👎.
| enum WorkComposerTriggerDetector { | ||
| private static let slashRegex = try! NSRegularExpression(pattern: "(?:^|\\s)/([^\\s/]*)$") | ||
| private static let atRegex = try! NSRegularExpression(pattern: "(?:^|\\s)@([^\\s@]*)$") | ||
| private static let atRegex = try! NSRegularExpression(pattern: "(?:^|[ \\t\\r\\n])@([^@\\r\\n]*)$") |
There was a problem hiding this comment.
On iOS, after selecting a file such as src/foo.ts, typing any prose after the inserted @src/foo.ts chip now matches this space-consuming regex again; fileSearchQuery reduces the query back to the path and the same suggestion strip reappears. Fresh evidence beyond the earlier desktop/TUI reports is that the iOS coordinator already tracks confirmed inserted ranges in chips, but detectTrigger() never excludes a chip followed by whitespace, so confirmed chips should terminate detection there as well.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328240be64
ℹ️ 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".
| const isRootLevelPath = !normalized.includes("/") && !normalized.includes("\\"); | ||
| if (!needle.includes("/") && !needle.includes("\\") && !isRootLevelPath) return -1; |
There was a problem hiding this comment.
When a generic multiword query is used in the Files overlay or global ADE search, this branch treats every root-level file as eligible for composer-style trailing-prose matching. For example, searching for package manager now returns a root package.json because it starts with the shortened prefix package, even though the complete query does not occur in the path; before this change it returned no filename match. Make this fallback an explicit composer-only quick-open mode rather than changing shared quick-open semantics.
Useful? React with 👍 / 👎.
| function matchesMentionTarget(target: string, query: string): boolean { | ||
| return target.includes(query) || query.startsWith(`${target} `); |
There was a problem hiding this comment.
In ADE Code, the new keep-alive matching is applied to lane/chat suggestions and PR titles, but the recent-commit filter still checks subject.includes(query). Consequently, after typing an exact commit subject followed by prose, such as @Fix parser please inspect, that commit disappears before Enter can select it, unlike the other newly supported multiword targets. Use this helper for commit subjects as well so selection can preserve the trailing prose.
Useful? React with 👍 / 👎.
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)
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
2752-2765: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset existing rich chips when a label is cleared.
hydrateMentionChipsInEditorreturns whenlabels.sizeis zero and skips existing chips when the label is empty. This differs fromcreateComposerTokenChipNode, which falls back to the canonical token. IfmentionLabelsclears an invalid or empty title while the rich editor stays mounted, the old title remains visible even though serialization still emits the canonical token.Update existing chips to display
tokenwhen no non-empty label exists, and do not return before the existing-chip pass.Proposed fix
- if (!editor || !labels.size) return false; + if (!editor) return false; let changed = false; editor.querySelectorAll<HTMLElement>("[data-composer-chip='mention']").forEach((chip) => { const token = chip.dataset.composerChipText; - const label = token ? labels.get(token)?.trim() : undefined; - if (!token || !label) return; + if (!token) return; + const label = labels.get(token)?.trim() || token; const labelNode = chip.firstElementChild; if (labelNode) labelNode.textContent = label; - chip.title = `${label} — ${token}`; + chip.title = label === token ? token : `${label} — ${token}`; });As per coding guidelines, record a named regression test or exact alternate verification for every accepted correctness finding. Add
falls back to the canonical mention token when a persisted rich mention label is clearedinapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx, then run the required desktop type check, tests, build, and lint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 2752 - 2765, The hydrateMentionChipsInEditor function must always process existing rich mention chips, including when mentionLabelsRef is empty or a label is blank. Remove the early return tied to labels.size, use the canonical token as the displayed label when no trimmed persisted label exists, and keep the title synchronized with that fallback. Add the named regression test “falls back to the canonical mention token when a persisted rich mention label is cleared” in the specified test file, then run the required desktop type check, tests, build, and lint.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsx`:
- Around line 589-594: Update the test “closes a confirmed spaced-file mention
while typing trailing prose” to advance MENTION_REMOTE_DEBOUNCE_MS after writing
the trailing prose, then perform the same async-effect flush used before the
first assertion. Keep the existing quickOpen assertion unchanged and ensure the
pending lookup has executed before it runs.
In `@apps/desktop/src/main/services/files/fileService.test.ts`:
- Around line 573-577: Update the regression query in the quickOpen test to use
the complete spaced fixture path, changing the query from “my review this” to
“my file review this” while preserving the existing named regression test and
assertions.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 2752-2765: The hydrateMentionChipsInEditor function must always
process existing rich mention chips, including when mentionLabelsRef is empty or
a label is blank. Remove the early return tied to labels.size, use the canonical
token as the displayed label when no trimmed persisted label exists, and keep
the title synchronized with that fallback. Add the named regression test “falls
back to the canonical mention token when a persisted rich mention label is
cleared” in the specified test file, then run the required desktop type check,
tests, build, and lint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a381905-f0a9-4850-9355-8bbc808030e2
📒 Files selected for processing (12)
apps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/main/services/files/fileSearchIndexService.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/shared/chatMentions.test.tsapps/desktop/src/shared/chatMentions.tsapps/desktop/src/shared/composerTriggers.test.tsapps/desktop/src/shared/composerTriggers.tsapps/ios/ADE/Views/Work/WorkComposerTypedTriggers.swiftapps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/shared/chatMentions.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/shared/composerTriggers.test.ts
- apps/desktop/src/main/services/files/fileSearchIndexService.ts
- apps/ios/ADETests/WorkComposerTriggerDetectorTests.swift
- apps/desktop/src/shared/chatMentions.ts
- apps/ade-cli/src/tuiClient/app.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aa444f9eb
ℹ️ 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".
| suggestions = WorkComposerSlashCatalog.suggestions(provider: provider, query: match.query) | ||
| case .at: | ||
| scheduleFileFetch(query: match.query) | ||
| scheduleFileFetch(query: WorkComposerTriggerDetector.fileSearchQuery(for: match.query)) |
There was a problem hiding this comment.
When an iOS user types an extensionless path followed by prose, such as @src/my review this, this line sends the whole multiword query to SyncService.quickOpen. The new host-side prefix matching only runs when allowComposerPrefixFallback is true, but the Swift request does not expose or send that flag, so src/my folder disappears and the new selection-range preservation cannot be reached. Extend the iOS sync request and enable the composer-only mode here.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| ); | ||
| return composerTriggerHasConfirmedPrefix(prompt, trigger, { | ||
| isFile: confirmedFile, | ||
| isMention: isChatMentionTokenBody, |
There was a problem hiding this comment.
After selecting a commit or PR suggestion, the inserted @commit:<sha> or @pr:<id> is not accepted by isChatMentionTokenBody, while confirmedFile only recognizes file rows. The new space-consuming trigger therefore remains active, renders an empty mention palette, and makes arrow keys palette-owned instead of moving through the prompt. Fresh evidence beyond the earlier confirmed-token report is that the same TUI still creates commit and PR suggestions in loadRemoteSuggestions; recognize every selectable canonical token or confirm it from selectedMentions.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a0a6e1ae0
ℹ️ 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".
| const STARTUP_RECONNECT_DELAY_MS = 3_000; | ||
|
|
||
| function matchesMentionTarget(target: string, query: string): boolean { | ||
| return target.includes(query) || query.startsWith(`${target} `); |
There was a problem hiding this comment.
When target labels overlap, this predicate treats every confirmed prefix equally. For example, with a lane named Foo and a commit titled Foo Bar, the query @Foo Bar please keeps both candidates; publishSuggestions at line 7400 always places local lane/chat rows before remote commit/PR rows, so Enter selects Foo and leaves Bar please instead of selecting the exact longer target. Fresh evidence beyond the earlier shared mention-title report is that this separate ADE Code helper also governs commit and PR matches without carrying prefix length into ordering; rank the longest confirmed label before preserving the existing source order.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c144a5e949
ℹ️ 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".
| // behind an explicit composer-only mode so generic quick-open searches keep | ||
| // their whole-query semantics. | ||
| const isRootLevelPath = !normalized.includes("/") && !normalized.includes("\\"); | ||
| if (!needle.includes("/") && !needle.includes("\\") && !isRootLevelPath) return -1; |
There was a problem hiding this comment.
When a nested extensionless file is referenced by basename and followed by prose—for example @README review this for docs/README—the full query has no direct match, and this guard rejects the indexed path because the query has no separator while the path does. The suggestion therefore disappears in the desktop, ADE Code, and iOS composers even though ordinary quick-open can match the same basename; apply the composer fallback to nested basename prefixes as well as root-level files.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337a5f302c
ℹ️ 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".
| for (let wordCount = words.length - 1; wordCount > 0; wordCount -= 1) { | ||
| const prefix = words.slice(0, wordCount).join(" "); | ||
| if (!allowRootLevelFile && !/[\\/]/.test(prefix)) continue; | ||
| if (selectedLabel.toLowerCase().startsWith(prefix.toLowerCase())) return prefix; |
There was a problem hiding this comment.
When composer quick-open returns a nested extensionless filename that continues beyond a basename prefix—for example, docs/my file for @my review this—this comparison checks my only against the full docs/my file path. The separate basename candidate is my file, so neither candidate matches the typed prefix and composerTriggerForSelection replaces the entire trigger, deleting review this. Fresh evidence beyond the existing nested-basename availability report is that the now-reachable selection path still loses prose; compare the accepted prefix against the basename as well, including in the mirrored Swift implementation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae5201d0a2
ℹ️ 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".
| const matchesPath = /[\\/]/.test(prefix) | ||
| ? selectedLabel.toLowerCase().startsWith(normalizedPrefix) | ||
| : pathComponents.some((component) => component.toLowerCase().startsWith(normalizedPrefix)); |
There was a problem hiding this comment.
When composer quick-open keeps a nested extensionless file through scorePathForNeedle's substring fallback—for example, @EAD review this returning docs/README—this startsWith-only check cannot recover EAD as the accepted file prefix. composerTriggerForSelection therefore replaces the full trigger and silently deletes review this; accept the same component-substring boundary used by the index and mirror that behavior in Swift.
Useful? React with 👍 / 👎.
| while (end < limit) { | ||
| const character = text[end]!; | ||
| if (character === "@" || character === "\r" || character === "\n") break; | ||
| if (character === " " || character === "\t") { |
There was a problem hiding this comment.
When a user selects a valid filename containing @, such as assets/icon@2x.png, in the desktop textarea or ADE Code, this unconditional break prevents the exact attached path from reaching isFile. The inserted token is consequently not recognized or rendered as a confirmed file chip, even though the previous non-whitespace parser accepted such paths; only terminate at @ when it is not part of a caller-confirmed file path.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
The PR expands chat mentions and file references to support multi-word labels while preserving trailing prose across desktop, TUI, and iOS, and improves multi-PR badge behavior.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (17): Last reviewed commit: "handle substring and at-sign file matche..." | Re-trigger Greptile
Context used:
ade codeTUI