Add fullscreen review panel for show_changes#75
Conversation
📝 WalkthroughWalkthroughThe review UI now builds shared file entries, supports inline and fullscreen presentations, adds file-tree navigation, and handles host display-mode transitions. New components, responsive styling, model tests, and the tree dependency support the workflow. ChangesReview workspace
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Waishnav
force-pushed
the
codex/fullscreen-review-panel
branch
from
c27164f to
ebe763c
Compare
![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
🧹 Nitpick comments (4)
package.json (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a glob pattern for tests to simplify the script.
As the number of test files grows, concatenating commands with
&&becomes difficult to maintain. Consider using a glob pattern withtsx --test(ornode --testif using the native Node test runner) to automatically discover and run all test files.🛠️ Proposed refactor
- "test": "tsx src/config.test.ts && tsx src/ui/card-types.test.ts && tsx src/ui/patch-display.test.ts && tsx src/ui/tool-display.test.ts && tsx src/ui/review-model.test.ts && tsx src/apply-patch.test.ts && tsx src/process-platform.test.ts && tsx src/process-sessions.test.ts && tsx src/local-agent-runtime.test.ts && tsx src/local-agent-adapters.test.ts && tsx src/local-agent-availability.test.ts && tsx src/local-agent-profiles.test.ts && tsx src/local-agent-targets.test.ts && tsx src/local-agent-store.test.ts && tsx src/roots.test.ts && tsx src/skills.test.ts && tsx src/workspaces.test.ts && tsx src/review-checkpoints.test.ts && tsx src/oauth-store.test.ts && tsx src/cli.test.ts", + "test": "tsx --test \"src/**/*.test.ts\"",🤖 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 `@package.json` at line 31, Update the package.json test script to use the test runner’s glob-based discovery via tsx --test, replacing the manually chained test file list. Ensure the pattern includes all relevant test files under src while preserving the existing test command behavior.src/ui/workspace-app.tsx (2)
283-300: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSimplify a no-op
Math.maxexpression.
(card.files ?? []).slice(0, 3).lengthis capped at 3, soMath.max(3, …)always evaluates to3regardless of the actual file count. The expression reads as if it scales with file count but it doesn't — simplify it to the literal it always resolves to.♻️ Suggested refactor
const visibleFileCount = isReviewTool(card.tool) && presentation === "inline" && !reviewFilesExpanded - ? Math.max(3, (card.files ?? []).slice(0, 3).length) + ? 3 : undefined;🤖 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 `@src/ui/workspace-app.tsx` around lines 283 - 300, In the visibleFileCount calculation within the review/patch tool branch, replace the redundant Math.max expression with the literal value it always produces. Preserve the existing conditions and leave the undefined fallback unchanged.
30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating
MountedPayload/PayloadRendererOptionsshape.This interface is manually kept in sync with the one in
review-payload.tsx(both needed thepresentationfield added in this PR). Sinceimport typeis fully erased at compile time, it won't affect the dynamic/lazy import of the module, so you can import the type directly instead of hand-duplicating it.♻️ Suggested refactor
-interface MountedPayload { - update(options: { - card: ToolResultCard; - hostContext?: HostContext; - errorMessage?: string | null; - visibleFileCount?: number; - presentation?: "inline" | "fullscreen"; - }): void; - unmount(): void; -} +import type { MountedPayload } from "./review-payload.js";🤖 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 `@src/ui/workspace-app.tsx` around lines 30 - 39, Remove the locally duplicated MountedPayload interface in workspace-app.tsx and import the existing payload type from review-payload.tsx using import type. Update the relevant mounting code to reference that shared type, preserving the current update options and unmount behavior.src/ui/workspace-app.css (1)
201-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
.review-header-actionsselectorsrc/ui/workspace-app.css:201-257—.review-header-actionshas no consumer in the reviewed TS/TSX; remove the dead selector or apply it to the header wrapper if it’s still needed.🤖 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 `@src/ui/workspace-app.css` around lines 201 - 257, Remove the unused .review-header-actions selector from the shared display and alignment rule in the review header styles. Keep .review-fullscreen-actions and .review-fullscreen-title behavior unchanged; only retain .review-header-actions if a verified header wrapper consumer requires those styles.
🤖 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 `@src/ui/review-model.ts`:
- Around line 51-53: Update the fallback status mapping in the review model to
call reviewFileStatus(fileDiff.type) instead of returning the hardcoded
"modified" value, while preserving the existing "renamed" branch for files whose
prevName differs from name.
In `@src/ui/workspace-app.tsx`:
- Around line 429-442: Update the fullscreen Review button visibility condition
in the surrounding header-rendering logic to use the already computed expandable
state (or equivalent isExpandableCard result) instead of requiring files.length
> 0, so patch-only review cards with card.payload.patch can show the button
while non-expandable cards remain hidden.
- Around line 101-117: Update the app.onhostcontextchanged handler to clear the
stale reviewDisplayModeError whenever the host display mode changes externally,
before rendering the updated review tool UI. Preserve the existing display-mode
change detection and render flow, and avoid clearing the error when the display
mode is unchanged.
---
Nitpick comments:
In `@package.json`:
- Line 31: Update the package.json test script to use the test runner’s
glob-based discovery via tsx --test, replacing the manually chained test file
list. Ensure the pattern includes all relevant test files under src while
preserving the existing test command behavior.
In `@src/ui/workspace-app.css`:
- Around line 201-257: Remove the unused .review-header-actions selector from
the shared display and alignment rule in the review header styles. Keep
.review-fullscreen-actions and .review-fullscreen-title behavior unchanged; only
retain .review-header-actions if a verified header wrapper consumer requires
those styles.
In `@src/ui/workspace-app.tsx`:
- Around line 283-300: In the visibleFileCount calculation within the
review/patch tool branch, replace the redundant Math.max expression with the
literal value it always produces. Preserve the existing conditions and leave the
undefined fallback unchanged.
- Around line 30-39: Remove the locally duplicated MountedPayload interface in
workspace-app.tsx and import the existing payload type from review-payload.tsx
using import type. Update the relevant mounting code to reference that shared
type, preserving the current update options and unmount behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5304498a-6df1-4948-a0b7-fce4713b06ae
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/ui/review-file-body.tsxsrc/ui/review-file-tree.tsxsrc/ui/review-fullscreen.tsxsrc/ui/review-model.test.tssrc/ui/review-model.tssrc/ui/review-payload.tsxsrc/ui/review-status-line.tsxsrc/ui/workspace-app.csssrc/ui/workspace-app.tsx
| status: fileDiff.prevName && fileDiff.prevName !== fileDiff.name | ||
| ? "renamed" | ||
| : "modified", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect `FileDiffMetadata` definition to see available properties for added/deleted files.
rg -A 15 'interface FileDiffMetadata|type FileDiffMetadata' node_modules/@pierre/diffsRepository: Waishnav/devspace
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the review model and any type definitions for diffs in the repo.
git ls-files 'src/ui/review-model.ts' 'package.json' 'pnpm-lock.yaml' 'yarn.lock' 'package-lock.json' '**/*diff*' '**/*review*' | sed -n '1,200p'
echo
echo '--- src/ui/review-model.ts ---'
cat -n src/ui/review-model.ts | sed -n '1,140p'
echo
echo '--- search for FileDiffMetadata / diff metadata shapes ---'
rg -n "FileDiffMetadata|isAdded|isDeleted|prevName|status:\s*fileDiff" src . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || trueRepository: Waishnav/devspace
Length of output: 5107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- package.json ---'
cat -n package.json | sed -n '1,220p'
echo
echo '--- package-lock references to `@pierre/diffs` ---'
rg -n '"`@pierre/diffs`"|`@pierre/diffs`' package-lock.json package.json || true
echo
echo '--- nearby package-lock context ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('package-lock.json')
if not p.exists():
print('package-lock.json missing')
raise SystemExit(0)
data = json.loads(p.read_text())
pkgs = data.get('packages', {})
for k, v in pkgs.items():
if 'node_modules/@pierre/diffs' in k:
print(k)
for key in ['version', 'resolved', 'integrity', 'dependencies']:
if key in v:
print(f' {key}: {v[key]}')
PYRepository: Waishnav/devspace
Length of output: 4578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/ui/review-model.test.ts ---'
cat -n src/ui/review-model.test.ts | sed -n '1,260p'
echo
echo '--- look for review-model consumers / status assumptions ---'
rg -n "reviewFileStatus|buildReviewFileEntries|status === \"added\"|status === \"deleted\"|status === \"renamed\"|status === \"modified\"" src/ui src --glob '!**/node_modules/**' || trueRepository: Waishnav/devspace
Length of output: 4618
Use the parsed file type in the fallback status mapping
The fallback branch only checks for renames, so unmatched added or deleted files will still be labeled modified. Reuse reviewFileStatus(fileDiff.type) here instead of hardcoding "modified".
🤖 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 `@src/ui/review-model.ts` around lines 51 - 53, Update the fallback status
mapping in the review model to call reviewFileStatus(fileDiff.type) instead of
returning the hardcoded "modified" value, while preserving the existing
"renamed" branch for files whose prevName differs from name.
| app.onhostcontextchanged = (ctx) => { | ||
| const previousDisplayMode = hostContext?.displayMode; | ||
| hostContext = { | ||
| ...hostContext, | ||
| ...ctx, | ||
| }; | ||
| applyHostContext(); | ||
| if ( | ||
| previousDisplayMode !== hostContext.displayMode && | ||
| card && | ||
| isReviewTool(card.tool) | ||
| ) { | ||
| render(); | ||
| return; | ||
| } | ||
| renderPayloadIfNeeded(); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale reviewDisplayModeError can persist across an external display-mode change.
reviewDisplayModeError is only cleared in requestReviewDisplayMode and on a new tool result. If the host changes displayMode through this handler (not via requestReviewDisplayMode), a previously shown error banner remains stuck in the new render.
🩹 Suggested fix
if (
previousDisplayMode !== hostContext.displayMode &&
card &&
isReviewTool(card.tool)
) {
+ reviewDisplayModeError = null;
render();
return;
}📝 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.
| app.onhostcontextchanged = (ctx) => { | |
| const previousDisplayMode = hostContext?.displayMode; | |
| hostContext = { | |
| ...hostContext, | |
| ...ctx, | |
| }; | |
| applyHostContext(); | |
| if ( | |
| previousDisplayMode !== hostContext.displayMode && | |
| card && | |
| isReviewTool(card.tool) | |
| ) { | |
| render(); | |
| return; | |
| } | |
| renderPayloadIfNeeded(); | |
| }; | |
| app.onhostcontextchanged = (ctx) => { | |
| const previousDisplayMode = hostContext?.displayMode; | |
| hostContext = { | |
| ...hostContext, | |
| ...ctx, | |
| }; | |
| applyHostContext(); | |
| if ( | |
| previousDisplayMode !== hostContext.displayMode && | |
| card && | |
| isReviewTool(card.tool) | |
| ) { | |
| reviewDisplayModeError = null; | |
| render(); | |
| return; | |
| } | |
| renderPayloadIfNeeded(); | |
| }; |
🤖 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 `@src/ui/workspace-app.tsx` around lines 101 - 117, Update the
app.onhostcontextchanged handler to clear the stale reviewDisplayModeError
whenever the host display mode changes externally, before rendering the updated
review tool UI. Preserve the existing display-mode change detection and render
flow, and avoid clearing the error when the display mode is unchanged.
| headerRow.append(header); | ||
| if (files.length > 0 && canRequestDisplayMode("fullscreen")) { | ||
| const reviewButton = element("button", { | ||
| className: "review-button", | ||
| type: "button", | ||
| text: reviewDisplayModePending ? "Opening…" : "Review", | ||
| disabled: reviewDisplayModePending, | ||
| }); | ||
| reviewButton.setAttribute("aria-busy", String(reviewDisplayModePending)); | ||
| reviewButton.addEventListener("click", () => { | ||
| void requestReviewDisplayMode("fullscreen"); | ||
| }); | ||
| headerRow.append(reviewButton); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fullscreen "Review" button hidden for patch-only review cards.
The button visibility check uses files.length > 0, but isExpandableCard (and the expandable variable already computed above) treats a review card as expandable via card.files?.length || card.payload?.patch. A show_changes card whose diff only lives in payload.patch (no files entries) can still be expanded inline, but the fullscreen entry point never appears for it.
🐛 Suggested fix
- if (files.length > 0 && canRequestDisplayMode("fullscreen")) {
+ if (expandable && canRequestDisplayMode("fullscreen")) {📝 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.
| headerRow.append(header); | |
| if (files.length > 0 && canRequestDisplayMode("fullscreen")) { | |
| const reviewButton = element("button", { | |
| className: "review-button", | |
| type: "button", | |
| text: reviewDisplayModePending ? "Opening…" : "Review", | |
| disabled: reviewDisplayModePending, | |
| }); | |
| reviewButton.setAttribute("aria-busy", String(reviewDisplayModePending)); | |
| reviewButton.addEventListener("click", () => { | |
| void requestReviewDisplayMode("fullscreen"); | |
| }); | |
| headerRow.append(reviewButton); | |
| } | |
| headerRow.append(header); | |
| if (expandable && canRequestDisplayMode("fullscreen")) { | |
| const reviewButton = element("button", { | |
| className: "review-button", | |
| type: "button", | |
| text: reviewDisplayModePending ? "Opening…" : "Review", | |
| disabled: reviewDisplayModePending, | |
| }); | |
| reviewButton.setAttribute("aria-busy", String(reviewDisplayModePending)); | |
| reviewButton.addEventListener("click", () => { | |
| void requestReviewDisplayMode("fullscreen"); | |
| }); | |
| headerRow.append(reviewButton); | |
| } |
🤖 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 `@src/ui/workspace-app.tsx` around lines 429 - 442, Update the fullscreen
Review button visibility condition in the surrounding header-rendering logic to
use the already computed expandable state (or equivalent isExpandableCard
result) instead of requiring files.length > 0, so patch-only review cards with
card.payload.patch can show the button while non-expandable cards remain hidden.
1 participant
Summary
show_changescardsValidation
npm cinpm run typechecknpm testnpm run buildnpx react-doctor@latest --verbose --scope changed(100/100)Summary by CodeRabbit
New Features
Tests