Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRepository-wide stylistic normalization (double → single quotes, JSX/type reflow) plus targeted API additions: Workspace context and provider exports, analytics URL/filter utilities, several components gained or expanded props (workspace, checks, cache, date pickers), and minor behavioral wiring (popover/filter/reset). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
studio/src/components/analytics/charts.tsx (1)
17-22:⚠️ Potential issue | 🟡 MinorAvoid truthiness checks for timestamps here.
A timestamp of
0will currently fall through the non-UTC branch and render as raw0instead of a formatted date. This shows up as a broken tooltip label for epoch-aligned data; check for a valid numeric value instead of relying on truthiness.Suggested fix
const labelFormatter = (label: number, utc?: boolean) => { - return utc - ? new Date(label).toUTCString() - : label - ? formatDateTime(label) - : label; + if (!Number.isFinite(label)) { + return label; + } + + return utc ? new Date(label).toUTCString() : formatDateTime(label); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/charts.tsx` around lines 17 - 22, The labelFormatter uses a truthiness check on label which treats 0 as falsy and returns raw 0; update the validation to explicitly check for a valid numeric timestamp (e.g. typeof label === "number" && Number.isFinite(label) or label !== null && label !== undefined) before calling formatDateTime, and keep the UTC branch using new Date(label). Modify the conditional in labelFormatter so it first tests numeric validity, then chooses utc ? new Date(label).toUTCString() : formatDateTime(label), otherwise return the original label.studio/src/lib/playground-url-state-encoding.ts (1)
22-25:⚠️ Potential issue | 🟠 MajorDon't serialize an empty
operation.This now emits
operation: ""for blank tabs, butPlaygroundStateSchemarequires a non-emptyoperationanddecompressStatevalidates against that schema. The result is a share URL that this codebase can't round-trip back into state.Suggested fix
- const stateToShare: PlaygroundUrlState = { - // Always include operation - operation: query ?? "", - }; + if (!query?.trim()) { + throw new Error("Cannot share an empty playground operation"); + } + + const stateToShare: PlaygroundUrlState = { + operation: query, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/lib/playground-url-state-encoding.ts` around lines 22 - 25, The code currently always sets operation: query ?? "" in the stateToShare object which serializes an empty string for blank tabs and breaks PlaygroundStateSchema validation; change the logic that builds stateToShare (the PlaygroundUrlState creation) to only include the operation property when query is non-empty (i.e., truthy), so omit operation entirely for blank tabs; ensure the surrounding code that calls decompressState/validates against PlaygroundStateSchema will receive no operation field instead of an empty string so round-tripping works.studio/src/components/analytics/trace-details.tsx (1)
84-132:⚠️ Potential issue | 🟠 MajorReset per-trace derived state before reading the new span.
This effect only updates
variableswhen the next trace exposesoperationVariables, andisTruncatednever flips back tofalse. Navigating from a trace with variables/truncated content to one without them will keep showing the previous payload and warning state.Suggested fix
useEffect(() => { if (!traceData || !operationContentData) { setVariables(""); setContent(""); + setTruncated(false); return; } @@ const content = operationContentData.operationContent; setContent(content); + setVariables(""); + setTruncated(false); const routerSpan = traceData.spans.find( (span) => !!span.attributes?.operationVariables, ); if (routerSpan) { - const variables = routerSpan.attributes?.operationVariables; + const variables = routerSpan.attributes?.operationVariables ?? ""; setVariables(variables);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/trace-details.tsx` around lines 84 - 132, The effect currently only clears state when traceData or operationContentData are missing, so navigating to a trace without operationVariables leaves the previous variables and isTruncated true; modify the useEffect (the effect that reads traceData and operationContentData, and the routerSpan lookup) to proactively reset derived per-trace state before reading the new span: call setVariables("") and setTruncated(false) (and keep setContent(content) when operationContentData is present) immediately after verifying traceData/operationContentData, and ensure that if routerSpan is not found you explicitly setVariables("") and setTruncated(false) so previous values are not preserved.studio/src/components/member-groups/group-resource-selector.tsx (1)
172-247:⚠️ Potential issue | 🟠 MajorUse real buttons for the tree row and checkbox controls.
Line 177 and Line 206 add
role="button"to adiv/span, but these controls still can't receive focus or be triggered from the keyboard. That makes expanding groups and toggling resources effectively mouse-only inside the popover. Please switch them to<button type="button">elements, or addtabIndex, keyboard handlers, and the appropriate ARIA state (aria-expanded/aria-checked).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/member-groups/group-resource-selector.tsx` around lines 172 - 247, The row and checkbox currently use non-focusable elements with role="button" — replace the outer div and inner span with real <button type="button"> elements (or make them focusable and keyboard-interactive) so keyboard users can activate them; preserve existing onClick logic (including early return when disabled) and use setExpanded/isExpanded for aria-expanded on the group button and selected/hasSelectedEveryChildren/hasSelectedSomeChildren for aria-checked on the checkbox button, keep disabled styling/behavior tied to the disabled prop, and ensure the checkbox button still calls toggleResources with the same arguments and retains e.preventDefault()/e.stopPropagation() behavior.
🧹 Nitpick comments (3)
studio/src/components/ui/calendar.tsx (1)
53-62: LGTM on the formatting changes.Semicolons properly added to the
displayNameassignment and export statement.Optional note (pre-existing, not introduced by this PR): The
...propsdestructuring inIconLeftandIconRighton lines 53-54 is unused. This could be cleaned up in a future refactor if desired:- IconLeft: ({ ...props }) => <ChevronLeftIcon className="h-4 w-4" />, - IconRight: ({ ...props }) => <ChevronRightIcon className="h-4 w-4" />, + IconLeft: () => <ChevronLeftIcon className="h-4 w-4" />, + IconRight: () => <ChevronRightIcon className="h-4 w-4" />,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/ui/calendar.tsx` around lines 53 - 62, The IconLeft and IconRight renderers include an unused destructured parameter ({ ...props }) which should be removed to clean up dead variables; update the JSX for IconLeft and IconRight to accept no props (e.g., replace "IconLeft: ({ ...props }) => <ChevronLeftIcon ... />" and the IconRight equivalent) so only ChevronLeftIcon and ChevronRightIcon are returned, leaving Calendar.displayName and the export unchanged.studio/src/components/checks/selected-checks-filters.tsx (1)
31-34: Consider removing the non-null assertion.The non-null assertion
!on line 32 contradicts the.filter(Boolean)on line 33. The assertion tells TypeScript that.find()always succeeds, but the filter suggests undefined values are expected. While.filter(Boolean)prevents runtime errors, the!is misleading for type safety.♻️ Suggested refactor
selectedOptions={selectedSubgraphs - .map((id) => subgraphs.find((sg) => sg.id === id)!) + .map((id) => subgraphs.find((sg) => sg.id === id)) .filter(Boolean) .map((sg) => JSON.stringify({ label: sg.name, value: sg.id }))}Note: This may require adding a type guard to satisfy TypeScript after the filter. Consider this refactor in a separate PR to keep formatting changes isolated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/selected-checks-filters.tsx` around lines 31 - 34, Remove the non-null assertion on the result of subgraphs.find in the selectedOptions computation and replace the runtime filter(Boolean) with a proper type guard so TypeScript knows undefined was filtered out; specifically, change the chain using selectedSubgraphs.map(id => subgraphs.find(sg => sg.id === id)!) to drop the "!" and use a filter with a type predicate (e.g., filter((sg): sg is Subgraph => Boolean(sg))) before the final .map that JSON.stringify's { label: sg.name, value: sg.id }, ensuring selectedOptions' items are correctly typed without the misleading non-null assertion.studio/sentry.client.config.ts (1)
57-72: Guard replay sample rates against invalid env values.These two fields still pass raw
parseFloat(...)results into Sentry. A malformed or out-of-range env value becomesNaNor>1, which makes replay sampling behavior hard to reason about. I'd validate/clamp once and reuse that helper here.♻️ Suggested hardening
+const parseSampleRate = (value: string | undefined) => { + const parsed = Number(value); + return Number.isFinite(parsed) && parsed >= 0 && parsed <= 1 ? parsed : 0; +}; + init({ @@ - replaysOnErrorSampleRate: isSentryFeatureReplayEnabled - ? parseFloat( - process.env.NEXT_PUBLIC_SENTRY_CLIENT_REPLAYS_ON_ERROR_SAMPLE_RATE || - "0", - ) - : 0, + replaysOnErrorSampleRate: isSentryFeatureReplayEnabled + ? parseSampleRate( + process.env.NEXT_PUBLIC_SENTRY_CLIENT_REPLAYS_ON_ERROR_SAMPLE_RATE, + ) + : 0, @@ - replaysSessionSampleRate: isSentryFeatureReplayEnabled - ? parseFloat( - process.env.NEXT_PUBLIC_SENTRY_CLIENT_REPLAYS_SESSION_SAMPLE_RATE || - "0", - ) - : 0, + replaysSessionSampleRate: isSentryFeatureReplayEnabled + ? parseSampleRate( + process.env.NEXT_PUBLIC_SENTRY_CLIENT_REPLAYS_SESSION_SAMPLE_RATE, + ) + : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/sentry.client.config.ts` around lines 57 - 72, The replay sample rates (replaysOnErrorSampleRate and replaysSessionSampleRate) currently use raw parseFloat(...) values which can be NaN or out of [0,1]; add a small helper (e.g., parseAndClampSampleRate or clampSampleRate) that accepts the env string and returns a number clamped to the 0–1 range (and falls back to 0 on invalid input), then replace the two parseFloat(...) usages in replaysOnErrorSampleRate and replaysSessionSampleRate to call that helper with the corresponding env var and default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/__tests__/playground-url-state-encoding-decoding.test.ts`:
- Around line 149-161: The test re-imports decompressState while the previous
test's vi.mock for "lz-string" is still active; after calling vi.resetModules()
and before the dynamic import of "../lib/playground-url-state-decoding", call
vi.unmock("lz-string") (or vi.resetAllMocks()/vi.restoreAllMocks() if you prefer
global restore) to remove the mock registration so
decompressFromEncodedURIComponent behaves normally; ensure this unmock call
appears right after vi.resetModules() and before importing decompressState and
using compressToEncodedURIComponent in the failing-schema test.
In `@studio/src/components/check-extensions/check-extensions-config.tsx`:
- Around line 57-63: Update the user-facing messages to match the validator's
localhost exception: when the URL validation in check-extensions-config.tsx (the
block that creates new URL(val) and calls ctx.addIssue) allows http://localhost,
change helper text and all error strings referenced near the other validation
blocks (around lines with similar checks and the messages at 70-80 and 309-313)
to explicitly allow http://localhost (or say "http://localhost or https://...")
instead of forcing "https://" only; ensure the same wording is used in the
ctx.addIssue message, any helperText/label for the endpoint field, and any other
related validation branches so the UI guidance matches the actual behavior.
In `@studio/src/components/dashboard/workspace-provider.tsx`:
- Around line 114-131: The namespace validation in setNamespaceCallback is
broken due to shadowing: the .some() callback's parameter named ns is compared
to itself, so the check always succeeds and the outer check never allows
updates; fix by renaming the iterator parameter (e.g., to candidate or
existingNs) and compare candidate.toLowerCase() === ns.toLowerCase() (where ns
is the callback argument), leaving the rest of the logic intact (setNamespace,
setStoredNamespace, applyParams) and keeping the same useCallback dependencies.
In `@studio/src/components/layout/dashboard-layout.tsx`:
- Around line 136-139: The current isBannerDisplayed boolean conflates multiple
banner states and causes incorrect layout when both the org banner and star
banner are shown; replace the boolean with a numeric bannerOffset computed from
the individual flags (use isOrganizationDeactivated,
isOrganizationPendingDeletion to add the org banner height and
isStarBannerDisabled to add the star banner height) and derive isBannerDisplayed
as bannerOffset > 0; then use bannerOffset in the container min-height
calculation instead of the old single-banner branch so the layout accounts for
one or two banners correctly.
In `@studio/src/components/member-groups/use-group-resources.ts`:
- Around line 128-145: The find() calls assume parents always exist and use
non-null assertions; update the lookups at the three sites
(accessibleResources.namespaces.find(...) used with mapNamespace/mapGraph,
accessibleResources.federatedGraphs.find(...) used with
mapFederatedGraph/mapGraph in getSubGraphs, and any
accessibleResources.graphs.find(...) usage) to guard against undefined: check
the result of each .find before using it, remove the "!" non-null assertions,
and skip/return early (or mark the resource as inaccessible) when the parent is
missing so mapNamespace, mapGraph, and mapSubGraph are only called with valid
parent objects and orphaned children are ignored instead of causing a crash.
In `@studio/src/components/ui/form.tsx`:
- Around line 25-27: The context defaults should be null so the provider
invariant can trigger: change the declarations like FormFieldContext (and the
other context(s) around lines 42-53 and 69-71) to use
createContext<FormFieldContextValue | null>(null), then update the consumers
that call useContext(FormFieldContext) to perform a null-check immediately
(before accessing fieldContext.name or itemContext.id) and throw a clear error
if null (e.g., "FormFieldContext provider missing — wrap in <FormField/>").
Ensure the guard runs before any dereference and apply the same pattern to the
related itemContext usage to prevent silent "undefined-..." IDs.
- Around line 148-153: The current body computation converts undefined messages
to the string "undefined" and causes rendering; update the logic in the
component using useFormField so that you only convert error?.message to a string
when error?.message is not null/undefined (e.g. check error?.message truthiness
or typeof before using String), otherwise fall back to children; ensure the
subsequent if (!body) guard still prevents rendering when there is no actual
message or children.
In `@studio/src/components/ui/tag-input/tag-input.tsx`:
- Around line 113-127: The code in tryAddTag is calling crypto.randomUUID()
directly (used also in trace-view, plan-view, and playground state hydration)
which can fail in older browsers; add a safe UUID fallback by importing or
implementing the same helper used in playground (e.g., the random-uuid helper)
or by swapping to a small UUID library, then replace direct crypto.randomUUID()
usages (e.g., in tryAddTag) with the safe helper (e.g., generateUUID()) so the
component calls the fallback when crypto.randomUUID is unavailable and keeps the
same return type ({id, text}).
In `@studio/src/components/ui/toast.tsx`:
- Around line 78-79: The destructive close-button’s focus offset color is
ineffective because there’s no offset width; update the class string on the
toast close-button (the classname containing
group-[.destructive]:focus:ring-offset-red-600) to also include a non-zero
offset width, e.g. add group-[.destructive]:focus:ring-offset-2 (or
focus:ring-offset-2) alongside the existing
group-[.destructive]:focus:ring-offset-red-600 so the ring offset color becomes
visible on focus.
In `@studio/src/hooks/use-pagination-params.ts`:
- Around line 6-14: The pagination hook currently casts router.query values to
string and uses Number.parseInt which yields NaN for malformed values or
string[] inputs; update the logic in use-pagination-params.ts around pageNumber
and pageSize to: safely coerce router.query.page and router.query.pageSize to
single strings (handle string[] by selecting the first element), call
Number.parseInt(value, 10), check isFinite/isNaN and fall back to default values
(1 for page, 20 for pageSize) before applying Math.max or clamp, and ensure
clamp receives a valid number for pageSize; adjust references to pageNumber and
pageSize so downstream offset calculations use the validated numbers.
In `@studio/src/hooks/use-session-storage.ts`:
- Around line 49-52: The non-browser guard in use-session-storage (inside the
setter that references key and window.sessionStorage / window.dispatchEvent)
logs a warning when typeof window === "undefined" but does not return, allowing
execution to fall through to the try/catch and cause a second warning; fix this
by adding an early return immediately after the console.warn inside that guard
so the function exits for SSR/tests and never reaches window.sessionStorage or
window.dispatchEvent.
In `@studio/src/hooks/use-share-playground-modal.ts`:
- Around line 147-157: The catch block in the URL generation flow (inside
useSharePlaygroundModal / the function that sets shareableUrl) only shows a
toast and leaves previous values intact, causing stale links to remain; update
the catch handler to clear/reset the share state by setting shareableUrl to
null/empty and warning to an empty string (and ensure any isGenerating flag is
reset if present) so the modal can't display or copy an old URL after a failure.
In `@studio/src/lib/playground-url-state-decoding.ts`:
- Around line 31-34: The error construction uses the wrong Zod property and
prints unreadable objects; replace reading result.error.errors with
result.error.issues and build a readable string by mapping each issue to include
issue.message and issue.path (e.g., join path with "." or JSON.stringify(path)
plus the message) when throwing the Error in the code that checks result.success
(the throw new Error(...) block that uses result); ensure you reference
ZodIssue.message and ZodIssue.path instead of toString().
In `@studio/src/pages/cosmo-managed-solution-terms.md`:
- Line 71: The markdown heading "## 10. Limitations of Liability.##" is
malformed; remove the trailing closing hashes and make it consistent with other
section headings by changing it to the same heading level (e.g., "### 10.
Limitations of Liability."); update the heading text used in the document (the
exact string "10. Limitations of Liability.") so it matches the format of
neighboring headings like "### 11. Term and Termination." and ensure there are
no extra hashes before or after the heading.
- Line 59: There's a spelling mistake in the BETA DISCLAIMER paragraph: change
the misspelled word "IDENTIFED" to "IDENTIFIED" in the paragraph that begins
with "BETA DISCLAIMER" (search for the exact phrase "BETA DISCLAIMER. FEATURES
OF THE SOFTWARE..." or the token "IDENTIFED") so the sentence reads "THAT ARE
IDENTIFIED BY WUNDERGRAPH AS “BETA”".
---
Outside diff comments:
In `@studio/src/components/analytics/charts.tsx`:
- Around line 17-22: The labelFormatter uses a truthiness check on label which
treats 0 as falsy and returns raw 0; update the validation to explicitly check
for a valid numeric timestamp (e.g. typeof label === "number" &&
Number.isFinite(label) or label !== null && label !== undefined) before calling
formatDateTime, and keep the UTC branch using new Date(label). Modify the
conditional in labelFormatter so it first tests numeric validity, then chooses
utc ? new Date(label).toUTCString() : formatDateTime(label), otherwise return
the original label.
In `@studio/src/components/analytics/trace-details.tsx`:
- Around line 84-132: The effect currently only clears state when traceData or
operationContentData are missing, so navigating to a trace without
operationVariables leaves the previous variables and isTruncated true; modify
the useEffect (the effect that reads traceData and operationContentData, and the
routerSpan lookup) to proactively reset derived per-trace state before reading
the new span: call setVariables("") and setTruncated(false) (and keep
setContent(content) when operationContentData is present) immediately after
verifying traceData/operationContentData, and ensure that if routerSpan is not
found you explicitly setVariables("") and setTruncated(false) so previous values
are not preserved.
In `@studio/src/components/member-groups/group-resource-selector.tsx`:
- Around line 172-247: The row and checkbox currently use non-focusable elements
with role="button" — replace the outer div and inner span with real <button
type="button"> elements (or make them focusable and keyboard-interactive) so
keyboard users can activate them; preserve existing onClick logic (including
early return when disabled) and use setExpanded/isExpanded for aria-expanded on
the group button and selected/hasSelectedEveryChildren/hasSelectedSomeChildren
for aria-checked on the checkbox button, keep disabled styling/behavior tied to
the disabled prop, and ensure the checkbox button still calls toggleResources
with the same arguments and retains e.preventDefault()/e.stopPropagation()
behavior.
In `@studio/src/lib/playground-url-state-encoding.ts`:
- Around line 22-25: The code currently always sets operation: query ?? "" in
the stateToShare object which serializes an empty string for blank tabs and
breaks PlaygroundStateSchema validation; change the logic that builds
stateToShare (the PlaygroundUrlState creation) to only include the operation
property when query is non-empty (i.e., truthy), so omit operation entirely for
blank tabs; ensure the surrounding code that calls decompressState/validates
against PlaygroundStateSchema will receive no operation field instead of an
empty string so round-tripping works.
---
Nitpick comments:
In `@studio/sentry.client.config.ts`:
- Around line 57-72: The replay sample rates (replaysOnErrorSampleRate and
replaysSessionSampleRate) currently use raw parseFloat(...) values which can be
NaN or out of [0,1]; add a small helper (e.g., parseAndClampSampleRate or
clampSampleRate) that accepts the env string and returns a number clamped to the
0–1 range (and falls back to 0 on invalid input), then replace the two
parseFloat(...) usages in replaysOnErrorSampleRate and replaysSessionSampleRate
to call that helper with the corresponding env var and default.
In `@studio/src/components/checks/selected-checks-filters.tsx`:
- Around line 31-34: Remove the non-null assertion on the result of
subgraphs.find in the selectedOptions computation and replace the runtime
filter(Boolean) with a proper type guard so TypeScript knows undefined was
filtered out; specifically, change the chain using selectedSubgraphs.map(id =>
subgraphs.find(sg => sg.id === id)!) to drop the "!" and use a filter with a
type predicate (e.g., filter((sg): sg is Subgraph => Boolean(sg))) before the
final .map that JSON.stringify's { label: sg.name, value: sg.id }, ensuring
selectedOptions' items are correctly typed without the misleading non-null
assertion.
In `@studio/src/components/ui/calendar.tsx`:
- Around line 53-62: The IconLeft and IconRight renderers include an unused
destructured parameter ({ ...props }) which should be removed to clean up dead
variables; update the JSX for IconLeft and IconRight to accept no props (e.g.,
replace "IconLeft: ({ ...props }) => <ChevronLeftIcon ... />" and the IconRight
equivalent) so only ChevronLeftIcon and ChevronRightIcon are returned, leaving
Calendar.displayName and the export unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3ae4aec-b3d4-439c-9f68-a8ca8deeb08e
📒 Files selected for processing (124)
studio/README.mdstudio/next.config.mjsstudio/sentry.client.config.tsstudio/sentry.edge.config.tsstudio/src/__tests__/playground-url-state-encoding-decoding.test.tsstudio/src/__tests__/schema-helpers.test.tsstudio/src/components/analytics/charts.tsxstudio/src/components/analytics/delta-badge.tsxstudio/src/components/analytics/filters.tsxstudio/src/components/analytics/toolbar.tsxstudio/src/components/analytics/trace-details.tsxstudio/src/components/analytics/useSyncTableWithQuery.tsstudio/src/components/app-provider.tsxstudio/src/components/audit-log-table.tsxstudio/src/components/auth/auth-components.tsxstudio/src/components/cache/cache-details-sheet.tsxstudio/src/components/cache/cache-warmer-config.tsxstudio/src/components/cache/operations-table.tsxstudio/src/components/changelog/changelog.tsxstudio/src/components/check-extensions/check-extensions-config.tsxstudio/src/components/checks/changes-table.tsxstudio/src/components/checks/checks-config.tsxstudio/src/components/checks/checks-filter-menu.tsxstudio/src/components/checks/graph-pruning-issues-table.tsxstudio/src/components/checks/lint-issues-table.tsxstudio/src/components/checks/operations.tsxstudio/src/components/checks/override.tsxstudio/src/components/checks/selected-checks-filters.tsxstudio/src/components/checks/subgraph-check-extension.tsxstudio/src/components/compose-status-bulb.tsxstudio/src/components/create-graph.tsxstudio/src/components/dashboard/graph-command-group.tsxstudio/src/components/dashboard/graph-selector.tsxstudio/src/components/dashboard/namespace-selector.tsxstudio/src/components/dashboard/workspace-command-wrapper.tsxstudio/src/components/dashboard/workspace-provider.tsxstudio/src/components/dashboard/workspace-selector.tsxstudio/src/components/empty-state.tsxstudio/src/components/feature-flag-details.tsxstudio/src/components/feature-flags-table.tsxstudio/src/components/federatedgraphs-cards.tsxstudio/src/components/group-select.tsxstudio/src/components/layout/analytics/active-campaign-script.tsxstudio/src/components/layout/analytics/gtm-script.tsxstudio/src/components/layout/dashboard-layout.tsxstudio/src/components/layout/sidenav.tsxstudio/src/components/layout/subgraph-layout.tsxstudio/src/components/layout/title-layout.tsxstudio/src/components/lint-policy/graph-pruning-config.tsxstudio/src/components/member-groups/create-group-dialog.tsxstudio/src/components/member-groups/delete-group-dialog.tsxstudio/src/components/member-groups/group-members-sheet.tsxstudio/src/components/member-groups/group-resource-selector.tsxstudio/src/components/member-groups/group-roles-command.tsxstudio/src/components/member-groups/group-row.tsxstudio/src/components/member-groups/group-rule-builder.tsxstudio/src/components/member-groups/group-sheet.tsxstudio/src/components/member-groups/use-group-resources.tsstudio/src/components/members/update-member-group-dialog.tsxstudio/src/components/multi-group-select.tsxstudio/src/components/notifications/components.tsxstudio/src/components/operations/client-usage-table.tsxstudio/src/components/overview/OverviewToolbar.tsxstudio/src/components/playground/share-playground-modal.tsxstudio/src/components/playground/types.tsstudio/src/components/popover-content-with-scrollable-content.tsxstudio/src/components/proposal/proposal-config.tsxstudio/src/components/reactflow-graph-node.tsxstudio/src/components/safe-markdown.tsxstudio/src/components/schema/empty-schema-state.tsxstudio/src/components/schema/toolbar.tsxstudio/src/components/settings/delete-organization.tsxstudio/src/components/settings/restore-organization.tsxstudio/src/components/subgraphs-table.tsxstudio/src/components/ui/accordion.tsxstudio/src/components/ui/alert-dialog.tsxstudio/src/components/ui/calendar.tsxstudio/src/components/ui/checkbox.tsxstudio/src/components/ui/dialog2.tsxstudio/src/components/ui/form.tsxstudio/src/components/ui/label.tsxstudio/src/components/ui/loader.tsxstudio/src/components/ui/popover.tsxstudio/src/components/ui/resizable.tsxstudio/src/components/ui/scroll-area.tsxstudio/src/components/ui/separator.tsxstudio/src/components/ui/skeleton.tsxstudio/src/components/ui/tag-input/tag-input.tsxstudio/src/components/ui/textarea.tsxstudio/src/components/ui/toast.tsxstudio/src/components/ui/toggle.tsxstudio/src/components/ui/tooltip.tsxstudio/src/components/ui/use-toast.tsstudio/src/components/user-menu.tsxstudio/src/components/webhook-delivery-details.tsxstudio/src/env.mjsstudio/src/hooks/use-check-user-access.tsstudio/src/hooks/use-event-callback.tsstudio/src/hooks/use-event-listener.tsstudio/src/hooks/use-fireworks.tsstudio/src/hooks/use-form.tsstudio/src/hooks/use-hydrate-playground-state-from-url.tsstudio/src/hooks/use-new-features-popup-disabled.tsstudio/src/hooks/use-pagination-params.tsstudio/src/hooks/use-session-storage.tsstudio/src/hooks/use-share-playground-modal.tsstudio/src/hooks/use-star-banner-disabled.tsstudio/src/hooks/use-user.tsstudio/src/hooks/use-workspace.tsstudio/src/lib/constants.tsstudio/src/lib/download-string-as-file.tsstudio/src/lib/format-date.tsstudio/src/lib/format-number.tsstudio/src/lib/format-status.tsstudio/src/lib/insights-helpers.tsstudio/src/lib/playground-storage.tsstudio/src/lib/playground-url-state-decoding.tsstudio/src/lib/playground-url-state-encoding.tsstudio/src/lib/schema-helpers.tsstudio/src/lib/signup-content.tsstudio/src/lib/utils.tsstudio/src/middleware.tsstudio/src/pages/cosmo-managed-solution-terms.mdstudio/src/styles/login.css
comatory
force-pushed
the
ondrej/eng-9087-cosmostudioci-fix-formatting-drift-enforce-formatting-in-ci-pt3
branch
from
38e637b to
d298d59
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
comatory
force-pushed
the
ondrej/eng-9087-cosmostudioci-fix-formatting-drift-enforce-formatting-in-ci-pt3
branch
from
d298d59 to
338208f
Compare
comatory
force-pushed
the
ondrej/eng-9087-cosmostudioci-fix-formatting-drift-enforce-formatting-in-ci-pt3
branch
from
338208f to
e562ec9
Compare
comatory
requested review from
JivusAyrus,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
studio/src/components/member-groups/group-members-sheet.tsx (1)
71-79:⚠️ Potential issue | 🟡 MinorGuard
createdAtbefore formatting.These values come from protobuf string fields, which default to
''when omitted. In that case, both rows render an invalid timestamp instead of a safe fallback. Please validate the value before callingformatDateTime.💡 Proposed fix
+const formatCreatedAt = (value: string) => { + if (!value) return null; + + const date = new Date(value); + return Number.isNaN(date.getTime()) ? null : formatDateTime(date); +}; ... - {data.members.map((member) => ( - <div key={`member-${member.id}`} className="space-y-1 px-4 py-2.5"> + {data.members.map((member) => { + const memberSince = formatCreatedAt(member.createdAt); + return ( + <div key={`member-${member.id}`} className="space-y-1 px-4 py-2.5"> <div className="flex items-center justify-start gap-x-2 truncate"> <UserIcon className="size-4 shrink-0" /> <span className="flex-grow truncate">{member.email}</span> </div> - <div className="text-xs text-muted-foreground"> - Member since {formatDateTime(new Date(member.createdAt))} - </div> + {memberSince && ( + <div className="text-xs text-muted-foreground"> + Member since {memberSince} + </div> + )} </div> - ))} + )})} ... - {data.apiKeys.map((key) => ( - <div key={`key-${key.id}`} className="space-y-1 px-4 py-2.5"> + {data.apiKeys.map((key) => { + const createdAt = formatCreatedAt(key.createdAt); + return ( + <div key={`key-${key.id}`} className="space-y-1 px-4 py-2.5"> <div className="flex items-center justify-start gap-x-2 truncate"> <KeyIcon className="size-4 shrink-0" /> <span className="flex-grow truncate">{key.name}</span> </div> - <div className="text-xs text-muted-foreground"> - Created at {formatDateTime(new Date(key.createdAt))} - </div> + {createdAt && ( + <div className="text-xs text-muted-foreground"> + Created at {createdAt} + </div> + )} </div> - ))} + )})}Also applies to: 91-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/member-groups/group-members-sheet.tsx` around lines 71 - 79, Guard calls to formatDateTime by checking member.createdAt before converting; in the members mapping inside GroupMembersSheet (the data.members.map(...) rendering block with key `member-${member.id}`) ensure you only call formatDateTime(new Date(member.createdAt)) when member.createdAt is a non-empty/valid string and otherwise render a safe fallback like "Unknown" or omit the date. Apply the same check in the other similar rendering block referenced (the rows around the 91-99 range) so no empty protobuf-createdAt strings are passed into formatDateTime.studio/src/components/graph-visualization.tsx (1)
115-130:⚠️ Potential issue | 🟡 MinorKeep subgraphs with missing metrics out of the ranked “Top 5”.
Line 119 returns
0whenever either side has no metrics, so items without metrics keep their original order and can still land in the sliced top-5 list ahead of measured subgraphs. That makes the ranking inaccurate when metrics are only partially available.Suggested fix
tempSubgraphs.sort((a, b) => { const metricA = subgraphMetrics?.find((x) => x.subgraphID === a.id); const metricB = subgraphMetrics?.find((x) => x.subgraphID === b.id); - if (!metricA || !metricB) { - return 0; - } - - if (topCategory === 'latency') { - return metricB.latency - metricA.latency; - } - if (topCategory === 'errorRate') { - return metricB.errorRate - metricA.errorRate; - } - - return metricB.requestRate - metricA.requestRate; + const metricValue = (metric?: SubgraphMetrics) => { + if (!metric) return Number.NEGATIVE_INFINITY; + if (topCategory === 'latency') return metric.latency; + if (topCategory === 'errorRate') return metric.errorRate; + return metric.requestRate; + }; + + const valueA = metricValue(metricA); + const valueB = metricValue(metricB); + + if (valueA === valueB) { + return 0; + } + + return valueB - valueA; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/graph-visualization.tsx` around lines 115 - 130, The comparator in tempSubgraphs.sort currently returns 0 when either metric is missing, allowing unmeasured subgraphs to remain in their original positions and potentially appear in the top 5; update the comparator used by tempSubgraphs.sort (the callback that queries subgraphMetrics by subgraphID and checks topCategory) so that missing metrics are ranked lower than measured ones — e.g., if both metrics are missing return 0, if metricA is missing return 1 (push a after b), if metricB is missing return -1 (push b after a) — then keep the existing comparisons for latency, errorRate, and requestRate unchanged.studio/src/components/date-picker-with-range.tsx (1)
87-100:⚠️ Potential issue | 🟠 MajorReopening a preset range now desynchronizes the dates and times.
In the
rangebranch,reset()refreshes the time inputs fromnew Date(), butselectedis still rebuilt from the staledateRangeprop. After reopening a quick range later,Applycan combine today’s times with yesterday’s dates; around midnight this can even emitend < start.Suggested fix
const reset = useCallback(() => { setSelectedRange(range); if (!range) { setStartTime(getFormattedTime(dateRange.start)); setEndTime(dateRange.end ? getFormattedTime(dateRange.end) : ''); + setSelectedDateTime(dateRange, undefined, false); } else { const end = new Date(); - setStartTime(getFormattedTime(subHours(end, range))); + const start = subHours(end, range); + setStartTime(getFormattedTime(start)); setEndTime(getFormattedTime(end)); - } - if (dateRange) { - setSelectedDateTime(dateRange, undefined, false); + setSelectedDateTime({ start, end }, undefined, false); } }, [dateRange, range]);Also applies to: 153-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/date-picker-with-range.tsx` around lines 87 - 100, The reset function uses new Date() to compute start/end times when a quick range is selected but still calls setSelectedDateTime(dateRange,...), causing desynced dates/times; change reset (and the similar block at 153-158) to compute the same start and end Date objects used for setStartTime/setEndTime when range is truthy (e.g., const end = new Date(); const start = subHours(end, range)) and then call setSelectedDateTime(start, end, false) and setSelectedRange(range) so the displayed dates and times come from the same computed values rather than the stale dateRange prop.studio/src/components/multi-group-select.tsx (1)
42-69:⚠️ Potential issue | 🟠 MajorUse a real button for the popover trigger.
This trigger is rendered as a
divwithrole="button", but it is not focusable and has noEnter/Spacehandling. That makes the selector inaccessible from the keyboard.♿ Proposed fix
<PopoverTrigger asChild> - <div - role="button" - className={buttonVariants({ - variant: 'outline', - className: cn( - 'relative h-auto min-h-11 w-full max-w-full justify-start gap-x-2', - disabled && 'cursor-not-allowed opacity-50 hover:!border-inherit hover:!bg-inherit', - ), - })} - > + <Button + type="button" + variant="outline" + disabled={disabled} + className={cn( + 'relative h-auto min-h-11 w-full max-w-full justify-start gap-x-2', + disabled && 'cursor-not-allowed opacity-50 hover:!border-inherit hover:!bg-inherit', + )} + > <div className="nowrap mr-3.5 flex w-1 flex-grow flex-wrap items-center justify-start gap-2"> {activeGroups.length > 0 ? ( activeGroups.map((group) => ( @@ </div> <span className="absolute inset-y-2 right-11 w-[1px] bg-border" /> <ChevronUpDownIcon className="size-4 flex-shrink-0" /> - </div> + </Button> </PopoverTrigger>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/multi-group-select.tsx` around lines 42 - 69, Replace the non-focusable div used as the PopoverTrigger child with a real button element so keyboard users can focus and activate it; inside the PopoverTrigger (where the current div with role="button" is rendered) use <button type="button"> (remove role="button"), add disabled={disabled} so native disabled behavior applies, preserve the existing className logic with buttonVariants/cn (including the disabled conditional), and keep the inner structure (activeGroups map, separator span, ChevronUpDownIcon) unchanged so the visual layout remains identical while enabling Enter/Space activation and focusability for the PopoverTrigger.studio/src/components/member-groups/group-resource-selector.tsx (1)
132-199:⚠️ Potential issue | 🟠 MajorThese selector controls are not keyboard accessible.
Both the row and the checkbox are clickable, but neither is keyboard-focusable nor handles
Enter/Space. That prevents keyboard users from expanding groups or toggling resource selection.studio/src/components/checks/override.tsx (1)
360-387:⚠️ Potential issue | 🟡 MinorMove the action row outside
SheetDescription.
SheetDescriptionrenders Radix's Dialog.Description element, which is a<p>tag. Nesting a<div>inside it violates HTML semantics and can trigger hydration warnings. Move the button row below the description instead of inside it.Suggested fix
- <SheetDescription> - Configure override for the operation with hash {operationHash}{' '} - <div className="mt-4 flex w-full items-center gap-2"> - <Button - variant="secondary" - className="" - onClick={() => { - copy(operationHash); - toast({ - description: 'Copied operation hash', - }); - }} - > - <ClipboardCopyIcon className="mr-3" /> - Copy Hash - </Button> - <OperationContentDialog - hash={operationHash} - trigger={ - <Button className="w-max" variant="secondary"> - View Operation Content - </Button> - } - federatedGraphName={graphContext?.graph?.name ?? ''} - namespace={graphContext?.graph?.namespace ?? ''} - /> - </div> - </SheetDescription> + <SheetDescription> + Configure override for the operation with hash {operationHash} + </SheetDescription> + <div className="mt-4 flex w-full items-center gap-2"> + <Button + variant="secondary" + className="" + onClick={() => { + copy(operationHash); + toast({ + description: 'Copied operation hash', + }); + }} + > + <ClipboardCopyIcon className="mr-3" /> + Copy Hash + </Button> + <OperationContentDialog + hash={operationHash} + trigger={ + <Button className="w-max" variant="secondary"> + View Operation Content + </Button> + } + federatedGraphName={graphContext?.graph?.name ?? ''} + namespace={graphContext?.graph?.namespace ?? ''} + /> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/override.tsx` around lines 360 - 387, The action row (the div with the two Buttons, ClipboardCopyIcon, copy/toast handlers, and the OperationContentDialog) is currently nested inside SheetDescription (which renders a <p>), causing invalid HTML; move that entire action row out of SheetDescription so the description only contains the text "Configure override for the operation with hash {operationHash}" and then render the <div className="mt-4 flex w-full items-center gap-2"> containing Button, ClipboardCopyIcon, copy(...)/toast(...), and OperationContentDialog (passing hash={operationHash}, federatedGraphName and namespace from graphContext) directly after the closed SheetDescription element to avoid nesting block elements inside a paragraph.studio/src/components/code-viewer.tsx (1)
129-145:⚠️ Potential issue | 🟡 MinorRender a non-link element in the gutter when
disableLinkingis enabled.Currently, setting
disableLinkingonly changes the href to'#'on line 129, but the element remains a<Link>component on line 138. This keeps the gutter tabbable and clickable, allowing URL hash mutations even when linking should be disabled. Replace the single<Link>with a conditional that renders a<span>whendisableLinkingis true and a<Link>when false.Proposed fix
- const href = disableLinking ? '#' : pathname + `#${lineNo}`; - return ( <div id={`id-${lineNo}`} key={i.toString()} {...getLineProps({ line })} className={hash === lineNo ? 'bg-secondary' : ''} > - <Link - href={href} - className={cn( - 'sticky left-0 mr-4 inline-block select-none border-r bg-background pr-2 text-right text-muted-foreground', - numberSectionWidth, - )} - > - <span>{i + 1}</span> - </Link> + {disableLinking ? ( + <span + className={cn( + 'sticky left-0 mr-4 inline-block select-none border-r bg-background pr-2 text-right text-muted-foreground', + numberSectionWidth, + )} + > + {i + 1} + </span> + ) : ( + <Link + href={`${pathname}#${lineNo}`} + className={cn( + 'sticky left-0 mr-4 inline-block select-none border-r bg-background pr-2 text-right text-muted-foreground', + numberSectionWidth, + )} + > + <span>{i + 1}</span> + </Link> + )} {line.map((token, key) => ( <span key={key} {...getTokenProps({ token })} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/code-viewer.tsx` around lines 129 - 145, The gutter currently always renders a Link component (Link) even when disableLinking is true, only changing href to '#' which leaves it tabbable and clickable; update the rendering inside the return so that when disableLinking is true you render a non-interactive span (e.g., <span> with the same classes including numberSectionWidth and the sticky/number styles) and when false you render the Link using href computed earlier; ensure you keep the same className composition, aria/visual structure (the inner <span>{i + 1}</span>), and preserve getLineProps, id={`id-${lineNo}`}, key, and conditional className that uses hash === lineNo.studio/src/components/dashboard/namespace-selector.tsx (1)
64-69:⚠️ Potential issue | 🟡 MinorMisleading comment: filter is reset on close, not open.
The comment states "Only reset the filter when the popover is opened" but the code resets the filter when
!v(i.e., when the popover is closing). This should be corrected to avoid confusion.📝 Proposed fix
onOpenChange={(v) => { setOpen(v); if (!v) { - // Only reset the filter when the popover is opened + // Reset the filter when the popover is closed setFilter(''); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/namespace-selector.tsx` around lines 64 - 69, The comment on the onOpenChange handler is misleading: it currently says "Only reset the filter when the popover is opened" while the condition uses if (!v) to reset via setFilter('') when the popover is closing; update the comment to accurately reflect the behavior (e.g., "Only reset the filter when the popover is closed") or, if the original intent was to reset on open, change the condition to if (v) and keep the comment—make this change around the onOpenChange callback that calls setOpen and setFilter.studio/src/components/settings/restore-organization.tsx (1)
20-33:⚠️ Potential issue | 🟠 MajorAdd a fallback for missing/unknown restore responses.
RestoreOrganizationResponse.responseis optional, so this block can currently close the dialog without any feedback when Line 22 is notOKand Line 29 has nodetails. For an org-level action, that leaves the user in a silent failure state.💡 Minimal fix
if (d.response?.code === EnumStatusCode.OK) { toast({ description: 'Organization restored successfully.', duration: 3000, }); await sessionQueryClient.refetchQueries(); - } else if (d.response?.details) { - toast({ description: d.response.details, duration: 3000 }); + } else { + toast({ + description: + d.response?.details ?? + 'Could not confirm that the organization was restored. Please refresh and try again.', + duration: 3000, + }); } setOpen(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/settings/restore-organization.tsx` around lines 20 - 33, The onSuccess handler for useMutation(restoreOrganization) currently always calls setOpen(false) and can close the dialog without feedback when response is missing or non-OK; change the control flow so setOpen(false) is only called when d.response?.code === EnumStatusCode.OK, and add a fallback else branch after the existing else if (d.response?.details) that shows a generic error toast (e.g., "Failed to restore organization" and optionally include d.response?.code or other info) so users get feedback when restoreResponse.response is absent or unknown; keep sessionQueryClient.refetchQueries() only in the OK branch.studio/src/components/analytics/metrics.tsx (3)
249-258:⚠️ Potential issue | 🟠 MajorRemove
nullquery params before pushing the next URL.
applyNewParamsis typed to acceptnull, andresetFilters()relies on that, but the implementation spreadsnullstraight intorouter.push. That makes the reset path diverge from the explicit delete-based cleanup used above.🧹 Suggested fix
const applyNewParams = useCallback( (newParams: Record<string, string | null>, unset?: string[]) => { - const mergedParams = Object.fromEntries(Object.entries(router.query).filter(([key]) => !unset?.includes(key))); + const nextQuery = Object.fromEntries( + Object.entries(router.query).filter(([key]) => !unset?.includes(key)), + ); + + for (const [key, value] of Object.entries(newParams)) { + if (value === null) { + delete nextQuery[key]; + continue; + } + nextQuery[key] = value; + } router.push({ - query: { - ...mergedParams, - ...newParams, - }, + query: nextQuery, }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/metrics.tsx` around lines 249 - 258, applyNewParams currently spreads null values into router.push which leaves literal null query params; modify applyNewParams (the function using router.push) to filter out entries where newParams has null values before merging — e.g., compute cleanedNew = Object.fromEntries(Object.entries(newParams).filter(([,v]) => v !== null)) and merge that (instead of newParams) with mergedParams so any null-valued keys are removed from the resulting query (preserving the existing unset logic used by resetFilters).
208-214:⚠️ Potential issue | 🟠 MajorGuard
filterStateagainst valid-but-wrong JSON.The cleanup effect only runs after render. A payload like
filterState=nullorfilterState={}is still valid JSON, so thistry/catchsucceeds and the first array-only consumer later crashes before the reset logic gets a chance to run.🛡️ Suggested guard
const selectedFilters = useMemo(() => { try { - return JSON.parse(router.query.filterState?.toString() ?? '[]'); + const parsed = JSON.parse(router.query.filterState?.toString() ?? '[]'); + const isValidFilterState = + Array.isArray(parsed) && + parsed.every((item) => { + if (!item || typeof item !== 'object') { + return false; + } + const candidate = item as { id?: unknown; value?: unknown }; + return ( + typeof candidate.id === 'string' && + Array.isArray(candidate.value) && + candidate.value.every((v) => typeof v === 'string') + ); + }); + + return isValidFilterState ? parsed : []; } catch { return []; } }, [router.query.filterState]);Also applies to: 217-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/metrics.tsx` around lines 208 - 214, The current selectedFilters useMemo parses router.query.filterState without validating the parsed shape, so valid-but-wrong JSON like "null" or "{}" yields non-array values and later array-only consumers crash before cleanup runs; update the selectedFilters logic (the useMemo that reads router.query.filterState) to parse safely and then check that the result is an Array (e.g., Array.isArray(parsed)), returning [] if parsing fails or the parsed value is not an array, and apply the same array-shape guard to the other filter-state consumers referenced in this file (the code paths around selectedFilters and the other consumers between the nearby blocks) so all callers always receive an array.
144-181:⚠️ Potential issue | 🟡 MinorPreserve the specific filter-limit reason.
checkFilterLimitsalready distinguishes URL length from the 10 KB payload cap, but this helper collapses that toboolean. The toast therefore always blames URL length, even when onlyMAX_FILTER_SIZEwas exceeded.💬 Suggested fix
export const wouldExceedUrlLimit = ( router: ReturnType<typeof useRouter>, currentFilters: { id: string; value: string[] }[], filterId: string, newValue: string[], -): boolean => { +): { exceeded: boolean; reason?: string } => { if (!newValue || newValue.length === 0) { // Removing filters - always allow - return false; + return { exceeded: false }; } @@ - const { exceeded } = checkFilterLimits(router, stringifiedFilters); - return exceeded; + return checkFilterLimits(router, stringifiedFilters); }; @@ - if (wouldExceedUrlLimit(router, currentSelectedFilters, filter.id, value)) { + const { exceeded, reason } = wouldExceedUrlLimit(router, currentSelectedFilters, filter.id, value); + if (exceeded) { toast({ title: 'Filter limit reached', - description: `Maximum URL length of ${MAX_URL_LENGTH.toLocaleString()} characters reached. Please remove some filters before adding new ones.`, + description: `${reason}. Please remove some filters before adding new ones.`, }); return false; // Validation failed - prevents onSelect from being called }Also applies to: 342-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/metrics.tsx` around lines 144 - 181, The helper wouldExceedUrlLimit currently converts checkFilterLimits's detailed result into a boolean, losing the specific reason (URL length vs MAX_FILTER_SIZE); change wouldExceedUrlLimit to return the full checkFilterLimits result (or at least the reason/code field) instead of just boolean so callers can show the correct toast message, e.g., call checkFilterLimits(router, stringifiedFilters) and propagate its returned {exceeded, reason} (or limitType) up; update the other similar helper usage (the other occurrence that also collapses the result) to do the same so the UI can distinguish URL-length errors from payload-size errors.
🧹 Nitpick comments (8)
studio/src/components/checks/composed-schema-changes-table.tsx (1)
38-48: Consider using a stable key instead of array index.Using array index
ias thekeyprop can cause issues if thechangesarray is reordered or items are inserted/removed, potentially leading to incorrect component state preservation. IfFederatedGraphSchemaChangehas a unique identifier, prefer using that.💡 Suggested improvement
If each change has a unique identifier:
- {changes.map((c, i) => ( - <Row - key={i} + {changes.map((c) => ( + <Row + key={`${c.changeType}-${c.message}-${c.federatedGraphName}`}Alternatively, if the combination of
changeType + message + federatedGraphNameis unique (as already used in theTableRowkey on line 78), you could use that as the map key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/composed-schema-changes-table.tsx` around lines 38 - 48, The map over changes uses the array index as key (changes.map((c, i) => <Row key={i} ... />)), which is fragile; change the key to a stable identifier from the FederatedGraphSchemaChange object (e.g., use c.id if available) or, if no single id exists, derive a stable composite key such as `${c.changeType}-${c.message}-${c.federatedGraphName}` to replace key={i} so the Row component preserves identity correctly.studio/src/components/user-menu.tsx (1)
16-27: Consider adding'playground:env'to the removal list.The trailing comma addition is fine. However, per the Playground API in
custom-scripts.tsx, the key'playground:env'is written to localStorage but is not included in this removal list. This is a pre-existing gap (not introduced by this PR), but worth addressing in a follow-up to ensure a complete logout cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/user-menu.tsx` around lines 16 - 27, Add the missing 'playground:env' key to the local storage cleanup array so logout clears the Playground environment too: update the localStorageKeysToRemove array (the constant named localStorageKeysToRemove in user-menu.tsx) to include 'playground:env' alongside the other playground-related keys so custom-scripts.tsx's stored key is removed during logout.studio/src/components/check-extensions/check-extensions-config.tsx (1)
151-159: Redundant nested fragments.The double fragment wrapping (
<> <> ... </> </>) is unnecessary. The outer fragment can be removed.✨ Suggested simplification
description: isLintingEnabledForNamespace ? ( 'Linting issues identified based on the configured rules for the namespace.' ) : ( - <> - <> - You must{' '} - <Link href={`/${organizationSlug}/policies?namespace=${namespace.name}`} className="text-primary"> - enable the linter - </Link>{' '} - for the namespace to be able to receive lint warnings and errors. - </> - </> + <> + You must{' '} + <Link href={`/${organizationSlug}/policies?namespace=${namespace.name}`} className="text-primary"> + enable the linter + </Link>{' '} + for the namespace to be able to receive lint warnings and errors. + </> ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/check-extensions/check-extensions-config.tsx` around lines 151 - 159, In the JSX inside the CheckExtensionsConfig component, remove the redundant outer fragment wrapping the nested fragment that contains the Link (the block referencing organizationSlug and namespace.name); keep a single fragment or the inner content directly so the JSX becomes non-nested and cleaner (locate the JSX that renders "You must <Link href={`/${organizationSlug}/policies?namespace=${namespace.name}`} ...>enable the linter</Link> for the namespace..." and delete the extra <>...</> wrapper).studio/src/components/checks/lint-issues-table.tsx (1)
44-46: Inconsistent fallback value compared tograph-pruning-issues-table.tsx.This line uses
'default'as the fallback whengraphContext?.graph?.namespaceis undefined, but thenamespacevariable fromuseWorkspace()is already available at line 31. The sibling componentgraph-pruning-issues-table.tsx(line 47) usesnamespaceas the fallback instead, which is more robust.Consider using the workspace namespace for consistency
router.push( - `/${user!.currentOrganization.slug}/policies?namespace=${graphContext?.graph?.namespace ?? 'default'}`, + `/${user!.currentOrganization.slug}/policies?namespace=${graphContext?.graph?.namespace ?? namespace}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/lint-issues-table.tsx` around lines 44 - 46, The router.push call in lint-issues-table.tsx uses a hardcoded fallback 'default' for graphContext?.graph?.namespace which is inconsistent with graph-pruning-issues-table.tsx; replace the fallback with the workspace-level namespace variable returned by useWorkspace() (the namespace identifier) so the push uses `graphContext?.graph?.namespace ?? namespace` when constructing the URL (keep the existing user!.currentOrganization.slug and router.push call intact).studio/src/components/checks/graph-pruning-issues-table.tsx (1)
58-74: Redundant condition in else-if branch.At line 66, the condition
else if (pruneIssues.length === 0 && !hasGraphPruningErrors)is redundant. Since we've already checkedpruneIssues.length === 0 && !isGraphPruningEnabledat line 37 andpruneIssues.length === 0 && hasGraphPruningErrorsat line 58, reaching this branch already implies both conditions. A simpleelsewould suffice.Simplify the condition
return ( <EmptyState icon={<CrossCircledIcon className="h-16 w-16 text-destructive" />} title="Graph Prune Check Failed" description='This check succeeded for the current federated graph, but it failed in one or more other affected federated graphs. Please check the "Affected Graphs" section to identify which graphs encountered graph pruning errors.' /> ); - } else if (pruneIssues.length === 0 && !hasGraphPruningErrors) { + } else if (pruneIssues.length === 0) { return (Or even simpler with just
else:- } else if (pruneIssues.length === 0 && !hasGraphPruningErrors) { + } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/graph-pruning-issues-table.tsx` around lines 58 - 74, The else-if condition checking "pruneIssues.length === 0 && !hasGraphPruningErrors" is redundant given prior branches; replace the "else if (pruneIssues.length === 0 && !hasGraphPruningErrors)" branch with a simple "else" so the final EmptyState (with CheckCircleIcon and title "Graph Prune Check Successful") runs as the catch-all when pruneIssues is empty and earlier branches did not match; update the branch that renders the EmptyState component (the one using CheckCircleIcon/title "Graph Prune Check Successful") accordingly and remove the redundant boolean check on hasGraphPruningErrors.studio/src/components/member-groups/group-sheet.tsx (1)
216-217: Consider resettingtmpDescriptionwhen the dialog closes.If a user opens the dialog, makes changes, and clicks Cancel, the modified
tmpDescriptionpersists in state. The next time the dialog opens, it shows the previously cancelled changes rather than the current saved description.This appears to be pre-existing behavior rather than introduced by this PR, but worth addressing for better UX.
💡 Suggested fix to reset state on dialog close
const [tmpDescription, setTmpDescription] = useState(description); const [open, setOpen] = useState(false); +const handleOpenChange = (isOpen: boolean) => { + setOpen(isOpen); + if (isOpen) { + setTmpDescription(description); + } +}; + return ( - <Dialog open={open} onOpenChange={setOpen}> + <Dialog open={open} onOpenChange={handleOpenChange}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/member-groups/group-sheet.tsx` around lines 216 - 217, The temporary description state tmpDescription is not reset when the dialog controlled by open/setOpen closes, so cancelled edits persist; fix by resetting tmpDescription to description when the dialog closes—either update the function that sets setOpen(false) to also call setTmpDescription(description) or add a useEffect watching open and description that when open becomes false (or when description changes) sets setTmpDescription(description); reference tmpDescription, setTmpDescription, open, setOpen and description to locate the change.studio/src/components/operations/operations-list.tsx (1)
177-177: Nit: Redundant template literal.The template literal is unnecessary since
slice()already returns a string.♻️ Suggested simplification
- {searchQuery ? highlightText(operation.hash.slice(0, 4), searchQuery) : `${operation.hash.slice(0, 4)}`} + {searchQuery ? highlightText(operation.hash.slice(0, 4), searchQuery) : operation.hash.slice(0, 4)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/operations/operations-list.tsx` at line 177, The template literal around operation.hash.slice(0, 4) is redundant; update the JSX so the fallback branch directly returns the sliced string instead of using `${operation.hash.slice(0, 4)}` (e.g., in the ternary expression that calls highlightText(operation.hash.slice(0, 4), searchQuery) when searchQuery is truthy, return operation.hash.slice(0, 4) when falsy).studio/src/components/multi-group-select.tsx (1)
12-22: Keep the new required prop out of this formatting-only pass.Making
onValueChangemandatory changes the public contract ofMultiGroupSelect; that's a behavioral/API change, not just formatting. I'd either keep this backward-compatible here or move it into a focused follow-up PR so the formatting sweep stays low-risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/multi-group-select.tsx` around lines 12 - 22, The PR made onValueChange required on MultiGroupSelect which changes the component's public API; revert that by restoring onValueChange as optional in the component props (use onValueChange?: (groups: OrganizationGroup[]) => void) and update any calls inside MultiGroupSelect to guard before invoking (e.g., if (onValueChange) onValueChange(...)) so behavior remains backward-compatible; leave the rest of MultiGroupSelect unchanged and move any intentional API change to a separate PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/analytics/data-table-primary-filter-menu.tsx`:
- Around line 58-64: The checkbox is toggling between controlled/uncontrolled
because checked can be undefined; update the DropdownMenuCheckboxItem usage to
always pass a boolean for checked (e.g. use !!filter.selectedOptions?.length)
and guard the callback by checking strict equality (checked === true) before
calling filter.onSelect, so use filter.onSelect?.(checked === true ? ['true'] :
undefined); modify the props on DropdownMenuCheckboxItem (checked and
onCheckedChange) accordingly to keep the component controlled.
In `@studio/src/components/analytics/getColumnData.tsx`:
- Around line 38-42: The Tailwind arbitrary-value syntax is wrong in the column
definitions (e.g., traceId.header.className 'w-[80]px' and the other width at
lines 53-57); update those className strings to use the correct syntax by moving
the unit inside the brackets (e.g., 'w-[80px]' and 'w-[100px]') so the width
arbitrary values are valid in getColumnData.tsx.
In `@studio/src/components/checks/override.tsx`:
- Around line 97-126: The explorer icon is still focusable/clickable when path
is missing because Button with asChild wraps a Link; change to conditionally
render: when path is truthy render the Button with asChild wrapping the Link
(preserving current href logic), and when path is falsy render a plain disabled
Button (no asChild/Link) so the disabled attribute is effective; add an
appropriate aria-label (e.g., "Open in Explorer") to the explorer Button/Link.
Do the same for the delete icon in the AlertDialogTrigger: ensure the trigger
has an aria-label (e.g., "Delete override") and if it should be non-interactive
in some states render a disabled Button instead of using asChild around a
focusable element. Target the Tooltip/TooltipTrigger/Button/Link/GlobeIcon block
and the AlertDialog/AlertDialogTrigger/Button delete block to apply these
changes.
In `@studio/src/components/checks/selected-checks-filters.tsx`:
- Around line 27-30: The selectedOptions mapping uses a non-null assertion on
the result of subgraphs.find(...) which contradicts the subsequent
.filter(Boolean); remove the `!` and instead guard against undefined by first
mapping to possibly-undefined items then filtering them before converting to
strings (e.g., change the pipeline around selectedSubgraphs.map(id =>
subgraphs.find(sg => sg.id === id)) to avoid `!`, filter out undefined results,
then map to JSON.stringify({ label: sg.name, value: sg.id })); update the
expression that supplies selectedOptions so TypeScript sees the `find` result
may be undefined and is filtered out before use.
In `@studio/src/components/code-viewer.tsx`:
- Around line 118-125: The gutter width logic in the numberSectionWidth
calculation uses an invalid Tailwind class 'w-18' (in numberSectionWidth in
code-viewer.tsx), so replace that branch with a valid Tailwind arbitrary width
(for example an arbitrary value like w-[72px] or w-[4.5rem]) to ensure 4-digit
line numbers get a fixed width; update the ternary that currently returns 'w-18'
to return the chosen arbitrary width string instead.
In `@studio/src/components/date-picker-with-range.tsx`:
- Around line 123-126: getValue currently coerces empty startTime/endTime ('')
into '00:00'/'23:59', causing Apply to emit a preset range even when the user
cleared the time; change getValue (and the similar block at lines 141-149) to
treat empty startTime/endTime as "no time" instead of defaulting—i.e., call
getFromDate/getToDate only when startTime/endTime are non-empty (or return a
DateRange with undefined/null time fields) so the component can switch to custom
mode; update logic around selected.start/selected.end, getFromDate, getToDate,
startTime and endTime accordingly.
In `@studio/src/components/lint-policy/linter-config.tsx`:
- Line 193: Fix the typo in the user-facing message by updating the description
string in the lint-policy configuration: locate the "description: 'Lint Policy
applied succesfully.'" entry in lint-policy/linter-config.tsx (the
object/property that sets the toast/notification message) and change
"succesfully" to "successfully" so the message reads "Lint Policy applied
successfully." Ensure no other occurrences remain in the same file.
In `@studio/src/components/member-groups/group-resource-selector.tsx`:
- Around line 62-65: The list items are keyed by index which can shuffle local
state in GroupSelectorItem; change the key in the availableResources.map that
renders GroupSelectorItem to use a stable resource identifier (e.g., res.id or
another unique property on the resource) instead of `resource-${index}` so the
component's local expanded state remains associated with the correct resource
when availableResources is reordered or refreshed; update the key on the
GroupSelectorItem invocation accordingly and ensure the chosen property is
always unique.
In `@studio/src/components/playground/share-playground-modal.tsx`:
- Around line 147-149: The share trigger button in share-playground-modal.tsx
(the Button rendering FiShare2 with className
"graphiql-toolbar-button"/"graphiql-toolbar-icon") is icon-only and lacks an
accessible name; add an aria-label="Share Playground" attribute to that Button
element so assistive technology announces it correctly (ensure the aria-label is
applied on the same Button that contains the FiShare2 icon).
- Around line 13-16: The file mixes the local Tooltip wrapper with raw Radix
imports which bypasses the wrapper's defaults; replace the import of
TooltipContent and TooltipTrigger from '@radix-ui/react-tooltip' with imports
from "@/components/ui/tooltip" so Tooltip, TooltipContent, and TooltipTrigger
come from the same local wrapper, and remove or replace the inline
className="rounded-md border bg-background px-2 py-1" on the tooltip content so
it uses the wrapper's default sideOffset, animations (e.g., fade-in-0
zoom-in-95), and color classes; update usages around Tooltip, TooltipContent,
and TooltipTrigger (including the duplicated pattern at lines ~155–167)
accordingly.
In `@studio/src/components/reactflow-graph-node.tsx`:
- Line 11: Remove the dark-mode ring tweak introduced in the className of the
container div in reactflow-graph-node.tsx: drop the `dark:ring-white/15` token
from the string in the top-level div (the JSX element that defines the node
container) so this formatting-only PR does not change visual styling; if this
visual tweak is intended, move it into a separate style/visual PR instead.
In `@studio/src/components/settings/restore-organization.tsx`:
- Around line 54-72: The restore action is only guarded at the trigger, so the
in-dialog Button still calls mutate({ userID: user?.id }) unconditionally;
update the dialog's restore flow to re-check permissions and avoid calling
mutate when the user is not an admin (e.g., disable the in-dialog Button when
!isAdmin and/or early-return inside the onClick handler), and ensure any onClick
uses a safe check (isAdmin && user?.id) before invoking mutate and preserves
previous setOpen behavior.
---
Outside diff comments:
In `@studio/src/components/analytics/metrics.tsx`:
- Around line 249-258: applyNewParams currently spreads null values into
router.push which leaves literal null query params; modify applyNewParams (the
function using router.push) to filter out entries where newParams has null
values before merging — e.g., compute cleanedNew =
Object.fromEntries(Object.entries(newParams).filter(([,v]) => v !== null)) and
merge that (instead of newParams) with mergedParams so any null-valued keys are
removed from the resulting query (preserving the existing unset logic used by
resetFilters).
- Around line 208-214: The current selectedFilters useMemo parses
router.query.filterState without validating the parsed shape, so valid-but-wrong
JSON like "null" or "{}" yields non-array values and later array-only consumers
crash before cleanup runs; update the selectedFilters logic (the useMemo that
reads router.query.filterState) to parse safely and then check that the result
is an Array (e.g., Array.isArray(parsed)), returning [] if parsing fails or the
parsed value is not an array, and apply the same array-shape guard to the other
filter-state consumers referenced in this file (the code paths around
selectedFilters and the other consumers between the nearby blocks) so all
callers always receive an array.
- Around line 144-181: The helper wouldExceedUrlLimit currently converts
checkFilterLimits's detailed result into a boolean, losing the specific reason
(URL length vs MAX_FILTER_SIZE); change wouldExceedUrlLimit to return the full
checkFilterLimits result (or at least the reason/code field) instead of just
boolean so callers can show the correct toast message, e.g., call
checkFilterLimits(router, stringifiedFilters) and propagate its returned
{exceeded, reason} (or limitType) up; update the other similar helper usage (the
other occurrence that also collapses the result) to do the same so the UI can
distinguish URL-length errors from payload-size errors.
In `@studio/src/components/checks/override.tsx`:
- Around line 360-387: The action row (the div with the two Buttons,
ClipboardCopyIcon, copy/toast handlers, and the OperationContentDialog) is
currently nested inside SheetDescription (which renders a <p>), causing invalid
HTML; move that entire action row out of SheetDescription so the description
only contains the text "Configure override for the operation with hash
{operationHash}" and then render the <div className="mt-4 flex w-full
items-center gap-2"> containing Button, ClipboardCopyIcon, copy(...)/toast(...),
and OperationContentDialog (passing hash={operationHash}, federatedGraphName and
namespace from graphContext) directly after the closed SheetDescription element
to avoid nesting block elements inside a paragraph.
In `@studio/src/components/code-viewer.tsx`:
- Around line 129-145: The gutter currently always renders a Link component
(Link) even when disableLinking is true, only changing href to '#' which leaves
it tabbable and clickable; update the rendering inside the return so that when
disableLinking is true you render a non-interactive span (e.g., <span> with the
same classes including numberSectionWidth and the sticky/number styles) and when
false you render the Link using href computed earlier; ensure you keep the same
className composition, aria/visual structure (the inner <span>{i + 1}</span>),
and preserve getLineProps, id={`id-${lineNo}`}, key, and conditional className
that uses hash === lineNo.
In `@studio/src/components/dashboard/namespace-selector.tsx`:
- Around line 64-69: The comment on the onOpenChange handler is misleading: it
currently says "Only reset the filter when the popover is opened" while the
condition uses if (!v) to reset via setFilter('') when the popover is closing;
update the comment to accurately reflect the behavior (e.g., "Only reset the
filter when the popover is closed") or, if the original intent was to reset on
open, change the condition to if (v) and keep the comment—make this change
around the onOpenChange callback that calls setOpen and setFilter.
In `@studio/src/components/date-picker-with-range.tsx`:
- Around line 87-100: The reset function uses new Date() to compute start/end
times when a quick range is selected but still calls
setSelectedDateTime(dateRange,...), causing desynced dates/times; change reset
(and the similar block at 153-158) to compute the same start and end Date
objects used for setStartTime/setEndTime when range is truthy (e.g., const end =
new Date(); const start = subHours(end, range)) and then call
setSelectedDateTime(start, end, false) and setSelectedRange(range) so the
displayed dates and times come from the same computed values rather than the
stale dateRange prop.
In `@studio/src/components/graph-visualization.tsx`:
- Around line 115-130: The comparator in tempSubgraphs.sort currently returns 0
when either metric is missing, allowing unmeasured subgraphs to remain in their
original positions and potentially appear in the top 5; update the comparator
used by tempSubgraphs.sort (the callback that queries subgraphMetrics by
subgraphID and checks topCategory) so that missing metrics are ranked lower than
measured ones — e.g., if both metrics are missing return 0, if metricA is
missing return 1 (push a after b), if metricB is missing return -1 (push b after
a) — then keep the existing comparisons for latency, errorRate, and requestRate
unchanged.
In `@studio/src/components/member-groups/group-members-sheet.tsx`:
- Around line 71-79: Guard calls to formatDateTime by checking member.createdAt
before converting; in the members mapping inside GroupMembersSheet (the
data.members.map(...) rendering block with key `member-${member.id}`) ensure you
only call formatDateTime(new Date(member.createdAt)) when member.createdAt is a
non-empty/valid string and otherwise render a safe fallback like "Unknown" or
omit the date. Apply the same check in the other similar rendering block
referenced (the rows around the 91-99 range) so no empty protobuf-createdAt
strings are passed into formatDateTime.
In `@studio/src/components/multi-group-select.tsx`:
- Around line 42-69: Replace the non-focusable div used as the PopoverTrigger
child with a real button element so keyboard users can focus and activate it;
inside the PopoverTrigger (where the current div with role="button" is rendered)
use <button type="button"> (remove role="button"), add disabled={disabled} so
native disabled behavior applies, preserve the existing className logic with
buttonVariants/cn (including the disabled conditional), and keep the inner
structure (activeGroups map, separator span, ChevronUpDownIcon) unchanged so the
visual layout remains identical while enabling Enter/Space activation and
focusability for the PopoverTrigger.
In `@studio/src/components/settings/restore-organization.tsx`:
- Around line 20-33: The onSuccess handler for useMutation(restoreOrganization)
currently always calls setOpen(false) and can close the dialog without feedback
when response is missing or non-OK; change the control flow so setOpen(false) is
only called when d.response?.code === EnumStatusCode.OK, and add a fallback else
branch after the existing else if (d.response?.details) that shows a generic
error toast (e.g., "Failed to restore organization" and optionally include
d.response?.code or other info) so users get feedback when
restoreResponse.response is absent or unknown; keep
sessionQueryClient.refetchQueries() only in the OK branch.
---
Nitpick comments:
In `@studio/src/components/check-extensions/check-extensions-config.tsx`:
- Around line 151-159: In the JSX inside the CheckExtensionsConfig component,
remove the redundant outer fragment wrapping the nested fragment that contains
the Link (the block referencing organizationSlug and namespace.name); keep a
single fragment or the inner content directly so the JSX becomes non-nested and
cleaner (locate the JSX that renders "You must <Link
href={`/${organizationSlug}/policies?namespace=${namespace.name}`} ...>enable
the linter</Link> for the namespace..." and delete the extra <>...</> wrapper).
In `@studio/src/components/checks/composed-schema-changes-table.tsx`:
- Around line 38-48: The map over changes uses the array index as key
(changes.map((c, i) => <Row key={i} ... />)), which is fragile; change the key
to a stable identifier from the FederatedGraphSchemaChange object (e.g., use
c.id if available) or, if no single id exists, derive a stable composite key
such as `${c.changeType}-${c.message}-${c.federatedGraphName}` to replace
key={i} so the Row component preserves identity correctly.
In `@studio/src/components/checks/graph-pruning-issues-table.tsx`:
- Around line 58-74: The else-if condition checking "pruneIssues.length === 0 &&
!hasGraphPruningErrors" is redundant given prior branches; replace the "else if
(pruneIssues.length === 0 && !hasGraphPruningErrors)" branch with a simple
"else" so the final EmptyState (with CheckCircleIcon and title "Graph Prune
Check Successful") runs as the catch-all when pruneIssues is empty and earlier
branches did not match; update the branch that renders the EmptyState component
(the one using CheckCircleIcon/title "Graph Prune Check Successful") accordingly
and remove the redundant boolean check on hasGraphPruningErrors.
In `@studio/src/components/checks/lint-issues-table.tsx`:
- Around line 44-46: The router.push call in lint-issues-table.tsx uses a
hardcoded fallback 'default' for graphContext?.graph?.namespace which is
inconsistent with graph-pruning-issues-table.tsx; replace the fallback with the
workspace-level namespace variable returned by useWorkspace() (the namespace
identifier) so the push uses `graphContext?.graph?.namespace ?? namespace` when
constructing the URL (keep the existing user!.currentOrganization.slug and
router.push call intact).
In `@studio/src/components/member-groups/group-sheet.tsx`:
- Around line 216-217: The temporary description state tmpDescription is not
reset when the dialog controlled by open/setOpen closes, so cancelled edits
persist; fix by resetting tmpDescription to description when the dialog
closes—either update the function that sets setOpen(false) to also call
setTmpDescription(description) or add a useEffect watching open and description
that when open becomes false (or when description changes) sets
setTmpDescription(description); reference tmpDescription, setTmpDescription,
open, setOpen and description to locate the change.
In `@studio/src/components/multi-group-select.tsx`:
- Around line 12-22: The PR made onValueChange required on MultiGroupSelect
which changes the component's public API; revert that by restoring onValueChange
as optional in the component props (use onValueChange?: (groups:
OrganizationGroup[]) => void) and update any calls inside MultiGroupSelect to
guard before invoking (e.g., if (onValueChange) onValueChange(...)) so behavior
remains backward-compatible; leave the rest of MultiGroupSelect unchanged and
move any intentional API change to a separate PR.
In `@studio/src/components/operations/operations-list.tsx`:
- Line 177: The template literal around operation.hash.slice(0, 4) is redundant;
update the JSX so the fallback branch directly returns the sliced string instead
of using `${operation.hash.slice(0, 4)}` (e.g., in the ternary expression that
calls highlightText(operation.hash.slice(0, 4), searchQuery) when searchQuery is
truthy, return operation.hash.slice(0, 4) when falsy).
In `@studio/src/components/user-menu.tsx`:
- Around line 16-27: Add the missing 'playground:env' key to the local storage
cleanup array so logout clears the Playground environment too: update the
localStorageKeysToRemove array (the constant named localStorageKeysToRemove in
user-menu.tsx) to include 'playground:env' alongside the other
playground-related keys so custom-scripts.tsx's stored key is removed during
logout.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
studio/src/components/check-extensions/check-extensions-config.tsx (1)
127-129:⚠️ Potential issue | 🟡 MinorMinor grammatical error in comment.
The comment has a typo: "lets give ask them" should be "let's ask them".
📝 Suggested fix
if (isSecretKeyAssigned && forceShowSecretKeyInput && !newConfig.secretKey?.trim().length) { - // The user has emptied the secret key, lets give ask them to confirm whether this is intended + // The user has emptied the secret key, let's ask them to confirm whether this is intended setShowConfirmationDialog(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/check-extensions/check-extensions-config.tsx` around lines 127 - 129, Fix the typo in the inline comment inside the if block that checks isSecretKeyAssigned, forceShowSecretKeyInput, and newConfig.secretKey; change "lets give ask them" to "let's ask them" so the comment reads clearly before the call to setShowConfirmationDialog.studio/src/components/layout/graph-layout.tsx (1)
63-75:⚠️ Potential issue | 🔴 CriticalDo not render children with an undefined
GraphContextvalue.At Line 64,
graphContextDatastaysundefineduntilgraphsDataarrives, but Line 190 still renders the children as soon as the main graph query succeeds. IfgetFederatedGraphByNameresolves first—orgetFederatedGraphsfails—you'll provideundefinedtoGraphContext, and consumers likestudio/src/components/graph-visualization.tsxandstudio/src/components/operations/operations-search.tsxcurrently dereference that context directly. This is a runtime crash path.Either gate rendering on the second query as well, or provide a non-undefined fallback for
graphsso the provider contract stays stable.Suggested fix
- const { data: graphsData } = useQuery(getFederatedGraphs); + const { + data: graphsData, + isLoading: isLoadingGraphs, + error: graphsError, + } = useQuery(getFederatedGraphs); const graphContextData = useMemo(() => { if (!data || !graphsData) { return undefined; } return { graph: data.graph, subgraphs: data.subgraphs, graphRequestToken: data.graphRequestToken, graphs: graphsData.graphs, featureFlagsInLatestValidComposition: data.featureFlagsInLatestValidComposition, featureSubgraphs: data.featureSubgraphs, }; }, [data, graphsData]); - if (isLoading) { + if (isLoading || isLoadingGraphs) { render = <Loader fullscreen />; - } else if (error || data?.response?.code !== EnumStatusCode.OK) { + } else if (error || graphsError || data?.response?.code !== EnumStatusCode.OK) { render = ( <div className="my-auto"> <EmptyState icon={<ExclamationTriangleIcon />} title="Could not retrieve your federated graph" description={data?.response?.details || error?.message || 'Please try again'} actions={<Button onClick={() => refetch()}>Retry</Button>} /> </div> ); + } else if (!graphContextData) { + render = <Loader fullscreen />; } else { render = <GraphContext.Provider value={graphContextData}>{children}</GraphContext.Provider>; }Also applies to: 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/layout/graph-layout.tsx` around lines 63 - 75, The GraphContext provider is being given undefined when graphsData hasn't arrived which causes consumers like graph-visualization.tsx and operations-search.tsx to crash; update the logic around graphContextData and the render path that renders children (where children are provided the GraphContext, near the code that uses getFederatedGraphByName/getFederatedGraphs) so that you either (A) do not render the children until graphsData is defined (gate the render by checking graphsData before returning the provider/children), or (B) ensure graphContextData is always an object (provide safe defaults such as graphs: [] and empty feature flags/subgraphs) so GraphContext consumers never receive undefined; change the memoized graphContextData and the component render to implement one of these two fixes consistently.studio/src/components/layout/sidenav.tsx (1)
164-168:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the mobile menu toggle.
Line 164 renders an icon-only button without an accessible label, so assistive tech will announce just “button”. Please add at least
type="button"plusaria-label/aria-expanded.Proposed fix
<button + type="button" className="flex items-center space-x-2 lg:hidden" + aria-label={showMobileMenu ? 'Close navigation menu' : 'Open navigation menu'} + aria-expanded={showMobileMenu} onClick={() => setShowMobileMenu(!showMobileMenu)} > {showMobileMenu ? <Cross2Icon className="h-5 w-5" /> : <HamburgerMenuIcon className="h-5 w-5" />} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/layout/sidenav.tsx` around lines 164 - 168, The mobile menu toggle button (the element using showMobileMenu, setShowMobileMenu with Cross2Icon/HamburgerMenuIcon) is missing accessible attributes; update that button to include type="button", an appropriate aria-label (e.g., "Toggle mobile menu") and aria-expanded={showMobileMenu} so screen readers get state, and ensure the onClick handler remains setShowMobileMenu(!showMobileMenu).studio/src/components/checks/lint-issues-table.tsx (1)
67-68:⚠️ Potential issue | 🟡 MinorMove
TableCaptionbefore table sections for valid HTML structure.
<TableCaption>must be the first child of<Table>per the HTML Standard. Currently rendering it after<TableBody>at line 116 creates invalid table semantics and can degrade assistive technology behavior.Proposed fix
<TableWrapper> <Table> + {caption && <TableCaption>{caption}</TableCaption>} <TableHeader> <TableRow> <TableHead className="w-[380px]">Severity</TableHead> <TableHead>Message</TableHead> {lintIssues[0].subgraphName && <TableHead>Subgraph</TableHead>} <TableHead className="w-[5px]"></TableHead> </TableRow> </TableHeader> <TableBody> {lintIssues.map((l, i) => ( <TableRow key={l.severity + l.message} className="group"> ... </TableRow> ))} </TableBody> - {caption && <TableCaption>{caption}</TableCaption>} </Table> </TableWrapper>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/lint-issues-table.tsx` around lines 67 - 68, The TableCaption is rendered after table sections which violates HTML spec; move the <TableCaption> element to be the first child inside the <Table> in the lint-issues-table component so it precedes <TableHeader>, <TableBody>, etc.; update the render in the lint-issues-table.tsx (look for the JSX that returns <Table> with <TableHeader> and <TableBody>) to place <TableCaption> immediately inside <Table> (before TableHeader/TableBody) to restore valid table semantics and improve assistive tech behavior.studio/src/components/checks/operations.tsx (1)
352-357:⚠️ Potential issue | 🟠 MajorUse the debounced filter value for bulk mutations.
Line 133 loads the table with
debouncedSearch, but these bulk actions send the livesearchstate. If the user types and immediately clicks an override action, the mutation can apply to a different set of operations than the rows currently on screen.♻️ Proposed fix
- search: applyOnlyFiltered ? search : undefined, + search: applyOnlyFiltered ? debouncedSearch : undefined, @@ - search: applyOnlyFiltered ? search : undefined, + search: applyOnlyFiltered ? debouncedSearch : undefined,Also applies to: 373-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/operations.tsx` around lines 352 - 357, The bulk mutation uses the live search state instead of the debounced value, causing mismatches between visible rows and the mutation target; update calls to toggleGlobalChangeOverrides (and the other similar call later) to pass debouncedSearch when applyOnlyFiltered is true (i.e., replace search with debouncedSearch in the payload), ensuring the filter used for the mutation matches the table load. Also verify the same change is applied to the second invocation around the later block (the one similar to lines 373-378).studio/src/components/analytics/metrics.tsx (1)
208-214:⚠️ Potential issue | 🟠 MajorValidate
filterStateafter parsing instead of casting it.
JSON.parseaccepts{}and other valid-but-wrong shapes. Those values are cast to the array type here, and later code calls.map()/.find()on them, so a malformed URL can crash the page before the reset effect runs.🛡️ Proposed fix
const selectedFilters = useMemo(() => { try { - return JSON.parse(router.query.filterState?.toString() ?? '[]'); + const parsed = JSON.parse(router.query.filterState?.toString() ?? '[]'); + if (!Array.isArray(parsed)) return []; + return parsed.filter( + (filter): filter is { id: string; value: string[] } => + !!filter && + typeof filter.id === 'string' && + Array.isArray(filter.value) && + filter.value.every((value) => typeof value === 'string'), + ); } catch { return []; } }, [router.query.filterState]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/analytics/metrics.tsx` around lines 208 - 214, The parsed value from router.query.filterState inside the useMemo for selectedFilters must be validated as an array before returning it; update the selectedFilters computation (the useMemo that reads router.query.filterState) to try JSON.parse but then check Array.isArray(parsed) and that each element matches the expected shape (or at least is an object/string as needed) and return parsed only if valid, otherwise return []; ensure you reference selectedFilters and router.query.filterState so malformed `{}` or other non-array JSON won't be cast and cause .map()/.find() crashes.studio/src/components/dashboard/namespace-selector.tsx (1)
64-69:⚠️ Potential issue | 🟡 MinorMisleading comment: filter is reset when closing, not opening.
The comment on line 67 states "Only reset the filter when the popover is opened" but the condition
if (!v)executes whenvisfalse, which is when the popover is closing. The behavior (resetting filter on close) is sensible, but the comment is inverted.📝 Proposed fix
onOpenChange={(v) => { setOpen(v); if (!v) { - // Only reset the filter when the popover is opened + // Reset the filter when the popover is closed setFilter(''); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/namespace-selector.tsx` around lines 64 - 69, The comment in the onOpenChange handler for the popover is incorrect: it says "Only reset the filter when the popover is opened" but the condition if (!v) resets the filter when the popover is closed; update the comment to accurately reflect that behavior (e.g., "Reset the filter when the popover is closed") or adjust the logic if the intended behavior was the opposite; locate the onOpenChange callback where setOpen(v) and setFilter('') are used and correct the comment to match the actual behavior of setFilter being called on close.
♻️ Duplicate comments (4)
studio/src/components/create-graph.tsx (1)
76-78:⚠️ Potential issue | 🟠 MajorEncode the graph name before building the redirect path.
Line 77 explicitly allows
/inname, but Line 116 interpolatesdata.nameas a raw path segment. A valid name liketeam/apiwould redirect to/graph/team/api, which will misroute unless the graph page is intentionally a catch-all route.💡 Suggested fix
- router.replace(`/${user?.currentOrganization.slug}/${namespace}/graph/${data.name}`); + router.replace(`/${user?.currentOrganization.slug}/${namespace}/graph/${encodeURIComponent(data.name)}`);Confirm the route shape first: if the graph page is not a catch-all param, names containing
/will break the post-create redirect.#!/bin/bash set -euo pipefail echo "Redirect construction:" sed -n '110,118p' studio/src/components/create-graph.tsx echo echo "Dynamic route files mentioning graph:" fd '\[.*graph.*\]\.(ts|tsx)$' echo echo "Graph-related route/path references:" rg -n -C2 '/graph/|graphSlug|graphName|encodeURIComponent|decodeURIComponent' -g '*.ts' -g '*.tsx'Also applies to: 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/create-graph.tsx` around lines 76 - 78, The redirect currently interpolates raw data.name into the path (e.g. `/graph/${data.name}`) while the regex allows '/' in names; to fix, ensure you URL-encode the graph name when constructing redirects by wrapping the segment with encodeURIComponent (e.g. `/graph/${encodeURIComponent(data.name)}`) in the post-create redirect code that uses data.name; additionally, verify whether the graph route is a catch-all—if it is not, either disallow '/' in the name validation (the RegExp in create-graph.tsx) or keep encoding to avoid misrouting.studio/src/components/code-viewer.tsx (1)
118-125:⚠️ Potential issue | 🟡 MinorReplace
w-18with a valid Tailwind width utility.
w-18is not in Tailwind's default width scale, so the 4-digit gutter branch never applies a fixed width. Use an arbitrary value here or add18to the theme.♻️ Proposed fix
- ? 'w-18' + ? 'w-[4.5rem]'#!/bin/bash rg -n "w-18" studio/src/components/code-viewer.tsx fd 'tailwind\.config\.(js|cjs|mjs|ts)$' -x sh -c ' echo "=== $1 ===" rg -n "extend|spacing|width|18" "$1" ' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/code-viewer.tsx` around lines 118 - 125, The conditional for numberSectionWidth in code-viewer.tsx uses an invalid Tailwind class "w-18"; update the 4-digit gutter branch in the numberSectionWidth ternary (the expression referencing allLines and producing 'w-18') to use a valid utility—either replace 'w-18' with a supported class or an arbitrary value (e.g., w-[72px]) or add 18 to the Tailwind theme—so the numberSectionWidth calculation in the numberSectionWidth/allLines logic applies a real width class.studio/src/components/date-picker-with-range.tsx (1)
123-149:⚠️ Potential issue | 🟠 MajorClearing a time field still leaves the picker in preset mode.
getValue()turns''back into00:00/23:59, andhandleTimeChange()only clearsselectedRangewhen the input is non-empty. After choosing a quick range, deleting a time can still apply both the presetrangeand a customdateRange.🛠️ Proposed fix
const getValue = (): DateRange => { return { - start: getFromDate(selected.start, startTime === '' ? '00:00' : startTime), - end: selected?.end && getToDate(selected.end, endTime === '' ? '23:59' : endTime), + start: startTime === '' ? selected.start : getFromDate(selected.start, startTime), + end: selected?.end && (endTime === '' ? selected.end : getToDate(selected.end, endTime)), }; }; @@ const handleTimeChange = (field: 'start' | 'end'): React.ChangeEventHandler<HTMLInputElement> => (e) => { const time = e.target.value; setTime(field, time); - if (time) { - // if time is set, then we need to reset the range to 'custom' - setSelectedRange(undefined); - } + setSelectedRange(undefined); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/date-picker-with-range.tsx` around lines 123 - 149, getValue currently maps empty start/end time strings to '00:00'/'23:59' and handleTimeChange only clears selectedRange when time is non-empty, allowing a cleared time input to still apply a preset range; fix by (1) updating getValue() so that when startTime === '' or endTime === '' it returns undefined for that side (i.e., don't call getFromDate/getToDate with default times), and (2) update handleTimeChange(field) to setSelectedRange(undefined) whenever the time becomes empty (falsy) so clearing the input forces the picker into custom mode; refer to getValue, handleTimeChange, setTime, setSelectedRange and getFromDate/getToDate to locate the changes.studio/src/components/dashboard/workspace-provider.tsx (1)
91-104:⚠️ Potential issue | 🔴 CriticalCritical bug: Self-comparison makes namespace validation always fail.
Line 93 compares
ns.toLowerCase()to itself within the.some()callback due to variable shadowing. The callback parameternsshadows the outer function parameterns, causing the comparison to always betrue, which makes!namespaces.some(...)alwaysfalse. As a result,setNamespaceCallbackwill never update the namespace.🐛 Proposed fix
const setNamespaceCallback = useCallback( - (ns: string, applyRouteParams: boolean) => { - if (!ns || namespace === ns || !namespaces.some((ns) => ns.toLowerCase() === ns.toLowerCase())) { + (newNs: string, applyRouteParams: boolean) => { + if (!newNs || namespace === newNs || !namespaces.some((ns) => ns.toLowerCase() === newNs.toLowerCase())) { return; } - setNamespace(ns); - setStoredNamespace(ns); + setNamespace(newNs); + setStoredNamespace(newNs); if (applyRouteParams) { - applyParams({ namespace: ns }); + applyParams({ namespace: newNs }); } }, [namespace, namespaces, setStoredNamespace, applyParams], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/workspace-provider.tsx` around lines 91 - 104, The namespace validation in setNamespaceCallback is broken due to parameter shadowing: the .some() callback reuses the name ns and compares it to itself, making the check always true and preventing updates; fix it by renaming the .some() callback parameter (e.g., to candidate or existing) and compare candidate.toLowerCase() !== ns.toLowerCase() (or === as needed) so the condition becomes if (!ns || namespace === ns || !namespaces.some(candidate => candidate.toLowerCase() === ns.toLowerCase())) return; keep the subsequent calls to setNamespace, setStoredNamespace, and applyParams unchanged.
🧹 Nitpick comments (1)
studio/src/components/cache/cache-details-sheet.tsx (1)
18-18: Consider removingReact.FC<any>to preserve type safety.Using
React.FC<any>negates the type checking provided by the inline prop type. Theanyallows any props to be passed without TypeScript validation.♻️ Proposed fix
-export const CacheDetailsSheet: React.FC<any> = ({ operations }: { operations: CacheWarmerOperation[] }) => { +export const CacheDetailsSheet = ({ operations }: { operations: CacheWarmerOperation[] }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/cache/cache-details-sheet.tsx` at line 18, The component declaration uses React.FC<any>, which defeats type checking for props; change the signature of CacheDetailsSheet to use a concrete prop type instead of any — either declare an interface/type for the props (e.g., Props { operations: CacheWarmerOperation[] }) and use React.FC<Props> or type the function directly like const CacheDetailsSheet = ({ operations }: { operations: CacheWarmerOperation[] }) => ...; update the component signature to reference the CacheWarmerOperation[] prop type so TypeScript enforces prop shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/analytics/charts.tsx`:
- Line 89: Replace the misspelled CSS keyword "currenColor" with the correct
"currentColor" in the CartesianGrid props so the grid inherits the surrounding
text color; locate the CartesianGrid components (the color="currenColor" prop
occurrences, e.g., in the CartesianGrid elements referenced around the
startLine/endLine) and update both instances to color="currentColor" (and keep
the other props like strokeWidth, vertical, strokeDasharray unchanged).
In `@studio/src/components/cache/operations-table.tsx`:
- Around line 39-40: The parsed pagination values can be NaN and should be
normalized before use: update the parsing for pageNumber and limit (from
router.query.page and router.query.pageSize) to validate the parsed integers,
defaulting to 1 for pageNumber and 10 for limit, and clamp them to sensible
positive integers (e.g., Math.max(parsedPage || 1, 1) and Math.max(parsedLimit
|| 10, 1)). Ensure the rest of the component (references to noOfPages and the
Pagination component) uses these normalized values so malformed query params
won’t propagate NaN through calculations or rendering.
In `@studio/src/components/checks/operations.tsx`:
- Around line 116-117: The page and pageSize values parsed from router.query can
be NaN and should be validated before being used; update the logic that computes
pageNumber and limit (where parseInt / Number.parseInt is used) to parse with a
radix, check for Number.isNaN, and fall back to safe defaults (e.g., pageNumber
= Math.max(1, parsedPage || 1) and limit = Math.max(1, parsedPageSize || 10));
ensure downstream usages (offset calculation and Math.ceil for total pages and
the RPC input) use these validated integers so pagination state and RPC inputs
never receive NaN.
In `@studio/src/components/feature-flag-details.tsx`:
- Around line 101-102: The UI text in feature-flag-details.tsx contains a
duplicated sentence in the description string; locate the description used in
the FeatureFlagDetails component (the JSX/const rendering the "None of the
feature subgraphs..." message) and remove the repeated clause so only one clear
instruction remains (e.g., "Please publish the feature subgraphs using the
command below."). Ensure spacing and punctuation remain correct after deletion.
In `@studio/src/components/layout/sidenav.tsx`:
- Around line 50-55: The conditional currently uses user?.invitations?.length &&
... which will render a literal 0 when invitations is an empty array; replace
that with an explicit boolean check such as user?.invitations &&
user.invitations.length > 0 (or !!user?.invitations?.length) where the badge is
rendered (the Link element showing "Invitations" and the
user?.invitations?.length check) so the badge block only mounts when there is at
least one invitation.
---
Outside diff comments:
In `@studio/src/components/analytics/metrics.tsx`:
- Around line 208-214: The parsed value from router.query.filterState inside the
useMemo for selectedFilters must be validated as an array before returning it;
update the selectedFilters computation (the useMemo that reads
router.query.filterState) to try JSON.parse but then check Array.isArray(parsed)
and that each element matches the expected shape (or at least is an
object/string as needed) and return parsed only if valid, otherwise return [];
ensure you reference selectedFilters and router.query.filterState so malformed
`{}` or other non-array JSON won't be cast and cause .map()/.find() crashes.
In `@studio/src/components/check-extensions/check-extensions-config.tsx`:
- Around line 127-129: Fix the typo in the inline comment inside the if block
that checks isSecretKeyAssigned, forceShowSecretKeyInput, and
newConfig.secretKey; change "lets give ask them" to "let's ask them" so the
comment reads clearly before the call to setShowConfirmationDialog.
In `@studio/src/components/checks/lint-issues-table.tsx`:
- Around line 67-68: The TableCaption is rendered after table sections which
violates HTML spec; move the <TableCaption> element to be the first child inside
the <Table> in the lint-issues-table component so it precedes <TableHeader>,
<TableBody>, etc.; update the render in the lint-issues-table.tsx (look for the
JSX that returns <Table> with <TableHeader> and <TableBody>) to place
<TableCaption> immediately inside <Table> (before TableHeader/TableBody) to
restore valid table semantics and improve assistive tech behavior.
In `@studio/src/components/checks/operations.tsx`:
- Around line 352-357: The bulk mutation uses the live search state instead of
the debounced value, causing mismatches between visible rows and the mutation
target; update calls to toggleGlobalChangeOverrides (and the other similar call
later) to pass debouncedSearch when applyOnlyFiltered is true (i.e., replace
search with debouncedSearch in the payload), ensuring the filter used for the
mutation matches the table load. Also verify the same change is applied to the
second invocation around the later block (the one similar to lines 373-378).
In `@studio/src/components/dashboard/namespace-selector.tsx`:
- Around line 64-69: The comment in the onOpenChange handler for the popover is
incorrect: it says "Only reset the filter when the popover is opened" but the
condition if (!v) resets the filter when the popover is closed; update the
comment to accurately reflect that behavior (e.g., "Reset the filter when the
popover is closed") or adjust the logic if the intended behavior was the
opposite; locate the onOpenChange callback where setOpen(v) and setFilter('')
are used and correct the comment to match the actual behavior of setFilter being
called on close.
In `@studio/src/components/layout/graph-layout.tsx`:
- Around line 63-75: The GraphContext provider is being given undefined when
graphsData hasn't arrived which causes consumers like graph-visualization.tsx
and operations-search.tsx to crash; update the logic around graphContextData and
the render path that renders children (where children are provided the
GraphContext, near the code that uses
getFederatedGraphByName/getFederatedGraphs) so that you either (A) do not render
the children until graphsData is defined (gate the render by checking graphsData
before returning the provider/children), or (B) ensure graphContextData is
always an object (provide safe defaults such as graphs: [] and empty feature
flags/subgraphs) so GraphContext consumers never receive undefined; change the
memoized graphContextData and the component render to implement one of these two
fixes consistently.
In `@studio/src/components/layout/sidenav.tsx`:
- Around line 164-168: The mobile menu toggle button (the element using
showMobileMenu, setShowMobileMenu with Cross2Icon/HamburgerMenuIcon) is missing
accessible attributes; update that button to include type="button", an
appropriate aria-label (e.g., "Toggle mobile menu") and
aria-expanded={showMobileMenu} so screen readers get state, and ensure the
onClick handler remains setShowMobileMenu(!showMobileMenu).
---
Duplicate comments:
In `@studio/src/components/code-viewer.tsx`:
- Around line 118-125: The conditional for numberSectionWidth in code-viewer.tsx
uses an invalid Tailwind class "w-18"; update the 4-digit gutter branch in the
numberSectionWidth ternary (the expression referencing allLines and producing
'w-18') to use a valid utility—either replace 'w-18' with a supported class or
an arbitrary value (e.g., w-[72px]) or add 18 to the Tailwind theme—so the
numberSectionWidth calculation in the numberSectionWidth/allLines logic applies
a real width class.
In `@studio/src/components/create-graph.tsx`:
- Around line 76-78: The redirect currently interpolates raw data.name into the
path (e.g. `/graph/${data.name}`) while the regex allows '/' in names; to fix,
ensure you URL-encode the graph name when constructing redirects by wrapping the
segment with encodeURIComponent (e.g. `/graph/${encodeURIComponent(data.name)}`)
in the post-create redirect code that uses data.name; additionally, verify
whether the graph route is a catch-all—if it is not, either disallow '/' in the
name validation (the RegExp in create-graph.tsx) or keep encoding to avoid
misrouting.
In `@studio/src/components/dashboard/workspace-provider.tsx`:
- Around line 91-104: The namespace validation in setNamespaceCallback is broken
due to parameter shadowing: the .some() callback reuses the name ns and compares
it to itself, making the check always true and preventing updates; fix it by
renaming the .some() callback parameter (e.g., to candidate or existing) and
compare candidate.toLowerCase() !== ns.toLowerCase() (or === as needed) so the
condition becomes if (!ns || namespace === ns || !namespaces.some(candidate =>
candidate.toLowerCase() === ns.toLowerCase())) return; keep the subsequent calls
to setNamespace, setStoredNamespace, and applyParams unchanged.
In `@studio/src/components/date-picker-with-range.tsx`:
- Around line 123-149: getValue currently maps empty start/end time strings to
'00:00'/'23:59' and handleTimeChange only clears selectedRange when time is
non-empty, allowing a cleared time input to still apply a preset range; fix by
(1) updating getValue() so that when startTime === '' or endTime === '' it
returns undefined for that side (i.e., don't call getFromDate/getToDate with
default times), and (2) update handleTimeChange(field) to
setSelectedRange(undefined) whenever the time becomes empty (falsy) so clearing
the input forces the picker into custom mode; refer to getValue,
handleTimeChange, setTime, setSelectedRange and getFromDate/getToDate to locate
the changes.
---
Nitpick comments:
In `@studio/src/components/cache/cache-details-sheet.tsx`:
- Line 18: The component declaration uses React.FC<any>, which defeats type
checking for props; change the signature of CacheDetailsSheet to use a concrete
prop type instead of any — either declare an interface/type for the props (e.g.,
Props { operations: CacheWarmerOperation[] }) and use React.FC<Props> or type
the function directly like const CacheDetailsSheet = ({ operations }: {
operations: CacheWarmerOperation[] }) => ...; update the component signature to
reference the CacheWarmerOperation[] prop type so TypeScript enforces prop
shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7000617a-3531-427e-96cd-122fced2f93d
📒 Files selected for processing (74)
studio/src/components/analytics/barlist.tsxstudio/src/components/analytics/charts.tsxstudio/src/components/analytics/data-table-faceted-filter.tsxstudio/src/components/analytics/data-table-group-menu.tsxstudio/src/components/analytics/data-table-pagination.tsxstudio/src/components/analytics/data-table-primary-filter-menu.tsxstudio/src/components/analytics/data-table.tsxstudio/src/components/analytics/delta-badge.tsxstudio/src/components/analytics/field-usage.tsxstudio/src/components/analytics/filters.tsxstudio/src/components/analytics/getColumnData.tsxstudio/src/components/analytics/metrics.tsxstudio/src/components/analytics/refresh-interval.tsxstudio/src/components/analytics/toolbar.tsxstudio/src/components/analytics/trace.tsxstudio/src/components/analytics/use-apply-params.tsxstudio/src/components/analytics/use-range.tsxstudio/src/components/app-provider.tsxstudio/src/components/audit-log-table.tsxstudio/src/components/auth/auth-components.tsxstudio/src/components/cache/cache-details-sheet.tsxstudio/src/components/cache/cache-warmer-config.tsxstudio/src/components/cache/operations-table.tsxstudio/src/components/changelog/changelog.tsxstudio/src/components/changelog/changes.tsxstudio/src/components/check-badge-icon.tsxstudio/src/components/check-extensions/check-extensions-config.tsxstudio/src/components/checks/changes-table.tsxstudio/src/components/checks/checks-config.tsxstudio/src/components/checks/checks-filter-menu.tsxstudio/src/components/checks/composed-schema-changes-table.tsxstudio/src/components/checks/graph-pruning-issues-table.tsxstudio/src/components/checks/lint-issues-table.tsxstudio/src/components/checks/operation-content.tsxstudio/src/components/checks/operations.tsxstudio/src/components/checks/override.tsxstudio/src/components/checks/proposal-matches-table.tsxstudio/src/components/checks/selected-checks-filters.tsxstudio/src/components/checks/subgraph-check-extension.tsxstudio/src/components/clients/delete-persisted-operation-dialog.tsxstudio/src/components/code-viewer.tsxstudio/src/components/compose-status-bulb.tsxstudio/src/components/compose-status.tsxstudio/src/components/composition-errors-banner.tsxstudio/src/components/composition-errors-dialog.tsxstudio/src/components/create-graph.tsxstudio/src/components/dashboard/NewFeaturesPopup.tsxstudio/src/components/dashboard/graph-command-group.tsxstudio/src/components/dashboard/graph-selector.tsxstudio/src/components/dashboard/namespace-selector.tsxstudio/src/components/dashboard/workspace-command-wrapper.tsxstudio/src/components/dashboard/workspace-provider.tsxstudio/src/components/dashboard/workspace-selector.tsxstudio/src/components/date-picker-with-range.tsxstudio/src/components/date-range-picker.tsxstudio/src/components/empty-state.tsxstudio/src/components/error-fallback.tsxstudio/src/components/feature-flag-details.tsxstudio/src/components/feature-flags-table.tsxstudio/src/components/federatedgraphs-cards.tsxstudio/src/components/graph-visualization.tsxstudio/src/components/group-select.tsxstudio/src/components/info-tooltip.tsxstudio/src/components/layout/analytics/active-campaign-script.tsxstudio/src/components/layout/analytics/gtm-script.tsxstudio/src/components/layout/dashboard-layout.tsxstudio/src/components/layout/footer.tsxstudio/src/components/layout/graph-layout.tsxstudio/src/components/layout/head.tsxstudio/src/components/layout/layout.tsxstudio/src/components/layout/settings-layout.tsxstudio/src/components/layout/sidenav.tsxstudio/src/components/layout/subgraph-layout.tsxstudio/src/components/layout/title-layout.tsx
✅ Files skipped from review due to trivial changes (8)
- studio/src/components/auth/auth-components.tsx
- studio/src/components/analytics/data-table-group-menu.tsx
- studio/src/components/layout/layout.tsx
- studio/src/components/check-badge-icon.tsx
- studio/src/components/feature-flags-table.tsx
- studio/src/components/dashboard/NewFeaturesPopup.tsx
- studio/src/components/analytics/trace.tsx
- studio/src/components/compose-status.tsx
🚧 Files skipped from review as they are similar to previous changes (33)
- studio/src/components/layout/footer.tsx
- studio/src/components/checks/checks-filter-menu.tsx
- studio/src/components/audit-log-table.tsx
- studio/src/components/composition-errors-dialog.tsx
- studio/src/components/analytics/use-range.tsx
- studio/src/components/layout/head.tsx
- studio/src/components/checks/operation-content.tsx
- studio/src/components/checks/subgraph-check-extension.tsx
- studio/src/components/layout/dashboard-layout.tsx
- studio/src/components/analytics/toolbar.tsx
- studio/src/components/changelog/changelog.tsx
- studio/src/components/checks/composed-schema-changes-table.tsx
- studio/src/components/graph-visualization.tsx
- studio/src/components/empty-state.tsx
- studio/src/components/dashboard/graph-selector.tsx
- studio/src/components/changelog/changes.tsx
- studio/src/components/checks/proposal-matches-table.tsx
- studio/src/components/checks/override.tsx
- studio/src/components/layout/settings-layout.tsx
- studio/src/components/analytics/use-apply-params.tsx
- studio/src/components/checks/changes-table.tsx
- studio/src/components/analytics/barlist.tsx
- studio/src/components/analytics/data-table-faceted-filter.tsx
- studio/src/components/layout/analytics/active-campaign-script.tsx
- studio/src/components/analytics/getColumnData.tsx
- studio/src/components/analytics/data-table.tsx
- studio/src/components/error-fallback.tsx
- studio/src/components/analytics/data-table-pagination.tsx
- studio/src/components/analytics/field-usage.tsx
- studio/src/components/checks/checks-config.tsx
- studio/src/components/composition-errors-banner.tsx
- studio/src/components/dashboard/graph-command-group.tsx
- studio/src/components/checks/selected-checks-filters.tsx
Aenimus
reviewed
Mar 9, 2026
Contributor
Author
|
@Aenimus do you want to address rest of the coderabbit issues? I think I covered like 50% (unless I missed some, Github failed to show some of them for me for some reason) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/lint-policy/linter-config.tsx (1)
123-154:⚠️ Potential issue | 🟠 MajorUse one source of truth for the linter enabled state.
Line 124 flips
linterEnabledoptimistically, but Lines 164, 178, 219, 225, 227, and 286 still readdata.linterEnabled. If the mutation fails on Lines 145-152, the switch stays toggled while the rest of the panel remains in the old server state. Either drive the whole panel fromlinterEnabledand roll it back inonError, or only update the toggle after a successful response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/lint-policy/linter-config.tsx` around lines 123 - 154, The component currently updates UI state optimistically via setLinterEnabled(checked) while the rest of the panel still reads data.linterEnabled causing inconsistent UI on failure; pick one source of truth — I recommend removing the optimistic set and only update the toggle after a successful mutate response (inspect mutate's onSuccess and setLinterEnabled there using the returned state), and in onError ensure you call refetch() and/or leave the toggle unchanged; alternatively, if you prefer optimistic updates, drive all reads in the panel from the local linterEnabled state (replace usages of data.linterEnabled with linterEnabled) and in mutate.onError roll back by calling setLinterEnabled(previousValue) and refetch(). Ensure you reference setLinterEnabled, mutate, onSuccess, onError, refetch, and data.linterEnabled to locate and adjust the code paths accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/analytics/charts.tsx`:
- Line 89: Replace the incorrect SVG attribute on the Recharts grid components:
find the two CartesianGrid usages in analytics/charts.tsx (the CartesianGrid JSX
elements) and change the prop from color="currentColor" to stroke="currentColor"
(and keep strokeWidth, vertical, strokeDasharray as-is) so grid lines are styled
via the stroke property instead of the color attribute.
In `@studio/src/components/lint-policy/linter-config.tsx`:
- Around line 232-233: The badge currently displays counts from countByCategory
(derived from data.configs) which goes stale; instead derive the per-category
counts from the local selection (selectedLintRules) so UI updates immediately.
Replace usage of countByCategory in the Badge with a computed value derived from
selectedLintRules (e.g., build a map by reducing selectedLintRules grouping by
category/index and counting entries) and render `${derivedCount[index] || 0} of
${lintCategory.rules.length}`; ensure the reducer/derived map is recomputed
whenever selectedLintRules changes (memoize with useMemo or recompute inside the
component).
---
Outside diff comments:
In `@studio/src/components/lint-policy/linter-config.tsx`:
- Around line 123-154: The component currently updates UI state optimistically
via setLinterEnabled(checked) while the rest of the panel still reads
data.linterEnabled causing inconsistent UI on failure; pick one source of truth
— I recommend removing the optimistic set and only update the toggle after a
successful mutate response (inspect mutate's onSuccess and setLinterEnabled
there using the returned state), and in onError ensure you call refetch() and/or
leave the toggle unchanged; alternatively, if you prefer optimistic updates,
drive all reads in the panel from the local linterEnabled state (replace usages
of data.linterEnabled with linterEnabled) and in mutate.onError roll back by
calling setLinterEnabled(previousValue) and refetch(). Ensure you reference
setLinterEnabled, mutate, onSuccess, onError, refetch, and data.linterEnabled to
locate and adjust the code paths accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87d9cb07-8500-436e-9695-5fa66bd5352d
📒 Files selected for processing (4)
studio/src/components/analytics/charts.tsxstudio/src/components/feature-flag-details.tsxstudio/src/components/layout/sidenav.tsxstudio/src/components/lint-policy/linter-config.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- studio/src/components/feature-flag-details.tsx
- studio/src/components/layout/sidenav.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/dashboard/workspace-provider.tsx`:
- Around line 69-79: The current useMemo for currentNamespace does an exact
match (wns.name === namespace) and thus misses valid namespaces that differ only
by case; update the lookup in the useMemo (the currentNamespace computation that
inspects data?.namespaces) to perform a case-insensitive comparison (e.g.,
compare wns.name.toLowerCase() === namespace.toLowerCase() or use a
case-insensitive comparator), preserving the existing fallback to new
WorkspaceNamespace({ id:'', name: DEFAULT_NAMESPACE_NAME, graphs: [] }) and
keeping the same dependencies ([isLoading, data?.namespaces, namespace]); ensure
you also guard against undefined names before calling toLowerCase().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48b762fc-beaf-46f1-aaae-e6bfe4ff7576
📒 Files selected for processing (1)
studio/src/components/dashboard/workspace-provider.tsx
…-drift-enforce-formatting-in-ci-pt3
JivusAyrus
approved these changes
Mar 10, 2026
Aenimus
deleted the
ondrej/eng-9087-cosmostudioci-fix-formatting-drift-enforce-formatting-in-ci-pt3
branch
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
New Features / UX
Bug Fixes
Checklist
Stack:
Fixes formatting drift in studio just for *.tsx. There will be follow up PR(s) to fix rest