diff --git a/docs/plans/2026-04-03-001-fix-review-button-routing-plan.md b/docs/plans/2026-04-03-001-fix-review-button-routing-plan.md new file mode 100644 index 0000000..fc36f38 --- /dev/null +++ b/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=` + +- [ ] **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=` → 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=` → 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 `` 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` diff --git a/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx b/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx index 944dab2..6711f69 100644 --- a/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/page.tsx +++ b/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(null); const deliverable = data as any; @@ -386,17 +382,8 @@ export default function DeliverableDetailPage() { {/* Action buttons */} {stage.status !== "BLOCKED" && (
-
- {/* Stage detail sheet for revisions & comments */} - { - if (!open) setSelectedStage(null); - }} - /> ); diff --git a/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx b/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx index ec938dd..be2d876 100644 --- a/src/app/(app)/projects/[projectId]/deliverables/[deliverableId]/review/page.tsx +++ b/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 (
{/* ── Top bar ──────────────────────────────────────────────── */} -
+
{/* Left: back + deliverable name */}
-
+
{selectedStage.stageDefinition?.order ?? selectedStage.template.order} - + {selectedStage.stageDefinition?.name ?? selectedStage.template.name} - +