Conversation
Signed-off-by: rafi <refaei.shikho@hotmail.com>
There was a problem hiding this comment.
Pull request overview
Adds support for selecting an “origin” (root node) for dependency views by introducing a reusable RootNodeSelector component and wiring an origin query param through the dependencies list and graph pages.
Changes:
- Introduce
useRootNodes()hook to fetch artifact root nodes via SWR. - Add
RootNodeSelectordropdown component that reads/writes theoriginquery parameter. - Wire
origininto dependency list (/components) and dependency graph (/dependency-graph) requests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/context/AssetVersionContext.tsx |
Adds useRootNodes() SWR hook for fetching artifact root nodes. |
src/components/RootNodeSelector.tsx |
New dropdown component for choosing/clearing an origin root node via query params. |
src/app/(loading-group)/.../dependencies/page.tsx |
Adds selector to UI and forwards origin to the components query. |
src/app/(loading-group)/.../dependencies/graph/page.tsx |
Adds selector to UI and forwards origin to the graph query. |
src/app/(loading-group)/.../artifacts/page.tsx |
Refactors root-nodes fetching to reuse useRootNodes(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| useEffect(() => { | ||
| handleSelect(undefined); | ||
| }, [artifactName]); |
There was a problem hiding this comment.
The useEffect dependency array omits handleSelect, even though it’s referenced inside the effect. This can lead to stale closures if pushQueryParameter ever changes. Include handleSelect (or inline the logic) to satisfy the rules-of-hooks/exhaustive-deps behavior.
| }, [artifactName]); | |
| }, [artifactName, handleSelect]); |
| <DropdownMenu open={open} onOpenChange={setOpen}> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant="outline"> | ||
| <div className="flex w-42 items-center justify-between h-4"> |
There was a problem hiding this comment.
w-42 is not a default Tailwind width utility, and tailwind.config.js doesn’t extend widths. This class will be ignored, so the button layout likely won’t match expectations. Use a valid utility (e.g., w-44) or an arbitrary value like w-[10.5rem].
| <div className="flex w-42 items-center justify-between h-4"> | |
| <div className="flex w-[10.5rem] items-center justify-between h-4"> |
| const rootnodeParam = searchParams?.get("origin"); | ||
|
|
||
| const [selectedRootNode, setSelectedRootNode] = useState( | ||
| getRootNodeName(rootnodeParam), | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| setSelectedRootNode(getRootNodeName(rootnodeParam)); | ||
| }, [rootnodeParam]); |
There was a problem hiding this comment.
rootnodeParam is not camelCased consistently with the rest of the file (should be rootNodeParam). Renaming improves readability and avoids subtle typos when adding related variables.
| const rootnodeParam = searchParams?.get("origin"); | |
| const [selectedRootNode, setSelectedRootNode] = useState( | |
| getRootNodeName(rootnodeParam), | |
| ); | |
| useEffect(() => { | |
| setSelectedRootNode(getRootNodeName(rootnodeParam)); | |
| }, [rootnodeParam]); | |
| const rootNodeParam = searchParams?.get("origin"); | |
| const [selectedRootNode, setSelectedRootNode] = useState( | |
| getRootNodeName(rootNodeParam), | |
| ); | |
| useEffect(() => { | |
| setSelectedRootNode(getRootNodeName(rootNodeParam)); | |
| }, [rootNodeParam]); |
| if (searchParams?.has("origin")) { | ||
| params.append( | ||
| "origin", | ||
| encodeURIComponent(searchParams.get("origin") as string), | ||
| ); |
There was a problem hiding this comment.
params.append() will URL-encode when you later call params.toString(). Pre-encoding the value with encodeURIComponent here can double-encode (especially problematic for origin, which is likely a URL). Append the raw value and let URLSearchParams handle encoding.
| const params = useDecodedParams() as { | ||
| organizationSlug: string; | ||
| projectSlug: string; | ||
| assetSlug: string; | ||
| assetVersionSlug: string; | ||
| }; |
There was a problem hiding this comment.
params is computed via useDecodedParams() but is no longer used after switching the SWR call to useRootNodes(). Please remove this unused variable to avoid lint/build failures.
| import { NoopUpdater, WithUpdater } from "./ClientContextWrapper"; | ||
| import { fetcher } from "@/data-fetcher/fetcher"; | ||
| import useSWR from "swr"; | ||
| import { useActiveAsset } from "@/hooks/useActiveAsset"; |
There was a problem hiding this comment.
useActiveAsset is imported but never used in this file. Please remove the unused import to avoid lint/build failures and keep the module dependency graph minimal.
| import { useActiveAsset } from "@/hooks/useActiveAsset"; |
| useEffect(() => { | ||
| handleSelect(undefined); | ||
| }, [artifactName]); |
There was a problem hiding this comment.
This effect clears the origin query param whenever artifactName changes, but it also runs on initial mount. That means deep-linking to a URL that already includes ?artifact=...&origin=... will immediately lose origin. Consider skipping the initial run (e.g., via a ref) or only clearing when the artifact actually changes from a previous value.
1 participant
No description provided.