refactor: deploy UI issues and way we access data#4995
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds ProjectDataProvider and DeploymentLayoutProvider; migrates many components from per-project live DB queries to provider-driven data access; converts per-project collection factories to global collections that require projectId filters; updates UI components to derive IDs from the new providers; upgrades TanStack query-db/react-db deps. Changes
Sequence DiagramsequenceDiagram
actor User
participant Route as Next.js Route Params
participant Provider as ProjectDataProvider
participant DeploymentCtx as DeploymentLayoutProvider
participant Collections as Global Collections
participant TRPC as TRPC Client
participant Component as Dashboard Component
User->>Route: Navigate to /projects/[projectId] or /deployments/[deploymentId]
Route->>Provider: Initialize with projectId
Route->>DeploymentCtx: Initialize with deploymentId
Provider->>Collections: Subscribe/query projects, deployments, domains, environments
Collections->>TRPC: TRPC queries filtered by projectId
TRPC-->>Collections: Return data
Collections-->>Provider: Emit data
Provider->>Provider: Memoize selectors & refetch functions
Provider-->>Component: Context (useProjectData) available
DeploymentCtx-->>Component: Context (useDeployment) available
Component->>Provider: Call selectors (getDeploymentById, getDomainsForDeployment)
User->>Component: Trigger action (promotion/rollback/refetch)
Component->>Provider: Call refetchAll() or specific refetch
Provider->>Collections: Trigger refetch
Collections->>TRPC: Re-query updated data
TRPC-->>Collections: New data
Collections-->>Provider: Emit updates
Provider-->>Component: Context updates cause re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
ogzhanolguncu
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/openapi-diff/page.tsx (1)
72-84:⚠️ Potential issue | 🟠 MajorThe diff query is disabled and never executes.
The
enabled: falseoption prevents this query from ever running. There's norefetch()call or mechanism to trigger the query when deployments are selected. ThediffDatawill always beundefined, so the diff content will never render.Should this be
enabled: !!selectedFromDeployment && !!selectedToDeploymentto fetch when both deployments are selected?🐛 Proposed fix
{ - enabled: false, + enabled: !!selectedFromDeployment && !!selectedToDeployment, },web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/table/deployments-list.tsx (1)
295-306:⚠️ Potential issue | 🟠 MajorBug:
liveDeploymentretrieves the current row's deployment instead of the actual live deployment.Line 295 calls
getDeploymentById(deployment.id)which returns the same deployment being rendered in the current row. Based on the prop usage inDeploymentListTableActions, theliveDeploymentis compared againstselectedDeployment.idto determine if rollback/promote actions should be enabled. WhenliveDeploymentequals the current row's deployment, the conditionselectedDeployment.id !== liveDeployment.idis always false, disabling these actions for all rows. The code should retrieve the actual live deployment usingliveDeploymentIdinstead.🐛 Proposed fix
render: ({ deployment, environment, }: { deployment: Deployment; environment?: Environment; }) => { - const liveDeployment = getDeploymentById(deployment.id); + const liveDeployment = liveDeploymentId ? getDeploymentById(liveDeploymentId) : undefined; return ( <div className="pl-5"> <DeploymentListTableActions selectedDeployment={deployment} liveDeployment={liveDeployment} environment={environment} /> </div> ); },web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/network/deployment-network-view.tsx (1)
34-63:⚠️ Potential issue | 🟡 MinorRemove the non-null assertion on
deploymentId.The
deploymentIdfromuseDeployment()is always a non-null string (the context provider validates it at the boundary), so thedeploymentId!non-null assertion is redundant and violates the repo's type-safety guideline. Simply passdeploymentIddirectly.Minimal fix
<InternalDevTreeGenerator // biome-ignore lint/style/noNonNullAssertion: will be fixed later, when we actually implement tRPC logic - deploymentId={deploymentId!} + deploymentId={deploymentId} onGenerate={setGeneratedTree}
🤖 Fix all issues with AI agents
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(overview)/navigations/use-deployment-breadcrumb-config.ts:
- Line 29: Replace the unsafe type assertion for workspaceSlug by validating
params.workspaceSlug at runtime: in useDeploymentBreadcrumbConfig, check if
params.workspaceSlug is a string (or if it's an array, pick the first element or
treat as error) and handle undefined accordingly (throw a clear error, return a
fallback, or navigate away). Use typeof params.workspaceSlug === "string" and
Array.isArray(params.workspaceSlug) to branch and avoid `as string`; ensure
subsequent code uses the validated local variable (workspaceSlug) only after
these checks.
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/details/env-variables-section/index.tsx:
- Around line 67-80: The icon-only toggle Button (the Button using
onClick={toggleExpanded} with the ChevronDown icon) lacks an accessible name;
add an aria-label describing the control (e.g., "Toggle environment variables"
or similar) to the Button and set aria-expanded={isExpanded} so screen readers
know the state; also mark the decorative icon (ChevronDown) as aria-hidden to
avoid duplicate announcements.
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/page.tsx:
- Around line 14-23: The default for environmentId uses environments.length at
init so it stays "" when environments load async; instead keep the useQueryState
default as an empty string and add a useEffect that watches environments and
environmentId, and when environments becomes non-empty and environmentId is
falsy call setEnvironmentId(environments[0].id) (use the same history/shallow
semantics if needed) to auto-select the first environment; refer to
useProjectData, environments, environmentId, setEnvironmentId, and useQueryState
in your change.
In `@web/apps/dashboard/components/ui/switch.tsx`:
- Around line 22-28: Remove the stray JSX text node following the
SwitchPrimitives.Thumb element (the {" "} after the component) in the switch
component; locate the SwitchPrimitives.Thumb in ui/switch.tsx (and any similar
occurrences) and delete the extraneous {" "} so no empty text node is rendered
and SSR/layout diffs are avoided.
🧹 Nitpick comments (8)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/details/custom-domains-section/custom-domain-row.tsx (1)
84-88: Empty catch blocks silently swallow errors.The empty
catch {}blocks at lines 87 and 108 discard mutation errors without any handling. Whiletoast.promiseshows error feedback, this pattern could mask debugging information and prevent error boundaries from catching issues.Consider logging the error or removing the try-catch if toast.promise already handles the error display adequately.
🔧 Proposed fix
try { await mutation; onDelete(); - } catch {} + } catch (err) { + // Error already shown via toast.promise + console.error("Failed to delete domain:", err); + }try { await mutation; onRetry(); - } catch {} + } catch (err) { + // Error already shown via toast.promise + console.error("Failed to retry verification:", err); + }Also applies to: 105-109
web/apps/dashboard/lib/collections/deploy/utils.ts (1)
8-9:anytype usage acknowledged with lint suppression.The
anytypes are used because TanStack DB doesn't expose internal types for thewhereexpression structure. The biome-ignore comments document this reasoning appropriately.For improved type safety in the future, consider creating a local type definition that mirrors the expected structure, even if not perfectly accurate:
type WhereExpr = { name?: string; type?: string; args?: Array<{ path?: string[]; type?: string; value?: unknown }>; };This would provide some IntelliSense benefits while acknowledging the type isn't officially exported. As per coding guidelines: "Never compromise type safety: No
any" — though this case has documented justification.Also applies to: 15-16, 63-64
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/table/utils/get-row-class.ts (1)
49-54: Theenvironmentparameter is unused.The function signature now accepts
{ deployment, environment }butenvironmentis never referenced in the function body. If this is preparatory for future functionality, consider adding a TODO comment. Otherwise, consider keeping the simpler signature until the parameter is needed.♻️ Option: Remove unused parameter
export const getRowClassName = ( - { deployment }: { deployment: Deployment; environment: Environment }, + deployment: Deployment, selectedDeploymentId: string | null, liveDeploymentId: string | null, isRolledBack: boolean, ) => {Note: This would require updating all call sites to pass
deploymentdirectly instead of the object.web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/data-provider.tsx (1)
42-44: Consider using thefaultlibrary for error handling.The coding guidelines specify using the
faultlibrary for error handling. Currently, a plainErroris thrown whenprojectIdis missing.As per coding guidelines: "Use
faultlibrary for error handling."web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (1)
92-98: Consider memoizing thedatacomputation.
datais computed on every render (Line 25-27) and included in theuseMemodependency array. SincegetDomainsForDeploymentlikely returns a new array reference each time, this could causemenuItemsto recompute unnecessarily.♻️ Suggested improvement
+ import { useMemo } from "react"; // ... const { getDomainsForDeployment } = useProjectData(); - const data = getDomainsForDeployment(selectedDeployment.id).map((domain) => ({ - host: domain.fullyQualifiedDomainName, - })); + const data = useMemo( + () => + getDomainsForDeployment(selectedDeployment.id).map((domain) => ({ + host: domain.fullyQualifiedDomainName, + })), + [getDomainsForDeployment, selectedDeployment.id], + );web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(overview)/components/sections/deployment-logs-section.tsx (1)
21-27: Confusing variable naming:isReadymeans the opposite.The variable
isReadyistruewhendeploymentStatus !== "ready", which is semantically inverted. This makes the code harder to understand at a glance.♻️ Suggested naming improvement
- const isReady = deploymentStatus !== "ready"; + const isBuilding = deploymentStatus !== "ready"; - const [tab, setTab] = useState(isReady ? "build-logs" : "requests"); + const [tab, setTab] = useState(isBuilding ? "build-logs" : "requests"); useEffect(() => { - setTab(isReady ? "build-logs" : "requests"); - }, [isReady]); + setTab(isBuilding ? "build-logs" : "requests"); + }, [isBuilding]);And on Line 35:
- <TabsTrigger value="requests" className="text-accent-12 text-[13px]" disabled={isReady}> + <TabsTrigger value="requests" className="text-accent-12 text-[13px]" disabled={isBuilding}>web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/hooks/use-deployments.ts (1)
35-44: Type assertionas Record<...>violates coding guidelines.The code uses
as Record<DeploymentListFilterField, (string | number)[]>which bypasses type safety. As per coding guidelines, avoidas Typeassertions.♻️ Type-safe alternative
const groupedFilters = filters.reduce( - (acc, f) => { + (acc: Partial<Record<DeploymentListFilterField, (string | number)[]>>, f) => { if (!acc[f.field]) { acc[f.field] = []; } - acc[f.field].push(f.value); + acc[f.field]!.push(f.value); return acc; }, - {} as Record<DeploymentListFilterField, (string | number)[]>, + {}, );Note: The
!onacc[f.field]!.pushis safe here because we just checked/initialized it, but if you want to avoid even that:const groupedFilters = filters.reduce<Partial<Record<DeploymentListFilterField, (string | number)[]>>>( (acc, f) => { const existing = acc[f.field] ?? []; existing.push(f.value); acc[f.field] = existing; return acc; }, {}, );web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/components/github-settings-client.tsx (1)
47-47: Minor: Extra whitespace in JSX.There's an extra space before
hasInstallations.♻️ Fix formatting
- <GitHubAppCard hasInstallations={true} /> + <GitHubAppCard hasInstallations={true} />Or even simpler:
- <GitHubAppCard hasInstallations={true} /> + <GitHubAppCard hasInstallations />
...rview)/deployments/[deploymentId]/(overview)/navigations/use-deployment-breadcrumb-config.ts
Show resolved
Hide resolved
...app)/[workspaceSlug]/projects/[projectId]/(overview)/details/env-variables-section/index.tsx
Show resolved
Hide resolved
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/page.tsx
Outdated
Show resolved
Hide resolved
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
Why do we now have another version of <Empty>? When we have one
https://engineering.unkey.com/design/components/empty
Why are we also creating a single component that we reuse dozens of time and placing it in a non shared location like components or somewhere. otherwise we end us with this.
import { DomainRow, DomainRowSkeleton, EmptySection } from "./(overview)/details/domain-row";
import { DomainRow, DomainRowSkeleton, EmptySection } from "../../../../../details/domain-row";
import { useProjectData } from "../../../../../data-provider";
please spend some time actually making this code reusable, reusing components we have, and if we can't place them in a place if you are going to reuse them dozens of time.
I don't want to waste time in the future, refactoring hundreds of components to use reusable components or figuring out which one is broken (Which happens so often)
wdym they are both pointing the same directory? Depending on the depth LSP picks the import path |
ogzhanolguncu
force-pushed
the
deploy-ui-issues
branch
from
bf08f10 to
425b04d
Compare
| import { useProjectData } from "../../../../data-provider"; | ||
| import { useDeployment } from "../../layout-provider"; | ||
|
|
||
| export function ScrollToBottomButton() { |
There was a problem hiding this comment.
@perkinsjr @chronark This is kinda experimental if we don't like it we can kill it. lemme know how you feel
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)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/network/deployment-network-view.tsx (1)
57-63:⚠️ Potential issue | 🟠 MajorRemove the non‑null assertion on deploymentId.
Line 62 uses
deploymentId!, which violates the type safety guideline. Gate the dev generator ondeploymentIdbeing defined instead.✅ Minimal fix
- {process.env.NODE_ENV === "development" && ( - <InternalDevTreeGenerator - // biome-ignore lint/style/noNonNullAssertion: will be fixed later, when we actually implement tRPC logic - deploymentId={deploymentId!} - onGenerate={setGeneratedTree} - onReset={() => setGeneratedTree(null)} - /> - )} + {process.env.NODE_ENV === "development" && deploymentId && ( + <InternalDevTreeGenerator + deploymentId={deploymentId} + onGenerate={setGeneratedTree} + onReset={() => setGeneratedTree(null)} + /> + )}As per coding guidelines: "Never compromise type safety: No
any, no!(non-null assertion), noas Type".
3 participants
What does this PR do?
So much stuff fixed here reported by the team. Major thing is way we consume tanstack db. We now pass frequently used queries to provider like a dependency injection then consume those. And scoping is so much better and easier, because we can easily fetch project specific resources instead of everything.
No more projectId, deploymentId prop drilling they are coming from URL directly but its behind hook to safe guard it.
How should this be tested?
make devgo run . dev seed localmake build-local-image DOCKERFILE=./demo_api/Dockerfile NAME=demo_api TAG=local CONTEXT=./demo_apigo run . deploy --project-id=proj_1RblymOt --root-key="unkey_VGuLaFSquizjTFhzpjqF3B" --env=production --api-base-url="http://localhost:7070" ctlptl-registry:5000/demo_api:local. Grab your rootkey and projectId from step 2localhost:3000and check deploy pages.