feat: improve namespace selector UI/UX#2161
Conversation
…-uxui-confusion # Conflicts: # cli/src/utils.ts # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go # shared/test/router.config.test.ts # shared/test/testdata/utils.ts
…-is-causing-uxui-confusion
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds a GetWorkspace RPC and workspace proto messages, regenerates connect code, implements server handler assembling namespaces→graphs→subgraphs, updates repositories to support multi-namespace filters, and introduces a WorkspaceContext/provider, hook, and UI components; Studio is migrated from router/user namespace resolution to workspace-driven context. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…-uxui-confusion # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go # connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts # connect/src/wg/cosmo/platform/v1/platform_connect.ts # controlplane/src/core/bufservices/PlatformService.ts # studio/src/pages/[organizationSlug]/[namespace]/subgraph/[subgraphSlug]/index.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/cache-operations.tsx (1)
173-176: Fix grammar in user-facing copy“operation have” → “operations have”; and consider “at planning time”.
- operation have priority over the top 100 operations computed by - planning time.{" "} + operations have priority over the top 100 operations computed at + planning time.{" "}studio/src/components/checks/changes-table.tsx (1)
291-307: Don’t create Links with missing dynamic paramsIf organizationSlug/namespace are undefined, Next.js can complain. Disable the button or fall back to router.query until ready.
- <Button - disabled={!path} + <Button + disabled={!path || !organizationSlug || !namespace} variant="ghost" size="icon-sm" asChild className="table-action" > <Link href={ path ? { pathname: `/[organizationSlug]/[namespace]/graph/[slug]/schema`, query: { organizationSlug, namespace, slug: router.query.slug, typename: path?.split(".")?.[0], }, } : "#" } > <GlobeIcon /> </Link>studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/feature-flag/[featureFlagCompositionId]/index.tsx (1)
88-90: Fix typo in tab query key
Update “ffCompostions” to “ffCompositions” in all instances to prevent broken deep links:
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/index.tsx (lines 522, 528, 711)
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/feature-flag/[featureFlagCompositionId]/index.tsx (line 88)
studio/src/components/lint-policy/linter-config.tsx (1)
195-199: Fix typo in success toast“succesfully” → “successfully”. User-facing string.
- description: "Lint Policy applied succesfully.", + description: "Lint Policy applied successfully.",studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/index.tsx (1)
104-110: Fix “Configure Proposals” link to use workspace namespaceThis still uses
router.query.namespace, which can diverge from the workspace selection.- router.push( - `/${user?.currentOrganization.slug}/policies?namespace=${router.query.namespace}#proposals`, - ); + router.push( + `/${user?.currentOrganization.slug}/policies?namespace=${namespace}#proposals`, + );studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (1)
1503-1544: Force Success button renders only when already forced + missing namespace in one callCurrently the “Force Success” action appears when
isForcedSuccessis true and is hidden otherwise, and one invocation omitsnamespace. Invert the condition and includenamespaceto avoid a no-op/broken mutation.- {data.check.isForcedSuccess ? ( + {!data.check.isForcedSuccess ? ( <div className="mt-2 flex space-x-2"> <ForceSuccess onSubmit={() => forceSuccess({ checkId: id, graphName: slug, - namespace, + namespace, }) } /> </div> ) : null} @@ - {data.check.isForcedSuccess ? ( + {!data.check.isForcedSuccess ? ( <div className="mt-2 flex space-x-2"> <ForceSuccess onSubmit={() => forceSuccess({ checkId: id, graphName: slug, + namespace, }) } /> </div> ) : null}Also applies to: 1558-1568
studio/src/components/analytics/field-usage.tsx (1)
264-267: Handle missing end date in dateRange JSON
dateRange.endcan be undefined; mirror the earlier fallback used in onDateRangeChange.- end: formatISO(dateRange.end), + end: formatISO(dateRange.end ?? dateRange.start),studio/src/pages/[organizationSlug]/feature-flags/[featureFlagSlug]/index.tsx (1)
61-70: Breadcrumb may render with an invalid org segment
organizationSlugcan be undefined on first render; the breadcrumb href becomes"/undefined/...". Prefer a router fallback.-const FeatureFlagBreadcrumb = () => { - const organizationSlug = useCurrentOrganization()?.slug; - const { namespace: { name: namespace } } = useWorkspace(); +const FeatureFlagBreadcrumb = () => { + const router = useRouter(); + const organizationSlug = + useCurrentOrganization()?.slug ?? + (router.query.organizationSlug as string | undefined); + const { namespace: { name: namespace } } = useWorkspace();studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/index.tsx (1)
730-738: Consistent org slug usage in page-level queries and linksMirror the same router fallback here to keep the page consistent.
- const organizationSlug = useCurrentOrganization()?.slug; + const organizationSlug = + useCurrentOrganization()?.slug ?? + (router.query.organizationSlug as string | undefined);studio/src/pages/[organizationSlug]/cache-warmer.tsx (2)
63-63: Fix user-facing typo in title“namesapce” → “namespace”.
- title="Could not retrieve the cache warmer config of the namesapce" + title="Could not retrieve the cache warmer config of the namespace"
99-103: Fix copy typos and consistency
- “opeartions” → “operations”
- “cacheWarmer” → “cache warmer”
- ? "Enable cache warmer to warm the router with your top opeartions." - : "Upgrade your billing plan to use this cacheWarmer."}{" "} + ? "Enable cache warmer to warm the router with your top operations." + : "Upgrade your billing plan to use the cache warmer."}{" "}studio/src/components/checks/lint-issues-table.tsx (1)
124-136: Harden the “View” link (guard and encode subgraph)
- Only render when
organizationSlugis present.- URL-encode
subgraphNameto avoid broken links.- <Link - href={`/${organizationSlug}/${ - namespace - }/graph/${router.query.slug}/checks/${ - router.query.checkId - }?tab=schema&${ - l.subgraphName - ? `subgraph=${l.subgraphName}` - : "" - }${ - l.issueLocation?.line - ? `#L${l.issueLocation?.line}` - : "" - }`} - > - View - </Link> + {organizationSlug && ( + <Link + href={`/${organizationSlug}/${namespace}/graph/${router.query.slug}/checks/${router.query.checkId}?tab=schema&${ + l.subgraphName ? `subgraph=${encodeURIComponent(l.subgraphName)}` : "" + }${l.issueLocation?.line ? `#L${l.issueLocation.line}` : ""}`} + > + View + </Link> + )}studio/src/components/analytics/metrics.tsx (1)
268-287: Gate the header link until route params are availablePrevents broken navigation and preserves UX.
- <Link - href={{ - pathname: `${router.pathname}/traces`, - query: { - organizationSlug, - namespace, - slug: router.query.slug, - filterState: router.query.filterState || "[]", - range, - dateRange, - ...queryParams, - }, - }} - className="inline-flex rounded-md px-2 py-1 hover:bg-muted" - > - {title} - <ChevronRightIcon className="h4 ml-1 w-4 transition-all group-hover:ml-2" /> - </Link> + {organizationSlug && namespace ? ( + <Link + href={{ + pathname: `${router.pathname}/traces`, + query: { + organizationSlug, + namespace, + slug: router.query.slug, + filterState: router.query.filterState || "[]", + range, + dateRange, + ...queryParams, + }, + }} + className="inline-flex rounded-md px-2 py-1 hover:bg-muted" + > + {title} + <ChevronRightIcon className="h4 ml-1 w-4 transition-all group-hover:ml-2" /> + </Link> + ) : ( + <span className="inline-flex rounded-md px-2 py-1 text-muted-foreground"> + {title} + </span> + )}studio/src/components/layout/graph-layout.tsx (2)
77-90: Avoid providing undefined GraphContext while children render.graphContextData is undefined until graphsData arrives, but children are rendered once the single graph query succeeds. This risks consumers reading an undefined context.
Consider gating render on both results:
const { data: graphsData, isLoading: graphsLoading } = useQuery(getFederatedGraphs); ... if (isLoading || graphsLoading || !graphsData) { render = <Loader fullscreen />; } else if (error || data?.response?.code !== EnumStatusCode.OK) { ... } else { render = <GraphContext.Provider value={graphContextData}>{children}</GraphContext.Provider>; }
92-181: Return empty nav until routing/workspace are ready to prevent invalid hrefs.When org/namespace/slug aren’t ready, SideNav should temporarily render no links.
Apply this diff:
const links: NavLink[] = useMemo(() => { + if (!organizationSlug || !namespace || !slug) { + return []; + } const basePath = `/${organizationSlug}/${namespace}/graph/${slug}`;studio/src/components/layout/subgraph-layout.tsx (2)
62-89: Return empty nav until routing/workspace are ready.Prevents SideNav hrefs like "/undefined/...".
Apply this diff:
const links: NavLink[] = useMemo(() => { - const basePath = `/${organizationSlug}/${namespace}/subgraph/${slug}`; + if (!organizationSlug || !namespace || !slug) { + return []; + } + const basePath = `/${organizationSlug}/${namespace}/subgraph/${slug}`;
117-117: Typo in Tailwind class: duplicated lg: prefix breaks grid on large screens.Fixes layout on lg+.
Apply this diff:
- <div className="flex min-h-screen w-full flex-1 flex-col bg-background font-sans antialiased lg:grid lg:lg:grid-cols-[auto_minmax(10px,1fr)] lg:divide-x"> + <div className="flex min-h-screen w-full flex-1 flex-col bg-background font-sans antialiased lg:grid lg:grid-cols-[auto_minmax(10px,1fr)] lg:divide-x">
🧹 Nitpick comments (62)
studio/src/lib/utils.ts (1)
15-26: Constrain key type and accept readonly arrays to avoid object-identity footguns.Using Set with non-primitive keys compares by reference; constraining to PropertyKey prevents surprises and makes intent clear. Also allow ReadonlyArray input.
-export function distinctBy<T, TKey>(source: T[], keySelector: (item: T) => TKey) { - const keys = new Set<TKey>(); - return source.filter((item) => { - const key = keySelector(item); - if (keys.has(key)) { - return false; - } - - keys.add(key); - return true; - }); -} +export function distinctBy<T, K extends PropertyKey>( + source: readonly T[], + keySelector: (item: T) => K +): T[] { + const seen = new Set<K>(); + return source.filter((item) => { + const key = keySelector(item); + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +}studio/src/components/ui/skeleton.tsx (1)
1-13: forwardRef + aria-hidden for better ergonomics and a11y.This enables refs for composition (e.g., animations) and hides the decorative skeleton from AT.
-import { cn } from "@/lib/utils" +import * as React from "react" +import { cn } from "@/lib/utils" -function Skeleton({ className, ...props }: React.ComponentProps<"div">) { - return ( - <div - data-slot="skeleton" - className={cn("bg-accent animate-pulse rounded-md", className)} - {...props} - /> - ) -} +const Skeleton = React.forwardRef<HTMLDivElement, React.ComponentProps<"div">>( + ({ className, ...props }, ref) => ( + <div + ref={ref} + data-slot="skeleton" + aria-hidden="true" + className={cn("bg-accent animate-pulse rounded-md", className)} + {...props} + /> + ) +) +Skeleton.displayName = "Skeleton" export { Skeleton }studio/src/components/changelog/changelog.tsx (1)
38-38: Encode route params and avoid rendering a bad href until data is ready.Prevent “/undefined/…” links and ensure safe URL construction.
- href={`/${organizationSlug}/${namespace}/graph/${slug}/compositions/${compositionId}`} + href={`/${encodeURIComponent(organizationSlug!)}/${encodeURIComponent(namespaceName!)}/graph/${encodeURIComponent(slug!)}/compositions/${encodeURIComponent(String(compositionId))}`}Optional (outside this line): gate the link until values exist to avoid relying on non-null assertions.
const isReady = !!(organizationSlug && namespaceName && slug && compositionId); // ... <Link href={ isReady ? `/${encodeURIComponent(organizationSlug)}/${encodeURIComponent(namespaceName)}/graph/${encodeURIComponent(slug)}/compositions/${encodeURIComponent(String(compositionId))}` : "#" } aria-disabled={!isReady} // prefetch={false} // consider if many rows to reduce network churn className="flex items-center gap-x-1 text-sm text-primary hover:underline" >controlplane/src/types/index.ts (1)
41-41: Clarify/guard precedence between namespaceId vs. namespaceIds.Today namespaceId silently overrides namespaceIds (see FederatedGraphRepository.list). Consider documenting this in the type or making them mutually exclusive at the type level to avoid ambiguous callers.
Example doc tweak:
export interface ListFilterOptions { namespaceId?: string; + /** If provided, used only when `namespaceId` is not set. */ namespaceIds?: string[]; limit: number; offset: number; query?: string; }controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
509-513: Multi-namespace filter LGTM; add small guard/tests.Behavior is clear: namespaceId takes precedence; else uses inArray(namespaceIds). Consider guarding against whitespace-only IDs and add a test for both fields present (namespaceId wins).
studio/src/components/dashboard/namespace-selector.tsx (5)
27-31: Pathname heuristic is brittle; prefer intent-based check.Length-based split can misclassify routes. Detect “[namespace]” explicitly and fall back otherwise.
- const pathname = useMemo( - () => router.pathname.split('/').length === 3 ? router.pathname : '/[organizationSlug]/graphs', - [router.pathname] - ); + const pathname = useMemo(() => { + return router.pathname.includes("[namespace]") + ? router.pathname + : "/[organizationSlug]/graphs"; + }, [router.pathname]);
55-61: Comment/code mismatch when resetting filter.Code resets on close, but the comment says “when opened”. Either change the comment or reset on open (recommended).
- onOpenChange={(v) => { - setOpen(v); - if (!v) { - // Only reset the filter when the popover is opened - setFilter(''); - } - }} + onOpenChange={(v) => { + setOpen(v); + if (v) { + // Reset filter on open so users start fresh + setFilter(''); + } + }}
32-33: Stable, readable list ordering.Sort namespace names for consistent UX.
- const namespaces = Array.from(namespaceByName.keys()); + const namespaces = useMemo( + () => Array.from(namespaceByName.keys()).sort((a, b) => a.localeCompare(b)), + [namespaceByName] + );
94-101: Harden external link.Add rel="noreferrer noopener" to the external docs link.
- <Link + <Link target="_blank" - className="text-primary" + rel="noreferrer noopener" + className="text-primary" href={`${docsBaseURL}/cli/essentials#namespaces`} >
114-121: Set workspace first to avoid UI flicker.Update workspace state before navigation so dependent components render immediately.
- onSelect={() => { - router.push({ + onSelect={() => { + setNamespace(ns, false); + router.push({ pathname, query: { organizationSlug, namespace: ns }, }); - - setOpen(false); - setNamespace(ns, false); + setOpen(false); }}>studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/index.tsx (2)
55-55: Avoid shadowing “namespace” and reduce brittle destructuringUsing a nested destructure + alias can break if the shape changes and shadows a common identifier. Prefer accessing namespace.name explicitly.
-const { namespace: { name: namespace } } = useWorkspace(); +const { namespace } = useWorkspace();
69-70: Pass the resolved name explicitlyMatch the tweak above by sending namespace.name to the query input.
- namespace, + namespace: namespace.name,studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsx (1)
1411-1412: Prefer explicit accessors and optional gatingMinor readability: avoid aliasing to “namespace” string; also consider exposing isLoading to gate initial fetches if needed.
-const { namespace: { name: namespace } } = useWorkspace(); +const { isLoading, namespace } = useWorkspace();Follow-up: use namespace.name where referenced in this file (e.g., request payload). If you see premature queries during workspace load, set enabled: !isLoading on the useQuery.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx (2)
75-75: Slightly simplify namespace accessAvoid aliasing to a common identifier and prefer direct property access.
-const { namespace: { name: namespace } } = useWorkspace(); +const { namespace } = useWorkspace();
92-93: Propagate the explicit name in requestsKeep the request self-documenting by sending namespace.name.
- namespace, + namespace: namespace.name,studio/src/hooks/use-workspace.ts (1)
4-11: Solid, typed hook with safety guardNice typed wrapper with an explicit error if the provider is missing. Consider adding a short JSDoc for discoverability.
export function useWorkspace(): WorkspaceContextType { + /** Access the current WorkspaceContext. Throws if used outside <WorkspaceProvider>. */ const context = useContext(WorkspaceContext);controlplane/src/core/bufservices/PlatformService.ts (1)
794-797: Expose GetWorkspace on PlatformServiceGood addition; mirrors other resolvers’ structure. Consider keeping method ordering consistent (grouping/alphabetical) to ease future diffs.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/changelog/index.tsx (1)
103-104: Pass explicit namespace nameAlign with the simpler access pattern.
- namespace, + namespace: namespace,If you adopt the earlier suggestion (
const { namespace } = useWorkspace()), then usenamespace.namehere:- namespace, + namespace: namespace.name,studio/src/components/schema/empty-schema-state.tsx (1)
11-11: Simplify namespace accessMinor readability nit.
-const { namespace: { name: namespace } } = useWorkspace(); +const { namespace } = useWorkspace();studio/src/components/feature-flags-table.tsx (2)
30-30: Simplify namespace accessPrefer accessing namespace.name directly elsewhere.
-const { namespace: { name: namespace } } = useWorkspace(); +const { namespace } = useWorkspace();
58-58: LGTM: CLI examples now interpolate workspace namespaceAll three commands correctly reference the active workspace. Consider centralizing CLI string builders to avoid drift across files.
I can extract a small helper (e.g., buildWgcCmd({ kind, namespace, ... })) and update call sites—want me to push a follow-up PR?
Also applies to: 64-64, 67-68
studio/src/components/cache/cache-warmer-config.tsx (1)
93-99: Capture/log errors for observabilityIgnoring the error param is fine for UX, but consider logging it (e.g., to Sentry) to aid debugging.
- onError: (_) => { + onError: (err) => { + console.error('configureCacheWarmer failed', err); toast({ description: "Could not set the cache warmer config. Please try again.", duration: 3000, }); },studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/cache-operations.tsx (1)
142-143: Avoid re-introducing namespace via queryWith workspace context in place, you likely don’t need to pass ?namespace=... in the navigation. Prefer relying on the provider to keep the current namespace.
- `/${user?.currentOrganization.slug}/cache-warmer?namespace=${namespace}`, + `/${user?.currentOrganization.slug}/cache-warmer`,If the destination still reads namespace from the URL, confirm and keep as-is.
studio/src/components/checks/changes-table.tsx (1)
155-156: Prefer consistent namespace sourceYou introduced namespace from useWorkspace here, but mutate/invalidate calls still use graphContext?.graph?.namespace. Standardize on the workspace namespace to avoid drift.
Example:namespace // instead of graphContext?.graph?.namespaceAlso ensure organizationSlug/namespace are present before building Links.
studio/src/components/lint-policy/linter-config.tsx (1)
146-154: Minor: remove unused error paramThe error variable isn’t used. Rename to _ or drop it to satisfy linters.
- onError: (error) => { + onError: () => { toast({ description: checkedSame for the onError handler in the “Apply” button section below.
studio/src/components/checks/override.tsx (1)
131-141: Guard against undefined organizationSlug in Explorer linkIf
useCurrentOrganization()hasn’t resolved,organizationSlugcan be undefined, producing broken hrefs.- organizationSlug, + organizationSlug: organizationSlug ?? (router.query.organizationSlug as string), - namespace, + namespace: namespace ?? (router.query.namespace as string),studio/src/components/analytics/toolbar.tsx (1)
31-35: Avoid building links with undefined org/namespaceOn initial render, hooks may return undefined. Prevent invalid dynamic route interpolation.
- const query: ParsedUrlQueryInput = { + if (!organizationSlug || !namespace) return null; + const query: ParsedUrlQueryInput = { organizationSlug, namespace, slug: router.query.slug, };studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (1)
266-277: Optional: hide Force Success by RBACConsider gating the Force Success control behind an access check (e.g., admin/developer), similar to other screens.
studio/src/components/analytics/field-usage.tsx (1)
251-256: Minor: reuse local slug variable for consistencyYou already captured
slugabove; use it here to avoid double-sourcing.- slug: router.query.slug, + slug,studio/src/pages/[organizationSlug]/policies.tsx (1)
28-29: Shorthand object property for readabilityUse
{ namespace }instead of{ namespace: namespace }.-} = useQuery(getNamespaceLintConfig, { - namespace: namespace, +} = useQuery(getNamespaceLintConfig, { + namespace, }); @@ -} = useQuery(getNamespaceGraphPruningConfig, { namespace: namespace }); +} = useQuery(getNamespaceGraphPruningConfig, { namespace }); @@ -} = useQuery(getNamespaceChecksConfig, { namespace: namespace }); +} = useQuery(getNamespaceChecksConfig, { namespace }); @@ - } = useQuery( - getNamespaceProposalConfig, - { - namespace: namespace, - }, + } = useQuery( + getNamespaceProposalConfig, + { + namespace, + },Also applies to: 36-37, 43-44, 53-54
studio/src/components/federatedgraphs-cards.tsx (2)
513-519: CLI robustness: quote namespace to tolerate special charactersSafer if namespaces ever include non-word chars.
- command={`npx wgc federated-graph create production --namespace ${namespace} --label-matcher ${labels} --routing-url http://localhost:3002/graphql`} + command={`npx wgc federated-graph create production --namespace "${namespace}" --label-matcher ${labels} --routing-url http://localhost:3002/graphql`} @@ - command={`npx wgc monograph create production --namespace ${namespace} --routing-url http://localhost:3002/graphql --graph-url http://localhost:4000/graphql`} + command={`npx wgc monograph create production --namespace "${namespace}" --routing-url http://localhost:3002/graphql --graph-url http://localhost:4000/graphql`}
150-158: UX: don’t redirect away on migration errorKeeping users on the dialog so they can fix input and retry is friendlier.
- onError: (_) => { + onError: (/* error */) => { toast({ description: "Could not migrate the graph. Please try again.", duration: 3000, }); setOpen(false); setIsMigrating(false); - router.replace(`/${currentOrg?.name}/graphs`); + // stay on page; no redirect on error },studio/src/components/overview/OverviewToolbar.tsx (1)
22-26: Minor: memoize query object to avoid needless re-rendersNot critical, but cheap to memoize when passed to multiple Links.
- const query = { - namespace, - organizationSlug, - slug: router.query.slug, - }; + const query = React.useMemo( + () => ({ namespace, organizationSlug, slug: router.query.slug }), + [namespace, organizationSlug, router.query.slug], + );studio/src/components/checks/graph-pruning-issues-table.tsx (3)
47-49: Unify org slug source and avoid non-null assertion elsewherePrefer a single, reliable org slug. Also prepare for absence to prevent malformed URLs.
Apply:
- const { namespace: { name: namespace } } = useWorkspace(); - const organizationSlug = useCurrentOrganization()?.slug; + const { namespace: { name: namespace } } = useWorkspace(); + const orgSlug = useCurrentOrganization()?.slug ?? user?.currentOrganization?.slug;
58-63: Guard navigation when org slug is missing; use unified slugAvoid pushing a path like "/undefined/...".
- router.push( - `/${user!.currentOrganization.slug}/policies?namespace=${ - graphContext?.graph?.namespace ?? namespace - }`, - ); + if (!orgSlug) return; + router.push( + `/${orgSlug}/policies?namespace=${graphContext?.graph?.namespace ?? namespace}`, + );
137-148: Harden URL building: encode params and normalize Next.js query types
- router.query.* can be string | string[]; normalize to first value.
- encode subgraphName; avoid stray “&” when absent.
- href={`/${organizationSlug}/${ - namespace - }/graph/${router.query.slug}/checks/${ - router.query.checkId - }?tab=schema&${ - l.subgraphName ? `subgraph=${l.subgraphName}` : "" - }${ - l.issueLocation?.line - ? `#L${l.issueLocation?.line}` - : "" - }`} + href={`/${orgSlug}/${namespace}/graph/${ + Array.isArray(router.query.slug) ? router.query.slug[0] : router.query.slug + }/checks/${ + Array.isArray(router.query.checkId) ? router.query.checkId[0] : router.query.checkId + }?tab=schema${ + l.subgraphName ? `&subgraph=${encodeURIComponent(l.subgraphName)}` : "" + }${ + l.issueLocation?.line ? `#L${l.issueLocation.line}` : "" + }`}studio/src/components/create-graph.tsx (3)
130-132: Invalidate (not refetch) to cover inactive queries; optionally prefetchrefetchQueries only refetches active queries by default. Invalidate is safer, and you can prefetch if you need data immediately.
- // We need to refresh the workspace after creating a graph - await queryClient.refetchQueries({ queryKey: createConnectQueryKey(getWorkspace) }); + // Refresh workspace cache after creating a graph + await queryClient.invalidateQueries({ queryKey: createConnectQueryKey(getWorkspace) }); + // Optional: prefetch if immediate freshness is required on next screen + // await queryClient.prefetchQuery({ queryKey: createConnectQueryKey(getWorkspace) });
50-54: Derive and validate org slug once; reuseAvoid “undefined” in paths if user context is momentarily unavailable.
const router = useRouter(); const user = useUser(); const { namespace: { name: namespace } } = useWorkspace(); + const orgSlug = user?.currentOrganization?.slug; const queryClient = useQueryClient();
132-134: Guard navigation if org slug is missingPrevents bad routes; also improves error UX.
- router.replace( - `/${user?.currentOrganization.slug}/${namespace}/graph/${data.name}`, - ); + if (!orgSlug) { + toast({ description: "Missing organization context.", duration: 3000 }); + return; + } + router.replace(`/${orgSlug}/${namespace}/graph/${data.name}`);studio/src/components/feature-flag-details.tsx (1)
84-85: Use the active workspace namespace in the enable command for consistencyOther CLI snippets are workspace-derived; align this one too.
- command={`npx wgc feature-flag enable <feature-flag-name> --namespace <namespace>`} + command={`npx wgc feature-flag enable <feature-flag-name> --namespace ${namespace}`}studio/src/pages/[organizationSlug]/cache-warmer.tsx (1)
170-170: Remove duplicate periodMinor text polish.
- "Configure cache warming to warm your router with top operations..", + "Configure cache warming to warm your router with top operations.",studio/src/components/analytics/metrics.tsx (1)
323-335: Avoid generating per-row links until params existSet
hreftoundefinedwhen required params are missing; BarList will render non-clickable rows.- href: isSubgraphAnalytics - ? undefined - : { + href: isSubgraphAnalytics || !organizationSlug || !namespace + ? undefined + : { pathname: `${router.pathname}/traces`, query: { organizationSlug, namespace, slug: router.query.slug, filterState: createFilterState({ operationName: row.name, operationHash: row.hash, }), range, dateRange, }, },studio/src/components/dashboard/graph-selector.tsx (1)
27-33: Fix misleading comment on filter reset timingCode resets
filteron close, but the comment says "when opened".- // Only reset the filter when the popover is opened + // Reset the filter when the popover is closedcontrolplane/src/core/bufservices/workspace/getWorkspace.ts (3)
50-58: Avoid “magic” limit=0; rely on undefined to mean “no limit”Repository checks
if (opts.limit), so0already behaves as “no limit”; passingundefinedclarifies intent.- offset: 0, // From the beginning - limit: 0, // Retrieve all federated graphs + offset: 0, // From the beginning + // limit omitted => retrieve all federated graphs
67-101: Potential N+1 on subgraphs retrievalOne DB call per federated graph can get expensive on large workspaces. Consider a repository method to fetch subgraphs for multiple federated graphs in one query and group in-memory.
104-107: Comment nit: you’re sorting graphs, not namespaces, hereThe namespaces were already sorted earlier; update the comment for accuracy.
- // Finally, sort the namespaces alphabetically + // Finally, sort each namespace's graphs alphabeticallyconnect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
141-156: Consider marking GetWorkspace as idempotent to enable safe GET and caching.
If the server guarantees no side effects, set NoSideEffects like analytics methods. This allows Connect-Web (with useHttpGet: true) to use HTTP GET and improves CDN/cache friendliness.Apply this diff:
export const getWorkspace = { localName: "getWorkspace", name: "GetWorkspace", kind: MethodKind.Unary, I: GetWorkspaceRequest, O: GetWorkspaceResponse, + idempotency: MethodIdempotency.NoSideEffects, service: { typeName: "wg.cosmo.platform.v1.PlatformService" } } as const;studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx (3)
171-176: Use current organization slug from context, not router.
Keeps slug source consistent with the rest of the page and resilient to route param drift.Apply this diff:
- const { namespace: { name: namespace } } = useWorkspace(); - const organizationSlug = router.query.organizationSlug as string; + const { namespace: { name: namespace } } = useWorkspace(); + const organizationSlug = useCurrentOrganization()?.slug ?? "";
359-365: Unify slug source in analytics link.
Use the org slug from context rather than router.query for consistency.Apply this diff:
<Link href={{ pathname: `/[organizationSlug]/[namespace]/graph/[slug]/analytics`, query: { - organizationSlug: router.query.organizationSlug, + organizationSlug, namespace, slug: router.query.slug, filterState: createFilterState({ operationPersistedId: op.id, }), }, }} >
607-616: URL-encode filterState to avoid malformed URLs.
filterState contains JSON; encode to ensure safe routing.Apply this diff:
- return `/${organizationSlug}/${namespace}/graph/${slug}/analytics?filterState=${JSON.stringify( - filters, - )}`; + return `/${organizationSlug}/${namespace}/graph/${slug}/analytics?filterState=${encodeURIComponent( + JSON.stringify(filters), + )}`; } else { - return `/${organizationSlug}/${namespace}/graph/${slug}/analytics/traces?filterState=${JSON.stringify( - filters, - )}`; + return `/${organizationSlug}/${namespace}/graph/${slug}/analytics/traces?filterState=${encodeURIComponent( + JSON.stringify(filters), + )}`; }studio/src/pages/[organizationSlug]/subgraphs.tsx (1)
41-46: Align namespace handling across queries.getSubgraphs uses the workspace namespace directly. getFeatureSubgraphs (Line 54) still falls back to "default". For consistency and fewer surprises, pass the workspace namespace there too.
Replace:
- namespace: namespace || "default"
With:- namespace
studio/src/components/dashboard/graph-command-group.tsx (3)
83-97: Harden route template computation and org slug availability.If organizationSlug is absent, router.push will construct an invalid URL. Consider a fallback to router.query.organizationSlug and add a quick guard.
- const organizationSlug = useCurrentOrganization()?.slug; + const organizationSlug = useCurrentOrganization()?.slug ?? (router.query.organizationSlug as string | undefined); + if (!organizationSlug) { + // Avoid pushing an invalid route; silently no-op or show a toast + }Confirm org slug is always available in contexts where this component renders.
99-106: Remove redundant React key on CommandItem.Key is only used by the immediate list renderer. Here, CommandItem isn’t directly in the map; the key belongs to the parent loops. This inner key is unnecessary.
- <CommandItem - key={`graph-${namespace.name}-${name}`} + <CommandItem
107-117: Decide whether namespace change should update URL params.If the upstream setNamespace applies route params by default, calling router.push here might double-update. Prefer a single source of truth: either rely on setNamespace(namespace, true) and let the provider update params, or keep router.push here and call setNamespace(namespace, false).
- setNamespace(namespace.name); + setNamespace(namespace.name, false);studio/src/components/dashboard/workspace-selector.tsx (1)
20-36: Slug matching is case-sensitive on one side; normalize and handle array type.router.query.slug can be string|string[]. Also, comparing lowercased name to raw slug can fail if slug isn’t lowercased.
- const currentSlug = router.query.slug as string; + const slugParam = router.query.slug; + const currentSlug = (Array.isArray(slugParam) ? slugParam[0] : slugParam)?.toLowerCase(); return [ - routeSegment === "graph" - ? namespace.graphs.find((graph) => graph.name.toLowerCase() === currentSlug) + routeSegment === "graph" + ? namespace.graphs.find((graph) => graph.name.toLowerCase() === currentSlug) : undefined,studio/src/components/dashboard/workspace-provider.tsx (1)
29-33: Handle namespace query param type and avoid defaulting namespaces to a placeholder array.router.query.namespace can be string|string[]; also, initializing namespaces with ["default"] can mask “no data yet” and affect validation.
- const namespaceParam = router.query.namespace as string; + const nsParam = router.query.namespace; + const namespaceParam = Array.isArray(nsParam) ? nsParam[0] : nsParam; - const [namespaces, setNamespaces] = useState([DEFAULT_NAMESPACE_NAME]); + const [namespaces, setNamespaces] = useState<string[]>([]);studio/src/components/dashboard/workspace-command-wrapper.tsx (3)
35-94: Tighten Fuse.js typing and reuse instances for graphs/subgraphs.Using Fuse hides type issues; also reusing specialized instances is clearer and marginally cheaper.
Apply this diff:
- const filteredGraphs = useMemo<WorkspaceNamespace[]>(() => { + const filteredGraphs = useMemo<WorkspaceNamespace[]>(() => { const filterValue = filter?.trim().toLowerCase() ?? ''; if (filterValue.length === 0) { // There is no filter, so return all namespaces return Array.from(namespaceByName.values()); } - const fuse = new Fuse<unknown>([], { keys: ['name'], threshold: 0.3, }); + const graphFuse = new Fuse<WorkspaceFederatedGraph>([], { keys: ['name'], threshold: 0.3, ignoreLocation: true }); + const subgraphFuse = new Fuse<WorkspaceSubgraph>([], { keys: ['name'], threshold: 0.3, ignoreLocation: true }); const searchResults: WorkspaceNamespace[] = []; for (const wns of Array.from(namespaceByName.values())) { @@ - for (const graph of wns.graphs) { - fuse.setCollection([graph]); - if (fuse.search(filterValue).length > 0) { + for (const graph of wns.graphs) { + graphFuse.setCollection([graph]); + if (graphFuse.search(filterValue).length > 0) { // The graph contains the filter value, we need to add it to the search results clonedWns.graphs.push(graph); continue; } @@ - fuse.setCollection(graph.subgraphs); - const matchingSubgraphs = fuse.search(filterValue); + subgraphFuse.setCollection(graph.subgraphs); + const matchingSubgraphs = subgraphFuse.search(filterValue);If desired, also sort namespaces/graphs alphabetically when not filtering to ensure stable ordering.
118-132: Use stable keys and drop the extra fragment.Index-based keys can cause stale renders; prefer id or name. The surrounding fragment is unnecessary.
Apply this diff:
- ) : filteredGraphs.map((wns, index) => ( - <> - <GraphCommandGroup - key={`namespace-${index}`} + ) : filteredGraphs.map((wns) => ( + <GraphCommandGroup + key={`namespace-${wns.id || wns.name}`} isFiltering={isFiltering} namespace={wns} - namespaceIndex={index} + namespaceIndex={0} activeGraphId={activeGraph?.id} activeSubgraphId={activeSubgraph?.id} setNamespace={(ns) => { setNamespace(ns, false); close(); }} /> - </> ))}
105-110: Minor a11y improvement: label the search input.Add aria-label for screen readers.
Apply this diff:
- {showFilter && (<CommandInput + {showFilter && (<CommandInput value={filter} onValueChange={setFilter} - placeholder="Search graphs and subgraphs" + placeholder="Search graphs and subgraphs" + aria-label="Search graphs and subgraphs" />)}proto/wg/cosmo/platform/v1/platform.proto (3)
2573-2599: Align field naming with file/style conventions (snake_case), especially target_idMost messages in this file use snake_case (e.g.,
target_idin FederatedGraph/Subgraph). New workspace messages introduce camelCase (targetId). For consistency and easier grepping across languages, prefer snake_case.message WorkspaceFederatedGraph { string id = 1; - string targetId = 2; + string target_id = 2; string name = 3; bool isContract = 4; repeated WorkspaceSubgraph subgraphs = 5; } message WorkspaceSubgraph { string id = 1; - string targetId = 2; + string target_id = 2; string name = 3; }Optional: add brief comments clarifying whether
nameis FQDN or short name to mirror earlier docs in this file.
2965-2967: Mark GetWorkspace as read-only and confirm auth scopingThis is a read path and should be safe to cache. Add idempotency for clarity, and ensure the server filters to the caller’s accessible namespaces.
- rpc GetWorkspace(GetWorkspaceRequest) returns (GetWorkspaceResponse) {} + rpc GetWorkspace(GetWorkspaceRequest) returns (GetWorkspaceResponse) { + option idempotency_level = NO_SIDE_EFFECTS; + }Please confirm the handler enforces org/namespace authorization derived from the authenticated user context.
352-357: Remove or integrate SubgraphMinimal
TheSubgraphMinimalmessage is defined but never referenced in any RPCs or other messages. Either remove it or replace existing ad-hoc triplets (e.g.GetSubgraphByNameResponse.LinkedSubgraph) with this type.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/components/federatedgraphs-cards.tsx (1)
309-309: Fix displayed Docker command: use--pull always, not-e pull=alwaysThe visible snippet shows an env var instead of the Docker flag, diverging from the copyable command and likely confusing users.
- <span>{` -e pull=always \\`}</span> + <span>{` --pull always \\`}</span>
♻️ Duplicate comments (1)
studio/src/components/federatedgraphs-cards.tsx (1)
148-149: Resolved: redirect now uses org slug (not name)This addresses the earlier 404 issue reported in past reviews.
Also applies to: 157-157
🧹 Nitpick comments (7)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
20353-20366: Empty GetWorkspaceRequest: OK, but note future-proofingEmpty request keeps RPC simple; if you foresee filters/pagination, prefer adding fields in proto now (server can ignore) and re-gen rather than editing generated TS later.
20384-20426: Payload growth risk: consider minimal vs full workspace viewsnamespaces[].graphs[].subgraphs[] can get large. If UI only needs names/ids for the selector, consider a lightweight RPC/fields (e.g., minimal=true) at the proto level to shrink payloads and speed up first paint. Change in proto; re-gen TS.
studio/src/components/federatedgraphs-cards.tsx (5)
91-93: Guard and URL-encode org slug before redirect
useCurrentOrganization()?.slugcan be undefined during initial render or on edge cases, and slugs may need encoding. Guard and encode to avoid/undefined/graphsor double slashes.Apply:
- const organizationSlug = useCurrentOrganization()?.slug; + const organizationSlug = useCurrentOrganization()?.slug; + const orgPath = organizationSlug ? encodeURIComponent(organizationSlug) : undefined; @@ - router.replace(`/${organizationSlug}/graphs`); + router.replace(orgPath ? `/${orgPath}/graphs` : "/"); @@ - router.replace(`/${organizationSlug}/graphs`); + router.replace(orgPath ? `/${orgPath}/graphs` : "/");If there is a preferred fallback route (e.g., a landing page), replace
/accordingly.Also applies to: 148-149, 157-157
150-158: Don’t redirect on migration error; keep dialog open so users can fix inputsClosing and redirecting on failure forces users to restart. Log the error and keep the dialog open.
- onError: (_) => { - toast({ - description: "Could not migrate the graph. Please try again.", - duration: 3000, - }); - setOpen(false); - setIsMigrating(false); - router.replace(`/${organizationSlug}/graphs`); - }, + onError: (err) => { + console.error("migrateFromApollo failed", err); + toast({ + description: "Could not migrate the graph. Please try again.", + duration: 3000, + }); + setIsMigrating(false); + // Keep dialog open so users can adjust inputs. + },
330-333: Quote dynamic values in the token commandNamespaces or graph names with special characters/spaces can break the command. Quote both.
- const createTokenCommand = `npx wgc router token create <name> ${namespace ? `-n ${namespace}` : ""} -g ${graphName}`; + const createTokenCommand = `npx wgc router token create <name> ${namespace ? `-n "${namespace}"` : ""} -g "${graphName}"`; @@ - {` ${namespace ? `-n ${namespace}` : ""} -g ${graphName}`} + {` ${namespace ? `-n "${namespace}"` : ""} -g "${graphName}"`}Also applies to: 390-395
513-514: Quote namespace in CLI create commandsPrevents shell parsing issues if the namespace contains special chars.
- command={`npx wgc federated-graph create production --namespace ${namespace} --label-matcher ${labels} --routing-url http://localhost:3002/graphql`} + command={`npx wgc federated-graph create production --namespace "${namespace}" --label-matcher ${labels} --routing-url http://localhost:3002/graphql`} @@ - command={`npx wgc monograph create production --namespace ${namespace} --routing-url http://localhost:3002/graphql --graph-url http://localhost:4000/graphql`} + command={`npx wgc monograph create production --namespace "${namespace}" --routing-url http://localhost:3002/graphql --graph-url http://localhost:4000/graphql`}Also applies to: 518-519
571-573: URL-encode path segments in Link to avoid broken routesEncode
namespaceandname(and slug if not guaranteed safe).- <Link - href={`/${user?.currentOrganization?.slug}/${graph.namespace}/graph/${graph.name}`} - className="project-list-item group" - > + <Link + href={`/${encodeURIComponent(user?.currentOrganization?.slug ?? "")}/${encodeURIComponent(graph.namespace)}/graph/${encodeURIComponent(graph.name)}`} + className="project-list-item group" + >Consider skipping render if
user?.currentOrganization?.slugis absent to avoid//routes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (4)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(3 hunks)studio/src/components/feature-flag-details.tsx(5 hunks)studio/src/components/federatedgraphs-cards.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- studio/src/components/feature-flag-details.tsx
- proto/wg/cosmo/platform/v1/platform.proto
🧰 Additional context used
🧬 Code graph analysis (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (21)
SubgraphMinimal(3712-3720)SubgraphMinimal(3735-3735)SubgraphMinimal(3750-3752)WorkspaceNamespace(24480-24488)WorkspaceNamespace(24503-24503)WorkspaceNamespace(24518-24520)WorkspaceFederatedGraph(24543-24553)WorkspaceFederatedGraph(24568-24568)WorkspaceFederatedGraph(24583-24585)WorkspaceSubgraph(24622-24630)WorkspaceSubgraph(24645-24645)WorkspaceSubgraph(24660-24662)GetWorkspaceRequest(24685-24689)GetWorkspaceRequest(24704-24704)GetWorkspaceRequest(24719-24721)GetWorkspaceResponse(24723-24730)GetWorkspaceResponse(24745-24745)GetWorkspaceResponse(24760-24762)Response(712-720)Response(735-735)Response(750-752)connect/src/wg/cosmo/node/v1/node_pb.ts (1)
Response(413-453)
studio/src/components/federatedgraphs-cards.tsx (2)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/hooks/use-current-organization.ts (1)
useCurrentOrganization(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (5)
2777-2824: SubgraphMinimal: field layout matches proto/go — looks goodFields id/name/namespace map 1:1 to the Go code, with correct numbers and scalar types. No issues spotted in the TS codegen.
20194-20302: WorkspaceNamespace/WorkspaceFederatedGraph: schema matches Go; good additionid/name/graphs and id/targetId/name/isContract/subgraphs align with Go code and intended workspace shape. No TS runtime issues.
2777-2824: SubgraphMinimal definitions are aligned across TS and Go
Fields id, name, and namespace are defined in the same order (1–3) in both languages.
20194-20302: No additional work on isContract/targetId needed
Verified that contract graphs are consistently flagged (viagraph.contract/isContract) in cards, status indicators, and command groups, and thattargetIdis used uniformly for routing and permissions throughout Studio.
20384-20408: Response type correctly bound: GetWorkspaceResponse.response uses the locally defined Response class (typeName = "wg.cosmo.platform.v1.Response") and nonode.v1.Responsereferences were found.studio/src/components/federatedgraphs-cards.tsx (3)
60-62: Hook migration to workspace/org context looks goodSwitching from router query params to
useWorkspaceanduseCurrentOrganizationaligns with the new source of truth.
473-474: Workspace hook usage in Empty looks goodConsistent with the new context-driven model.
128-129: Namespace “name” is correct – the service usesnamespaceRepo.byName(req.namespace)(migrateFromApollo.ts:47 ff.), so passinguseWorkspace().namespace.namematches the RPC contract.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
699-706: Potential SQL injection via raw string in LIKE onsubgraphs.id.
sql.raw(... '%${opts.query}%')interpolates unsanitized input. Prefer parameterized expressions withlike(sql\...`, value)`.Apply this diff in both
listandlistAvailable:- conditions.push( - or( - like(schema.targets.name, `%${opts.query}%`), - sql.raw(`${getTableName(schema.subgraphs)}.${schema.subgraphs.id.name}::text like '%${opts.query}%'`), - ), - ); + conditions.push( + or( + like(schema.targets.name, `%${opts.query}%`), + // Safe casting with parameterization + like(sql`${schema.subgraphs.id}::text`, `%${opts.query}%`), + ), + );Also applies to: 761-768
♻️ Duplicate comments (2)
proto/wg/cosmo/platform/v1/platform.proto (1)
316-322: Past concern appears resolved: no includeSubgraphs flag present anymorePrevious feedback about includeSubgraphs without response wiring looks addressed; nothing to change here.
Also applies to: 331-349, 351-355
controlplane/src/types/index.ts (1)
40-40: Switching tonamespaceIds?: string[]is the right call.This aligns with multi-namespace filtering and addresses prior feedback about supporting multiple namespaces.
Run to spot any lingering call sites still using
namespaceIdin filter objects:#!/bin/bash # Find object-literal uses of `namespaceId:` (may surface some false positives in SQL strings). rg -nP -C2 --type=ts '\bnamespaceId\s*:' # Find type/interface references to `namespaceId?` rg -nP -C2 --type=ts '\bnamespaceId\?\s*:'
🧹 Nitpick comments (5)
proto/wg/cosmo/platform/v1/platform.proto (4)
2566-2585: Prefer snake_case field names for new messages to align with proto style and most of this fileSuggest renaming targetId → target_id and isContract → is_contract for consistency.
message WorkspaceFederatedGraph { string id = 1; - string targetId = 2; + string target_id = 2; string name = 3; - bool isContract = 4; + bool is_contract = 4; repeated WorkspaceSubgraph subgraphs = 5; } message WorkspaceSubgraph { string id = 1; - string targetId = 2; + string target_id = 2; string name = 3; }
2566-2570: Name collision risk with existing Namespace messageWorkspaceNamespace mirrors Namespace (id, name) but adds graphs. Consider a name like WorkspaceViewNamespace or reuse Namespace where possible to reduce cognitive load.
2586-2591: Consider payload controls (large orgs)GetWorkspace may return many graphs/subgraphs. Consider optional include_subgraphs, per-namespace filters, or pagination in the request to cap response size.
2958-2960: Mark GetWorkspace as read-onlyAdd idempotency_level = NO_SIDE_EFFECTS for clearer semantics and gateway caching eligibility, similar to other read-only RPCs in this file.
- rpc GetWorkspace(GetWorkspaceRequest) returns (GetWorkspaceResponse) {} + rpc GetWorkspace(GetWorkspaceRequest) returns (GetWorkspaceResponse) { + option idempotency_level = NO_SIDE_EFFECTS; + }controlplane/src/core/repositories/SubgraphRepository.ts (1)
736-747: Avoid N+1 queries when building DTOs.Looping over targets and calling
byTargetIdper row scales poorly. Consider expanding the select/join to fetch fields needed forSubgraphDTOin one query (including namespace name, labels, and schemaVersion columns) and mapping directly.Also applies to: 806-815
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (9)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(1 hunks)controlplane/src/core/bufservices/federated-graph/getFederatedGraphs.ts(1 hunks)controlplane/src/core/bufservices/namespace/deleteNamespace.ts(1 hunks)controlplane/src/core/bufservices/subgraph/getSubgraphs.ts(1 hunks)controlplane/src/core/bufservices/workspace/getWorkspace.ts(1 hunks)controlplane/src/core/repositories/FederatedGraphRepository.ts(1 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(3 hunks)controlplane/src/types/index.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- controlplane/src/core/repositories/FederatedGraphRepository.ts
- controlplane/src/core/bufservices/workspace/getWorkspace.ts
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/namespace/deleteNamespace.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/subgraph/getSubgraphs.ts
🧬 Code graph analysis (1)
controlplane/src/core/bufservices/subgraph/getSubgraphs.ts (2)
controlplane/src/core/repositories/FederatedGraphRepository.ts (2)
list(503-552)count(555-570)controlplane/src/core/repositories/SubgraphRepository.ts (2)
list(689-749)count(819-854)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: CodeQL
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (4)
controlplane/src/core/bufservices/federated-graph/getFederatedGraphs.ts (1)
51-51: Good migration to multi-namespace filter.Passing
namespaceIds: namespaceId ? [namespaceId] : undefinedmatches the repo’s new contract and preserves the old semantics when no namespace is provided.controlplane/src/core/bufservices/namespace/deleteNamespace.ts (1)
83-96: LGTM: namespace deletion now usesnamespaceIdsconsistently.Using
limit: 0/offset: 0to fetch all works with current repo semantics; no functional regression.controlplane/src/core/repositories/SubgraphRepository.ts (1)
695-697: LGTM: multi-namespace filtering is correctly guarded and applied.
inArray(schema.targets.namespaceId, opts.namespaceIds)only when non-empty preserves “all namespaces” behavior.Also applies to: 757-759, 825-827
controlplane/src/core/bufservices/subgraph/getSubgraphs.ts (1)
42-42: LGTM: callers now passnamespaceIdsconsistently.Deriving
namespaceIdsfrom a singlenamespaceIdpreserves prior behavior without widening results unexpectedly.Also applies to: 46-46, 53-53
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
studio/src/components/dashboard/namespace-selector.tsx (4)
58-60: Fix misleading comment (logic resets on close, not on open).- // Only reset the filter when the popover is opened + // Reset the filter when the popover closes
32-32: Deterministic, readable ordering of namespaces.
Sort once with useMemo so the list is stable and easier to scan.- const namespaces = Array.from(namespaceByName.keys()); + const namespaces = useMemo( + () => Array.from(namespaceByName.keys()).sort((a, b) => a.localeCompare(b)), + [namespaceByName] + );
37-49: Add accessible labels to link/trigger and expose expanded state.
Improves a11y for screen readers.<Link href={{ pathname, query: { organizationSlug, namespace: namespace.name }, }} className={cn( "bg-primary/15 hover:bg-primary/30 text-primary transition-colors duration-150 pl-3 pr-2 py-1.5 rounded-l-lg text-sm flex-shrink-0", truncateNamespace && "max-w-[180px] lg:max-w-xs truncate" )} onClick={() => setNamespace(namespace.name, false)} + aria-label={`Current namespace: ${namespace.name}. Open graphs for this namespace.`} > @@ <button type="button" className={cn( "bg-primary/15 hover:bg-primary/30 text-primary transition-colors duration-150 text-sm flex-shrink-0 border-none outline-none", isViewingGraphOrSubgraph ? "rounded-r-lg pl-2 pr-3 py-2" : "flex justify-start items-center gap-4 rounded-lg px-3 py-1.5" )} + aria-haspopup="listbox" + aria-expanded={isOpen} + aria-label="Select namespace" >Also applies to: 64-71
98-103: External link hardening: add rel to target=_blank.
Prevents reverse tabnabbing.- <Link - target="_blank" - className="text-primary" - href={`${docsBaseURL}/cli/essentials#namespaces`} - > + <Link + target="_blank" + rel="noreferrer noopener" + className="text-primary" + href={`${docsBaseURL}/cli/essentials#namespaces`} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
studio/src/components/dashboard/graph-selector.tsx(1 hunks)studio/src/components/dashboard/namespace-selector.tsx(1 hunks)studio/src/components/dashboard/workspace-selector.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- studio/src/components/dashboard/workspace-selector.tsx
- studio/src/components/dashboard/graph-selector.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/components/dashboard/namespace-selector.tsx (5)
studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)studio/src/hooks/use-current-organization.ts (1)
useCurrentOrganization(3-6)studio/src/lib/utils.ts (1)
cn(6-8)studio/src/components/dashboard/workspace-command-wrapper.tsx (1)
WorkspaceCommandWrapper(25-139)studio/src/lib/constants.ts (1)
docsBaseURL(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
wilsonrivera
deleted the
wilson/eng-7026-default-namespace-is-causing-uxui-confusion
branch
4 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Checklist
namespace-selector.mov
On top of the new namespace selector, I've gone thru all files that retrieve the namespace and organization slug from the query parameters to construct a new URL and replaced them with using the current namespace and the current organization slug for the authenticated user