feat(desktop): v2 review tab — PR info, checks, comments#3463
Merged
feat(desktop): v2 review tab — PR info, checks, comments#3463
Conversation
Port the v1 Review tab functionality into the v2 workspace sidebar, replacing the "Checks — Coming soon" stub. Uses v2-native host-service endpoints (git.getPullRequest, git.getPullRequestThreads) instead of v1 electron endpoints. Features: - PR header with title, state icon, review decision badge - Collapsible CI checks section with status icons and links - PR comments with avatars, timestamps, and preview text - Copy individual comments or copy all to clipboard - Comment pane: click a comment to view rendered markdown in a tab - GitHub link in pane header to open comment on GitHub - Copyable tables in rendered markdown
Contributor
|
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 a v2 Workspace Sidebar "Review" tab and CommentPane: new Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar as "Workspace Sidebar\n(Review Tab)"
participant Hook as "useReviewTab Hook"
participant tRPC as "Electron tRPC"
participant PaneRegistry as "Pane Registry"
participant CommentPane as "CommentPane"
User->>Sidebar: Open Review tab
Sidebar->>Hook: mount → fetch PR and threads
Hook->>tRPC: getPullRequest(workspaceId)
Hook->>tRPC: getPullRequestThreads(workspaceId)
tRPC-->>Hook: PR + threads
Hook->>Hook: normalize PR, checks, comments
Hook-->>Sidebar: render ReviewTabContent(pr, comments)
User->>Sidebar: Click comment (open)
Sidebar->>PaneRegistry: open/reuse "comment" pane with CommentPaneData
PaneRegistry->>CommentPane: render CommentPane
User->>CommentPane: Click "Copy All"
CommentPane->>tRPC: copyText(payload)
tRPC-->>CommentPane: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
- Fix memory leaks: add timeout cleanup refs in CommentPane and
CopyableTable, clear previous timeout on rapid clicks
- Add error state: show "Unable to load review status" when tRPC
query fails instead of infinite loading spinner
- Fix getIcon fallback: show MessageSquare icon when avatarUrl is
missing instead of rendering <img src={undefined}>
- Add aria-label to comment open button for screen readers
- Add aria-hidden to decorative arrow icon in PRHeader
- Add group-focus-visible:opacity-100 to PRHeader arrow for keyboard nav
Greptile SummaryThis PR ships the v2 workspace Review tab — replacing the "Checks — Coming soon" stub with a full-featured panel showing the PR header (title, state, review decision), collapsible CI checks with pass/fail/pending icons, and a threaded comments list. Clicking a comment opens a
Confidence Score: 4/5Safe to merge after fixing the commentsCountLabel count mismatch; all other concerns are P2 or informational. One P1 logic bug (commentsCountLabel showing total instead of active count) is a one-line fix and doesn't break core functionality — comments still display correctly, just the badge count in the header is off. The rest of the implementation is well-structured, correctly handles edge cases (loading/error/no-PR states, rapid clicks, timer cleanup), uses safe markdown rendering, and follows the existing v2 hook pattern. No data loss, security, or runtime error risks. CommentsSection.tsx — commentsCountLabel count mismatch; SidebarHeader.tsx — badge condition semantics change worth verifying against all current tabs. Important Files Changed
Sequence DiagramsequenceDiagram
participant WS as WorkspaceSidebar
participant URT as useReviewTab
participant TRPC as workspaceTrpc
participant RTC as ReviewTabContent
participant CS as CommentsSection
participant CP as CommentPane
participant PR as page.tsx (openCommentPane)
WS->>URT: useReviewTab({ workspaceId, onOpenComment })
URT->>TRPC: git.getPullRequest (10s interval)
TRPC-->>URT: V2PullRequest | null
URT->>TRPC: git.getPullRequestThreads (30s interval, enabled if PR exists)
TRPC-->>URT: { reviewThreads[], conversationComments[] }
URT->>URT: normalizeThreadsToComments()
URT->>URT: openCommentCount = unresolved.length
URT-->>WS: SidebarTabDefinition { id:review, badge:openCommentCount, content }
WS->>RTC: render(pr, comments, isLoading, isError)
RTC->>CS: render(comments, isLoading, onOpenComment)
CS->>CS: activeComments = comments.filter(!isResolved)
CS->>CS: resolvedComments = comments.filter(isResolved)
note over CS,PR: User clicks a comment row
CS->>PR: onOpenComment(CommentPaneData)
PR->>PR: search state.tabs for existing comment pane
alt Existing comment pane found
PR->>PR: setPaneData(paneId, comment)
PR->>PR: setActiveTab + setActivePane
else No existing pane
PR->>PR: addTab with kind=comment
end
PR->>CP: render CommentPane with updated data
CP->>CP: ReactMarkdown + rehypeRaw + rehypeSanitize
Reviews (1): Last reviewed commit: "fix(desktop): address review tab PR feed..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
| }, [copyToClipboard, activeComments, markCopied]); | ||
|
|
||
| const commentsCountLabel = isLoading ? "..." : comments.length; |
There was a problem hiding this comment.
commentsCountLabel uses comments.length (total = active + resolved combined), but the "Comments" collapsible section only renders activeComments. This creates a confusing mismatch — e.g., the header shows "Comments 5" while only 3 rows are visible, with the remaining 2 appearing under the separate "Resolved" section.
Change it to activeComments.length so the count matches what's actually displayed:
Suggested change
| const commentsCountLabel = isLoading ? "..." : comments.length; | |
| const commentsCountLabel = isLoading ? "..." : activeComments.length; |
Comment on lines
30
to
32
| )} | ||
| > | ||
| {tab.label} |
There was a problem hiding this comment.
Removing the tab.badge > 0 guard means any tab returning badge: 0 will now render the badge span. The useChangesTab hook uses badge: totalChanges > 0 ? totalChanges : undefined (never 0), so there's no regression from existing tabs. However, useReviewTab always returns badge: openCommentCount — including 0 — which is the driver for this change.
The intent is clearly to show "0" on the Review tab when there are no open comments. If that's the desired UX, the change is correct. It's worth noting that any future tab returning badge: 0 will also display it, which may be surprising. A narrower approach would have useReviewTab return badge: openCommentCount only when pr != null and let callers stay consistent:
Suggested change
| )} | |
| > | |
| {tab.label} | |
| {tab.badge != null && tab.badge > 0 && ( |
…and instead ensure useReviewTab always returns a value (even 0) only when a PR is active. This avoids making the global badge condition permanently lenient.
Comment on lines
+181
to
+196
| for (const thread of data.reviewThreads) { | ||
| const first = thread.comments[0]; | ||
| if (!first) continue; | ||
| comments.push({ | ||
| id: first.id, | ||
| authorLogin: first.author.login, | ||
| avatarUrl: first.author.avatarUrl || undefined, | ||
| body: first.body, | ||
| createdAt: first.createdAt, | ||
| url: undefined, | ||
| kind: "review", | ||
| path: thread.path || undefined, | ||
| line: thread.line ?? undefined, | ||
| isResolved: thread.isResolved, | ||
| threadId: thread.id, | ||
| }); |
There was a problem hiding this comment.
url: undefined is hardcoded for every review thread comment. This is correct given the PullRequestReviewComment interface in the host-service (id, databaseId, author, body, createdAt — no url field), so no code change is needed.
However, this means the "Open on GitHub" action button will never appear in CommentRow for review comments, and the GitHub icon link in the CommentPane header will also be absent. Conversation comments (htmlUrl) correctly get the link, but review comments don't — which can be a noticeable UX gap since review comments make up the bulk of code-review feedback. Consider whether the host-service router can be updated to include the url (GitHub returns html_url on review comments via the REST API), so users can navigate to the exact review comment on GitHub.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx (1)
122-130: Consider adding a brief comment explaining the CodeRabbit special case.The
resolveCheckUrlfunction has special handling for "coderabbit" checks to redirect to the PR URL. A brief comment would help future maintainers understand this intentional fallback.📝 Optional documentation
function resolveCheckUrl( check: NormalizedCheck, prUrl: string, ): string | undefined { if (check.url) return check.url; + // CodeRabbit checks don't provide direct URLs; link to PR instead const name = check.name.trim().toLowerCase(); if (name.includes("coderabbit") || name.includes("code rabbit")) return prUrl; return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx around lines 122 - 130, The resolveCheckUrl function contains a special-case fallback that routes checks whose name contains "coderabbit" or "code rabbit" to the PR URL but lacks an explanatory comment; add a short inline comment above or within resolveCheckUrl (referencing resolveCheckUrl, NormalizedCheck, check.url, check.name, and prUrl) explaining that CodeRabbit checks intentionally fallback to the PR URL when no explicit check.url is provided so future maintainers understand this behavior.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/PRHeader/PRHeader.tsx (1)
31-48: Consider adding an accessible name to the PR link.The link wrapping the PR title relies on the visible title text for its accessible name. For better screen reader experience, consider adding an explicit
aria-labelthat includes "Open PR on GitHub" or similar context.♻️ Optional improvement
<a href={pr.url} target="_blank" rel="noopener noreferrer" className="group flex items-center gap-1.5 cursor-pointer" + aria-label={`Open pull request: ${pr.title} on GitHub`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/PRHeader/PRHeader.tsx around lines 31 - 48, The anchor wrapping the PR title (in PRHeader.tsx around the <a> using pr.url and pr.title with PRIcon and LuArrowUpRight) lacks an explicit accessible name; update the anchor to include an aria-label like "Open PR on GitHub: {pr.title}" (or "Open PR: {pr.title}") so screen readers get clear context while preserving existing children and attributes.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx (2)
305-312: Avoid rendering a no-op action button when pane opening is unavailable.
onOpenis optional, but the row is always a clickable<button>. IfonOpenis undefined, this creates a focusable control with no action. Consider rendering non-interactive content (or disabling the button) in that state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx around lines 305 - 312, The current row always renders an interactive <button> that calls handleClick, but onOpen is optional so the control can be focusable with no action; update the render in CommentsSection so when onOpen is undefined it does not render a clickable button — either render a non-interactive container (e.g., a <div> or <span>) showing {content} with the same layout and aria-label, or render the <button> with disabled and appropriate aria-disabled state; ensure handleClick only exists when onOpen is present and reference the existing identifiers (handleClick, onOpen, content, comment.authorLogin) when changing the JSX.
243-344: SplitCommentRowinto its own file.This file defines multiple React components under a
componentspath. Please moveCommentRowinto its ownCommentRow/CommentRow.tsx(with barrel export) and keepCommentsSection.tsxfocused on the section component.As per coding guidelines,
**/components/**/*.tsx: "Keep one component per file; no multi-component files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx around lines 243 - 344, Extract the CommentRow component and its CommentRowProps interface into a new folder CommentRow/CommentRow.tsx and export it (named export CommentRow); create a barrel file CommentRow/index.ts that re-exports the component, then update the original CommentsSection.tsx to import { CommentRow } from './CommentRow'. Move all local helper and UI imports used by CommentRow (formatShortAge, getPreviewText, Avatar, AvatarImage, AvatarFallback, LuCheck, LuCopy, LuArrowUpRight) into the new file so it remains self-contained, and ensure the onOpen, onCopy prop signatures and the normalized comment shape (NormalizedComment/CommentPaneData) are imported or re-exported as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx:
- Around line 113-115: The rendering key for CheckRow is currently just
check.name which can duplicate; update either the NormalizedCheck type to
include a unique id from the raw API (e.g., checkRunId) and use that as the key
in relevantChecks.map, or add a stable fallback by combining name with the map
index (e.g., `${check.name}-${index}`) when mapping in the ChecksSection
component so keys are unique; adjust where NormalizedCheck is constructed to
populate the id if you choose the first option and update CheckRow props
accordingly.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:
- Around line 61-70: The copy handlers (e.g., handleCopySingle and the
multiple-copy handler using copyToClipboard and markCopied) currently only
handle successful promise resolution; update them to explicitly handle rejected
promises by adding a .catch or try/catch around copyToClipboard and call an
error-handling path (e.g., markCopyFailed or processLogger/errorToast) with
context like `comment:${comment.id}` or relevant workspace id so users get
feedback on failure; ensure you still call markCopied on success and do not
swallow errors silently.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx:
- Around line 75-80: The tab's error UI only uses prQuery.isError so failures
from threadsQuery (git.getPullRequestThreads) are shown as "no comments" instead
of an error; update the ReviewTabContent props in useReviewTab to propagate the
threads query failure by combining errors (e.g., set isError to prQuery.isError
|| threadsQuery.isError) and, if applicable, pass threadsQuery.error to
ReviewTabContent or an appropriate prop so the component can render the error
state for threads as well.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx:
- Around line 34-43: The then() callback in handleCopyAll can set state after
unmount; to fix, add a mounted ref (e.g., const mountedRef = useRef(true)) and
in a useEffect cleanup set mountedRef.current = false and clear copyTimerRef if
present; then in the promise resolution inside handleCopyAll (after
electronTrpcClient.external.copyText.mutate(...).then(...)) guard calls to
setCopied and setting copyTimerRef with if (!mountedRef.current) return; and
ensure the timeout is cleared in the cleanup so copyTimerRef doesn't outlive the
component.
- Around line 115-138: The handleCopy callback in CommentPane may call setCopied
and manipulate timerRef after the component unmounts; add a mounted ref (e.g.,
isMountedRef) initialized true in a useEffect and set to false on cleanup, then
in handleCopy's .then() guard all state/timer actions with if
(!isMountedRef.current) return; also ensure the effect cleanup clears timerRef
(clearTimeout) to avoid leaks; update references to tableRef, timerRef,
setCopied, and electronTrpcClient.external.copyText.mutate in this change.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx:
- Around line 122-130: The resolveCheckUrl function contains a special-case
fallback that routes checks whose name contains "coderabbit" or "code rabbit" to
the PR URL but lacks an explanatory comment; add a short inline comment above or
within resolveCheckUrl (referencing resolveCheckUrl, NormalizedCheck, check.url,
check.name, and prUrl) explaining that CodeRabbit checks intentionally fallback
to the PR URL when no explicit check.url is provided so future maintainers
understand this behavior.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:
- Around line 305-312: The current row always renders an interactive <button>
that calls handleClick, but onOpen is optional so the control can be focusable
with no action; update the render in CommentsSection so when onOpen is undefined
it does not render a clickable button — either render a non-interactive
container (e.g., a <div> or <span>) showing {content} with the same layout and
aria-label, or render the <button> with disabled and appropriate aria-disabled
state; ensure handleClick only exists when onOpen is present and reference the
existing identifiers (handleClick, onOpen, content, comment.authorLogin) when
changing the JSX.
- Around line 243-344: Extract the CommentRow component and its CommentRowProps
interface into a new folder CommentRow/CommentRow.tsx and export it (named
export CommentRow); create a barrel file CommentRow/index.ts that re-exports the
component, then update the original CommentsSection.tsx to import { CommentRow }
from './CommentRow'. Move all local helper and UI imports used by CommentRow
(formatShortAge, getPreviewText, Avatar, AvatarImage, AvatarFallback, LuCheck,
LuCopy, LuArrowUpRight) into the new file so it remains self-contained, and
ensure the onOpen, onCopy prop signatures and the normalized comment shape
(NormalizedComment/CommentPaneData) are imported or re-exported as needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/PRHeader/PRHeader.tsx:
- Around line 31-48: The anchor wrapping the PR title (in PRHeader.tsx around
the <a> using pr.url and pr.title with PRIcon and LuArrowUpRight) lacks an
explicit accessible name; update the anchor to include an aria-label like "Open
PR on GitHub: {pr.title}" (or "Open PR: {pr.title}") so screen readers get clear
context while preserving existing children and attributes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93b39d55-5836-4d4c-b48d-2c3c3d09ab47
📒 Files selected for processing (20)
apps/desktop/plans/20260413-1600-v2-review-tab.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/PRHeader/PRHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/PRHeader/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ReviewTabContent/ReviewTabContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ReviewTabContent/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/comment-pane.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
Comment on lines
+75
to
+80
| <ReviewTabContent | ||
| pr={pr} | ||
| comments={comments} | ||
| isLoading={prQuery.isLoading} | ||
| isError={prQuery.isError} | ||
| isCommentsLoading={threadsQuery.isLoading} |
Contributor
There was a problem hiding this comment.
Propagate thread-query failures into the tab error state.
Right now only prQuery.isError drives the error UI. If git.getPullRequestThreads fails, the tab can degrade into “no comments” instead of surfacing an error.
Suggested fix
<ReviewTabContent
pr={pr}
comments={comments}
isLoading={prQuery.isLoading}
- isError={prQuery.isError}
+ isError={prQuery.isError || threadsQuery.isError}
isCommentsLoading={threadsQuery.isLoading}
onOpenComment={onOpenComment}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ReviewTabContent | |
| pr={pr} | |
| comments={comments} | |
| isLoading={prQuery.isLoading} | |
| isError={prQuery.isError} | |
| isCommentsLoading={threadsQuery.isLoading} | |
| <ReviewTabContent | |
| pr={pr} | |
| comments={comments} | |
| isLoading={prQuery.isLoading} | |
| isError={prQuery.isError || threadsQuery.isError} | |
| isCommentsLoading={threadsQuery.isLoading} | |
| onOpenComment={onOpenComment} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
around lines 75 - 80, The tab's error UI only uses prQuery.isError so failures
from threadsQuery (git.getPullRequestThreads) are shown as "no comments" instead
of an error; update the ReviewTabContent props in useReviewTab to propagate the
threads query failure by combining errors (e.g., set isError to prQuery.isError
|| threadsQuery.isError) and, if applicable, pass threadsQuery.error to
ReviewTabContent or an appropriate prop so the component can render the error
state for threads as well.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx:35">
P2: Handle copyText failures explicitly; this promise chain currently has no rejection handler and can trigger unhandled promise rejections in the renderer.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx:130">
P2: Add rejection handling for the table copy promise to avoid unhandled promise rejections on copy failures.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:63">
P2: Handle copy failures explicitly to avoid unhandled promise rejections.
(Based on your team's feedback about handling async errors explicitly and avoiding unhandled rejections.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:99">
P2: `commentsCountLabel` uses `comments.length` (total including resolved), but the "Comments" collapsible section only renders `activeComments`. This creates a confusing mismatch — e.g., the header shows "5" while only 3 rows are visible. Use `activeComments.length` so the count matches the displayed rows.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx:79">
P2: Thread-query failures are not surfaced in the error state. If `git.getPullRequestThreads` fails, the tab silently degrades to showing "no comments" instead of displaying the error UI. Include `threadsQuery.isError` in the error condition.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx:114">
P2: Using `check.name` as the sole React key is fragile. GitHub allows multiple check runs with the same name (e.g., from different workflows or retries), which would produce duplicate keys and cause rendering issues. Use a composite key like `` `${check.name}-${index}` `` or include a unique ID from the API.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| }, [copyToClipboard, activeComments, markCopied]); | ||
|
|
||
| const commentsCountLabel = isLoading ? "..." : comments.length; |
Contributor
There was a problem hiding this comment.
P2: commentsCountLabel uses comments.length (total including resolved), but the "Comments" collapsible section only renders activeComments. This creates a confusing mismatch — e.g., the header shows "5" while only 3 rows are visible. Use activeComments.length so the count matches the displayed rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx, line 99:
<comment>`commentsCountLabel` uses `comments.length` (total including resolved), but the "Comments" collapsible section only renders `activeComments`. This creates a confusing mismatch — e.g., the header shows "5" while only 3 rows are visible. Use `activeComments.length` so the count matches the displayed rows.</comment>
<file context>
@@ -0,0 +1,344 @@
+ });
+ }, [copyToClipboard, activeComments, markCopied]);
+
+ const commentsCountLabel = isLoading ? "..." : comments.length;
+ const copyAllLabel =
+ copiedActionKey === "comments:all" ? "Copied" : "Copy all";
</file context>
| pr={pr} | ||
| comments={comments} | ||
| isLoading={prQuery.isLoading} | ||
| isError={prQuery.isError} |
Contributor
There was a problem hiding this comment.
P2: Thread-query failures are not surfaced in the error state. If git.getPullRequestThreads fails, the tab silently degrades to showing "no comments" instead of displaying the error UI. Include threadsQuery.isError in the error condition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx, line 79:
<comment>Thread-query failures are not surfaced in the error state. If `git.getPullRequestThreads` fails, the tab silently degrades to showing "no comments" instead of displaying the error UI. Include `threadsQuery.isError` in the error condition.</comment>
<file context>
@@ -0,0 +1,220 @@
+ pr={pr}
+ comments={comments}
+ isLoading={prQuery.isLoading}
+ isError={prQuery.isError}
+ isCommentsLoading={threadsQuery.isLoading}
+ onOpenComment={onOpenComment}
</file context>
Prevents unhandled promise rejections in the renderer if electronTrpcClient.external.copyText.mutate() fails.
Add isMountedRef to both CommentPane and CopyableTable to prevent setCopied calls after unmount when the async clipboard IPC resolves after the component is gone.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx (1)
249-350: ExtractCommentRowinto its own component file.
CommentsSection.tsxcurrently contains two components (CommentsSectionandCommentRow). Please splitCommentRowinto its own folder/file (CommentRow/CommentRow.tsx+index.ts) and import it here.Suggested refactor sketch
- interface CommentRowProps { - ... - } - - function CommentRow(...) { - ... - } + import { CommentRow } from "../CommentRow";// CommentRow/CommentRow.tsx export interface CommentRowProps { comment: NormalizedComment; copiedActionKey: string | null; onCopy: (comment: NormalizedComment) => void; onOpen?: (comment: CommentPaneData) => void; } export function CommentRow(props: CommentRowProps) { // moved implementation }// CommentRow/index.ts export { CommentRow } from "./CommentRow"; export type { CommentRowProps } from "./CommentRow";As per coding guidelines
**/components/**/*.tsx: “Keep one component per file; no multi-component files.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx around lines 249 - 350, Separate the CommentRow component into its own module: create a new file that exports the CommentRow function and its CommentRowProps/NormalizedComment/CommentPaneData types (matching the interface and implementation currently inside CommentsSection), move the entire CommentRow implementation there, and add an index export that re-exports CommentRow and its props type; then update the original file to import { CommentRow } (and the props type if needed) and remove the in-file CommentRow definition so only CommentsSection remains. Ensure the moved component preserves handleClick, onCopy, copiedActionKey logic, avatar/rendering JSX, and aria labels exactly as in the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:
- Around line 249-350: Separate the CommentRow component into its own module:
create a new file that exports the CommentRow function and its
CommentRowProps/NormalizedComment/CommentPaneData types (matching the interface
and implementation currently inside CommentsSection), move the entire CommentRow
implementation there, and add an index export that re-exports CommentRow and its
props type; then update the original file to import { CommentRow } (and the
props type if needed) and remove the in-file CommentRow definition so only
CommentsSection remains. Ensure the moved component preserves handleClick,
onCopy, copiedActionKey logic, avatar/rendering JSX, and aria labels exactly as
in the original.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ef65b68-1f1d-4669-b50b-0807aa7efd85
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx (2)
16-16: Use a tsconfig alias import fortypesinstead of a deep relative path.Line 16 is brittle with
../../../../types, especially since this file already uses alias-style imports (e.g.,renderer/...) elsewhere.As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx at line 16, Replace the brittle deep-relative import of the types in CommentPane.tsx (currently importing CommentPaneData and PaneViewerData via "../../../../types") with the project tsconfig alias used elsewhere (e.g., the same "renderer/..." alias pattern), updating the import to reference the aliased path that exports CommentPaneData and PaneViewerData so the file uses the canonical tsconfig alias instead of "../../../../types".
107-181: SplitCopyableTableinto its own component file.This file currently defines multiple components (
CommentPaneandCopyableTable). Please moveCopyableTableto its own component folder/file and import it here.♻️ Suggested structure change
-const commentComponents = { - table: ({ children }: { children?: ReactNode }) => ( - <CopyableTable>{children}</CopyableTable> - ), -}; - -function CopyableTable({ children }: { children?: ReactNode }) { - ... -} +import { CopyableTable } from "./CopyableTable"; + +const commentComponents = { + table: ({ children }: { children?: ReactNode }) => ( + <CopyableTable>{children}</CopyableTable> + ), +};// CommentPane/CopyableTable/CopyableTable.tsx export function CopyableTable({ children }: { children?: ReactNode }) { // moved unchanged logic }// CommentPane/CopyableTable/index.ts export { CopyableTable } from "./CopyableTable";As per coding guidelines,
**/components/**/*.tsx:Keep one component per file; no multi-component filesandOrganize components with one folder per component: ComponentName/ComponentName.tsx + index.ts for barrel export.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx around lines 107 - 181, The file defines two components (CommentPane and CopyableTable); split CopyableTable into its own component folder/file, export it from CommentPane/CopyableTable/index.ts, and import it into the original file. Specifically, move the CopyableTable function (including its hooks: useRef, useState, useEffect, useCallback and handleCopy logic and JSX) into CommentPane/CopyableTable/CopyableTable.tsx, add a barrel export in CommentPane/CopyableTable/index.ts, update the original file to import { CopyableTable } and keep commentComponents.table referencing CopyableTable unchanged, and ensure any references to electronTrpcClient and React types remain imported in the new file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx:
- Line 16: Replace the brittle deep-relative import of the types in
CommentPane.tsx (currently importing CommentPaneData and PaneViewerData via
"../../../../types") with the project tsconfig alias used elsewhere (e.g., the
same "renderer/..." alias pattern), updating the import to reference the aliased
path that exports CommentPaneData and PaneViewerData so the file uses the
canonical tsconfig alias instead of "../../../../types".
- Around line 107-181: The file defines two components (CommentPane and
CopyableTable); split CopyableTable into its own component folder/file, export
it from CommentPane/CopyableTable/index.ts, and import it into the original
file. Specifically, move the CopyableTable function (including its hooks:
useRef, useState, useEffect, useCallback and handleCopy logic and JSX) into
CommentPane/CopyableTable/CopyableTable.tsx, add a barrel export in
CommentPane/CopyableTable/index.ts, update the original file to import {
CopyableTable } and keep commentComponents.table referencing CopyableTable
unchanged, and ensure any references to electronTrpcClient and React types
remain imported in the new file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8afcd95-1373-4df1-a3b8-1d8e2f541655
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/CommentPane/CommentPane.tsx
The threads query had staleTime: 30_000 which cached empty results when the query fired before the host-service had PR data synced. Remove staleTime so every mount/refocus fetches fresh data (background polling at 30s still runs via refetchInterval). Also tighten the enabled condition to use prQuery.isSuccess instead of !!prQuery.data for clearer intent, and use composite key for CheckRow to handle duplicate check names.
AviPeltz
force-pushed
the
review-v2-screen
branch
from
921d64b to
278b7f9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx (1)
113-115:⚠️ Potential issue | 🟠 MajorReplace the index-based key with a stable check identifier.
Line 115 still uses
indexin the key. This is unstable on reordering and also tripslint/suspicious/noArrayIndexKey.Suggested direction
- relevantChecks.map((check, index) => ( + relevantChecks.map((check) => ( <CheckRow - key={`${check.name}-${index}`} + key={check.id} check={check} prUrl={prUrl} /> ))If
NormalizedCheckcurrently has noid, add one during normalization (e.g., check run id) and thread it through.#!/bin/bash set -euo pipefail CHECKS_FILE='apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx' HOOK_ROOT='apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab' echo "1) Verify index-based key usage in ChecksSection:" rg -n 'key=\{`\$\{check\.name\}-\$\{index\}`\}' "$CHECKS_FILE" echo echo "2) Inspect NormalizedCheck type for a stable id field:" rg -n --type ts --type tsx 'interface NormalizedCheck|type NormalizedCheck|id\s*:' "$HOOK_ROOT" echo echo "3) Locate check normalization points to confirm whether id can be propagated:" rg -n --type ts --type tsx 'normalize|normalized|checks.*map|checkRun|statusCheck' "$HOOK_ROOT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx around lines 113 - 115, The CheckRow list uses a fragile key `${check.name}-${index}`; replace the index with a stable identifier: ensure the NormalizedCheck type includes a unique id (e.g., checkRun id) at normalization time, propagate that id through relevantChecks, and change the key on CheckRow to use that stable field (e.g., key={check.id} or key={`${check.id}-${check.name}`}) in the component rendering loop that maps relevantChecks to CheckRow.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx (1)
136-175: SplitCheckRowinto its own component file.Line 136 introduces a second component in
ChecksSection.tsx. Please moveCheckRowto its own folder/file and re-export viaindex.tsto match repo component structure rules.As per coding guidelines, "Keep one component per file; no multi-component files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx around lines 136 - 175, Move the CheckRow component out of ChecksSection.tsx into its own component folder (e.g., a new folder named CheckRow) with a file like CheckRow.tsx that exports the CheckRow React component (retain usage of checkIconConfig, resolveCheckUrl, NormalizedCheck typing and LuArrowUpRight import as needed). Add an index.ts that re-exports CheckRow as the default or named export to match repo conventions, then replace the inline CheckRow definition in ChecksSection.tsx with an import from the new module and ensure props and usage (check, prUrl) remain unchanged. Run a quick type/build check to fix any missing imports (cn, checkIconConfig, resolveCheckUrl, LuArrowUpRight) and update exports/import paths accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx:
- Around line 113-115: The CheckRow list uses a fragile key
`${check.name}-${index}`; replace the index with a stable identifier: ensure the
NormalizedCheck type includes a unique id (e.g., checkRun id) at normalization
time, propagate that id through relevantChecks, and change the key on CheckRow
to use that stable field (e.g., key={check.id} or
key={`${check.id}-${check.name}`}) in the component rendering loop that maps
relevantChecks to CheckRow.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsx:
- Around line 136-175: Move the CheckRow component out of ChecksSection.tsx into
its own component folder (e.g., a new folder named CheckRow) with a file like
CheckRow.tsx that exports the CheckRow React component (retain usage of
checkIconConfig, resolveCheckUrl, NormalizedCheck typing and LuArrowUpRight
import as needed). Add an index.ts that re-exports CheckRow as the default or
named export to match repo conventions, then replace the inline CheckRow
definition in ChecksSection.tsx with an import from the new module and ensure
props and usage (check, prUrl) remain unchanged. Run a quick type/build check to
fix any missing imports (cn, checkIconConfig, resolveCheckUrl, LuArrowUpRight)
and update exports/import paths accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5ac870d-0829-45bb-ad99-9d6d3fc55e99
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ChecksSection/ChecksSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
…atus - Add isMountedRef to CommentsSection so markCopied doesn't set state after unmount (same pattern as CommentPane/CopyableTable) - Rewrite computeChecksStatus as single loop instead of 3 passes (filter + some(failure) + some(pending) each called coerceCheckStatus)
…nt pane Use the existing CodeBlock component which handles mermaid via @streamdown/mermaid and syntax highlighting via react-syntax-highlighter.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx (1)
75-80:⚠️ Potential issue | 🟠 MajorInclude thread-query failures in the tab error state.
At Line 79, only
prQuery.isErroris forwarded. IfgetPullRequestThreadsfails, the tab can render a misleading non-error state.Suggested fix
<ReviewTabContent pr={pr} comments={comments} isLoading={prQuery.isLoading} - isError={prQuery.isError} + isError={prQuery.isError || threadsQuery.isError} isCommentsLoading={threadsQuery.isLoading} onOpenComment={onOpenComment} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx around lines 75 - 80, The ReviewTabContent is only receiving prQuery.isError for its isError prop so thread-query failures aren't surfaced; update the prop passed to ReviewTabContent (the isError prop) to combine both error states (e.g. isError={prQuery.isError || threadsQuery.isError}) so failures from getPullRequestThreads (threadsQuery) are reflected in the tab error state.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx (1)
260-354: MoveCommentRowinto its own component file.
CommentsSection.tsxcurrently defines bothCommentsSectionandCommentRow. SplitCommentRowinto its own folder/file pair (with barrel export) to align with the component structure rules.As per coding guidelines:
**/components/**/*.tsx: “Keep one component per file; no multi-component files.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx around lines 260 - 354, Extract the CommentRow component into its own folder and file (e.g., components/CommentRow/index.tsx) and export it via a barrel (components/CommentRow/index.ts) so CommentsSection imports the standalone component; move the CommentRow function and its related types (CommentRowProps, handleClick, isCopied, age, content JSX) out of CommentsSection.tsx, ensure the new file imports the same dependencies (Avatar, AvatarImage, AvatarFallback, LuCheck, LuCopy, LuArrowUpRight, formatShortAge, getPreviewText) and keeps the same props API (comment, copiedActionKey, onCopy, onOpen), then update CommentsSection to import CommentRow from the new barrel and remove the local definition so only CommentsSection remains in the original file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:
- Around line 162-167: The empty-panel bug is caused by checking comments.length
instead of activeComments.length in the CommentsSection render branch; update
the conditional in the CommentsSection component so it checks
activeComments.length === 0 (and renders an explicit "No open comments" empty
state) instead of comments.length === 0 so resolved comments don't produce a
blank panel; ensure the same message is used where activeComments is mapped and
keep unchanged behavior when there are zero total comments if you want a
different message for “no comments at all.”
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx:
- Around line 75-80: The ReviewTabContent is only receiving prQuery.isError for
its isError prop so thread-query failures aren't surfaced; update the prop
passed to ReviewTabContent (the isError prop) to combine both error states (e.g.
isError={prQuery.isError || threadsQuery.isError}) so failures from
getPullRequestThreads (threadsQuery) are reflected in the tab error state.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx:
- Around line 260-354: Extract the CommentRow component into its own folder and
file (e.g., components/CommentRow/index.tsx) and export it via a barrel
(components/CommentRow/index.ts) so CommentsSection imports the standalone
component; move the CommentRow function and its related types (CommentRowProps,
handleClick, isCopied, age, content JSX) out of CommentsSection.tsx, ensure the
new file imports the same dependencies (Avatar, AvatarImage, AvatarFallback,
LuCheck, LuCopy, LuArrowUpRight, formatShortAge, getPreviewText) and keeps the
same props API (comment, copiedActionKey, onCopy, onOpen), then update
CommentsSection to import CommentRow from the new barrel and remove the local
definition so only CommentsSection remains in the original file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf66e0ec-0dd3-4bff-b915-6bdfa50bd39c
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
Comment on lines
+162
to
+167
| ) : comments.length === 0 ? ( | ||
| <div className="px-1.5 py-1 text-xs text-muted-foreground"> | ||
| No comments yet. | ||
| </div> | ||
| ) : ( | ||
| activeComments.map((comment) => ( |
Contributor
There was a problem hiding this comment.
Show an explicit empty state when there are no open comments.
At Line 162, the condition checks comments.length === 0, but this section renders activeComments. If all comments are resolved, the panel appears blank instead of showing a message.
Suggested fix
- ) : comments.length === 0 ? (
+ ) : activeComments.length === 0 ? (
<div className="px-1.5 py-1 text-xs text-muted-foreground">
- No comments yet.
+ No open comments.
</div>
) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/CommentsSection/CommentsSection.tsx
around lines 162 - 167, The empty-panel bug is caused by checking
comments.length instead of activeComments.length in the CommentsSection render
branch; update the conditional in the CommentsSection component so it checks
activeComments.length === 0 (and renders an explicit "No open comments" empty
state) instead of comments.length === 0 so resolved comments don't produce a
blank panel; ensure the same message is used where activeComments is mapped and
keep unchanged behavior when there are zero total comments if you want a
different message for “no comments at all.”
- Hide the "mermaid" label and action buttons on the block wrapper - Strip the outer border/padding so it blends with the page - Target the SVG element itself to lighten its background color
Revert the custom Streamdown/SyntaxHighlighter approach. Go back to using the shared CodeBlock component. Instead of changing the mermaid canvas background, override the SVG text fill and node/edge label colors via CSS to use --foreground, making them readable on both light and dark themes.
Use Streamdown directly with custom themeVariables to set light text colors (primaryTextColor, nodeTextColor, labelTextColor, etc.) on the dark mermaid theme. Removes the CSS hacks that couldn't override mermaid's inline SVG styles.
- Use theme: "base" with full dark color palette so themeVariables actually control node fills (dark gray nodes, light text) - Hide "mermaid" label and action buttons via CSS - Strip background/border from outer block and inner wrapper so diagram sits directly on the page background
Contributor
|
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 |
Move code block rendering out of useMemo into a proper React component so the component identity is stable across renders. Prevents unmount/ remount of Streamdown/SyntaxHighlighter when the theme changes. Mermaid theme variable objects extracted as module-level constants.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 15, 2026
…uperset-sh#3463) * feat(desktop): add Review tab to v2 workspace sidebar Port the v1 Review tab functionality into the v2 workspace sidebar, replacing the "Checks — Coming soon" stub. Uses v2-native host-service endpoints (git.getPullRequest, git.getPullRequestThreads) instead of v1 electron endpoints. Features: - PR header with title, state icon, review decision badge - Collapsible CI checks section with status icons and links - PR comments with avatars, timestamps, and preview text - Copy individual comments or copy all to clipboard - Comment pane: click a comment to view rendered markdown in a tab - GitHub link in pane header to open comment on GitHub - Copyable tables in rendered markdown * fix(desktop): address review tab PR feedback - Fix memory leaks: add timeout cleanup refs in CommentPane and CopyableTable, clear previous timeout on rapid clicks - Add error state: show "Unable to load review status" when tRPC query fails instead of infinite loading spinner - Fix getIcon fallback: show MessageSquare icon when avatarUrl is missing instead of rendering <img src={undefined}> - Add aria-label to comment open button for screen readers - Add aria-hidden to decorative arrow icon in PRHeader - Add group-focus-visible:opacity-100 to PRHeader arrow for keyboard nav * fix(desktop): add .catch() to all clipboard promise chains Prevents unhandled promise rejections in the renderer if electronTrpcClient.external.copyText.mutate() fails. * fix(desktop): guard state updates after unmount in CommentPane Add isMountedRef to both CommentPane and CopyableTable to prevent setCopied calls after unmount when the async clipboard IPC resolves after the component is gone. * fix(desktop): fix comments not loading until refresh The threads query had staleTime: 30_000 which cached empty results when the query fired before the host-service had PR data synced. Remove staleTime so every mount/refocus fetches fresh data (background polling at 30s still runs via refetchInterval). Also tighten the enabled condition to use prQuery.isSuccess instead of !!prQuery.data for clearer intent, and use composite key for CheckRow to handle duplicate check names. * fix(desktop): unmount guard in CommentsSection + single-pass check status - Add isMountedRef to CommentsSection so markCopied doesn't set state after unmount (same pattern as CommentPane/CopyableTable) - Rewrite computeChecksStatus as single loop instead of 3 passes (filter + some(failure) + some(pending) each called coerceCheckStatus) * feat(desktop): render mermaid diagrams + syntax highlighting in comment pane Use the existing CodeBlock component which handles mermaid via @streamdown/mermaid and syntax highlighting via react-syntax-highlighter. * fix(desktop): lighten mermaid diagram background in comment pane * fix(desktop): hide mermaid label + lighten actual diagram background - Hide the "mermaid" label and action buttons on the block wrapper - Strip the outer border/padding so it blends with the page - Target the SVG element itself to lighten its background color * fix(desktop): revert hacky mermaid overrides, use CSS text color instead Revert the custom Streamdown/SyntaxHighlighter approach. Go back to using the shared CodeBlock component. Instead of changing the mermaid canvas background, override the SVG text fill and node/edge label colors via CSS to use --foreground, making them readable on both light and dark themes. * fix(desktop): use mermaid themeVariables for light text in dark mode Use Streamdown directly with custom themeVariables to set light text colors (primaryTextColor, nodeTextColor, labelTextColor, etc.) on the dark mermaid theme. Removes the CSS hacks that couldn't override mermaid's inline SVG styles. * fix(desktop): mermaid dark theme contrast + hide label/wrapper chrome - Use theme: "base" with full dark color palette so themeVariables actually control node fills (dark gray nodes, light text) - Hide "mermaid" label and action buttons via CSS - Strip background/border from outer block and inner wrapper so diagram sits directly on the page background * fix(desktop): extract CommentCodeBlock as standalone component Move code block rendering out of useMemo into a proper React component so the component identity is stable across renders. Prevents unmount/ remount of Streamdown/SyntaxHighlighter when the theme changes. Mermaid theme variable objects extracted as module-level constants.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 15, 2026
Three functional issues Codex found on upstream superset-sh#3463 cherry-pick: 1. useReviewTab.tsx normalizeThreadsToComments only picked thread.comments[0], silently dropping every reply in a review thread. v1 flattened all comments per thread. Loop over thread.comments instead so the listing and badge match the real PR state. 2. CommentPane.tsx fed ReactMarkdown the raw comment body without overriding <a> or <img>. GitHub-supplied links would navigate the main window away, and arbitrary remote images would load. Add CommentLink (opens via electronTrpcClient.external.openUrl) and CommentImage (uses SafeImage, which only renders data: URLs). 3. useReviewTab.tsx returned badge: openCommentCount unconditionally, so the tab showed a "0" badge when there were no open comments. Changes tab already suppresses zero badges; match that. Deferred (upstream bug, host-service schema change required): - Review comments never get a GitHub URL because the GraphQL query and host-service types in packages/host-service don't fetch/expose reviewComment.url. Fixing this needs host-service router changes and is out of scope for this cherry-pick.
4 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 15, 2026
chore(upstream): PR6 v2 review tab first pass (superset-sh#3463)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 15, 2026
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
7 tasks
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
<details>blocks, and a "Copy All" buttongit.getPullRequest,git.getPullRequestThreads) instead of v1 electron endpointsTest plan
Summary by cubic
Adds a Review tab to the v2 workspace sidebar, replacing the “Checks — Coming soon” stub. It shows PR info, CI checks, and PR comments; clicking a comment opens a Markdown Comment pane.
New Features
Streamdownwith a dark-contrast theme and hides block chrome.workspaceTrpc.git.getPullRequestandworkspaceTrpc.git.getPullRequestThreadsfrom@superset/workspace-clientwith normalized data; Review tab badge shows open comment count (including 0), and sidebar badges render zero counts.Bug Fixes
Written for commit e6253be. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation