refactor: redesign PR status with visual prominence#143
Merged
Conversation
…antic colors Replace muted text with tinted pills and state-colored links. PR status is now visually discoverable at a glance: amber tint for "Checks running", blue tint for "Awaiting review", solid colors for action states (red=fix, green=merge). Architecture change: Pure state machine pattern using derivePRActionState() that maps PRStatus to a discriminated union (PRActionState), then exhaustive pattern matching in JSX. Eliminates conditional logic scattered across component. Changes: - New src/features/workspace/lib/prState.ts: pure derivation function - Rewritten PRActions.tsx: state machine UI, no redundant chips or status bars - Deleted PRStatusBar.tsx (dead code, confirmed via grep) - PR link promoted from text-text-tertiary to state-colored at 80% opacity with PR icon (#N → [icon] #3) - StatusText now renders tinted pills: pending=amber, review=blue - GhWarning gets amber tint to match visual system - Review button moved to DiffFilesTree footer in prior session (inserts prompt) Consensus design synthesized from 5-designer exploration: - Fukasawa's state-colored link philosophy - 4 designers agreed on tinted pills for non-actionable states - 3-tier hierarchy: solid bg (act) → tinted bg (aware) → tinted text (navigate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughCentralizes PR status interpretation with a new PRActionState and derivePRActionState, refactors PR UI to state-driven rendering, updates backend CI/PR resolution and error handling (including a "network" error), adds a REVIEW_CODE prompt and review-insert flow wired through UI components, and removes legacy PRStatusBar and related handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Diff as DiffFilesTree
participant Panel as RightSidePanel
participant Main as MainContent
participant Chat as WorkspaceChatInput
User->>Diff: Click "Review Changes"
Diff->>Panel: call onReview()
Panel->>Main: forward onReview()
Main->>Chat: insert REVIEW_CODE into input
Chat->>User: shows prefilled review prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/routes/workspaces.ts`:
- Around line 262-277: The code sets lastError when an attempt fails but later
uses it to decide whether to return a "network" error even if a subsequent
attempt succeeded; update the loop around runGh so that when a runGh call
succeeds you clear lastError (or set a boolean like attemptSucceeded = true) and
only map lastError to "network" if no attempt ever succeeded; modify the block
handling result.success in the for-of over attempts (the loop that iterates over
{ repoArg, headArg }) so successful branches reset/mark success and only after
the loop, when no successful lookup occurred, use lastError to decide returning
c.json({ has_pr: false, error: lastError ? "network" : null }).
In `@src/features/workspace/lib/prState.ts`:
- Around line 62-71: The function handling PR state in
src/features/workspace/lib/prState.ts currently maps only "timeout" and
"network" errors on prStatus; add explicit checks for prStatus?.error ===
"gh_not_installed" and prStatus?.error === "gh_not_authenticated" (before the
"!prStatus?.has_pr || !prStatus.pr_number" branch) and return { type: "error",
reason: "gh_not_installed" } and { type: "error", reason: "gh_not_authenticated"
} respectively so those backend error cases are surfaced instead of falling
through to the "no_pr" result; locate the logic around the prStatus variable in
this file and insert the new branches immediately after the existing
timeout/network checks.
In `@src/features/workspace/ui/PRActions.tsx`:
- Around line 176-199: PRLink currently uses an if-chain and may render an
anchor with an empty href (state.prUrl) which navigates to the current page;
update PRLink to use ts-pattern to exhaustively match PRActionState (import and
use match(...).with(...).exhaustive()) for all state.type variants and return
null for non-PR states, and additionally guard against missing/empty state.prUrl
by not rendering the <a> (or rendering a non-link element) when prUrl is falsy;
ensure you still use PR_LINK_COLORS[state.type] for styling only in the matched
"pr" case so TypeScript narrows state safely.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (13)
.claude/agent-memory/code-reviewer/MEMORY.mdbackend/src/routes/workspaces.tsshared/types/github.tssrc/app/layouts/MainContent.tsxsrc/app/layouts/RightSidePanel.tsxsrc/app/layouts/hooks/useWorkspaceActions.tssrc/features/session/lib/sessionPrompts.tssrc/features/workspace/api/workspace.queries.tssrc/features/workspace/lib/prState.tssrc/features/workspace/ui/CodePanelContent.tsxsrc/features/workspace/ui/DiffFilesTree.tsxsrc/features/workspace/ui/PRActions.tsxsrc/features/workspace/ui/PRStatusBar.tsx
💤 Files with no reviewable changes (1)
- src/features/workspace/ui/PRStatusBar.tsx
Comment on lines
+176
to
+199
| function PRLink({ state }: { state: PRActionState }) { | ||
| if (state.type === "gh_unavailable" || state.type === "no_pr" || state.type === "error") return null; | ||
|
|
||
| return ( | ||
| <Tooltip delayDuration={200}> | ||
| <TooltipTrigger asChild> | ||
| <a | ||
| href={state.prUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className={cn( | ||
| "flex items-center gap-1 rounded-md px-1.5 py-1 text-xs font-semibold transition-colors duration-200 ease", | ||
| PR_LINK_COLORS[state.type] ?? "text-text-secondary hover:text-text-primary", | ||
| )} | ||
| > | ||
| <GitPullRequestCreate className="h-3 w-3" /> | ||
| #{state.prNumber} | ||
| </a> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom"> | ||
| <p className="text-xs">Open PR in browser</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find the relevant files
fd -t f "PRActions.tsx" --exec wc -l {} \;Repository: zvadaadam/box-ide
Length of output: 106
🏁 Script executed:
# Read the PRActions.tsx file to verify the code snippet
cat -n src/features/workspace/ui/PRActions.tsx | head -250Repository: zvadaadam/box-ide
Length of output: 10141
🏁 Script executed:
# Also check the github.ts types file to understand PRActionState
fd -t f "github.ts" | grep -v node_modulesRepository: zvadaadam/box-ide
Length of output: 83
🏁 Script executed:
# Read the rest of PRActions.tsx to see PR_LINK_COLORS and any other relevant code
cat -n src/features/workspace/ui/PRActions.tsx | sed -n '200,390p'Repository: zvadaadam/box-ide
Length of output: 7495
🏁 Script executed:
# Read github.ts to understand PRStatus and PRActionState types
cat -n src/shared/types/github.tsRepository: zvadaadam/box-ide
Length of output: 119
🏁 Script executed:
# Also check prState.ts to understand how PRActionState is derived
fd -t f "prState.ts" | head -5Repository: zvadaadam/box-ide
Length of output: 98
🏁 Script executed:
# Find the correct types file
fd -t f "github" --ext tsRepository: zvadaadam/box-ide
Length of output: 277
🏁 Script executed:
# Read the prState.ts to understand PRActionState definition
cat -n src/features/workspace/lib/prState.tsRepository: zvadaadam/box-ide
Length of output: 4718
🏁 Script executed:
# Check the shared/types directory for the types
fd -t f "types" | grep -E "shared|index"Repository: zvadaadam/box-ide
Length of output: 43
🏁 Script executed:
# Look for index.ts in shared/types
cat -n src/shared/types/index.tsRepository: zvadaadam/box-ide
Length of output: 1147
🏁 Script executed:
# List the shared/types directory
ls -la src/shared/types/Repository: zvadaadam/box-ide
Length of output: 229
🏁 Script executed:
# Find where PRStatus is defined
rg "type PRStatus" -A 10Repository: zvadaadam/box-ide
Length of output: 43
🏁 Script executed:
# Check the tsconfig or path aliases
cat -n tsconfig.json | head -50Repository: zvadaadam/box-ide
Length of output: 1426
🏁 Script executed:
# Check the shared directory at repo root
ls -la shared/Repository: zvadaadam/box-ide
Length of output: 343
🏁 Script executed:
# Look for PRStatus definition in shared directory
find shared -name "*.ts" -o -name "*.d.ts" | head -20Repository: zvadaadam/box-ide
Length of output: 285
🏁 Script executed:
# Read the github.ts file
cat -n shared/types/github.tsRepository: zvadaadam/box-ide
Length of output: 1448
Guard missing PR URLs and refactor PRLink to use ts-pattern for exhaustive state matching.
Line 177 uses an if-chain instead of ts-pattern despite the codebase already adopting ts-pattern elsewhere. Additionally, line 183 can render an anchor with an empty href when prUrl is unavailable, which navigates to the current page instead of the PR.
💡 Proposed patch
-import { match } from "ts-pattern";
+import { match, P } from "ts-pattern";
...
function PRLink({ state }: { state: PRActionState }) {
- if (state.type === "gh_unavailable" || state.type === "no_pr" || state.type === "error") return null;
-
- return (
- <Tooltip delayDuration={200}>
- <TooltipTrigger asChild>
- <a
- href={state.prUrl}
- target="_blank"
- rel="noopener noreferrer"
- className={cn(
- "flex items-center gap-1 rounded-md px-1.5 py-1 text-xs font-semibold transition-colors duration-200 ease",
- PR_LINK_COLORS[state.type] ?? "text-text-secondary hover:text-text-primary",
- )}
- >
- <GitPullRequestCreate className="h-3 w-3" />
- #{state.prNumber}
- </a>
- </TooltipTrigger>
- <TooltipContent side="bottom">
- <p className="text-xs">Open PR in browser</p>
- </TooltipContent>
- </Tooltip>
- );
+ return match(state)
+ .with({ type: P.union("gh_unavailable", "no_pr", "error") }, () => null)
+ .with(
+ {
+ type: P.union(
+ "merged",
+ "closed",
+ "conflicts",
+ "ci_failing",
+ "changes_requested",
+ "ci_pending",
+ "ready_to_merge",
+ "awaiting_review",
+ ),
+ },
+ (s) => {
+ const className = cn(
+ "flex items-center gap-1 rounded-md px-1.5 py-1 text-xs font-semibold transition-colors duration-200 ease",
+ PR_LINK_COLORS[s.type] ?? "text-text-secondary hover:text-text-primary",
+ );
+
+ const content = (
+ <>
+ <GitPullRequestCreate className="h-3 w-3" />
+ #{s.prNumber}
+ </>
+ );
+
+ if (!s.prUrl) return <span className={className}>{content}</span>;
+
+ return (
+ <Tooltip delayDuration={200}>
+ <TooltipTrigger asChild>
+ <a href={s.prUrl} target="_blank" rel="noopener noreferrer" className={className}>
+ {content}
+ </a>
+ </TooltipTrigger>
+ <TooltipContent side="bottom">
+ <p className="text-xs">Open PR in browser</p>
+ </TooltipContent>
+ </Tooltip>
+ );
+ },
+ )
+ .exhaustive();
}Aligns with coding guidelines: **/*.{ts,tsx} requires ts-pattern for discriminated unions with .exhaustive() to catch missing cases at compile time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/workspace/ui/PRActions.tsx` around lines 176 - 199, PRLink
currently uses an if-chain and may render an anchor with an empty href
(state.prUrl) which navigates to the current page; update PRLink to use
ts-pattern to exhaustively match PRActionState (import and use
match(...).with(...).exhaustive()) for all state.type variants and return null
for non-PR states, and additionally guard against missing/empty state.prUrl by
not rendering the <a> (or rendering a non-link element) when prUrl is falsy;
ensure you still use PR_LINK_COLORS[state.type] for styling only in the matched
"pr" case so TypeScript narrows state safely.
… edge case Backend: A successful `gh pr list` response on the origin attempt no longer surfaces a stale `lastError` from a failed upstream attempt. Adds `hadSuccessfulResponse` flag so `error: "network"` is only returned when every attempt actually failed. Frontend: `derivePRActionState` now handles `gh_not_installed` and `gh_not_authenticated` errors from the PR status endpoint directly, covering the 5-minute `ghStatus` stale window where gh could be uninstalled/deauthed between status checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/workspace/lib/prState.ts`:
- Around line 52-58: The ghStatus gating currently runs before considering a
fresh prStatus and can return gh_unavailable from stale ghStatus; change the
logic in prState.ts so that a fresh/successful PR payload (prStatus) takes
precedence over ghStatus — either move the ghStatus checks to after prStatus
handling or add a guard that skips returning {type: "gh_unavailable"} when
prStatus indicates a fresh/ready/successful state (check the prStatus variable
and any freshness flags used elsewhere in this file), and apply the same fix for
the additional ghStatus checks in the later block (the section around the 60-76
range) so stale ghStatus cannot override an active prStatus.
Comment on lines
+52
to
+58
| // gh CLI gates everything | ||
| if (ghStatus && !ghStatus.isInstalled) { | ||
| return { type: "gh_unavailable", reason: "not_installed" }; | ||
| } | ||
| if (ghStatus && ghStatus.isInstalled && !ghStatus.isAuthenticated) { | ||
| return { type: "gh_unavailable", reason: "not_authenticated" }; | ||
| } |
There was a problem hiding this comment.
ghStatus should not override a fresh prStatus payload.
With current ordering, stale cached ghStatus can return gh_unavailable even when prStatus is fresh and successful, hiding valid PR actions.
💡 Proposed fix
export function derivePRActionState(
prStatus: PRStatus | null,
ghStatus: GhCliStatus | null | undefined,
targetBranch: string,
): PRActionState {
- // gh CLI gates everything
- if (ghStatus && !ghStatus.isInstalled) {
- return { type: "gh_unavailable", reason: "not_installed" };
- }
- if (ghStatus && ghStatus.isInstalled && !ghStatus.isAuthenticated) {
- return { type: "gh_unavailable", reason: "not_authenticated" };
- }
-
// gh CLI errors from the PR status endpoint — can arrive during the ghStatus
// 5-minute stale window (e.g., gh is uninstalled between status checks).
if (prStatus?.error === "gh_not_installed") {
return { type: "gh_unavailable", reason: "not_installed" };
@@
if (prStatus?.error === "network") {
return { type: "error", reason: "network" };
}
+
+ // Only use cached ghStatus as a fallback when PR status is unavailable.
+ if (!prStatus && ghStatus && !ghStatus.isInstalled) {
+ return { type: "gh_unavailable", reason: "not_installed" };
+ }
+ if (!prStatus && ghStatus?.isInstalled && !ghStatus.isAuthenticated) {
+ return { type: "gh_unavailable", reason: "not_authenticated" };
+ }Also applies to: 60-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/workspace/lib/prState.ts` around lines 52 - 58, The ghStatus
gating currently runs before considering a fresh prStatus and can return
gh_unavailable from stale ghStatus; change the logic in prState.ts so that a
fresh/successful PR payload (prStatus) takes precedence over ghStatus — either
move the ghStatus checks to after prStatus handling or add a guard that skips
returning {type: "gh_unavailable"} when prStatus indicates a
fresh/ready/successful state (check the prStatus variable and any freshness
flags used elsewhere in this file), and apply the same fix for the additional
ghStatus checks in the later block (the section around the 60-76 range) so stale
ghStatus cannot override an active prStatus.
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
Redesigned PR status indicator from muted text to visually prominent tinted pills with state-colored links. PR status is now discoverable at a glance.
Visual changes:
#3→[PR icon] #3(amber text) — links now state-colored at 80% opacitytext-text-muted)text-text-muted)Architecture:
Implemented pure state machine pattern:
src/features/workspace/lib/prState.ts— purederivePRActionState()functionPRActionStatediscriminated unionPRStatusBar.tsx(confirmed via grep)Design consensus:
Synthesized from 5-designer exploration (Rams, Ive, Vignelli, Fukasawa, Jongerius):
Related work:
Test plan
#3in amber#3in blue#3in red, "Fix CI" button solid red#3in red, "Resolve Conflicts" button solid red#3in green, "Merge" button solid green#3in gray, "Archive" button solid primary🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation