Metrics dashboards #3019
Metrics dashboards #3019
Conversation
The action had a lot of logic in and by moving it into the service we can re-use the service for API querying
|
WalkthroughThe diff adds a full metrics/dashboard feature set and related infra: a MetricsDashboard DB model with migrations and Prisma schema updates; a QueryEditor (rich SQL editor, multi-view results, AI title generation) and QueryWidget/MetricWidget/BigNumberCard UI components; numerous new Remix routes, resource actions, presenters, and services for dashboards and widgets; TSQL compiler extensions and time-bucketing support (timeBucket(), calculateTimeBucketInterval, TimeRange); side-menu preferences and reorder/collapse support with new hooks (useDashboardEditor, useReorderableList, useInterval, useRevalidateOnParam); many new/updated UI components (ScopeFilter, QueuesFilter, DashboardList, CreateDashboardButton, SaveToDashboardDialog, TitleWidget, etc.); utilities, path builders, tests, and package dependency updates. 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)
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ 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)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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.
| onClick={isSideMenuCollapsed ? undefined : handleToggle} | ||
| style={{ cursor: isSideMenuCollapsed ? "default" : "pointer" }} | ||
| > | ||
| <h2 className="text-xs whitespace-nowrap">{title}</h2> | ||
| <motion.div | ||
| initial={isCollapsed} | ||
| animate={{ rotate: isCollapsed ? -90 : 0 }} | ||
| transition={{ duration: 0.2 }} | ||
| > | ||
| <ToggleArrowIcon className="size-2" /> | ||
| </motion.div> | ||
| <div className="flex items-center gap-1 text-text-dimmed transition group-hover/section:text-text-bright"> | ||
| <h2 className="whitespace-nowrap text-xs">{title}</h2> | ||
| <motion.div | ||
| initial={isCollapsed} | ||
| animate={{ rotate: isCollapsed ? -90 : 0 }} | ||
| transition={{ duration: 0.2 }} | ||
| > | ||
| <ToggleArrowIcon className="size-2" /> | ||
| </motion.div> | ||
| </div> | ||
| {headerAction && <div className="flex items-center">{headerAction}</div>} |
There was a problem hiding this comment.
Clicking headerAction will also trigger section collapse toggle.
The headerAction is rendered inside the motion.div that has onClick={handleToggle}. Any click on the action element (e.g., a "+" button) will bubble up and toggle the section's collapsed state. You need to stop propagation on the action container.
Proposed fix
- {headerAction && <div className="flex items-center">{headerAction}</div>}
+ {headerAction && (
+ <div className="flex items-center" onClick={(e) => e.stopPropagation()}>
+ {headerAction}
+ </div>
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={isSideMenuCollapsed ? undefined : handleToggle} | |
| style={{ cursor: isSideMenuCollapsed ? "default" : "pointer" }} | |
| > | |
| <h2 className="text-xs whitespace-nowrap">{title}</h2> | |
| <motion.div | |
| initial={isCollapsed} | |
| animate={{ rotate: isCollapsed ? -90 : 0 }} | |
| transition={{ duration: 0.2 }} | |
| > | |
| <ToggleArrowIcon className="size-2" /> | |
| </motion.div> | |
| <div className="flex items-center gap-1 text-text-dimmed transition group-hover/section:text-text-bright"> | |
| <h2 className="whitespace-nowrap text-xs">{title}</h2> | |
| <motion.div | |
| initial={isCollapsed} | |
| animate={{ rotate: isCollapsed ? -90 : 0 }} | |
| transition={{ duration: 0.2 }} | |
| > | |
| <ToggleArrowIcon className="size-2" /> | |
| </motion.div> | |
| </div> | |
| {headerAction && <div className="flex items-center">{headerAction}</div>} | |
| onClick={isSideMenuCollapsed ? undefined : handleToggle} | |
| style={{ cursor: isSideMenuCollapsed ? "default" : "pointer" }} | |
| > | |
| <div className="flex items-center gap-1 text-text-dimmed transition group-hover/section:text-text-bright"> | |
| <h2 className="whitespace-nowrap text-xs">{title}</h2> | |
| <motion.div | |
| initial={isCollapsed} | |
| animate={{ rotate: isCollapsed ? -90 : 0 }} | |
| transition={{ duration: 0.2 }} | |
| > | |
| <ToggleArrowIcon className="size-2" /> | |
| </motion.div> | |
| </div> | |
| {headerAction && ( | |
| <div className="flex items-center" onClick={(e) => e.stopPropagation()}> | |
| {headerAction} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/navigation/SideMenuSection.tsx` around lines 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.
| <Header3 | ||
| className={cn( | ||
| "drag-handle mb-3 flex items-center justify-between gap-2 px-3", | ||
| draggable && "cursor-grab active:cursor-grabbing" | ||
| )} |
There was a problem hiding this comment.
drag-handle class is applied unconditionally, even when draggable is false.
The drag-handle class on line 28 is outside the conditional, so non-draggable card headers will still be identified as drag handles by whatever drag library consumes this class. This likely makes cards draggable even when they shouldn't be.
Proposed fix: make `drag-handle` conditional on `draggable`
<Header3
className={cn(
- "drag-handle mb-3 flex items-center justify-between gap-2 px-3",
- draggable && "cursor-grab active:cursor-grabbing"
+ "mb-3 flex items-center justify-between gap-2 px-3",
+ draggable && "drag-handle cursor-grab active:cursor-grabbing"
)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Header3 | |
| className={cn( | |
| "drag-handle mb-3 flex items-center justify-between gap-2 px-3", | |
| draggable && "cursor-grab active:cursor-grabbing" | |
| )} | |
| <Header3 | |
| className={cn( | |
| "mb-3 flex items-center justify-between gap-2 px-3", | |
| draggable && "drag-handle cursor-grab active:cursor-grabbing" | |
| )} | |
| > |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/charts/Card.tsx` around lines 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.
| useEffect(() => { | ||
| if (!interval || interval <= 0 || disabled) return; | ||
|
|
||
| const intervalId = setInterval(() => { | ||
| callback(); | ||
| }, interval); | ||
|
|
||
| return () => clearInterval(intervalId); | ||
| }, [interval, disabled]); | ||
|
|
||
| // On focus | ||
| useEffect(() => { | ||
| if (!onFocus || disabled) return; | ||
|
|
||
| const handleFocus = () => { | ||
| if (document.visibilityState === "visible") { | ||
| callback(); | ||
| } | ||
| }; | ||
|
|
||
| // Revalidate when the page becomes visible | ||
| document.addEventListener("visibilitychange", handleFocus); | ||
| // Revalidate when the window gains focus | ||
| window.addEventListener("focus", handleFocus); | ||
|
|
||
| return () => { | ||
| document.removeEventListener("visibilitychange", handleFocus); | ||
| window.removeEventListener("focus", handleFocus); | ||
| }; | ||
| }, [onFocus, disabled]); |
There was a problem hiding this comment.
Stale closure: callback is missing from all effect dependency arrays.
If the callback reference changes between renders (e.g., an inline arrow function), all three effects will keep invoking the original stale closure. The standard fix is to store the latest callback in a ref:
♻️ Proposed fix using a callback ref
-import { useEffect } from "react";
+import { useEffect, useRef } from "react";
...
export function useInterval({
interval,
onLoad = true,
onFocus = true,
disabled = false,
callback,
}: UseIntervalOptions) {
+ const callbackRef = useRef(callback);
+ useEffect(() => {
+ callbackRef.current = callback;
+ });
+
// On interval
useEffect(() => {
if (!interval || interval <= 0 || disabled) return;
const intervalId = setInterval(() => {
- callback();
+ callbackRef.current();
}, interval);
return () => clearInterval(intervalId);
}, [interval, disabled]);
// On focus
useEffect(() => {
if (!onFocus || disabled) return;
const handleFocus = () => {
if (document.visibilityState === "visible") {
- callback();
+ callbackRef.current();
}
};
document.addEventListener("visibilitychange", handleFocus);
window.addEventListener("focus", handleFocus);
return () => {
document.removeEventListener("visibilitychange", handleFocus);
window.removeEventListener("focus", handleFocus);
};
}, [onFocus, disabled]);
// On load
useEffect(() => {
- if (disabled) return;
- callback();
+ if (disabled || !onLoad) return;
+ callbackRef.current();
}, []);
}🤖 Prompt for AI Agents
In `@apps/webapp/app/hooks/useInterval.ts` around lines 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.
| // On load | ||
| useEffect(() => { | ||
| if (disabled) return; | ||
| callback(); | ||
| }, []); |
There was a problem hiding this comment.
Bug: onLoad option is never checked — callback always fires on mount.
The onLoad parameter is destructured but this effect never references it, so the callback runs on mount even when onLoad is explicitly set to false.
🐛 Proposed fix
// On load
useEffect(() => {
- if (disabled) return;
+ if (disabled || !onLoad) return;
callback();
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // On load | |
| useEffect(() => { | |
| if (disabled) return; | |
| callback(); | |
| }, []); | |
| // On load | |
| useEffect(() => { | |
| if (disabled || !onLoad) return; | |
| callback(); | |
| }, []); |
🤖 Prompt for AI Agents
In `@apps/webapp/app/hooks/useInterval.ts` around lines 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.
| export async function hasAccessToEnvironment({ | ||
| environmentId, | ||
| projectId, | ||
| organizationId, | ||
| userId, | ||
| }: { | ||
| environmentId: string; | ||
| projectId: string; | ||
| organizationId: string; | ||
| userId: string; | ||
| }): Promise<boolean> { | ||
| const environment = await $replica.runtimeEnvironment.findFirst({ | ||
| where: { | ||
| id: environmentId, | ||
| projectId: projectId, | ||
| organizationId: organizationId, | ||
| organization: { members: { some: { userId } } }, | ||
| }, | ||
| }); | ||
|
|
||
| return environment !== null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how hasAccessToEnvironment is called
rg -n --type=ts 'hasAccessToEnvironment' -A 3 -B 3Repository: triggerdotdev/trigger.dev
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Look at findEnvironmentBySlug to understand DEVELOPMENT environment handling
rg -n --type=ts 'findEnvironmentBySlug' -A 15 -B 3 apps/webapp/app/models/runtimeEnvironment.server.ts | head -100Repository: triggerdotdev/trigger.dev
Length of output: 556
🏁 Script executed:
#!/bin/bash
# Check the environment type constants and how DEVELOPMENT is defined
rg -n 'DEVELOPMENT|environment.*type' --type=ts apps/webapp/app/models/runtimeEnvironment.server.ts | head -20Repository: triggerdotdev/trigger.dev
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Get the full findEnvironmentBySlug function to see complete DEVELOPMENT handling
sed -n '116,145p' apps/webapp/app/models/runtimeEnvironment.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 669
🏁 Script executed:
#!/bin/bash
# Check the metrics route in detail to understand the access control flow
cat -n apps/webapp/app/routes/resources.metric.tsx | head -100Repository: triggerdotdev/trigger.dev
Length of output: 3332
🏁 Script executed:
#!/bin/bash
# Check if there are any environment type checks in the executeQuery or query execution path
rg -n 'environment.*type|DEVELOPMENT' apps/webapp/app/services/queryService.server.ts | head -30Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check the entire executeQuery function to see if there's filtering by environment type
cat -n apps/webapp/app/services/queryService.server.ts | head -80Repository: triggerdotdev/trigger.dev
Length of output: 3306
🏁 Script executed:
#!/bin/bash
# Search for where environment type might be validated in the metrics context
rg -n 'type.*DEVELOPMENT|scope.*environment' apps/webapp/app/routes/resources.metric.tsx apps/webapp/app/services/queryService.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Look at the broader context of how environment scope is handled in queries
rg -n 'scope.*ENVIRONMENT|ENVIRONMENT.*scope' --type=ts apps/webapp/app/services/queryService.server.ts -A 5 -B 5 | head -60Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check if there are other access control functions that handle DEVELOPMENT environments differently
rg -n 'DEVELOPMENT.*userId|userId.*DEVELOPMENT|orgMember.*userId' --type=ts apps/webapp/app/models/runtimeEnvironment.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Look at the context around line 266 to see what function has the DEVELOPMENT check
sed -n '260,275p' apps/webapp/app/models/runtimeEnvironment.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 460
🏁 Script executed:
#!/bin/bash
# Check if hasAccessToEnvironment is used anywhere else besides the metrics route
rg -n 'hasAccessToEnvironment' --type=ts -B 5 -A 5 apps/webapp/appRepository: triggerdotdev/trigger.dev
Length of output: 2605
Add environment type check for DEVELOPMENT environments in hasAccessToEnvironment.
findEnvironmentBySlug restricts DEVELOPMENT environments to the owning user via an OR condition with orgMember: { userId }. The new hasAccessToEnvironment function only checks org membership, allowing any org member to pass the access check for another user's DEVELOPMENT environment. Since the metrics route uses this function to gate data access, this creates a security gap where dev environment metrics could be exposed to unintended org members.
Apply the same OR condition pattern used in findEnvironmentBySlug (lines 125–135) to restrict DEVELOPMENT environment access to the owning user.
🤖 Prompt for AI Agents
In `@apps/webapp/app/models/runtimeEnvironment.server.ts` around lines 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.
| await prisma.metricsDashboard.update({ | ||
| where: { id: dashboard.id }, | ||
| data: { | ||
| layout: JSON.stringify(updatedLayout), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Read-modify-write on layout JSON is susceptible to lost updates under concurrency.
Every operation reads the current layout, modifies it in memory, and writes it back. If two concurrent requests (e.g., two users editing the same dashboard, or a rapid double-click triggering duplicate submissions) execute simultaneously, the second write will silently overwrite the first's changes.
Consider wrapping the read + write in a Prisma $transaction with a serializable isolation level, or using an optimistic concurrency check (e.g., a version/updatedAt column compared at write time).
🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/resources.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx
around lines 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.
|
|
||
| // Handle item order update | ||
| if (result.data.organizationId && result.data.listId && result.data.itemOrder) { | ||
| const orderResult = z.array(z.string()).safeParse(JSON.parse(result.data.itemOrder)); |
There was a problem hiding this comment.
Unhandled JSON.parse will crash on malformed input.
If itemOrder contains invalid JSON, JSON.parse throws a SyntaxError, resulting in an unhandled 500 error. Wrap it in a try/catch or use zod's z.string().transform() for safe parsing.
Proposed fix
if (result.data.organizationId && result.data.listId && result.data.itemOrder) {
- const orderResult = z.array(z.string()).safeParse(JSON.parse(result.data.itemOrder));
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(result.data.itemOrder);
+ } catch {
+ return json({ success: false, error: "Invalid item order" }, { status: 400 });
+ }
+ const orderResult = z.array(z.string()).safeParse(parsed);
if (orderResult.success) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orderResult = z.array(z.string()).safeParse(JSON.parse(result.data.itemOrder)); | |
| if (result.data.organizationId && result.data.listId && result.data.itemOrder) { | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(result.data.itemOrder); | |
| } catch { | |
| return json({ success: false, error: "Invalid item order" }, { status: 400 }); | |
| } | |
| const orderResult = z.array(z.string()).safeParse(parsed); | |
| if (orderResult.success) { |
🤖 Prompt for AI Agents
In `@apps/webapp/app/routes/resources.preferences.sidemenu.tsx` at 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.
| "title" TEXT NOT NULL, | ||
| "organizationId" TEXT NOT NULL, | ||
| "projectId" TEXT, | ||
| "ownerId" TEXT NOT NULL, |
There was a problem hiding this comment.
ownerId is NOT NULL but the foreign key uses ON DELETE SET NULL — this will fail at runtime.
When a referenced User row is deleted, PostgreSQL will attempt to set ownerId to NULL, which violates the NOT NULL constraint and throws an error. Either make ownerId nullable or change the FK action to ON DELETE CASCADE or ON DELETE RESTRICT.
Option A: Make ownerId nullable
- "ownerId" TEXT NOT NULL,
+ "ownerId" TEXT,Option B: Change FK to RESTRICT (prevent user deletion if they own dashboards)
-ALTER TABLE "public"."MetricsDashboard" ADD CONSTRAINT "MetricsDashboard_ownerId_fkey" FOREIGN KEY ("ownerId") REFERENCES "public"."User" ("id") ON DELETE SET NULL ON UPDATE CASCADE;
+ALTER TABLE "public"."MetricsDashboard" ADD CONSTRAINT "MetricsDashboard_ownerId_fkey" FOREIGN KEY ("ownerId") REFERENCES "public"."User" ("id") ON DELETE RESTRICT ON UPDATE CASCADE;Also applies to: 24-25
🤖 Prompt for AI Agents
In
`@internal-packages/database/prisma/migrations/20260201130503_metrics_dashboard_table_created/migration.sql`
at 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.
| @@ -0,0 +1,5 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "public"."MetricsDashboard" ADD COLUMN "friendlyId" TEXT NOT NULL; | |||
There was a problem hiding this comment.
NOT NULL without DEFAULT will fail if rows already exist.
If any MetricsDashboard rows were created between the table-creation migration and this one, this ALTER TABLE will error out because existing rows cannot satisfy the NOT NULL constraint without a default value.
A safer pattern:
🛡️ Proposed fix
- ALTER TABLE "public"."MetricsDashboard" ADD COLUMN "friendlyId" TEXT NOT NULL;
+ ALTER TABLE "public"."MetricsDashboard" ADD COLUMN "friendlyId" TEXT;
+ -- backfill existing rows (if any) with unique IDs
+ UPDATE "public"."MetricsDashboard" SET "friendlyId" = gen_random_uuid()::text WHERE "friendlyId" IS NULL;
+ ALTER TABLE "public"."MetricsDashboard" ALTER COLUMN "friendlyId" SET NOT NULL;🤖 Prompt for AI Agents
In
`@internal-packages/database/prisma/migrations/20260202100000_add_friendlyid_to_metrics_dashboard/migration.sql`
at 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.
| /// Who created the dashboard | ||
| owner User @relation(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade) | ||
| ownerId String |
There was a problem hiding this comment.
onDelete: SetNull on a non-nullable ownerId will fail at the database level.
The owner relation specifies onDelete: SetNull, but ownerId is declared as String (non-nullable). When the referenced User is deleted, PostgreSQL will attempt to set ownerId to NULL, violating the NOT NULL constraint and causing a runtime error.
Either make ownerId optional or change the delete behavior:
Option A: Make ownerId nullable
- owner User `@relation`(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade)
- ownerId String
+ owner User? `@relation`(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade)
+ ownerId String?Option B: Use Cascade delete instead
- owner User `@relation`(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade)
+ owner User `@relation`(fields: [ownerId], references: [id], onDelete: Cascade, onUpdate: Cascade)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Who created the dashboard | |
| owner User @relation(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade) | |
| ownerId String | |
| /// Who created the dashboard | |
| owner User? `@relation`(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade) | |
| ownerId String? |
| /// Who created the dashboard | |
| owner User @relation(fields: [ownerId], references: [id], onDelete: SetNull, onUpdate: Cascade) | |
| ownerId String | |
| /// Who created the dashboard | |
| owner User `@relation`(fields: [ownerId], references: [id], onDelete: Cascade, onUpdate: Cascade) | |
| ownerId String |
🤖 Prompt for AI Agents
In `@internal-packages/database/prisma/schema.prisma` around lines 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).
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.
| case "chart": { | ||
| return ( | ||
| <> | ||
| <QueryResultsChart | ||
| rows={data.rows} | ||
| columns={data.columns} | ||
| config={config} | ||
| onViewAllLegendItems={() => setIsFullscreen(true)} | ||
| /> | ||
| <Dialog open={isFullscreen} onOpenChange={setIsFullscreen}> | ||
| <DialogContent fullscreen> | ||
| <DialogHeader>{title}</DialogHeader> | ||
| <div className="h-full min-h-0 w-full flex-1 overflow-hidden pt-4"> | ||
| <QueryResultsChart | ||
| rows={data.rows} | ||
| columns={data.columns} | ||
| config={config} | ||
| onViewAllLegendItems={() => setIsFullscreen(true)} | ||
| isLoading={isLoading} | ||
| /> | ||
| </div> | ||
| </DialogContent> | ||
| </Dialog> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Bug: isLoading not passed to the inline QueryResultsChart.
The inline (non-fullscreen) QueryResultsChart on line 351 doesn't receive isLoading, while the fullscreen version on line 366 does. This means the inline chart won't show its loading state. The bignumber case correctly passes isLoading to both instances.
🐛 Proposed fix
<QueryResultsChart
rows={data.rows}
columns={data.columns}
config={config}
onViewAllLegendItems={() => setIsFullscreen(true)}
+ isLoading={isLoading}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "chart": { | |
| return ( | |
| <> | |
| <QueryResultsChart | |
| rows={data.rows} | |
| columns={data.columns} | |
| config={config} | |
| onViewAllLegendItems={() => setIsFullscreen(true)} | |
| /> | |
| <Dialog open={isFullscreen} onOpenChange={setIsFullscreen}> | |
| <DialogContent fullscreen> | |
| <DialogHeader>{title}</DialogHeader> | |
| <div className="h-full min-h-0 w-full flex-1 overflow-hidden pt-4"> | |
| <QueryResultsChart | |
| rows={data.rows} | |
| columns={data.columns} | |
| config={config} | |
| onViewAllLegendItems={() => setIsFullscreen(true)} | |
| isLoading={isLoading} | |
| /> | |
| </div> | |
| </DialogContent> | |
| </Dialog> | |
| </> | |
| ); | |
| case "chart": { | |
| return ( | |
| <> | |
| <QueryResultsChart | |
| rows={data.rows} | |
| columns={data.columns} | |
| config={config} | |
| onViewAllLegendItems={() => setIsFullscreen(true)} | |
| isLoading={isLoading} | |
| /> | |
| <Dialog open={isFullscreen} onOpenChange={setIsFullscreen}> | |
| <DialogContent fullscreen> | |
| <DialogHeader>{title}</DialogHeader> | |
| <div className="h-full min-h-0 w-full flex-1 overflow-hidden pt-4"> | |
| <QueryResultsChart | |
| rows={data.rows} | |
| columns={data.columns} | |
| config={config} | |
| onViewAllLegendItems={() => setIsFullscreen(true)} | |
| isLoading={isLoading} | |
| /> | |
| </div> | |
| </DialogContent> | |
| </Dialog> | |
| </> | |
| ); |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/metrics/QueryWidget.tsx` around lines 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.
| const period = value("period"); | ||
| const from = value("from"); | ||
| const to = value("to"); | ||
| const scope = (value("scope") as QueryScope) ?? "environment"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for QueryScope definition
rg -n "QueryScope" --type ts --type tsx -B2 -A2 | head -50Repository: triggerdotdev/trigger.dev
Length of output: 96
🏁 Script executed:
#!/bin/bash
# View the file around line 148 to see imports and context
head -200 "apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.metrics.\$dashboardKey/route.tsx" | tail -80Repository: triggerdotdev/trigger.dev
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Find the file with correct path escaping
find . -type f -name "route.tsx" -path "*metrics*" | head -5Repository: triggerdotdev/trigger.dev
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Search for QueryScope definition using proper rg syntax
rg "QueryScope" -t ts -t tsx -B2 -A2 2>/dev/null | head -50Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Read the file directly - use proper escaping
cat -n "./apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.metrics.\$dashboardKey/route.tsx" | sed -n '1,180p'Repository: triggerdotdev/trigger.dev
Length of output: 7644
🏁 Script executed:
#!/bin/bash
# Search for QueryScope more broadly
rg "QueryScope" --multiline-dotall 2>/dev/null | head -20Repository: triggerdotdev/trigger.dev
Length of output: 2728
Validate scope search parameter with Zod instead of unsafe as cast.
value("scope") comes directly from the URL query string. Casting it with as QueryScope bypasses runtime validation, allowing any arbitrary string to pass through as a valid QueryScope. This creates a type safety gap where downstream code may receive unexpected values.
Since QueryScope is a union of specific strings ("organization" | "project" | "environment") and Zod is already imported, validate the input instead:
Proposed fix
- const scope = (value("scope") as QueryScope) ?? "environment";
+ const scope = z.enum(["environment", "project", "organization"]).catch("environment").parse(value("scope") ?? "environment");Alternatively, using safeParse() with a .success check (the codebase pattern from the learnings) provides clearer type safety:
const parsed = z.enum(["environment", "project", "organization"]).safeParse(value("scope") ?? "environment");
const scope = parsed.success ? parsed.data : "environment";🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.metrics.$dashboardKey/route.tsx
at 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.
2 participants
Summary
What changed