Fix ADE-77 TUI papercuts and ADE-70 Files Workbench gaps#646
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
More reviews will be available in 38 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe PR adds terminal restore safety and connection-retry UX to the ADE CLI TUI, debounces and caches remote ChangesADE CLI TUI hardening and UX improvements
Desktop files workbench and supporting services
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
arul28
force-pushed
the
ade/linear-validation-audit-047d470e
branch
from
96260d8 to
e36c66a
Compare
arul28
force-pushed
the
ade/linear-validation-audit-047d470e
branch
from
e36c66a to
26a85a7
Compare
arul28
changed the title
![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 (3)
apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx (1)
352-359: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a collision-safe React key for legacy options.
Now that all options render,
key={option.value}can collide when values repeat, which can cause unstable row updates.🐛 Proposed fix
- <Box key={option.value} flexDirection="row"> + <Box key={`${option.value}:${index}`} flexDirection="row">🤖 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/components/ApprovalPrompt.tsx` around lines 352 - 359, The legacy options list in ApprovalPrompt now renders every entry, so using key={option.value} can create duplicate React keys when values repeat. Update the key generation in the options.map block to use a collision-safe identifier that is unique per rendered row, such as the existing optionId or another stable composite based on the option’s position and identity. Keep the change localized to the ApprovalPrompt rendering logic so each Box gets a unique key.apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts (1)
772-817: 🩺 Stability & Availability | 🟠 MajorAwait async
dispose()in both disconnect branches
entry.dispose(true, false)now returnsPromise<void>here, but both paths fire-and-forget it. Rejected disposals can escape the local.catch(), anddisconnect()can finish before teardown completes. Await or return the promise in both branches, and in the resolved-entry path wait for it alongsideforwardsClosed.🤖 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/main/services/remoteRuntime/remoteConnectionPool.ts` around lines 772 - 817, The disconnect flow in remoteConnectionPool’s disconnect() is not awaiting async cleanup from entry.dispose(true, false), so teardown can finish early and disposal rejections can bypass the local catch. Update both the resolved-entry branch and the pending-entry promise handler to await or return the dispose promise, and make the resolved-entry path wait for that cleanup together with forwardsClosed before exiting.apps/ade-cli/src/tuiClient/app.tsx (1)
6843-6935: 🎯 Functional Correctness | 🟡 MinorAdd retry coverage for embedded ADE CLI launches. The startup-reconnect test exercises only one mocked
connectToAdepath; add aforceEmbedded/headless case so retry behavior is covered in both embedded and socket-backed modes.🤖 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 6843 - 6935, The startup retry coverage in app.tsx only exercises the socket-backed connectToAde flow, so add a test case that runs the same retry path with forceEmbedded enabled and headless mode to cover embedded ADE CLI launches as well. Update the existing startup-reconnect test setup around connectToAde and the retry effect in retryStartupConnection/useEffect so both embedded and socket-backed startup failures verify the automatic reconnect behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
apps/ade-cli/src/tuiClient/components/SlashPalette.tsx (1)
137-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShort-circuit before building
rows.When the query is not slash-prefixed, the component returns
nullanyway, so computingpaletteCommands(...)first is avoidable work.♻️ Proposed change
- const rows = paletteCommands(query, userCommands, { provider }); - if (!query.startsWith("/")) return null; + if (!query.startsWith("/")) return null; + const rows = paletteCommands(query, userCommands, { provider });🤖 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/components/SlashPalette.tsx` around lines 137 - 139, Move the non-slash guard in SlashPalette before the paletteCommands call so the component returns null immediately when query does not start with "/". Use the existing query.startsWith("/") check in SlashPalette to short-circuit first, then build rows only for valid slash queries to avoid unnecessary work.apps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsx (1)
244-249: 📐 Maintainability & Code Quality | 🔵 TrivialUse the shared reconnect delay constant here.
3_000matchesSTARTUP_RECONNECT_DELAY_MSinapps/ade-cli/src/tuiClient/app.tsx. Export that constant and use it in this test so the assertion stays aligned if the reconnect schedule changes.🤖 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/__tests__/appPolling.test.tsx` around lines 244 - 249, The test is hardcoding the reconnect wait instead of using the shared startup reconnect delay, so update the polling test to reference the exported reconnect delay constant from app.tsx rather than 3_000. Expose STARTUP_RECONNECT_DELAY_MS from app.tsx if needed, then use that symbol in appPolling.test.tsx so the expectation stays aligned with reconnect timing changes.
🤖 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/app.tsx`:
- Around line 10765-10775: While disconnected in app.tsx, the input guard in the
connection error branch should swallow every key except the explicit exit and
retry actions handled by requestAppExit, retryStartupConnection, and isCtrlInput
for Ctrl+C. Update the early-return logic around connectionRef.current and error
so keys like Esc, Tab, and other global shortcuts do not fall through and mutate
hidden TUI state; keep only the allowed retry/exit paths and block everything
else until the connection is restored.
In `@apps/desktop/src/renderer/components/files/v2/EditorGroup.tsx`:
- Around line 104-114: The global keydown listener in EditorGroup’s useEffect is
too broad and can intercept Cmd/Ctrl+S from unrelated inputs while the group is
active. Update the EditorGroup shortcut handling so saveActive() only fires when
focus is within this group and not inside Monaco or other text inputs/dialog
controls, using the existing activeTab, canEdit, and props.isActiveGroup checks
plus a focus/container-based guard around the onKey handler.
---
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 6843-6935: The startup retry coverage in app.tsx only exercises
the socket-backed connectToAde flow, so add a test case that runs the same retry
path with forceEmbedded enabled and headless mode to cover embedded ADE CLI
launches as well. Update the existing startup-reconnect test setup around
connectToAde and the retry effect in retryStartupConnection/useEffect so both
embedded and socket-backed startup failures verify the automatic reconnect
behavior.
In `@apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx`:
- Around line 352-359: The legacy options list in ApprovalPrompt now renders
every entry, so using key={option.value} can create duplicate React keys when
values repeat. Update the key generation in the options.map block to use a
collision-safe identifier that is unique per rendered row, such as the existing
optionId or another stable composite based on the option’s position and
identity. Keep the change localized to the ApprovalPrompt rendering logic so
each Box gets a unique key.
In `@apps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts`:
- Around line 772-817: The disconnect flow in remoteConnectionPool’s
disconnect() is not awaiting async cleanup from entry.dispose(true, false), so
teardown can finish early and disposal rejections can bypass the local catch.
Update both the resolved-entry branch and the pending-entry promise handler to
await or return the dispose promise, and make the resolved-entry path wait for
that cleanup together with forwardsClosed before exiting.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsx`:
- Around line 244-249: The test is hardcoding the reconnect wait instead of
using the shared startup reconnect delay, so update the polling test to
reference the exported reconnect delay constant from app.tsx rather than 3_000.
Expose STARTUP_RECONNECT_DELAY_MS from app.tsx if needed, then use that symbol
in appPolling.test.tsx so the expectation stays aligned with reconnect timing
changes.
In `@apps/ade-cli/src/tuiClient/components/SlashPalette.tsx`:
- Around line 137-139: Move the non-slash guard in SlashPalette before the
paletteCommands call so the component returns null immediately when query does
not start with "/". Use the existing query.startsWith("/") check in SlashPalette
to short-circuit first, then build rows only for valid slash queries to avoid
unnecessary work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: feac72d9-1d64-481f-adc0-a3558a510f34
⛔ Files ignored due to path filters (7)
CHANGELOG.mdis excluded by!*.mdchangelog/index.mdxis excluded by!changelog/**changelog/v1.2.9.mdxis excluded by!changelog/**docs.jsonis excluded by!docs.jsondocs/features/ade-code/README.mdis excluded by!docs/**docs/features/files-and-editor/README.mdis excluded by!docs/**docs/features/files-and-editor/editor-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (25)
apps/ade-cli/src/tuiClient/__tests__/ApprovalPrompt.test.tsxapps/ade-cli/src/tuiClient/__tests__/Palettes.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/ade-cli/src/tuiClient/components/MentionPalette.tsxapps/ade-cli/src/tuiClient/components/SlashPalette.tsxapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/renderer/components/files/monacoModelRegistry.test.tsapps/desktop/src/renderer/components/files/monacoModelRegistry.tsapps/desktop/src/renderer/components/files/v2/EditorGroup.tsxapps/desktop/src/renderer/components/files/v2/EditorGroups.tsxapps/desktop/src/renderer/components/files/v2/FilesWorkbench.tsxapps/desktop/src/renderer/components/files/v2/ViewerHost.tsxapps/desktop/src/renderer/components/files/v2/WarmEmptyState.tsxapps/desktop/src/renderer/components/files/v2/overlays.tsxapps/desktop/src/renderer/components/files/v2/recentFiles.test.tsapps/desktop/src/renderer/components/files/v2/recentFiles.tsapps/desktop/src/renderer/components/files/v2/viewers/CodeViewer.tsxapps/desktop/src/renderer/components/files/v2/viewers/types.tsapps/desktop/src/renderer/components/history/CommitDetailPanel.tsxapps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.test.tsapps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.ts
Refs ADE-77 Refs ADE-70
arul28
force-pushed
the
ade/linear-validation-audit-047d470e
branch
from
26a85a7 to
41f1900
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+116
to
134
| ...Array.from({ length: Math.max(0, VISIBLE_ROWS - 1) }, () => bodyLine("", paletteWidth)), | ||
| ]; | ||
| const footer = bottomLine("Type to filter · Esc close", paletteWidth); | ||
| return ( | ||
| <Box width={paletteWidth} flexDirection="column"> | ||
| {paletteLine(header, theme.color.violet)} | ||
| {rowLines.map((line, index) => ( | ||
| <React.Fragment key={index}> | ||
| {paletteLine(line, index === 0 ? theme.color.t3 : theme.color.t2)} | ||
| </React.Fragment> | ||
| ))} | ||
| {paletteLine(bodyLine("No reference matches this query.", paletteWidth), theme.color.t3)} | ||
| {paletteLine(footer, theme.color.t4)} | ||
| </Box> | ||
| ); | ||
| } | ||
| const total = suggestions.length; | ||
| const safeIndex = Math.max(0, Math.min(selectedIndex, total - 1)); | ||
| const half = Math.floor(VISIBLE_ROWS / 2); |
There was a problem hiding this comment.
The empty-state branch renders VISIBLE_ROWS body lines (the first already containing " No references found") and then an extra paletteLine(bodyLine("No reference matches this query.", ...)) outside the mapped rows. This adds an unwanted extra body line, making the empty-state palette one row taller than the populated one, and displays two overlapping "no results" messages. The identical pattern exists in SlashPalette.tsx (" No matching commands" + "No command matches this query."). The standalone paletteLine call after the map should be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ade-cli/src/tuiClient/components/MentionPalette.tsx
Line: 116-134
Comment:
**Redundant "no results" line in empty-state palette**
The empty-state branch renders `VISIBLE_ROWS` body lines (the first already containing `" No references found"`) and then an extra `paletteLine(bodyLine("No reference matches this query.", ...))` outside the mapped rows. This adds an unwanted extra body line, making the empty-state palette one row taller than the populated one, and displays two overlapping "no results" messages. The identical pattern exists in `SlashPalette.tsx` (`" No matching commands"` + `"No command matches this query."`). The standalone `paletteLine` call after the map should be removed.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
9757
to
9760
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| setStreaming(false); | ||
| setError(message); | ||
| if (submittedValue.trim() || promptAttachments.length) { |
There was a problem hiding this comment.
setError(message) was removed from the chat submit catch block. After this change, when a chat send fails the prompt is restored (so the user can retry) but there is no visible indication of what went wrong — the error is swallowed. If the intent was to stop using the connection-error banner for transient chat errors, a toast or inline per-message error indicator should replace it; otherwise users are left wondering why their message did not go through.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ade-cli/src/tuiClient/app.tsx
Line: 9757-9760
Comment:
**Chat submit errors are now silently discarded**
`setError(message)` was removed from the chat submit catch block. After this change, when a chat send fails the prompt is restored (so the user can retry) but there is no visible indication of what went wrong — the error is swallowed. If the intent was to stop using the connection-error banner for transient chat errors, a toast or inline per-message error indicator should replace it; otherwise users are left wondering why their message did not go through.
How can I resolve this? If you propose a fix, please make it concise.
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
Validation
Greptile Summary
This PR closes two feature-gap milestones (ADE-77 TUI papercuts and ADE-70 Files Workbench v2 blockers) across the CLI and desktop apps, plus a flaky test fix in the remote-runtime connection pool.
@-mention remote fetches, startup auto-retry UI withr-key shortcut, terminal interactive-mode restore on every exit path, distinct selected/unselected image-chip glyphs, and no-match palette rows for both slash and mention palettes. Both empty-state palette components render a redundant extra body line, making the palette one row taller than the populated state and displaying two overlapping "no results" messages.reloadTokensByPathfor clean-editor watcher reloads, dirty-state confirmation guards before workspace switch/rename/delete, diff-mode save viaregistry.getValue, localStorage-persisted recents, and abeforeunloadgate. The reload flow callsrefreshCleanbefore disposing the previousonDidChangeContentsubscription; the stale listener invokesonEdit, silently promoting preview tabs to permanent on external file changes.closeLocalPortForwardsForTargetis now async and awaitsnet.Server.closecallbacks, eliminating the audit-flake in port-forward disconnect tests.Confidence Score: 4/5
Safe to merge with awareness of three cosmetic/behavioral regressions introduced alongside the larger fixes.
The palette empty-state rendering emits a duplicate 'no results' row in both MentionPalette and SlashPalette, making those widgets visually inconsistent. The refreshClean→setValue→stale-listener chain silently promotes preview tabs to permanent on external file reloads. A chat submit error is no longer surfaced to the user. None of these are data-destroying, and the core dirty-buffer, watcher-reload, and terminal-restore logic is well-structured, but three separate behavioral regressions in the changed paths warrant attention before shipping.
MentionPalette.tsx and SlashPalette.tsx (duplicate empty-state row); CodeViewer.tsx (subscription disposal ordering around refreshClean); app.tsx (removed chat error display).
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Watcher as File Watcher participant FW as FilesWorkbench participant VH as ViewerHost participant CV as CodeViewer participant Reg as ModelRegistry Watcher->>FW: file changed (workspaceId, path) FW->>FW: "tab open & not dirty?" FW->>FW: "setReloadTokensByPath({ [path]: token+1 })" FW->>VH: reloadToken prop changes VH->>VH: useFileContent refetches content VH->>CV: content.content changes CV->>Reg: refreshClean(path, newContent) Reg->>Reg: model clean? setValue(newContent) Note over CV,Reg: stale onDidChangeContent fires here calling onEdit() which promotes preview tab CV->>CV: changeSubRef.current?.dispose() CV->>CV: subscribe new onDidChangeContent%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Watcher as File Watcher participant FW as FilesWorkbench participant VH as ViewerHost participant CV as CodeViewer participant Reg as ModelRegistry Watcher->>FW: file changed (workspaceId, path) FW->>FW: "tab open & not dirty?" FW->>FW: "setReloadTokensByPath({ [path]: token+1 })" FW->>VH: reloadToken prop changes VH->>VH: useFileContent refetches content VH->>CV: content.content changes CV->>Reg: refreshClean(path, newContent) Reg->>Reg: model clean? setValue(newContent) Note over CV,Reg: stale onDidChangeContent fires here calling onEdit() which promotes preview tab CV->>CV: changeSubRef.current?.dispose() CV->>CV: subscribe new onDidChangeContentComments Outside Diff (2)
apps/desktop/src/renderer/components/files/v2/viewers/CodeViewer.tsx, line 158-165 (link)onDidChangeContentlistener fires duringrefreshClean, promoting preview tabsattachModelcallsregistry.refreshClean(...)before disposing the oldchangeSubRef.current. WhenrefreshCleandetermines the model is clean and callsmodel.setValue(newContent), Monaco firesonDidChangeContentsynchronously — against the still-live old listener. That listener callsonEditCb?.(t.path), which invokesonPromoteTab(group.id, path)and permanently promotes the tab.changeSubRef.current?.dispose()runs only afterward, too late to suppress the event.Concretely: a user single-clicks a file (preview tab), an external tool modifies it, the watcher increments
reloadToken,content.contentchanges,attachModelruns,setValuefires the stale listener, and the preview tab becomes permanent silently. A simple fix is to dispose the old subscription before callingrefreshClean.Prompt To Fix With AI
apps/ade-cli/src/tuiClient/components/SlashPalette.tsx, line 148-165 (link)Same pattern as
MentionPalette.tsx: after mappingrowLines(which already fillsvisibleRowsbody slots with" No matching commands"at index 0), an additional standalonepaletteLine(bodyLine("No command matches this query.", ...))is rendered. This produces a palette that is one row taller than the populated state and displays two "no results" messages simultaneously.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix TUI papercuts and Files Workbench da..." | Re-trigger Greptile