Review UI bug fixes

docs/plans/2026-04-03-001-fix-review-button-routing-plan.md

@@ -0,0 +1,162 @@
+---
+title: "fix: Review button routes to correct stage and remove redundant Revisions & Comments"
+type: fix
+status: completed
+date: 2026-04-03
+---
+
+# fix: Review button routes to correct stage and remove redundant Revisions & Comments
+
+## Overview
+
+Two related issues on the deliverable detail page: (1) the "Review" button navigates to the review page but always defaults to the first non-blocked stage (typically "Brief Intake") instead of the stage the user clicked, and (2) the "Revisions & Comments" sheet is now redundant since the review page provides a richer superset of that functionality.
+
+## Problem Frame
+
+When a user clicks "Review" on a specific stage like "Catalog Images", they expect to land on the review page with that stage selected. Instead, the review page ignores which stage was clicked and auto-selects the first non-blocked stage. This forces the user to manually toggle to the correct stage every time.
+
+The "Revisions & Comments" button opens a side sheet with revision management and threaded comments. The review page's sidebar (`ReviewSidebar`) now provides revision timeline, feedback/annotation management, and richer visual review tools — making the sheet redundant.
+
+## Requirements Trace
+
+- R1. Clicking "Review" on a stage must open the review page with that stage pre-selected
+- R2. Remove the "Revisions & Comments" button and stage-detail-sheet from the deliverable detail page
+- R3. No regression in review page behavior when navigated to without a stage param (default to first non-blocked stage)
+
+## Scope Boundaries
+
+- Not changing the review page's sidebar, annotation, or video review functionality
+- Not adding comments to the review page (that's a separate feature if needed later)
+- Not removing the `CommentThread` or `RevisionList` components themselves — they may be used elsewhere or re-used later
+
+## Context & Research
+
+### Relevant Code and Patterns
+
+- **Review button**: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx:398-409` — Link to `/projects/{pid}/deliverables/{did}/review` with no stage context
+- **Review page stage selection**: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx:103-170` — `selectedStageId` state initialized to `null`, auto-select useEffect picks first non-blocked stage
+- **Stage detail sheet**: `src/components/stages/stage-detail-sheet.tsx` — the redundant Revisions & Comments sheet
+- **Review sidebar**: `src/components/review/review-sidebar.tsx` — the review page's sidebar that supersedes the sheet
+- The project uses `nuqs` for URL state management, but simple query params via `useSearchParams` from Next.js are also fine for a one-time initial value
+
+### Institutional Learnings
+
+- No prior solutions documented for routing or navigation patterns
+
+## Key Technical Decisions
+
+- **Use query parameter `?stage={stageId}` instead of URL segment**: The review page route is already `[deliverableId]/review`. Adding a query param is simpler than restructuring the route and only needs to seed the initial state, not stay in sync.
+- **Read param once on mount, not continuously**: The `selectedStageId` is local state. The query param seeds it; subsequent stage switches within the review page don't need to update the URL.
+- **Remove the sheet entirely from the deliverable page**: Rather than hiding or feature-flagging it, clean-remove the button, state, and sheet rendering. The `StageDetailSheet` component file and its dependencies (`RevisionList`, `CommentThread`) stay in the codebase since they're standalone components.
+
+## Open Questions
+
+### Resolved During Planning
+
+- **Should we use `nuqs` or `useSearchParams`?** Use `useSearchParams` from `next/navigation` — the param is read once on mount to seed state, not continuously synced. No need for nuqs here.
+
+### Deferred to Implementation
+
+- None
+
+## Implementation Units
+
+- [ ] **Unit 1: Pass stage ID via query param from Review button**
+
+  **Goal:** Make the Review button include the clicked stage's ID in the URL
+
+  **Requirements:** R1
+
+  **Dependencies:** None
+
+  **Files:**
+  - Modify: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx`
+
+  **Approach:**
+  - Change the `Link` href from `/projects/${projectId}/deliverables/${deliverableId}/review` to include `?stage=${stage.id}`
+  - The stage variable is already available in the `.map()` callback where the button is rendered
+
+  **Patterns to follow:**
+  - Existing `Link` usage in the same file
+
+  **Test expectation:** none — pure URL string change in a Link component, verified manually
+
+  **Verification:**
+  - Clicking "Review" on any stage produces a URL like `/projects/.../review?stage=<stageId>`
+
+- [ ] **Unit 2: Read stage query param on review page mount**
+
+  **Goal:** Review page pre-selects the stage specified in the URL query param
+
+  **Requirements:** R1, R3
+
+  **Dependencies:** Unit 1
+
+  **Files:**
+  - Modify: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx`
+
+  **Approach:**
+  - Import `useSearchParams` from `next/navigation`
+  - Read `searchParams.get("stage")` once
+  - Modify the auto-select `useEffect` (lines 164-170): if a `stage` param exists and matches a stage ID in the loaded stages array, use it as the initial selection instead of defaulting to the first non-blocked stage
+  - Keep the existing fallback logic for when no param is provided (R3)
+
+  **Patterns to follow:**
+  - Existing `useParams` usage at the top of the same component
+
+  **Test scenarios:**
+  - Happy path: Navigate with `?stage=<validId>` → that stage is selected on load
+  - Happy path: Navigate without `?stage` param → first non-blocked stage selected (existing behavior preserved)
+  - Edge case: Navigate with `?stage=<invalidId>` → falls back to first non-blocked stage
+
+  **Verification:**
+  - Opening the review page via a stage's Review button shows that stage selected
+  - Opening the review page without a stage param still defaults correctly
+  - Opening with a bogus stage param doesn't break the page
+
+- [ ] **Unit 3: Remove Revisions & Comments button and sheet from deliverable page**
+
+  **Goal:** Clean up the redundant "Revisions & Comments" UI from the deliverable detail page
+
+  **Requirements:** R2
+
+  **Dependencies:** None (can be done in parallel with Units 1-2)
+
+  **Files:**
+  - Modify: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx`
+
+  **Approach:**
+  - Remove the `selectedStage` state and `setSelectedStage` handler used to open the sheet
+  - Remove the "Revisions & Comments" `Button` from the stage card action buttons area (lines 389-397)
+  - Remove the `<StageDetailSheet>` rendering at the bottom of the component
+  - Remove the import of `StageDetailSheet`
+  - Remove any now-unused imports (e.g., `FileText` icon if only used there)
+
+  **Patterns to follow:**
+  - Keep the `StageDetailSheet` component file intact — just remove its usage from this page
+
+  **Test expectation:** none — removal of UI elements, verified manually
+
+  **Verification:**
+  - No "Revisions & Comments" button visible on any stage card
+  - No sheet opens on the deliverable page
+  - The Review button still works and is the sole action for non-blocked stages
+
+## System-Wide Impact
+
+- **Interaction graph:** The `StageDetailSheet` component is only used on the deliverable detail page. Removing its usage here has no side effects elsewhere.
+- **API surface parity:** No API changes needed — the review page already fetches all necessary data.
+- **Unchanged invariants:** The review page's sidebar functionality, video review, annotation system, and all review page features remain untouched.
+
+## Risks & Dependencies
+
+| Risk | Mitigation |
+|------|------------|
+| Stage ID format mismatch between URL and data | The same `stage.id` is used to build the URL and to match in `stages.find()` — no transformation needed |
+| Users bookmarking old review URLs without `?stage` | Fallback behavior (R3) handles this gracefully |
+
+## Sources & References
+
+- Review button: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx:398-409`
+- Review page stage selection: `src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx:103-170`
+- Stage detail sheet: `src/components/stages/stage-detail-sheet.tsx`

src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx

@@ -1,6 +1,5 @@
 "use client";
 
-import { useState } from "react";
 import { useParams } from "next/navigation";
 import Link from "next/link";
 import { format } from "date-fns";
@@ -8,7 +7,6 @@ import {
   ArrowLeft,
   ChevronRight,
   Eye,
-  FileText,
   Lock,
   RotateCcw,
   Shield,
@@ -29,7 +27,6 @@ import {
 import { StageDatePopover } from "@/components/stages/stage-date-popover";
 import { PipelineProgress } from "@/components/deliverables/pipeline-progress";
 import { StageStatusBadge } from "@/components/stages/stage-status-badge";
-import { StageDetailSheet } from "@/components/stages/stage-detail-sheet";
 import { FeedbackIndicator } from "@/components/stages/feedback-indicator";
 import { TooltipProvider } from "@/components/ui/tooltip";
 import { cn } from "@/lib/utils";
@@ -69,7 +66,6 @@ export default function DeliverableDetailPage() {
   }>();
   const { data, isLoading } = useDeliverable(projectId, deliverableId);
   const updateStage = useUpdateStageStatus();
-  const [selectedStage, setSelectedStage] = useState<any>(null);
 
   const deliverable = data as any;
 
@@ -386,17 +382,8 @@ export default function DeliverableDetailPage() {
                   {/* Action buttons */}
                   {stage.status !== "BLOCKED" && (
                     <div className="flex items-center gap-1.5">
-                      <Button
-                        size="sm"
-                        variant="ghost"
-                        className="h-6 text-[10px] text-[var(--muted-foreground)]"
-                        onClick={() => setSelectedStage(stage)}
-                      >
-                        <FileText className="mr-1 h-3 w-3" />
-                        Revisions & Comments
-                      </Button>
                       <Link
-                        href={`/projects/${projectId}/deliverables/${deliverableId}/review`}
+                        href={`/projects/${projectId}/deliverables/${deliverableId}/review?stage=${stage.id}`}
                       >
                         <Button
                           size="sm"
@@ -416,14 +403,6 @@ export default function DeliverableDetailPage() {
         </div>
       </div>
 
-      {/* Stage detail sheet for revisions & comments */}
-      <StageDetailSheet
-        stage={selectedStage}
-        open={!!selectedStage}
-        onOpenChange={(open) => {
-          if (!open) setSelectedStage(null);
-        }}
-      />
     </div>
     </TooltipProvider>
   );

src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx

@@ -1,7 +1,7 @@
 "use client";
 
 import { useCallback, useEffect, useMemo, useRef, useState } from "react";
-import { useParams } from "next/navigation";
+import { useParams, useSearchParams } from "next/navigation";
 import Link from "next/link";
 import {
   ArrowLeft,
@@ -92,6 +92,8 @@ export default function ReviewPage() {
     projectId: string;
     deliverableId: string;
   }>();
+  const searchParams = useSearchParams();
+  const initialStageId = searchParams.get("stage");
   const queryClient = useQueryClient();
 
   const { data: deliverableData, isLoading: delLoading } = useDeliverable(
@@ -160,14 +162,17 @@ export default function ReviewPage() {
     );
   }, [deliverable]);
 
-  // Auto-select first stage
+  // Auto-select stage from URL param, or fall back to first non-blocked stage
   useEffect(() => {
     if (stages.length > 0 && !selectedStageId) {
-      const first =
+      const fromParam = initialStageId
+        ? stages.find((s: any) => s.id === initialStageId)
+        : null;
+      const first = fromParam ??
         stages.find((s: any) => s.status !== "BLOCKED") ?? stages[0];
       setSelectedStageId(first.id);
     }
-  }, [stages, selectedStageId]);
+  }, [stages, selectedStageId, initialStageId]);
 
   const selectedStage = stages.find((s: any) => s.id === selectedStageId);
   const stageIdx = stages.findIndex((s: any) => s.id === selectedStageId);
@@ -522,7 +527,7 @@ export default function ReviewPage() {
   return (
     <div className="flex h-full flex-col -m-4 md:-m-6">
       {/* ── Top bar ──────────────────────────────────────────────── */}
-      <div className="flex items-center justify-between border-b bg-[var(--card)] px-4 py-2">
+      <div className="grid grid-cols-[1fr_auto_1fr] items-center border-b bg-[var(--card)] px-4 py-2">
         {/* Left: back + deliverable name */}
         <div className="flex items-center gap-3">
           <Link
@@ -550,14 +555,14 @@ export default function ReviewPage() {
             >
               <ChevronLeft className="h-3.5 w-3.5" />
             </Button>
-            <div className="flex items-center gap-1.5 rounded-md border bg-[var(--background)] px-2.5 py-1">
+            <div className="flex w-72 items-center justify-center gap-1.5 rounded-md border bg-[var(--background)] px-2.5 py-1">
               <span className="font-mono text-[10px] text-[var(--muted-foreground)]">
                 {selectedStage.stageDefinition?.order ?? selectedStage.template.order}
               </span>
-              <span className="text-xs font-medium">
+              <span className="truncate text-xs font-medium">
                 {selectedStage.stageDefinition?.name ?? selectedStage.template.name}
               </span>
-              <StageStatusBadge status={selectedStage.status} />
+              <StageStatusBadge status={selectedStage.status} className="shrink-0" />
             </div>
             <Button
               size="sm"
@@ -572,7 +577,7 @@ export default function ReviewPage() {
         )}
 
         {/* Right: actions */}
-        <div className="flex items-center gap-1.5">
+        <div className="flex items-center justify-end gap-1.5">
           {/* Image/Video toggle */}
           {hasImageAttachment && hasVideoAttachment && (
             <div className="flex items-center rounded-md border bg-[var(--background)]">