feat: open single plan result from url#1034
Conversation
📝 WalkthroughWalkthroughThe changes refactor Changes
Sequence DiagramsequenceDiagram
actor User
participant Page as Page Component
participant Hook as usePlanResultParam
participant Dialog as PlanDiffDialog
participant URL as URL/Query Params
participant API as TRPC Query
User->>Page: Click "View diff" button
Page->>Hook: onViewDiff(resultId)
Hook->>URL: Update search params with resultId
URL->>Page: searchParams change triggers re-render
Page->>Dialog: Pass open=true + resultId
Dialog->>API: Execute resultDiff query (guarded by open && resultId)
API->>Dialog: Return diff data
Dialog->>User: Display diff content
User->>Dialog: Click close/dismiss
Dialog->>Page: Call onOpenChange(false)
Page->>Hook: closeResult()
Hook->>URL: Remove resultId from search params
URL->>Page: searchParams change triggers re-render
Page->>Dialog: Pass open=false
Dialog->>User: Dialog closes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Adds support for deep-linking directly to a single deployment plan result diff by driving the diff dialog from a resultId URL query parameter (Issue #1032).
Changes:
- Introduces a
usePlanResultParamhook to read/writeresultIdin search params. - Refactors the plan results table to open a single, page-level
PlanDiffDialoginstead of per-row dialog triggers. - Updates
PlanDiffDialogto be a controlled component (open/onOpenChange) and accept an optionalresultId.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx | Wires the results table “View diff” action to a URL param and renders a controlled diff dialog. |
| apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts | New hook for managing resultId in the URL search params. |
| apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx | Converts dialog to controlled mode and adjusts diff query behavior for optional resultId. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const resultId = searchParams.get("resultId") ?? undefined; | ||
|
|
||
| const openResult = (id: string) => { |
There was a problem hiding this comment.
searchParams.get("resultId") can return an empty string when the URL contains ?resultId=. In that case resultId becomes "" (not undefined), which will cause the diff dialog to open and attempt to fetch a diff for an invalid id. Consider normalizing empty strings to undefined (e.g., treat "" as absent) and optionally guarding openResult against being called with an empty id.
| const resultId = searchParams.get("resultId") ?? undefined; | |
| const openResult = (id: string) => { | |
| const resultId = searchParams.get("resultId") || undefined; | |
| const openResult = (id: string) => { | |
| if (!id) { | |
| return; | |
| } |
| resultId={resultId} | ||
| title={activeResult ? resultTitle(activeResult) : ""} | ||
| open={resultId != null} |
There was a problem hiding this comment.
The dialog title can be an empty string when resultId is present in the URL but activeResult hasn’t loaded yet (or the id is invalid). This leaves the dialog without an accessible title and can create a confusing blank header. Consider providing a non-empty fallback title (e.g. "Plan diff"/"Loading…") and/or closing the dialog when resultId doesn’t match any loaded result.
| const diffQuery = trpc.deployment.plans.resultDiff.useQuery( | ||
| { deploymentId, resultId }, | ||
| { enabled: open }, | ||
| { deploymentId, resultId: resultId ?? "" }, | ||
| { enabled: open && resultId != null }, |
There was a problem hiding this comment.
enabled: open && resultId != null will still run the query when resultId is an empty string (e.g. URL ?resultId=). That will call the API with resultId: "" due to resultId ?? "". Tighten the guard to require a non-empty id (e.g. !!resultId) and avoid passing a placeholder empty string as the query input.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx (1)
146-212:⚠️ Potential issue | 🟡 MinorDialog can open with an empty title (and fire a query) for a stale/invalid
resultIdin the URL.When the page loads with
?resultId=...deep-linked,resultsis empty during the initial fetch, soactiveResultisundefinedand the dialog opens with an emptytitle. The same happens permanently if the URLresultIddoesn't correspond to any row inresults(stale link, wrong plan, etc.) — the dialog stays open with a blank header whilePlanDiffDialogstill issuesresultDiffwith that id.Consider (a) gating
openonactiveResult != nullonceresultsQueryhas settled, or (b) auto-callingcloseResult()when results have loaded but no match is found, to avoid a stuck dialog with a blank title on invalid deep links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx around lines 146 - 212, The dialog can open with an empty title because PlanDiffDialog's open prop only checks resultId while results are still loading or the id is invalid; update the component to only open the dialog when an actual result is found or to auto-close when results finish loading with no match: change the PlanDiffDialog props to use open={resultId != null && activeResult != null && !resultsQuery.isLoading} (or alternatively, add an effect that calls closeResult() when resultsQuery.isLoading becomes false and activeResult is undefined) and keep title={activeResult ? resultTitle(activeResult) : ""}; reference PlanDiffDialog, activeResult, resultId, resultsQuery, closeResult, and resultTitle to locate the change.
🧹 Nitpick comments (1)
apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts (1)
3-20: Optional: stabilize callbacks and add explicit return type.Minor polish —
openResult/closeResultare recreated on every render, which will invalidate memoization for any consumer that depends on them. Also, per the repo's TS guideline to use explicit types, consider annotating the hook's return shape.Proposed refactor
-import { useSearchParams } from "react-router"; - -export function usePlanResultParam() { - const [searchParams, setSearchParams] = useSearchParams(); - const resultId = searchParams.get("resultId") ?? undefined; - - const openResult = (id: string) => { - const newParams = new URLSearchParams(searchParams); - newParams.set("resultId", id); - setSearchParams(newParams); - }; - - const closeResult = () => { - const newParams = new URLSearchParams(searchParams); - newParams.delete("resultId"); - setSearchParams(newParams); - }; - - return { resultId, openResult, closeResult }; -} +import { useCallback } from "react"; +import { useSearchParams } from "react-router"; + +interface UsePlanResultParam { + resultId: string | undefined; + openResult: (id: string) => void; + closeResult: () => void; +} + +export function usePlanResultParam(): UsePlanResultParam { + const [searchParams, setSearchParams] = useSearchParams(); + const resultId = searchParams.get("resultId") ?? undefined; + + const openResult = useCallback( + (id: string) => { + setSearchParams((prev) => { + const next = new URLSearchParams(prev); + next.set("resultId", id); + return next; + }); + }, + [setSearchParams], + ); + + const closeResult = useCallback(() => { + setSearchParams((prev) => { + const next = new URLSearchParams(prev); + next.delete("resultId"); + return next; + }); + }, [setSearchParams]); + + return { resultId, openResult, closeResult }; +}Using the functional updater form for
setSearchParamsalso avoids closing over a potentially stalesearchParamsvalue if these are called in quick succession.As per coding guidelines: "Use TypeScript with explicit types; prefer
interfacefor public APIs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts` around lines 3 - 20, The hook usePlanResultParam recreates openResult/closeResult each render and lacks an explicit return type; fix by defining an exported interface (e.g., PlanResultParam) for the return shape and annotate usePlanResultParam(): PlanResultParam, and wrap openResult and closeResult with React.useCallback so they are stable, using the functional updater form of setSearchParams inside each callback to avoid closing over stale searchParams; reference the functions usePlanResultParam, openResult, closeResult and setSearchParams when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx:
- Around line 146-212: The dialog can open with an empty title because
PlanDiffDialog's open prop only checks resultId while results are still loading
or the id is invalid; update the component to only open the dialog when an
actual result is found or to auto-close when results finish loading with no
match: change the PlanDiffDialog props to use open={resultId != null &&
activeResult != null && !resultsQuery.isLoading} (or alternatively, add an
effect that calls closeResult() when resultsQuery.isLoading becomes false and
activeResult is undefined) and keep title={activeResult ?
resultTitle(activeResult) : ""}; reference PlanDiffDialog, activeResult,
resultId, resultsQuery, closeResult, and resultTitle to locate the change.
---
Nitpick comments:
In `@apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts`:
- Around line 3-20: The hook usePlanResultParam recreates openResult/closeResult
each render and lacks an explicit return type; fix by defining an exported
interface (e.g., PlanResultParam) for the return shape and annotate
usePlanResultParam(): PlanResultParam, and wrap openResult and closeResult with
React.useCallback so they are stable, using the functional updater form of
setSearchParams inside each callback to avoid closing over stale searchParams;
reference the functions usePlanResultParam, openResult, closeResult and
setSearchParams when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48cc6eda-8f14-466b-b828-9b11166579a7
📒 Files selected for processing (3)
apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsxapps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.tsapps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx
fixes #1032
Summary by CodeRabbit