Metrics dashboards #3019
Metrics dashboards #3019
Conversation
|
|
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:
WalkthroughAdds a metrics & dashboards system across backend, database, and frontend: new MetricsDashboard Prisma model and migrations; server presenters and services for dashboard loading and query execution (including default-period and time-range handling); TSQL compiler/time-bucketing support (TimeRange, timeBucket(), calculateTimeBucketInterval) with tests; many new and updated Remix routes and resource actions for dashboards and metric widgets; a comprehensive QueryEditor and QueryWidget/MetricWidget UI with BigNumberCard; multiple new React components, hooks, filters, dialogs, and reorderable list support; side‑menu preference/storage changes; path helpers, CSS/tailwind updates, and added react-grid-layout/react-resizable deps. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/webapp/app/components/code/AIQueryInput.tsx (1)
78-172:⚠️ Potential issue | 🟠 MajorStale closure:
processStreamEventis not insubmitQuery's dependency array.
submitQuery(line 78) callsprocessStreamEvent(lines 143, 154), butprocessStreamEventis not listed insubmitQuery'suseCallbackdeps (line 171). IfonQueryGeneratedoronTimeFilterChangechange between renders, the version ofprocessStreamEventcaptured bysubmitQuerywill be stale, potentially calling outdated callbacks.Suggested fix — add to deps or use a ref
Simplest fix: add
processStreamEventto the dependency array. However, sinceprocessStreamEventdepends ononQueryGenerated/onTimeFilterChange, this will causesubmitQueryto be recreated more often. A more robust pattern is to store the event callbacks in refs so neitheruseCallbackneeds to re-create.Minimal fix:
}, - [isLoading, resourcePath, mode, getCurrentQuery] + [isLoading, resourcePath, mode, getCurrentQuery, processStreamEvent] );Note: this introduces a circular concern since
submitQuerywould need to be defined afterprocessStreamEvent. You'd need to reorder the hooks (moveprocessStreamEventabovesubmitQuery) or use a ref-based approach to break the cycle.Also applies to: 174-205
internal-packages/tsql/src/query/validator.ts (1)
233-243:⚠️ Potential issue | 🟡 MinorInconsistent casing: implicit aliases are not lowercased before insertion.
Explicit aliases are stored lowercased (line 234), and lookups use
.toLowerCase()(line 443). However, implicit names fromgetImplicitName(line 239) are added as-is. For example,NULLfrom a constant expression would be stored verbatim but looked up as"null", causing a false "unknown column" error if someone references it in ORDER BY.Proposed fix
const implicitName = getImplicitName(expr); if (implicitName) { - context.selectAliases.add(implicitName); + context.selectAliases.add(implicitName.toLowerCase()); }apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
93-107:⚠️ Potential issue | 🟡 MinorInconsistent response shapes across error branches.
The success response (Line 214) includes
queryIdandmaxQueryPeriod, and the query-error/catch branches (Lines 197, 229) includequeryId: nullbut omitmaxQueryPeriod. The early-exit error responses (Lines 93, 111, 129, 156) omit bothqueryIdandmaxQueryPeriodentirely.This means the consuming
QueryEditorfetcher must handle varying response shapes. Consider normalizing all branches to return the same set of keys (withnulldefaults) to avoid runtime surprises.🔧 Example: add missing fields to early error responses
return typedjson( { error: "Unauthorized", rows: null, columns: null, stats: null, hiddenColumns: null, reachedMaxRows: null, explainOutput: null, generatedSql: null, periodClipped: null, + queryId: null, + maxQueryPeriod: null, }, { status: 403 } );Apply the same to the 404 and 400-validation branches.
Also applies to: 196-244
apps/webapp/app/components/navigation/SideMenu.tsx (1)
155-206:⚠️ Potential issue | 🟡 MinorRapid section toggles can lose preference updates due to single-slot pending state.
The
pendingPreferencesRefstores only onesectionId/sectionCollapsedpair at a time. If a user toggles two different sections within the 500ms debounce window, the first toggle is overwritten by the spread merge on Line 177-180:pendingPreferencesRef.current = { ...pendingPreferencesRef.current, ...data, // overwrites previous sectionId/sectionCollapsed };For example: toggling "metrics" collapsed → then "manage" collapsed within 500ms means only the "manage" change is persisted.
While unlikely in normal usage, consider storing pending section changes as a map (e.g.,
pendingSections: Record<SideMenuSectionId, boolean>) so multiple section toggles within the debounce window are all captured.🔧 Sketch of a fix
const pendingPreferencesRef = useRef<{ isCollapsed?: boolean; - sectionId?: SideMenuSectionId; - sectionCollapsed?: boolean; + sections?: Record<string, boolean>; }>({});Then in the submit callback, iterate over
pending.sectionsand append each as form data (or serialize as JSON). The server endpoint would need to handle multiple section updates accordingly.
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/navigation/SideMenuSection.tsx`:
- Around line 49-62: The headerAction element inside SideMenuSection is
triggering the section toggle because clicks bubble to the parent onClick
(controlled by handleToggle and isSideMenuCollapsed); update the headerAction
container (the div rendering headerAction) to stop event propagation on clicks
(e.g., call event.stopPropagation() in an onClick handler) so interacting with
headerAction does not invoke handleToggle, while preserving existing conditional
onClick behavior on the section wrapper.
In `@apps/webapp/app/components/primitives/charts/Card.tsx`:
- Around line 26-30: The Header3 element is always given the "drag-handle" class
which makes non-draggable cards appear as drag handles; update the className
expression on Header3 (where cn(...) is used) to include "drag-handle" only when
the draggable prop is true (i.e., conditionally append "drag-handle" alongside
the existing draggable && "cursor-grab active:cursor-grabbing" logic) so the
drag handle class is added only for draggable cards.
In `@apps/webapp/app/hooks/useInterval.ts`:
- Around line 51-55: The useEffect in useInterval currently always invokes
callback on mount because it ignores the destructured onLoad option; update the
effect in the useInterval hook (the block that runs "On load") to check both
disabled and onLoad before calling callback (e.g., return early if disabled ||
!onLoad) and include the relevant dependencies (callback, disabled, onLoad) in
the dependency array so the behavior respects the onLoad flag and stays stable.
- Around line 20-49: The effects are capturing a stale callback because
`callback` is not in any dependency arrays; fix by storing the latest `callback`
in a ref (e.g., `const latestCallback = useRef(callback); useEffect(() => {
latestCallback.current = callback; }, [callback]);`) then use
`latestCallback.current()` inside the setInterval and inside `handleFocus`
instead of `callback`; keep the existing dependency arrays for `interval`,
`disabled`, and `onFocus` so the timers/listeners are recreated only when their
controls change while always invoking the latest callback.
In `@apps/webapp/app/models/runtimeEnvironment.server.ts`:
- Around line 312-333: The hasAccessToEnvironment function currently only checks
org membership and must be changed to mirror the OR condition used in
findEnvironmentBySlug so DEVELOPMENT environments are accessible only to their
owner; update the runtimeEnvironment.findFirst where clause in
hasAccessToEnvironment to use an OR array that allows access if either (1) type
is 'DEVELOPMENT' and ownerUserId (or owner: { userId } if that's the relation)
equals the requesting userId, or (2) the organization has a member with that
userId (organization: { members: { some: { userId } } }); ensure you reference
the same fields used in findEnvironmentBySlug (type, ownerUserId/owner and
organization.members) so dev envs are restricted to the owning user while other
types still allow org members.
In `@apps/webapp/app/routes/resources.metric.tsx`:
- Around line 188-202: The catch currently swallows non-abort errors and only
clears loading, leaving stale/empty UI; update the fetch error path (the promise
chain that calls res.json() and the .catch handling controller.signal) to, when
not an AbortError and controller.signal is not aborted, set an error response
via setResponse (e.g. a MetricWidgetActionResponse representing failure or
containing err.message) and then setIsLoading(false) so the widget can render an
error state instead of stale data; ensure you handle JSON parse/network errors
the same way as non-OK responses.
In
`@apps/webapp/app/routes/resources.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:
- Around line 256-261: The current read-modify-write that calls
prisma.metricsDashboard.update with layout: JSON.stringify(updatedLayout) can
lose concurrent edits; change this to either (A) perform the read and write
inside a Prisma transaction with serializable isolation (use
prisma.$transaction([...], { isolationLevel: 'Serializable' }) and fetch the
dashboard, apply the layout change, then update within that transaction on the
same id) or (B) add an optimistic-concurrency check by adding a version or
updatedAt column to the metricsDashboard model, read the row first (including
version/updatedAt), compute updatedLayout, and then when calling
prisma.metricsDashboard.update include a where clause that matches the current
version/updatedAt and increment/update it in the same update; if the update
affects zero rows, surface a conflict error to the caller so the client can
retry/merge.
- Around line 126-131: The dashboard lookup using
prisma.metricsDashboard.findFirst only filters by friendlyId and organizationId,
which allows dashboards from other projects in the same organization to be
returned; update the where clause to also require projectId: project.id so the
query only returns dashboards scoped to the current project (adjust the
prisma.metricsDashboard.findFirst call that uses friendlyId, organizationId, and
add projectId referencing project.id).
In `@apps/webapp/app/routes/resources.preferences.sidemenu.tsx`:
- Line 42: The JSON.parse on result.data.itemOrder is unprotected and can throw,
so change the parsing to handle malformed input: either wrap
JSON.parse(result.data.itemOrder) in a try/catch and fall back to an empty array
(or appropriate default) before passing to z.array(z.string()).safeParse, or
replace the manual parse with a zod transform (e.g., z.string().transform(s => {
try { return JSON.parse(s) } catch { return [] } }) used in place of the raw
input) so that the code that computes orderResult (the
z.array(z.string()).safeParse invocation) never receives a thrown error from
parsing.
In
`@internal-packages/database/prisma/migrations/20260201130503_metrics_dashboard_table_created/migration.sql`:
- Line 8: The ownerId column in the metrics_dashboard table is declared NOT NULL
but the foreign key uses ON DELETE SET NULL which will cause runtime errors; fix
by either making the ownerId column nullable (change "ownerId" TEXT NOT NULL to
"ownerId" TEXT) or by changing the foreign key action (the FK on
metrics_dashboard referencing users(id)) to a non-nulling behavior such as ON
DELETE CASCADE or ON DELETE RESTRICT so deletions don't attempt to set ownerId
to NULL; update the migration SQL for the metrics_dashboard table (and the
related FK statements referenced on the same migration lines) to use the chosen
option consistently.
In
`@internal-packages/database/prisma/migrations/20260202100000_add_friendlyid_to_metrics_dashboard/migration.sql`:
- Line 2: The ALTER TABLE adds a NOT NULL "friendlyId" to "MetricsDashboard"
which will fail if rows already exist; modify the migration to first add the
column as nullable (or with a temporary DEFAULT), backfill existing rows with
appropriate values for "friendlyId" (e.g., generate or copy a unique value), and
only then alter the column to SET NOT NULL (and remove the temporary DEFAULT if
used); target the migration file that adds "friendlyId" in order to implement
the three-step add/backfill/lock-down change.
In `@internal-packages/database/prisma/schema.prisma`:
- Around line 2508-2510: The relation for owner uses onDelete: SetNull but
ownerId is non-nullable (ownerId String), which will cause DB errors; fix by
either making ownerId optional (change ownerId to String? and update any code
that assumes presence) so SetNull can apply, or change the relation delete
behavior to onDelete: Cascade (or Restrict) to keep ownerId non-nullable—update
the owner relation and ownerId field in the schema and any related model logic
(references: owner relation, ownerId field).
🟡 Minor comments (18)
apps/webapp/app/tailwind.css-155-161 (1)
155-161:⚠️ Potential issue | 🟡 MinorDuplicate
[data-code-block-container]selector with conflicting values.
& [data-code-block-container]is declared at both Line 156 and Line 220 within.streamdown-container. The second rule (Line 220–222) setsmy-2 border-charcoal-700, which overridesmy-0andborder-charcoal-650from the first rule (Line 156–158), making those values dead code. This looks unintentional — please consolidate into a single rule with the intended values.Proposed consolidation (pick the intended values)
/* Code block styling */ & [data-code-block-container] { - `@apply` rounded border-charcoal-650 my-0; + `@apply` rounded my-2 border-charcoal-700; } & [data-code-block] { border-top: none; }And remove the duplicate block further down:
/* Streamdown code block container */ - & [data-code-block-container] { - `@apply` my-2 border-charcoal-700; - }Also applies to: 219-222
apps/webapp/app/hooks/useRevalidateOnParam.ts-36-56 (1)
36-56:⚠️ Potential issue | 🟡 MinorUnstable
paramArrayreference causes the effect to re-run on every render.
paramArrayis a new array on each render, so React's referential equality check in the dependency array always sees it as "changed." While thehasParamguard prevents side-effects when no trigger param is present, this is still unnecessary work and could cause double-fires during the render cycle when params are present.Memoize the derived array or depend on the stable
paramprop instead.Proposed fix
+import { useEffect, useMemo } from "react"; -import { useEffect } from "react"; import { useRevalidator, useSearchParams } from "@remix-run/react"; ... export function useRevalidateOnParam({ param, onRevalidate }: UseRevalidateOnParamOptions) { const [searchParams, setSearchParams] = useSearchParams(); const revalidator = useRevalidator(); - const paramArray = Array.isArray(param) ? param : [param]; + const paramArray = useMemo( + () => (Array.isArray(param) ? param : [param]), + [param] + ); useEffect(() => { ... }, [searchParams, setSearchParams, revalidator, paramArray, onRevalidate]); }Note: if
paramis always passed as an inline array literal (e.g.{ param: ["a","b"] }), evenuseMemokeyed onparamwon't help because the outer array is also a fresh reference. In that case, considerJSON.stringify/custom comparison or document that callers must stabilize the value.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx-390-391 (1)
390-391:⚠️ Potential issue | 🟡 MinorDuplicating a title widget incorrectly counts against the widget limit.
In the
addcase (Lines 211-214), title widgets are explicitly excluded from the limit check. However, in theduplicatecase,checkWidgetLimit()is called unconditionally before the widget type is known. Move the limit check after resolving the original widget and skip it for title widgets, consistent withadd.🐛 Proposed fix
case "duplicate": { - await checkWidgetLimit(); - const rawData = { widgetId: formData.get("widgetId"), newId: formData.get("newId"), }; const result = DuplicateWidgetSchema.safeParse(rawData); if (!result.success) { throw new Response("Invalid form data: " + result.error.message, { status: 400 }); } const { widgetId } = result.data; // Find the original widget const originalWidget = existingLayout.widgets[widgetId]; if (!originalWidget) { throw new Response("Widget not found", { status: 404 }); } + // Title widgets don't count against the limit + if (originalWidget.display.type !== "title") { + await checkWidgetLimit(); + } + // Find the original layout itemapps/webapp/app/components/metrics/QueuesFilter.tsx-105-120 (1)
105-120:⚠️ Potential issue | 🟡 MinorUse the debounced callback parameter
sinstead of the closure-capturedsearchValue.Line 110 checks
searchValue(the current render's value) while usings(the debounced value) for the query param. In a fast-typing scenario these can differ, leading to a mismatch where the guard passes but the wrong value was already used, or vice versa.Suggested fix
useDebounceEffect( searchValue, (s) => { const searchParams = new URLSearchParams(); searchParams.set("per_page", "25"); - if (searchValue) { + if (s) { searchParams.set("query", s); } fetcher.load(apps/webapp/app/components/primitives/charts/BigNumberCard.tsx-62-80 (1)
62-80:⚠️ Potential issue | 🟡 Minor
Math.min(...values)/Math.max(...values)can blow the call stack on large arrays.If
valueshas more than ~100K elements, the spread intoMath.min/Math.maxwill exceed the maximum call stack size. Consider using areduce-based approach for safety.Suggested fix
case "min": - return Math.min(...values); + return values.reduce((a, b) => Math.min(a, b), values[0]); case "max": - return Math.max(...values); + return values.reduce((a, b) => Math.max(a, b), values[0]);apps/webapp/app/components/navigation/SideMenuItem.tsx-85-105 (1)
85-105:⚠️ Potential issue | 🟡 MinorClick on the action overlay may propagate to the underlying
<Link>.The action
div(Line 99) is absolutely positioned over the link. If theactionReactNode doesn't calle.stopPropagation()/e.preventDefault(), clicks will bubble to the<Link>and trigger navigation. Consider addingonClick={e => e.preventDefault()}on the action wrapper, or document this expectation for consumers.Suggested fix on the wrapper
- <div className="absolute top-1 right-1 bottom-1 flex aspect-square items-center justify-center rounded group-hover/menuitem:bg-charcoal-750"> + <div + className="absolute top-1 right-1 bottom-1 flex aspect-square items-center justify-center rounded group-hover/menuitem:bg-charcoal-750" + onClick={(e) => e.preventDefault()} + > {action} </div>apps/webapp/app/hooks/useDashboardEditor.ts-337-363 (1)
337-363:⚠️ Potential issue | 🟡 MinorPotential race on widget limit under rapid interaction.
countedWidgetsis captured at render time. IfaddWidgetorduplicateWidgetis called multiple times before React re-renders, the limit check on line 340 will use the same stale count, potentially allowing over-limit additions. Since the server should also enforce the limit, this is low-risk but worth noting.apps/webapp/app/presenters/v3/MetricDashboardPresenter.server.ts-103-111 (1)
103-111:⚠️ Potential issue | 🟡 MinorUnguarded
JSON.parseon database-sourced string.If
layoutDatacontains malformed JSON (e.g., due to a bad migration or manual DB edit),JSON.parseon line 104 will throw aSyntaxErrorthat isn't caught. This would result in a 500 for the user rather than a clear error.🛡️ Proposed fix
`#getLayout`(layoutData: string): DashboardLayout { - const json = JSON.parse(layoutData); + let json: unknown; + try { + json = JSON.parse(layoutData); + } catch { + throw new Error("Dashboard layout contains invalid JSON"); + } const parsedLayout = DashboardLayout.safeParse(json); if (!parsedLayout.success) { throw fromZodError(parsedLayout.error); } return parsedLayout.data; }apps/webapp/app/hooks/useDashboardEditor.ts-263-266 (1)
263-266:⚠️ Potential issue | 🟡 MinorStatic analysis: arrow implicitly returns from
forEachcallback.Biome flags this because the arrow shorthand
=> formData.set(k, v)implicitly returns the result. Use braces to make it a statement body:🔧 Proposed fix
- Object.entries(task.data).forEach(([k, v]) => formData.set(k, v)); + Object.entries(task.data).forEach(([k, v]) => { formData.set(k, v); });apps/webapp/app/presenters/v3/BuiltInDashboards.server.ts-4-86 (1)
4-86:⚠️ Potential issue | 🟡 MinorAll three widgets have identical titles and queries; widgets
aandcare fully identical.Widgets
aandcshare the same title ("Runs by status"), the same query, and the same display config (table). This looks like copy-paste placeholder content. If this is intentional for the draft, consider differentiating titles and queries to provide a more useful default dashboard (e.g., different metrics like run counts, durations, or costs).apps/webapp/app/components/navigation/DashboardList.tsx-72-119 (1)
72-119:⚠️ Potential issue | 🟡 MinorInconsistent icon and color choices between reorderable and static rendering paths.
When
isCollapsedis true:
- Reorderable path uses
IconChartHistogram(Line 80) with color"text-customDashboards"(Lines 85-86)- Static path uses
LineChartIcon(Line 108) with color"text-customDashboards"(Lines 113-114)When
isCollapsedis false (tree connector icons):
- Reorderable path passes
undefinedfor icon colors (Lines 85-86)- Static path passes
"text-charcoal-700"(Lines 113-114)The collapsed-state icon mismatch (
IconChartHistogramvsLineChartIcon) and the non-collapsed color difference look unintentional. If these are deliberate, consider adding a comment explaining the distinction.apps/webapp/app/routes/resources.metric.tsx-85-92 (1)
85-92:⚠️ Potential issue | 🟡 MinorAuthorization failure returns HTTP 400 instead of 403.
When
hasAccessToEnvironmentreturns false, the response should use403 Forbiddenrather than400 Bad Requestto correctly convey the semantics.Suggested fix
if (!hasAccess) { return json( { success: false as const, error: "You don't have permission for this resource", }, - { status: 400 } + { status: 403 } ); }apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx-145-145 (1)
145-145:⚠️ Potential issue | 🟡 MinorUnsafe cast of
scopesearch param toQueryScope.
value("scope") as QueryScopebypasses runtime validation. If someone manually edits the URL with an invalid scope value, it'll be passed through as-is to downstream queries. Consider validating against the expected union, e.g. via a Zod parse or a simple allowlist check.Suggested fix
- const scope = (value("scope") as QueryScope) ?? "environment"; + const rawScope = value("scope"); + const scope: QueryScope = + rawScope === "organization" || rawScope === "project" || rawScope === "environment" + ? rawScope + : "environment";apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx-496-496 (1)
496-496:⚠️ Potential issue | 🟡 MinorPotential division by zero if
widgetLimits.limitis 0.
(widgetLimits.used / widgetLimits.limit) * 62.8and the tooltip percentage calculation at line 503 will produceInfinityorNaNifwidgetLimitPerDashboardis 0. While unlikely in practice, a guard would be defensive:Suggested fix
- strokeDasharray={`${(widgetLimits.used / widgetLimits.limit) * 62.8} 62.8`} + strokeDasharray={`${widgetLimits.limit > 0 ? (widgetLimits.used / widgetLimits.limit) * 62.8 : 0} 62.8`}apps/webapp/app/components/query/QueryEditor.tsx-220-222 (1)
220-222:⚠️ Potential issue | 🟡 MinorRegex for
triggered_atdetection may false-positive on non-WHERE usage.The regex
/\bWHERE\b[\s\S]*\btriggered_at\b/imatchestriggered_atanywhere afterWHERE, including inGROUP BY,ORDER BY, or subquery contexts. For example,WHERE status = 'ok' GROUP BY triggered_atwould incorrectly disable the time filter. This is a UX hint so the impact is low, but worth noting.apps/webapp/app/components/query/QueryEditor.tsx-1116-1131 (1)
1116-1131:⚠️ Potential issue | 🟡 Minor
{periodClipped && (...)}can render0as text ifperiodClippedis0.In React,
{0 && <JSX>}renders the string"0"rather than nothing. While aperiodClippedvalue of0is unlikely, the prop typenumber | nullallows it. Use a boolean check to be safe.Proposed fix
- {periodClipped && ( + {periodClipped != null && periodClipped > 0 && (apps/webapp/app/components/query/QueryEditor.tsx-1024-1048 (1)
1024-1048:⚠️ Potential issue | 🟡 MinorNo error handling on
navigator.clipboard.writeText.
navigator.clipboard.writeTextreturns a Promise and can reject (permissions, insecure context, etc.). The call is fire-and-forget here, which will produce an unhandled rejection. Consider adding a.catch()or wrapping with try/catch and showing a toast on failure.Proposed fix (example for one handler)
const handleCopyCSV = () => { const csv = rowsToCSV(rows, columns); - navigator.clipboard.writeText(csv); + navigator.clipboard.writeText(csv).catch(() => { + // Silently fail or show a notification + }); setIsOpen(false); };Apply similarly to
handleCopyJSON.apps/webapp/app/components/metrics/QueryWidget.tsx-349-373 (1)
349-373:⚠️ Potential issue | 🟡 Minor
isLoadingnot passed toQueryResultsChartin normal view, only in fullscreen.In the
"chart"case, the non-fullscreenQueryResultsChart(line 352) doesn't receiveisLoading, while the fullscreen variant (line 367) does. This means the normal chart view won't reflect the loading state. This looks like an oversight — the normal view should also receiveisLoading.Proposed fix
<QueryResultsChart rows={data.rows} columns={data.columns} config={config} onViewAllLegendItems={() => setIsFullscreen(true)} + isLoading={isLoading} />
🧹 Nitpick comments (38)
apps/webapp/app/components/AlphaBadge.tsx (1)
34-61: Consider extracting a shared component to reduce duplication.
BetaBadgeandBetaTitleare nearly identical toAlphaBadgeandAlphaTitle, differing only in the label text and tooltip string. A single parameterized component would eliminate the copy-paste.♻️ Example: shared StageBadge / StageTitle
+type Stage = "Alpha" | "Beta"; + +function StageBadge({ + stage, + inline = false, + className, +}: { + stage: Stage; + inline?: boolean; + className?: string; +}) { + return ( + <SimpleTooltip + button={ + <Badge variant="extra-small" className={cn(inline ? "inline-grid" : "", className)}> + {stage} + </Badge> + } + content={`This feature is in ${stage}.`} + disableHoverableContent + /> + ); +} + +export function AlphaBadge(props: Omit<Parameters<typeof StageBadge>[0], "stage">) { + return <StageBadge stage="Alpha" {...props} />; +} + +export function BetaBadge(props: Omit<Parameters<typeof StageBadge>[0], "stage">) { + return <StageBadge stage="Beta" {...props} />; +} + +function StageTitle({ stage, children }: { stage: Stage; children: React.ReactNode }) { + return ( + <> + <span>{children}</span> + <StageBadge stage={stage} /> + </> + ); +} + +export function AlphaTitle({ children }: { children: React.ReactNode }) { + return <StageTitle stage="Alpha">{children}</StageTitle>; +} + +export function BetaTitle({ children }: { children: React.ReactNode }) { + return <StageTitle stage="Beta">{children}</StageTitle>; +}apps/webapp/app/components/primitives/ClientTabs.tsx (1)
191-195:data-[state=inactive]:hiddenplaced afterclassNameprevents consumer overrides.Placing the
hiddenutility afterclassNameincn()means a consumer cannot override the inactive-hidden behavior via theclassNameprop (Tailwind merge will favor the last conflicting utility). If that's intentional — i.e., inactive content should always be hidden — this is fine. If consumers might need to keep inactive content visible (e.g., for animations or measuring layout), consider swapping the order soclassNamewins.apps/webapp/app/components/code/AIQueryInput.tsx (2)
32-42: Usetypeinstead ofinterfaceforAIQueryInputProps.Per coding guidelines, prefer
typeoverinterfacefor TypeScript definitions.Suggested fix
-interface AIQueryInputProps { +type AIQueryInputProps = { onQueryGenerated: (query: string) => void; /** Called when the AI sets a time filter - updates URL search params */ onTimeFilterChange?: (filter: AITimeFilter) => void; /** Set this to a prompt to auto-populate and immediately submit */ autoSubmitPrompt?: string; /** Change this to force re-submission even if prompt is the same */ autoSubmitKey?: number; /** Get the current query in the editor (used for edit mode) */ getCurrentQuery?: () => string; -} +};As per coding guidelines,
**/*.{ts,tsx}: Use types over interfaces for TypeScript.
140-147: Silently swallowed JSON parse errors may hide protocol issues.Both SSE parsing blocks (lines 144 and 156) have empty
catchblocks. While this is understandable for robustness against partial/malformed chunks, consider logging a warning in development to aid debugging when the stream format changes unexpectedly.Also applies to: 152-158
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx (3)
22-41: Extract the duplicated config JSON transform into a shared helper.The
configfield transform logic (JSON parse →QueryWidgetConfig.safeParse→ error handling) is duplicated verbatim betweenAddWidgetSchemaandUpdateWidgetSchema. Extract it once and reuse it.♻️ Proposed refactor
+const widgetConfigTransform = z.string().transform((str, ctx) => { + try { + const parsed = JSON.parse(str); + const result = QueryWidgetConfig.safeParse(parsed); + if (!result.success) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "Invalid widget config", + }); + return z.NEVER; + } + return result.data; + } catch { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "Invalid JSON", + }); + return z.NEVER; + } +}); + const AddWidgetSchema = z.object({ widgetId: z.string().min(1, "Widget ID is required").optional(), title: z.string().min(1, "Title is required"), query: z.string().default(""), - config: z.string().transform((str, ctx) => { - try { - ... - } - }), + config: widgetConfigTransform, }); const UpdateWidgetSchema = z.object({ widgetId: z.string().min(1, "Widget ID is required"), title: z.string().min(1, "Title is required"), query: z.string().min(1, "Query is required"), - config: z.string().transform((str, ctx) => { - try { - ... - } - }), + config: widgetConfigTransform, });Also applies to: 48-67
178-183: Unsafeas anycast on plan limits — consider typing or validating with zod.The
as anycast discards all type safety. If the plan limits shape ever changes, this will silently fall through to the default (16) with no indication of a problem. Consider defining a zod schema for the expected limits shape and usingsafeParse, or at minimum a narrower type assertion.♻️ Proposed approach
// Define expected shape const MetricWidgetLimit = z.union([ z.number(), z.object({ number: z.number() }), ]); // In checkWidgetLimit: const rawLimit = (plan?.v3Subscription?.plan?.limits as Record<string, unknown>) ?.metricWidgetsPerDashboard; const parsed = MetricWidgetLimit.safeParse(rawLimit); const widgetLimit = parsed.success ? (typeof parsed.data === "number" ? parsed.data : parsed.data.number) : 16;
475-479: No validation that saved layout items correspond to existing widgets.The
layoutaction accepts any array ofLayoutItems and writes them directly, without verifying that everyivalue maps to a key inexistingLayout.widgets, or that no widget is orphaned (present inwidgetsbut missing fromlayout). A buggy client could create orphaned widgets or phantom layout entries. Consider adding a sanity check if data integrity matters here.apps/webapp/app/components/navigation/TreeConnectors.tsx (1)
5-16:classNamecannot override the text color due tocn()ordering.In
cn("overflow-visible", className, "text-charcoal-600"),tailwind-mergegives precedence to the last conflicting utility. Sincetext-charcoal-600comes afterclassName, anytext-*class passed viaclassNamewill be silently overridden. The same issue applies toTreeConnectorEndon Line 21.If the color should be customizable, move
classNameto the end:Proposed fix
- className={cn("overflow-visible", className, "text-charcoal-600")} + className={cn("overflow-visible text-charcoal-600", className)}If the color is intentionally locked, a brief comment would prevent future confusion.
apps/webapp/app/v3/services/aiQueryService.server.ts (1)
446-457: Prompt duplication betweenbuildSystemPromptandbuildEditSystemPromptis significant.The two system prompts share ~90% of their content (syntax guide, functions, rules). Every time a rule is added or modified (like the timeBucket additions here), both must be updated in lockstep. Consider extracting the shared sections into helper methods or template fragments to keep them DRY.
apps/webapp/app/components/primitives/Popover.tsx (1)
297-297:PopoverVerticalEllipseVariantis not exported, unlikePopoverArrowTriggerVariant.If external consumers need to type the
variantprop (e.g., storing it in state or passing it through props), they won't have access to this type. Consider exporting it for consistency withPopoverArrowTriggerVariant.Proposed fix
-export type { PopoverArrowTriggerVariant }; +export type { PopoverArrowTriggerVariant, PopoverVerticalEllipseVariant };apps/webapp/app/components/primitives/charts/BigNumberCard.tsx (1)
11-16: Usetypeinstead ofinterfaceforBigNumberCardProps.As per coding guidelines,
**/*.{ts,tsx}: Use types over interfaces for TypeScript.Suggested fix
-interface BigNumberCardProps { - rows: Record<string, unknown>[]; - columns: OutputColumnMetadata[]; - config: BigNumberConfiguration; - isLoading?: boolean; -} +type BigNumberCardProps = { + rows: Record<string, unknown>[]; + columns: OutputColumnMetadata[]; + config: BigNumberConfiguration; + isLoading?: boolean; +};apps/webapp/app/components/navigation/useReorderableList.ts (1)
39-42: Missing dependencies in theuseEffect—initialOrder,items,itemKeyare used but not listed.The effect body reads
initialOrder,items, anditemKey, but onlyorganizationIdis in the dependency array. While the comment explains the intent (sync on org change only), React's rules-of-hooks lint will flag this, and if items or initialOrder change independently (e.g., after a dashboard is created/deleted within the same org), the localorderstate won't reflect the updated preference.Consider either adding the deps or using a ref-based pattern to silence the lint rule intentionally:
useEffect(() => { setOrder(initialOrder ?? items.map(itemKey)); - }, [organizationId]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally reset only on org change + }, [organizationId]);apps/webapp/app/hooks/useDashboardEditor.ts (2)
252-303: No rollback on sync failure — optimistic state may diverge from server.When
processNextSynccatches an error, the failed task is discarded and the local state (already updated optimistically) is never reverted. This means the user sees widgets/layout changes that the server never persisted. If the user navigates away and back, they'll see the server's state, which could be confusing.Consider at minimum dispatching a
RESET_STATEon error (by fetching fresh data), or implementing a retry with backoff for transient failures. TheonSyncErrorcallback is a good hook for this, but it pushes the burden entirely to the consumer.
214-214:JSON.stringifyon every render to derive the effect dependency.
initialDataJsonis computed unconditionally on each render. For dashboards with many widgets this could become noticeable. ConsideruseMemoto avoid re-serializing wheninitialDatahasn't changed:♻️ Proposed fix
- const initialDataJson = JSON.stringify({ layout: initialData.layout, widgets: initialData.widgets }); + const initialDataJson = useMemo( + () => JSON.stringify({ layout: initialData.layout, widgets: initialData.widgets }), + [initialData.layout, initialData.widgets] + );apps/webapp/app/services/dashboardPreferences.server.ts (2)
23-24: Import placed mid-file instead of at the top.The
importofSideMenuSectionIdsits between type declarations rather than with the other imports at lines 1-4. Move it to the top of the file with the other imports for consistency.♻️ Proposed fix
import { z } from "zod"; import { prisma } from "~/db.server"; import { logger } from "./logger.server"; import { type UserFromSession } from "./session.server"; +import { type SideMenuSectionId } from "~/components/navigation/sideMenuTypes"; +export type { SideMenuSectionId };And remove lines 23-24 from their current location.
187-232:updateItemOrderalways writes to DB — consider adding change detection.Unlike
updateSideMenuPreferenceswhich checks for changes before persisting (line 159),updateItemOrderunconditionally writes the updated preferences. Since this is called on every drag-stop, it could result in redundant DB writes when the user drops an item back in its original position (the hook already short-circuits viaJSON.stringifycomparison, but if something slips through or the comparison diverges, this is a safety net).internal-packages/database/prisma/schema.prisma (1)
2515-2517: Consider usingJsontype instead ofStringfor thelayoutfield.Storing structured layout data as a
Stringmeans Prisma won't validate it's valid JSON at the database level. UsingJsonwould provide built-in JSON validation and native querying support in PostgreSQL. The PR summary mentions extractingwidgetCountfrom layout JSON — aJsoncolumn would make that more natural.apps/webapp/app/presenters/v3/BuiltInDashboards.server.ts (1)
2-2: Unusedzodimport.
zfrom"zod"is imported but never used in this file.import { type BuiltInDashboard } from "./MetricDashboardPresenter.server"; -import { z } from "zod";apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.create.tsx (2)
32-37: Fragile limit extraction withas anycast bypasses type safety.The
as anycast discards all type information, and the nested optional chain with a magic fallback of3is brittle. If the plan limits schema changes, this will silently use the wrong default. Consider defining a proper type for plan limits or using zod to parse/validate the limits object.
15-15: Consider using a function declaration for the action export.As per coding guidelines, prefer function declarations. Remix actions work fine as exported function declarations.
Proposed change
-export const action = async ({ request, params }: ActionFunctionArgs) => { +export async function action({ request, params }: ActionFunctionArgs) { const userId = await requireUserId(request); ... -}; +}As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use function declarations instead of default exports.apps/webapp/app/routes/resources.preferences.sidemenu.tsx (1)
58-58: Unnecessary type assertion.
result.data.sectionIdis already typed asSideMenuSectionId | undefinedfrom the zod schema, so theas SideMenuSectionIdcast is redundant. Theundefinedcase is already excluded by the conditional check.Proposed fix
- sectionId: result.data.sectionId as SideMenuSectionId, + sectionId: result.data.sectionId,internal-packages/tsql/src/query/time_buckets.ts (1)
12-17: Usetypeinstead ofinterfaceforTimeBucketInterval.Per coding guidelines, prefer
typeoverinterfacefor TypeScript definitions.Suggested fix
-export interface TimeBucketInterval { - /** The numeric value of the interval (e.g., 5 for "5 MINUTE") */ - value: number; - /** The time unit */ - unit: "SECOND" | "MINUTE" | "HOUR" | "DAY" | "WEEK" | "MONTH"; -} +export type TimeBucketInterval = { + /** The numeric value of the interval (e.g., 5 for "5 MINUTE") */ + value: number; + /** The time unit */ + unit: "SECOND" | "MINUTE" | "HOUR" | "DAY" | "WEEK" | "MONTH"; +};As per coding guidelines:
**/*.{ts,tsx}: "Use types over interfaces for TypeScript".apps/webapp/app/routes/_app.orgs.$organizationSlug/route.tsx (1)
127-143: Consider using zod to validate the layout JSON structure.The layout is parsed with
JSON.parseand cast viaas Record<string, unknown>without schema validation. Per coding guidelines, zod should be used for validation inapps/webapp. A lightweight zod schema would make this more robust and catch unexpected layout shapes early.Suggested approach
+import { z } from "zod"; + +const DashboardLayoutSchema = z.object({ + widgets: z.record(z.unknown()).optional(), +}).passthrough(); + const customDashboardsWithWidgetCount = customDashboards.map((d) => { let widgetCount = 0; try { - const layout = JSON.parse(String(d.layout)) as Record<string, unknown>; - const widgets = layout.widgets; - if (widgets && typeof widgets === "object") { - widgetCount = Object.keys(widgets as Record<string, unknown>).length; - } + const layout = DashboardLayoutSchema.safeParse(JSON.parse(String(d.layout))); + if (layout.success && layout.data.widgets) { + widgetCount = Object.keys(layout.data.widgets).length; + } } catch { // ignore parse errors }As per coding guidelines:
{packages/core,apps/webapp}/**/*.{ts,tsx}: "Use zod for validation in packages/core and apps/webapp".apps/webapp/app/components/metrics/SaveToDashboardDialog.tsx (2)
6-11: Consolidate imports from~/hooks/useOrganizations.Two separate import statements pull from the same module. Merge them for cleanliness.
Suggested fix
-import { - useCustomDashboards, - useWidgetLimitPerDashboard, -} from "~/hooks/useOrganizations"; -import { useProject } from "~/hooks/useProject"; -import { useOrganization } from "~/hooks/useOrganizations"; +import { + useCustomDashboards, + useOrganization, + useWidgetLimitPerDashboard, +} from "~/hooks/useOrganizations"; +import { useProject } from "~/hooks/useProject";
49-51: Consider creating a dedicated path builder for dashboard widget resource endpoints.The form action URL is constructed inline with string interpolation, but no path builder utility currently exists for this resource endpoint. While
v3CustomDashboardPathexists inpathBuilder.ts, it builds navigation paths (/metrics/custom/{id}), not resource endpoints. Creating a dedicated builder (similar tov3RunIdempotencyKeyResetPathor other resource path functions) would centralize this URL pattern and prevent drift, especially since the same hardcoded pattern appears in multiple locations.internal-packages/database/prisma/migrations/20260201130503_metrics_dashboard_table_created/migration.sql (1)
16-16: Consider adding an index onorganizationIdfor the loader query pattern.The org-level loader at
_app.orgs.$organizationSlug/route.tsxqueriesWHERE organizationId = ? ORDER BY createdAt DESC. The existing index on(projectId, createdAt)won't serve this query. An index on(organizationId, createdAt DESC)would be beneficial, especially as dashboard counts grow. Since this is a new table, it can be included here withoutCONCURRENTLY.Suggested addition
CREATE INDEX "MetricsDashboard_projectId_createdAt_idx" ON "public"."MetricsDashboard" ("projectId", "createdAt" DESC); + +-- CreateIndex +CREATE INDEX "MetricsDashboard_organizationId_createdAt_idx" ON "public"."MetricsDashboard" ("organizationId", "createdAt" DESC);internal-packages/tsql/src/index.ts (1)
474-542: Coding guideline: prefertypeoverinterfacefor TypeScript.
CompileTSQLOptionsis declared as aninterface. The coding guidelines for**/*.{ts,tsx}specify using types over interfaces.That said, this appears to be a pre-existing declaration (only the
timeRangefield is new), so this is a low-priority nit if you'd prefer to address it later.♻️ Optional: convert to type alias
-export interface CompileTSQLOptions { +export type CompileTSQLOptions = { /** Schema definitions for allowed tables and columns */ tableSchema: TableSchema[]; // ... rest of fields timeRange?: TimeRange; -} +};As per coding guidelines,
**/*.{ts,tsx}: Use types over interfaces for TypeScript.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHistoryPopover.tsx (1)
4-5: Mixed usage of local Popover wrapper and raw Radix primitives.The component uses the local
Popover/PopoverTriggerwrappers but bypassesPopoverContentin favor ofPopoverPrimitive.Content. This creates an inconsistency — if the localPopoverContentdefaults evolve (e.g., accessibility attributes, animation changes), this component won't pick them up.Consider either extending the local
PopoverContentto support the needed customization, or documenting why the raw primitive is needed here.apps/webapp/app/components/navigation/DashboardDialogs.tsx (1)
44-47:as anycast on plan limits bypasses type safety.
(plan?.v3Subscription?.plan?.limits as any)?.metricDashboardsappears here and in the custom dashboard route. If the plan limits type doesn't includemetricDashboardsyet, consider extending the type definition to include it rather than casting toanyin multiple places.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx (1)
362-389: Duplicate widget-limit dialog content in two places.The "exceeded widget limit" dialog at lines 362-389 (inline in NavBar) and lines 607-626 (standalone dialog triggered by hook) contain nearly identical content. Consider extracting a shared
WidgetLimitDialogContentcomponent to reduce duplication.Also applies to: 607-626
apps/webapp/app/services/queryService.server.ts (1)
21-21: Importing time utilities from a UI component file into a server service.
timeFiltersandtimeFilterFromToare imported from~/components/runs/v3/SharedFilters, which is a UI component module. Server-side services shouldn't depend on component files. Consider extracting these time utilities into a shared utility module (e.g.,~/utils/timeFilters.ts) that both the component and the service can import.apps/webapp/app/components/metrics/QueryWidget.tsx (1)
400-406:"title"type silently returnsnull— consider documenting or guarding at the caller level.The
"title"case returnsnullwith a comment that these are rendered byTitleWidget. SinceQueryWidgetstill renders a fullCardwrapper (header, menu, fullscreen button, loading bar) around thisnullbody, callers must know not to wrap title widgets inQueryWidget. This contract is implicit. A runtime guard or a prop-level exclusion of"title"fromQueryWidgetConfigfor this component's props would make the API less error-prone.apps/webapp/app/components/query/QueryEditor.tsx (6)
162-167: Usetypeinstead ofinterfaceper coding guidelines.The coding guidelines require using types over interfaces for TypeScript.
Proposed fix
-/** Handle for imperatively setting the query from outside */ -interface QueryEditorFormHandle { - setQuery: (query: string) => void; - setScope: (scope: QueryScope) => void; - getQuery: () => string; - setTimeFilter: (filter: { period?: string; from?: string; to?: string }) => void; -} +/** Handle for imperatively setting the query from outside */ +type QueryEditorFormHandle = { + setQuery: (query: string) => void; + setScope: (scope: QueryScope) => void; + getQuery: () => string; + setTimeFilter: (filter: { period?: string; from?: string; to?: string }) => void; +};As per coding guidelines, "Use types over interfaces for TypeScript".
445-477:titleFetcherin the dependency array causes this effect to evaluate on every render.Remix fetcher objects are re-created each render, so including
titleFetcherin the dep array means the effect body runs on every render. The internal guards (shouldGenerateTitle,titleFetcher.state === "idle", etc.) prevent repeated submissions, so this isn't a bug, but it's wasteful. Consider depending ontitleFetcher.stateinstead.Proposed fix
}, [ results, shouldGenerateTitle, historyTitle, hasUserTitle, - titleFetcher, + titleFetcher.state, organization.slug, project.slug, environment.slug, ]);Note: you'd also need to extract
titleFetcher.submitto a stable ref or usetitleFetcher.submitdirectly in the dep array if the linter complains.
505-509: MissingdefaultChartConfiginuseEffectdependency array.The
initialChartConfigis checked inside the effect but not listed in the dependency array. React's exhaustive-deps rule would flag this. While the intent (only reset on schema change) is clear, the stale closure could become a bug ifinitialChartConfigever changes during the component lifecycle (e.g., parent re-keying). Suppressing with an eslint-disable comment would make the intent explicit.Proposed fix — make intent explicit
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- Only reset when column schema changes, not when initialChartConfig changes useEffect(() => { if (columnsKey && !initialChartConfig) { setChartConfig(defaultChartConfig); } }, [columnsKey, initialChartConfig]);
1231-1235:useEffectreferencesbigNumberConfigandonBigNumberConfigChangebut only depends on[columns].This will trigger an exhaustive-deps lint warning. More importantly, the spread
{ ...bigNumberConfig, column: ... }captures a potentially stalebigNumberConfig— if the user changed other config fields between column-schema changes, those changes could be lost. Sincecolumnschanges infrequently (only on new query results), the practical risk is low, but the pattern is fragile.Proposed fix using functional callback pattern
useEffect(() => { - if (!bigNumberConfig.column && numericColumns.length > 0) { - onBigNumberConfigChange({ ...bigNumberConfig, column: numericColumns[0].name }); - } - }, [columns]); + if (numericColumns.length > 0) { + onBigNumberConfigChange((prev) => { + if (!prev.column) { + return { ...prev, column: numericColumns[0].name }; + } + return prev; + }); + } + // eslint-disable-next-line react-hooks/exhaustive-deps -- Auto-select only when columns change + }, [columns]);Note: this requires
onBigNumberConfigChangeto accept a callback form (state updater). If the parent usessetBigNumberConfigdirectly, this already works. Otherwise, keep the current approach but add the eslint-disable comment to document intent.
935-1013: Duplicate rename dialog pattern — extract a shared component.This
QueryTitlecomponent contains a rename dialog (lines 977-1010) that is nearly identical to the rename dialog inQueryWidget.tsx(lines 265-299). Both share the same form structure, state management (renameValue,isDialogOpen), and submission logic. Consider extracting a sharedRenameDialogcomponent to reduce duplication.
540-551:saveDatais computed during render by reading a ref (editorRef.current?.getQuery()).Refs are not reactive —
currentQuery(line 541) won't trigger a re-render when the user types. This meanssaveDatacould be stale when passed tosave(saveData)on lines 771, 821, 871. Since clicking a save button typically triggers a re-render that re-reads the ref, this is likely fine in practice, but the pattern is subtle. Consider computingsaveDatalazily inside the save callback or converting the query to state if freshness is critical.
...s.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx
Outdated
Show resolved
Hide resolved
...ages/database/prisma/migrations/20260201130503_metrics_dashboard_table_created/migration.sql
Show resolved
Hide resolved
.../database/prisma/migrations/20260202100000_add_friendlyid_to_metrics_dashboard/migration.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/metrics/QueryWidget.tsx`:
- Around line 348-372: The inline QueryResultsChart instance is missing the
isLoading prop, so add isLoading={isLoading} to the first QueryResultsChart (the
non-fullscreen one) so both chart instances receive loading state; update the
JSX for the component named QueryResultsChart (the one passed rows={data.rows}
columns={data.columns} config={config} onViewAllLegendItems={...}) to include
isLoading={isLoading} matching the fullscreen instance.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx:
- Line 148: The code unsafely casts value("scope") to QueryScope causing
unvalidated input; replace the cast with Zod validation by defining/using
z.enum(["environment","project","organization"]) (or the existing QueryScope
schema) and call safeParse(value("scope") ?? "environment"), then set scope to
parsed.data when parsed.success is true and fall back to "environment"
otherwise; update the line using value("scope") as QueryScope to use the
safeParse result so downstream code receives a validated QueryScope.
🧹 Nitpick comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx (2)
210-257: Widgets and layout items may go out of sync.
Object.entries(widgets)drives rendering, but layout positioning is separate. If a widget key exists inwidgetsbut not inlayout, ReactGridLayout will auto-place it (potentially overlapping). Conversely, orphaned layout entries (no matching widget) are silently ignored.Since
MetricDashboardis exported as a public component accepting both props independently, consider either:
- Iterating
layoutand looking up widgets (so only positioned widgets render), or- Adding a guard/filter to ensure only widgets with matching layout entries are rendered.
This is a minor robustness concern for the editable/custom dashboard use case.
195-208: Inline config objects recreated every render.
gridConfig,resizeConfig, anddragConfigare new object references on each render, which may cause unnecessary reconciliation inReactGridLayout. Consider extracting them as constants or memoizing withuseMemo.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/components/metrics/TitleWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/components/metrics/TitleWidget.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/metrics/QueryWidget.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/metrics/QueryWidget.tsx (6)
apps/webapp/app/components/primitives/Buttons.tsx (1)
Button(296-329)apps/webapp/app/components/primitives/Popover.tsx (4)
Popover(286-286)PopoverVerticalEllipseTrigger(294-294)PopoverContent(288-288)PopoverMenuItem(290-290)apps/webapp/app/components/primitives/Dialog.tsx (4)
Dialog(117-117)DialogContent(119-119)DialogHeader(120-120)DialogFooter(121-121)apps/webapp/app/components/code/TSQLResultsTable.tsx (1)
TSQLResultsTable(885-1186)apps/webapp/app/components/code/QueryResultsChart.tsx (1)
QueryResultsChart(725-941)apps/webapp/app/components/primitives/charts/BigNumberCard.tsx (1)
BigNumberCard(120-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/components/metrics/QueryWidget.tsx (3)
32-110: Well-structured Zod schemas and discriminated union for widget configs.Good use of Zod discriminated union for
QueryWidgetConfigand shared config option objects to keep the schemas DRY.
150-301: QueryWidget component looks solid.Clean separation of concerns with the popover menu, rename dialog, and delegation to
QueryWidgetBody. The rename dialog correctly resetsrenameValuefromtitleStringon each open, and the form prevents empty submissions.
399-406: Title case returnsnull— consider a brief comment or blank state.The
"title"case returnsnullwith a comment that title widgets are rendered elsewhere. This is fine for correctness. Just noting that if aQueryWidgetis ever accidentally given a"title"config, it will render a card with an empty body. TheassertNeverin default ensures exhaustiveness — good.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx (3)
39-74: LGTM — loader is well-structured.Good use of
Promise.allfor parallel data fetching and zod-based param validation. The 404 guards for project/environment are clear.
265-293: LGTM — clean responsive-width hook.Good use of
ResizeObserverwith proper cleanup, and themountedflag prevents SSR/hydration mismatches withReactGridLayout.
76-104: LGTM!Clean page component. Using
key={key}onMetricDashboardensures a full remount when navigating between dashboards, which correctly resets internal state.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
....orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx:
- Around line 496-510: Guard against division-by-zero from widgetLimits.limit by
computing a safe ratio/percent before using it in strokeDasharray and content:
create a local safeRatio = widgetLimits.limit > 0 ? widgetLimits.used /
widgetLimits.limit : (widgetLimits.used > 0 ? 1 : 0) and safePercent =
Math.round(safeRatio * 100) (or clamp between 0 and 100 to avoid NaN/Infinity),
then use safeRatio in the strokeDasharray interpolation and safePercent for the
tooltip content; update the JSX that references strokeDasharray and content to
use these safe values (symbols: widgetLimits, strokeDasharray, content,
widgetLimitPerDashboard, widgetIsAtLimit).
🧹 Nitpick comments (5)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx (5)
120-178: Action handles delete/rename correctly.Auth, ownership checks, and the switch-based dispatch are clean. One minor note: the rename validation (lines 162–164) uses a manual type check rather than Zod, which is the preferred validation approach per project guidelines. Not blocking since it's a simple string presence check.
200-203: Unsafeas anycast loses type safety on plan limits.Line 202 casts
plan?.v3Subscription?.plan?.limitstoany, which defeats TypeScript's guarantees and makesplanLimits.canExceedaccess unchecked. Consider defining a proper type or using Zod to parse the limits object.Suggested approach
- const planLimits = (plan?.v3Subscription?.plan?.limits as any)?.metricWidgetsPerDashboard; - const canExceedWidgets = typeof planLimits === "object" && planLimits.canExceed === true; + const planLimits = plan?.v3Subscription?.plan?.limits; + const metricWidgetsPerDashboard = + planLimits && typeof planLimits === "object" && "metricWidgetsPerDashboard" in planLimits + ? (planLimits as Record<string, unknown>).metricWidgetsPerDashboard + : undefined; + const canExceedWidgets = + typeof metricWidgetsPerDashboard === "object" && + metricWidgetsPerDashboard !== null && + (metricWidgetsPerDashboard as Record<string, unknown>).canExceed === true;Ideally, define a Zod schema for plan limits so this is parsed once and reused. As per coding guidelines, use Zod for validation in
apps/webapp.
362-428: Duplicated "Add title" button block.The "Add title" button (lines 391–402 and 415–426) is identical in both branches of the
widgetIsAtLimitternary. Extract it to reduce duplication.Example extraction
+ const addTitleButton = ( + <Button + variant="secondary/small" + LeadingIcon={Type} + className="pl-1.5" + iconSpacing="gap-x-1" + onClick={() => { + setNewTitleValue(""); + setShowAddTitleDialog(true); + }} + > + Add title + </Button> + );Then use
{addTitleButton}in both branches.
613-633: Widget limit dialog duplicates the "Add chart" limit dialog content (lines 365–390).The dialog body and footer for the widget-limit exceeded state are essentially identical in two places. Consider extracting a shared
WidgetLimitExceededContentcomponent to keep them in sync.
638-709: RenameDashboardDialog — dialog closes on any idle transition, not just the rename action.The
useEffecton line 649–653 setsisOpen(false)whenevernavigation.statereturns to"idle", regardless of which action completed. If another form on the page is submitted while this dialog is open, it would also close. In practice the modal likely blocks other interactions, so this is low-risk, but guarding on the rename action would be more precise.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx (2)
66-118: Loader looks solid.Proper auth, 404 handling, parallel data fetching with
Promise.all, and environment variable access throughenv.server.ts. Clean structure.
711-768: DeleteDashboardDialog follows a clean pattern — LGTM.The delete form submits inline with
name="action" value="delete", and the confirmation UX with disabled state during submission is well-handled.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...$organizationSlug.projects.$projectParam.env.$envParam.metrics.custom.$dashboardId/route.tsx
Outdated
Show resolved
Hide resolved
matt-aitken
force-pushed
the
metrics-dashboard
branch
from
3e9a11c to
cc2b111
Compare
The action had a lot of logic in and by moving it into the service we can re-use the service for API querying
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!isDateBased || !timeGranularity) return undefined; | ||
| let lastLabel = ""; | ||
| return (value: number) => { | ||
| const date = new Date(value); | ||
| return formatDateByGranularity(date, timeGranularity); | ||
| const label = formatDateByGranularity(date, timeGranularity); | ||
| if (label === lastLabel) return ""; | ||
| lastLabel = label; |
There was a problem hiding this comment.
🟡 Stale lastLabel closure in xAxisTickFormatter causes tick labels to disappear on chart interaction
The xAxisTickFormatter created inside useMemo captures a mutable lastLabel variable in its closure. This variable persists its value across Recharts re-render cycles (e.g., tooltip hover). When all visible ticks produce the same formatted label (e.g., all data points within a single day on a "days" granularity chart), the first tick's label on re-render matches the stale lastLabel from the previous cycle and is incorrectly hidden — causing all tick labels to disappear on hover.
Root Cause and Reproduction
The lastLabel variable is initialized to "" only when the useMemo factory re-runs (i.e., when isDateBased or timeGranularity change). Between Recharts rendering passes that don't change those dependencies, the closure retains lastLabel from the last tick of the previous pass.
Reproduction scenario:
- A time-series chart where all data points fall within one day (e.g., hourly data for Jan 4)
- Granularity is "days" → all ticks format to "Jan 4"
- First render: tick 1 shows "Jan 4", all subsequent ticks are deduplicated (hidden).
lastLabel= "Jan 4". - User hovers the chart → Recharts re-renders ticks → tick 1 produces "Jan 4" which equals stale
lastLabel→ hidden. - All tick labels disappear.
The fix is to reset lastLabel at the start of each rendering pass, e.g., by moving the deduplication state into the dateAxisTick renderer or using a ref that resets when the tick renderer is invoked for index 0.
Prompt for agents
In apps/webapp/app/components/code/QueryResultsChart.tsx, the xAxisTickFormatter useMemo (around lines 776-785) creates a mutable lastLabel variable in its closure that persists across Recharts render cycles. To fix this, move the deduplication logic into the dateAxisTick renderer so that lastLabel is reset each time Recharts calls the full set of tick renderers. One approach: instead of putting lastLabel inside xAxisTickFormatter, create a ref (e.g. lastLabelRef = useRef('')) and reset it to '' inside a useEffect that runs before each paint, or alternatively track the render pass inside dateAxisTick by detecting when the tick index resets to the minimum value and clearing lastLabel at that point.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const currentQuery = editorRef.current?.getQuery() ?? ""; | ||
| const saveData: QueryEditorSaveData = { |
There was a problem hiding this comment.
🔴 saveData.query reads stale ref value because parent doesn't re-render on child query edits
In the QueryEditor component, the saveData object passed to the save render prop reads the current query via editorRef.current?.getQuery() during the parent's render. However, editing the query only triggers a re-render in the child QueryEditorForm (via useState), not in the parent QueryEditor. This means the save button captures a stale query from the parent's last render.
Detailed Explanation
At apps/webapp/app/components/query/QueryEditor.tsx:552-553:
const currentQuery = editorRef.current?.getQuery() ?? "";
const saveData: QueryEditorSaveData = {
title: queryTitle ?? "Untitled Query",
query: currentQuery,
...
};The editorRef is attached to QueryEditorForm, which manages query state internally. When the user types, setQuery re-renders QueryEditorForm and updates the imperative handle, but the parent QueryEditor does not re-render. So currentQuery reflects whatever the query was at the parent's last render.
The save(saveData) render prop (line 842-843 in dashboard mode) creates a button with the stale saveData captured in its onClick closure.
Impact: If a user edits the query after running it (which triggers a parent re-render) but before clicking save, the widget is saved with the old query, silently discarding the user's latest edits. This affects the dashboard-add and dashboard-edit modes, not standalone mode (which opens a dialog that triggers a fresh re-render).
Prompt for agents
In apps/webapp/app/components/query/QueryEditor.tsx around lines 552-553, the saveData object reads currentQuery from editorRef.current?.getQuery() during the parent's render. Since the parent doesn't re-render when the child's query state changes, this value can be stale. To fix this, avoid computing saveData during render. Instead, modify handleSave (line 276) to read the query fresh from the ref at call time: change handleSave to read editorRef.current?.getQuery() inside the callback rather than relying on pre-computed saveData. For example, update renderSaveForm to not receive pre-computed data, and instead have the button's onClick read fresh values from the ref when clicked. An alternative approach is to lift the query state up to QueryEditor (passing it down as a controlled prop) so that query changes trigger parent re-renders.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
What changed