Query improvements #2905
Query improvements #2905
Conversation
|
WalkthroughThis pull request introduces a comprehensive chart refactoring, time-filtering capabilities, and significant UI improvements across the query interface. It adds a new compound chart system (ChartRoot, ChartBar, ChartLine, ChartZoom, ChartLegend) replacing legacy Recharts patterns, integrates time-based filtering throughout the query pipeline with server-side fallback WHERE clauses, refactors TSQLResultsTable to use TanStack React Table with virtualization, restructures ChartConfigPanel for multi-series support, and adds extensive query documentation components (TRQL guide, table schema, examples, AI assistance). Multiple new hooks manage highlight state and zoom interactions, while the query service now supports query history deduplication and time-range filtering. Dependencies include TanStack React Table and updated Recharts. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/code/TSQLResultsTable.tsx`:
- Around line 956-1049: The empty-state check currently uses rows.length which
ignores active table filters; change the condition to check the table's current
row model instead (e.g. replace rows.length === 0 with
table.getRowModel().rows.length === 0 or the existing tableRows variable if
present) so the "No results" UI is shown when filters reduce the visible rows to
zero; update the if condition where rows is checked in the component (around the
top of the empty-state JSX block) to use table.getRowModel().rows.length (or
tableRows.length) and keep the rest of the rendering logic unchanged.
- Around line 105-121: The datetime formatting branch in TSQLResultsTable uses
toLocaleDateString but passes time options (hour, minute, second) which are
ignored; update the code in the isDateTimeType handling (where value is parsed
into const date = new Date(value)) to call date.toLocaleString("en-GB", { day:
"numeric", month: "short", year: "numeric", hour: "2-digit", minute: "2-digit",
second: "2-digit" }) instead of toLocaleDateString, preserving the try/catch and
the fallback return String(value).
In `@apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsx`:
- Around line 74-91: The currentData computed in the useMemo (referencing
highlight.activePayload and totals) stores raw hover values which can be
strings; coerce hovered values to numbers when building hoverData so legend
rendering matches currentTotal/AnimatedNumber expectations—replace use of
item.value with Number(item.value) || 0 inside the reduce that constructs
hoverData (in the currentData useMemo) so the returned merged object contains
numeric values only.
In `@apps/webapp/app/components/primitives/charts/ChartLine.tsx`:
- Around line 235-306: The LegacyChartLineProps and ChartLine function currently
declare a useGlobalDateRange prop that is never used; remove useGlobalDateRange
from the LegacyChartLineProps type, delete the parameter and its default value
from the ChartLine signature, and remove any mention in the JSDoc comment so the
API is not misleading; also search for and update any callers/tests that pass
useGlobalDateRange (or forward it to ChartRoot if you instead choose to
implement the behavior) so the codebase remains consistent.
In `@apps/webapp/app/components/primitives/charts/ChartLoading.tsx`:
- Around line 116-146: ChartBarLoadingBackground (and the other loading
component that uses Math.random between lines 183–197) is producing
non‑deterministic DOM during render causing SSR/client hydration mismatch; fix
by moving any Math.random() calls out of the render path — generate the bar/line
heights after mount (e.g., compute the array of random heights inside a
useEffect/useLayoutEffect and store in state or useRef seeded on mount) and
render deterministically from that state/ref, ensuring initial server render
uses a stable placeholder and updates on the client.
In `@apps/webapp/app/components/primitives/charts/ChartRoot.tsx`:
- Around line 166-178: useSeriesTotal currently sums every property except
dataKey, which accidentally includes numeric metadata; update it to only sum
known series keys from the chart context. Retrieve dataKeys from useChartContext
and in the useMemo reduce iterate over dataKeys (for each seriesKey do
acc[seriesKey] = (acc[seriesKey]||0) + Number(item[seriesKey]||0)), remove
Object.entries usage, and include dataKeys in the useMemo dependency array so
totals correctly reflect only series fields.
In `@apps/webapp/app/components/primitives/charts/DateRangeContext.tsx`:
- Around line 41-50: The helpers toISODateString and parseISODateString are
using mismatched UTC vs local behavior causing day shifts across timezones;
update toISODateString to build the YYYY-MM-DD string from local components
(date.getFullYear(), date.getMonth()+1, date.getDate()) with zero-padding rather
than date.toISOString(), and update parseISODateString to parse the YYYY-MM-DD
parts and construct a local Date via new Date(year, month-1, day) so both
functions use local dates consistently; modify the implementations in
DateRangeContext.tsx for the functions toISODateString and parseISODateString
accordingly.
In `@apps/webapp/app/components/primitives/Tooltip.tsx`:
- Line 126: The className on InformationCircleIcon contains an invalid Tailwind
utility `flex-0`; update the JSX in Tooltip.tsx where InformationCircleIcon is
rendered (look for the className using buttonClassName) and replace `flex-0`
with a valid utility such as `flex-none` (or `shrink-0` if you only want to
prevent shrinking) so the icon behaves correctly in a flex container.
In `@apps/webapp/app/components/runs/v3/SharedFilters.tsx`:
- Around line 315-333: The useEffect in SharedFilters (using previousSearch and
onApply) and TimeDropdown.applySelection both trigger onApply, causing duplicate
applies; keep the location-driven path and stop the duplicate by removing the
direct prop-through from TimeDropdown or preventing TimeDropdown.applySelection
from calling onApply immediately. Specifically, stop passing the onApply prop
into the TimeDropdown instance (or alter TimeDropdown.applySelection to only
emit a local state change and not call onApply when the URL/location will drive
applies), ensuring SharedFilters.useEffect remains the single source that
invokes onApply based on location.search changes (references: SharedFilters
useEffect, previousSearch, onApply; TimeDropdown and its applySelection method).
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsx:
- Around line 14-18: The AITimeFilter interface is duplicated across server and
client files; extract it into a single exported type alias (change from
"interface AITimeFilter" to "export type AITimeFilter = { period?: string;
from?: string; to?: string }") in a new shared types file colocated with the
query route (so both server and client code can import it), then update all
usages/imports in aiQueryService.server.ts, ai-generate.tsx, route.tsx,
AITabContent.tsx, QueryHelpSidebar.tsx, and AIQueryInput.tsx to import the named
type from that new file (and remove any imports from .server files in client
components). Ensure the type is exported so server and client modules can
reference it.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx:
- Around line 368-371: The current detection assigned to queryHasTriggeredAt
uses the regex /\bWHERE\b[\s\S]*\btriggered_at\b/i which can match occurrences
inside string literals or comments; update the logic to first remove/strip SQL
string literals and comments (single-quoted, double-quoted, backtick, -- line
comments and /* */ block comments) from the raw query, then run the existing
/\bWHERE\b[\s\S]*\btriggered_at\b/i test against the cleaned query; implement
this as a small helper (e.g., stripSqlLiteralsOrComments) and use it where
queryHasTriggeredAt is computed so the UI only disables the time filter for real
column references.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts:
- Around line 22-28: formatBytes computes a negative unit index for 0 < bytes <
1 (e.g. byte_seconds) because Math.log(bytes) < 0; update the unit index
calculation in formatBytes to clamp the computed index to a minimum of 0 and
maximum of sizes.length - 1 (e.g. use Math.max(0, Math.floor(Math.log(bytes) /
Math.log(k))) then Math.min(..., sizes.length - 1)) so sub‑1 values resolve to
the "B" unit and avoid sizes[-1].
- Around line 7-19: The parsing code in utils.ts that computes readRows,
readBytes, elapsedNs, and byteSeconds should guard against non-numeric inputs
and potential precision loss: validate parsed values with Number.isFinite and
fall back to a safe default (e.g., 0 or a placeholder) when parseInt/parseFloat
yields NaN, and when counters may exceed Number.MAX_SAFE_INTEGER, parse as
BigInt and branch to use BigInt-safe formatting (or coerce to string) for
readRows/readBytes/byteSeconds before calling formatBytes; ensure formattedTime
calculation checks for finite elapsedNs and handles BigInt or very large values
safely so the UI never displays "NaN" or loses precision.
In `@apps/webapp/package.json`:
- Line 194: Bump the dependency "recharts" in package.json to "^2.15.2" (replace
the existing "recharts" entry) and then run the project's Prettier formatting
before committing; ensure the package.json line containing the "recharts" key is
updated and formatted to match the repo's Prettier rules.
🧹 Nitpick comments (32)
apps/webapp/app/assets/icons/ArrowTopRightBottomLeftIcon.tsx (1)
4-19: Duplicate overlapping paths detected.Paths 1 and 3 share the same starting point (
M14.8258 10.5L20.125 5.20083) and draw similar geometry for the top-right arrow. Similarly, paths 2 and 4 both start atM2 21.0625Vfor the bottom-left arrow. This results in redundant overlapping SVG content that increases file size without visual benefit.Consider consolidating to just 2 paths (one per arrow) or verifying the intended design from the source.
apps/webapp/app/assets/icons/AbacusIcon.tsx (1)
4-4: OrphanedclipPathreference.The
<g>element referencesclipPath="url(#clip0_16909_120578)", but no<clipPath>definition with that ID exists in the SVG. The browser ignores invalid references, so the icon renders correctly, but this is dead code likely left over from a design tool export.Either add the missing
<clipPath>definition or remove the unused attribute:🧹 Suggested cleanup
- <g clipPath="url(`#clip0_16909_120578`)"> + <g>apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
274-288: Prefer type aliases over interfaces for TS exports.Convert
TimeFilterApplyValuesandTimeFilterPropstotypeto align with the TS style guideline.♻️ Proposed refactor
-export interface TimeFilterApplyValues { - period?: string; - from?: string; - to?: string; -} +export type TimeFilterApplyValues = { + period?: string; + from?: string; + to?: string; +}; -export interface TimeFilterProps { - defaultPeriod?: string; - /** Label name used in the filter display, defaults to "Created" */ - labelName?: string; - applyShortcut?: ShortcutDefinition | undefined; - /** Callback when the user applies a time filter selection, receives the applied values */ - onApply?: (values: TimeFilterApplyValues) => void; -} +export type TimeFilterProps = { + defaultPeriod?: string; + /** Label name used in the filter display, defaults to "Created" */ + labelName?: string; + applyShortcut?: ShortcutDefinition | undefined; + /** Callback when the user applies a time filter selection, receives the applied values */ + onApply?: (values: TimeFilterApplyValues) => void; +};As per coding guidelines, please prefer types over interfaces in TS.
apps/webapp/app/components/primitives/SegmentedControl.tsx (1)
80-86: Prefer named export over default export.This file still uses a default export, which conflicts with the repo guideline for TS/TSX. Consider switching to a named export and updating imports accordingly.
♻️ Proposed refactor
-export default function SegmentedControl({ +export function SegmentedControl({ name, value, defaultValue, options, variant = "secondary/medium", fullWidth, onChange, }: SegmentedControlProps) {apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/TRQLGuideContent.tsx (1)
71-138: Consider extracting duplicated query strings.The query strings are duplicated between the
codeprop and theonTrycallback. Consider defining them as constants to avoid maintenance issues.Example refactor for basic queries section
+const BASIC_QUERY_EXAMPLE = `SELECT run_id, task_identifier, status +FROM runs +LIMIT 10`; + <TryableCodeBlock - code={`SELECT run_id, task_identifier, status -FROM runs -LIMIT 10`} - onTry={() => - onTryExample( - `SELECT run_id, task_identifier, status -FROM runs -LIMIT 10`, - "environment" - ) - } + code={BASIC_QUERY_EXAMPLE} + onTry={() => onTryExample(BASIC_QUERY_EXAMPLE, "environment")} />apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (2)
467-471: Consider using zod schema forAITimeFiltertype consistency.Since
ActionSchemaalready defines the same fields with zod, you could derive this type from the schema for consistency and to avoid duplication. However, this is optional since the current approach is also valid.Alternative using zod inference
// If you want to derive from ActionSchema: type AITimeFilter = Pick<z.infer<typeof ActionSchema>, 'period' | 'from' | 'to'>;
513-513: Simplify the loading state check.The condition can be simplified since
navigation.formMethodis only set when submitting, so checkingstate === "loading"with a POST method is redundant.Simplified check
- const isLoading = (navigation.state === "submitting" || navigation.state === "loading") && navigation.formMethod === "POST"; + const isLoading = navigation.state === "submitting" && navigation.formMethod === "POST";Note: If you need to show loading during the redirect/revalidation phase after submission, the current approach is correct. Verify the intended behavior.
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx (1)
127-139: Consider adding runtime validation for tool args.The type assertion on line 130 assumes
part.argsmatches the expected shape. While this is reasonable given the tool name check, consider using Zod validation for safer handling of AI-generated arguments.♻️ Optional: Add runtime validation
+const TimeFilterArgsSchema = z.object({ + period: z.string().optional(), + from: z.string().optional(), + to: z.string().optional(), +}); + // If it's a setTimeFilter call, emit the time_filter event immediately if (part.toolName === "setTimeFilter") { - const args = part.args as { period?: string; from?: string; to?: string }; + const parsed = TimeFilterArgsSchema.safeParse(part.args); + if (parsed.success) { + const args = parsed.data; sendEvent({ type: "time_filter", filter: { period: args.period, from: args.from, to: args.to, }, }); + } }apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx (1)
5-9: DuplicateAITimeFilterdefinition.Same duplication issue as noted in
QueryHelpSidebar.tsx. Import from a shared location instead.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHistoryPopover.tsx (1)
38-77: Remove dead code inhighlightSQL.The
suffixvariable is always an empty string (line 41), making the conditional block on lines 72-74 unreachable. This appears to be leftover from a truncation feature that was removed.🧹 Remove dead code
function highlightSQL(query: string): React.ReactNode[] { // Normalize whitespace for display (let CSS line-clamp handle truncation) const normalized = query.replace(/\s+/g, " ").slice(0, 200); - const suffix = ""; // Create a regex pattern that matches keywords as whole words (case insensitive) const keywordPattern = new RegExp( `\\b(${SQL_KEYWORDS.map((k) => k.replace(/\s+/g, "\\s+")).join("|")})\\b`, "gi" ); const parts: React.ReactNode[] = []; let lastIndex = 0; let match; while ((match = keywordPattern.exec(normalized)) !== null) { // Add text before the match if (match.index > lastIndex) { parts.push(normalized.slice(lastIndex, match.index)); } // Add the highlighted keyword parts.push( <span key={match.index} className="text-[`#c678dd`]"> {match[0]} </span> ); lastIndex = keywordPattern.lastIndex; } // Add remaining text if (lastIndex < normalized.length) { parts.push(normalized.slice(lastIndex)); } - if (suffix) { - parts.push(suffix); - } - return parts; }apps/webapp/app/v3/services/aiQueryService.server.ts (4)
14-18: Usetypeinstead ofinterfaceper coding guidelines.The coding guidelines specify using types over interfaces for TypeScript.
Suggested fix
-export interface AITimeFilter { - period?: string; - from?: string; - to?: string; -} +export type AITimeFilter = { + period?: string; + from?: string; + to?: string; +};
49-53: Usetypeinstead ofinterfaceper coding guidelines.Suggested fix
-interface QueryValidationResult { - valid: boolean; - syntaxError?: string; - issues: ValidationIssue[]; -} +type QueryValidationResult = { + valid: boolean; + syntaxError?: string; + issues: ValidationIssue[]; +};
88-145: Significant code duplication in tool definitions.The
validateTSQLQuery,getTableSchema, andsetTimeFiltertools are defined identically in bothstreamQueryandcallmethods. Consider extracting the tools object into a private method to reduce duplication and maintenance burden.Suggested refactor
private buildTools() { return { validateTSQLQuery: tool({ description: "Validate a TSQL query...", parameters: z.object({ query: z.string().describe("The TSQL query to validate") }), execute: async ({ query }) => this.validateQuery(query), }), getTableSchema: tool({ description: "Get detailed schema information...", parameters: z.object({ tableName: z.string().optional().describe("...") }), execute: async ({ tableName }) => this.getSchemaInfo(tableName), }), setTimeFilter: tool({ description: "Set the time filter for the query page UI...", parameters: z.object({ /* ... */ }), execute: async ({ period, from, to }) => { this.pendingTimeFilter = { period, from, to }; return { success: true, message: /* ... */ }; }, }), }; }Also applies to: 187-244
112-145: Consider validating that at least one time filter parameter is provided.All parameters (
period,from,to) are optional. If the AI callssetTimeFilterwith no arguments,pendingTimeFilterwill be set to{ period: undefined, from: undefined, to: undefined }, which is effectively empty. Consider adding validation or using zod's.refine()to ensure at least one parameter is provided.apps/webapp/app/components/code/AIQueryInput.tsx (1)
23-27: Duplicated type definition.
AITimeFilteris defined here and also inapps/webapp/app/v3/services/aiQueryService.server.ts. Consider importing from a shared location to avoid drift between the definitions.Additionally, per coding guidelines, use
typeinstead ofinterface.Suggested fix
-interface AITimeFilter { - period?: string; - from?: string; - to?: string; -} +type AITimeFilter = { + period?: string; + from?: string; + to?: string; +};internal-packages/tsql/src/index.ts (1)
440-457: Usetypeinstead ofinterfaceper coding guidelines.Suggested fix
-export interface SimpleComparisonFallback { - /** The comparison operator */ - op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte"; - /** The value to compare against */ - value: Date | string | number; -} +export type SimpleComparisonFallback = { + /** The comparison operator */ + op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte"; + /** The value to compare against */ + value: Date | string | number; +}; -export interface BetweenFallback { - /** The between operator */ - op: "between"; - /** The low bound of the range */ - low: Date | string | number; - /** The high bound of the range */ - high: Date | string | number; -} +export type BetweenFallback = { + /** The between operator */ + op: "between"; + /** The low bound of the range */ + low: Date | string | number; + /** The high bound of the range */ + high: Date | string | number; +};apps/webapp/app/components/code/ChartConfigPanel.tsx (1)
15-41: Prefer type aliases over interfaces in TS files.
Per repo guidelines, please convertChartConfigurationandChartConfigPanelPropstotypealiases.As per coding guidelines, please use types over interfaces.♻️ Proposed refactor
-export interface ChartConfiguration { +export type ChartConfiguration = { chartType: ChartType; xAxisColumn: string | null; yAxisColumns: string[]; groupByColumn: string | null; stacked: boolean; sortByColumn: string | null; sortDirection: SortDirection; aggregation: AggregationType; -} +}; -interface ChartConfigPanelProps { +type ChartConfigPanelProps = { columns: OutputColumnMetadata[]; config: ChartConfiguration; onChange: (config: ChartConfiguration) => void; className?: string; -} +};apps/webapp/app/components/primitives/AnimatedNumber.tsx (1)
8-13: MissingmotionValuein useEffect dependency array.The
motionValueis used inside the effect but not listed in the dependency array. WhileuseMotionValuereturns a stable reference, ESLint's exhaustive-deps rule would flag this. Consider adding it for completeness.Suggested fix
useEffect(() => { animate(motionValue, value, { duration, ease: "easeInOut", }); - }, [value, duration]); + }, [motionValue, value, duration]);apps/webapp/app/components/primitives/charts/Card.tsx (1)
5-16: Consider using function declaration instead of arrow function.Per coding guidelines, function declarations are preferred over arrow function exports. This applies to the main
Cardcomponent.Suggested refactor
-export const Card = ({ children, className }: { children: ReactNode; className?: string }) => { - return ( +export function Card({ children, className }: { children: ReactNode; className?: string }) { + return ( <div className={cn( "flex flex-col rounded-lg border border-grid-bright bg-background-bright pb-2 pt-4", className )} > {children} </div> ); -}; +}apps/webapp/app/components/primitives/charts/hooks/useHighlightState.ts (1)
3-12: Consider stronger typing foractivePayload.The
activePayload: any[] | nullcould benefit from a more specific type. If the payload structure is known (e.g., from Recharts), consider defining a type for it to improve type safety and IDE support.// Example: If payload items have a known structure type PayloadItem = { dataKey: string; value: number; payload: Record<string, unknown>; // ... other Recharts payload fields }; export type HighlightState = { activeBarKey: string | null; activeDataPointIndex: number | null; activePayload: PayloadItem[] | null; tooltipActive: boolean; };apps/webapp/app/components/primitives/charts/ChartBar.tsx (1)
221-228: Variable shadowing in event handlers.The
indexparameter inonClick(line 221) andonMouseEnter(line 222) shadows the outer loop variable. While the code works correctly (using the parameterindexwhich represents the data point index), this could be confusing.Suggested rename for clarity
- onClick={(data, index, e) => handleBarClick(data, e)} - onMouseEnter={(entry, index) => { + onClick={(data, _dataIndex, e) => handleBarClick(data, e)} + onMouseEnter={(entry, dataIndex) => { if (entry.tooltipPayload?.[0]) { const { dataKey: hoveredKey } = entry.tooltipPayload[0]; - highlight.setHoveredBar(hoveredKey, index); + highlight.setHoveredBar(hoveredKey, dataIndex); } }}apps/webapp/app/components/code/TSQLResultsTable.tsx (2)
216-220: Prefer a type alias forColumnMetaThe codebase guideline is to use
typeoverinterfacein TS/TSX.♻️ Suggested change
-interface ColumnMeta { - outputColumn: OutputColumnMetadata; - alignment: "left" | "right"; -} +type ColumnMeta = { + outputColumn: OutputColumnMetadata; + alignment: "left" | "right"; +};As per coding guidelines, please align this with the project’s TS style.
722-748: Use a button element for the copy affordanceThe clickable icon is a
spanwith an onClick handler, which isn’t keyboard-accessible. Abuttonwith an aria-label improves accessibility.♿ Suggested change
- <span + <button + type="button" + aria-label="Copy cell value" onClick={(e) => { e.stopPropagation(); e.preventDefault(); copy(); }} - className="absolute right-1 top-1/2 z-10 flex -translate-y-1/2 cursor-pointer" + className="absolute right-1 top-1/2 z-10 flex -translate-y-1/2 cursor-pointer" > <SimpleTooltip button={ <span className={cn( "flex size-6 items-center justify-center rounded border border-charcoal-650 bg-charcoal-750", copied ? "text-green-500" : "text-text-dimmed hover:border-charcoal-600 hover:bg-charcoal-700 hover:text-text-bright" )} > {copied ? ( <ClipboardCheckIcon className="size-3.5" /> ) : ( <ClipboardIcon className="size-3.5" /> )} </span> } content={copied ? "Copied!" : "Copy"} disableHoverableContent /> - </span> + </button>apps/webapp/app/components/primitives/charts/Chart.tsx (2)
244-248: Consider strengthening the type forExtendedLegendPayload.The
payload?: { remainingCount?: number }type is loosely defined. Since this extends from Recharts' formatter parameter, you might want to be more explicit about what additional properties are expected beyondremainingCount.
303-320: Avoid usinganyfor event handler parameters.The
onMouseEnterandonMouseLeaveprops are typed as(e: any) => void. Consider using a more specific type based on the legend item payload structure.- onMouseEnter?: (e: any) => void; - onMouseLeave?: (e: any) => void; + onMouseEnter?: (item: ExtendedLegendPayload) => void; + onMouseLeave?: (item: ExtendedLegendPayload) => void;apps/webapp/app/components/code/QueryResultsChart.tsx (1)
63-79: Consider extracting time constants to avoid duplication.The time constants (
SECOND,MINUTE,HOUR,DAY,WEEK,MONTH,YEAR) are duplicated acrossdetectTimeGranularity,detectDataInterval,fillTimeGaps, andgenerateTimeTicks. Consider extracting them to module-level constants.// At module level const SECOND = 1000; const MINUTE = 60 * SECOND; const HOUR = 60 * MINUTE; const DAY = 24 * HOUR; const WEEK = 7 * DAY; const MONTH = 30 * DAY; const YEAR = 365 * DAY;apps/webapp/app/routes/storybook.charts/route.tsx (2)
32-47:filterDataByDateRangeuses exact string matching which may be fragile.The function uses
findIndexwith exact string equality (item[dataKey] === startDate). This works only if the date strings in the data exactly match the format fromDateRangeContext. If formats differ (e.g., ISO with/without time components), filtering will fail silently and return all data.Consider using date comparison instead:
function filterDataByDateRange<T extends Record<string, any>>( data: T[], dataKey: string, startDate: string | undefined, endDate: string | undefined ): T[] { if (!startDate || !endDate) return data; const start = new Date(startDate).getTime(); const end = new Date(endDate).getTime(); return data.filter((item) => { const itemDate = new Date(item[dataKey]).getTime(); return itemDate >= start && itemDate <= end; }); }
55-61: Remove or implement the commented-out zoom sync logic.The
handleZoomChangefunction has commented-out code for syncing withDateRangeContext. Either implement it or remove the comment to avoid confusion. Currently it only logs to console.const handleZoomChange = (range: ZoomRange) => { console.log("Zoom changed:", range); - // Update the shared date range context so all charts sync - // dateRange?.setDateRange(range.start, range.end); - // In a real app, you would fetch new data here based on the range: - // fetchChartData(range.start, range.end).then(setData); + // TODO: Implement server-side data fetching when zooming + dateRange?.setDateRange(range.start, range.end); };apps/webapp/app/components/primitives/charts/ChartZoom.tsx (3)
38-45: Unused props and variables inChartZoomcomponent.The
ChartZoomcomponent acceptssyncWithDateRangeandminDataPointsprops but doesn't use them. Additionally,globalDateRangeis fetched but never used. These are only used inuseZoomHandlers.Either:
- Remove unused props from
ChartZoomPropsif they're only foruseZoomHandlers- Or document that these props are for composing with
useZoomHandlers-export function ChartZoom({ syncWithDateRange = false, minDataPoints = 3 }: ChartZoomProps) { - const { zoom, data, dataKey, onZoomChange } = useChartContext(); - const globalDateRange = useDateRange(); +export function ChartZoom(_props: ChartZoomProps) { + const { zoom } = useChartContext();
58-61: Avoid usinganyfor event parameter.The
onClickhandler usesanytype. Consider using a more specific type or at leastReact.MouseEvent.- onClick={(e: any) => { - e?.stopPropagation?.(); + onClick={(e: React.MouseEvent) => { + e.stopPropagation();
86-103: Use more specific types instead ofanyfor tooltip props.The
ZoomTooltipPropsusesany[]forpayload. Consider using a proper type based on what Recharts provides or creating a specific type for your use case.export type ZoomTooltipProps = { active?: boolean; - payload?: any[]; + payload?: Array<{ value?: number; name?: string; payload?: Record<string, unknown> }>; label?: string;apps/webapp/app/components/primitives/charts/ChartContext.tsx (1)
13-36: Consider a generic type for chart data.The
data: any[]type is very loose. Consider using a generic or a more specific base type to improve type safety:export type ChartContextValue<T extends Record<string, unknown> = Record<string, unknown>> = { // Core data config: ChartConfig; data: T[]; dataKey: string; // ... };However, this might require propagating the generic through multiple components, so
any[]may be acceptable for flexibility.
| // Handle DateTime types - format for display | ||
| if (isDateTimeType(column.type)) { | ||
| if (typeof value === "string") { | ||
| try { | ||
| const date = new Date(value); | ||
| // Format as a searchable string: "15 Jan 2026 12:34:56" | ||
| return date.toLocaleDateString("en-GB", { | ||
| day: "numeric", | ||
| month: "short", | ||
| year: "numeric", | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| second: "2-digit", | ||
| }); | ||
| } catch { | ||
| return String(value); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "TSQLResultsTable.tsx" | head -20Repository: triggerdotdev/trigger.dev
Length of output: 123
🌐 Web query:
toLocaleDateString with time options hour minute second JavaScript behavior
💡 Result:
Short answer: toLocaleDateString is intended for the date portion, but if you pass time-related options (hour/minute/second or timeZoneName) the ECMAScript Intl rules allow implementations to negotiate a format that may include time components — so behavior is implementation‑dependent. Use toLocaleTimeString / toLocaleString or Intl.DateTimeFormat if you need predictable time output.
References:
- toLocaleDateString docs (notes about allowed component options and format negotiation). [1]
- Intl.DateTimeFormat (hour/minute/second, timeStyle/dateStyle rules). [2]
- toLocaleTimeString (shows hour/minute/second behavior and equivalence to Intl.DateTimeFormat). [3]
Sources:
[1] MDN — Date.prototype.toLocaleDateString().
[2] MDN — Intl.DateTimeFormat() constructor.
[3] MDN — Date.prototype.toLocaleTimeString().
🏁 Script executed:
cat -n apps/webapp/app/components/code/TSQLResultsTable.tsx | sed -n '105,121p'Repository: triggerdotdev/trigger.dev
Length of output: 704
🏁 Script executed:
# Check the broader context around this code section
cat -n apps/webapp/app/components/code/TSQLResultsTable.tsx | sed -n '95,130p'Repository: triggerdotdev/trigger.dev
Length of output: 1217
🏁 Script executed:
# Check if there are any related tests or comments about this formatting
grep -n "toLocaleDateString\|toLocaleString\|isDateTimeType" apps/webapp/app/components/code/TSQLResultsTable.tsxRepository: triggerdotdev/trigger.dev
Length of output: 291
🏁 Script executed:
# Let's check the full context of this file to understand the function signature
head -40 apps/webapp/app/components/code/TSQLResultsTable.tsxRepository: triggerdotdev/trigger.dev
Length of output: 1712
🌐 Web query:
ECMAScript Intl.DateTimeFormat toLocaleDateString vs toLocaleString time components specification
💡 Result:
Short answer
- toLocaleDateString produces a date-only formatting because it passes options through ToDateTimeOptions with the "date","date" fields (time components are omitted).
- toLocaleString (and Intl.DateTimeFormat without restricting to date-only) uses ToDateTimeOptions with "any","all" and may include time components if present in options or resolved by the formatter.
So use toLocaleDateString when you want date-only output; use toLocaleString or Intl.DateTimeFormat when you need date+time or explicit time components. (402.ecma-international.org)
Details / spec pointers
- The ECMAScript Internationalization spec defines the abstract steps: Date.prototype.toLocaleDateString calls ToDateTimeOptions(..., "date", "date") before creating Intl.DateTimeFormat, which restricts options to date components. Date.prototype.toLocaleString calls ToDateTimeOptions(..., "any", "all"), allowing time components. (402.ecma-international.org)
- The allowed date/time component option names (year, month, day, hour, minute, second, timeZoneName, etc.) and their permitted values are defined in the DateTimeFormat section. (402.ecma-international.org)
- MDN documents the practical effect: these Date.prototype helpers delegate to Intl.DateTimeFormat and toLocaleDateString is the date-only shortcut while toLocaleString can return both date and time depending on options/resolution. (developer.mozilla.org)
If you want, I can show concise example code demonstrating the difference.
Citations:
- 1: https://402.ecma-international.org/1.0/?utm_source=openai
- 2: https://402.ecma-international.org/1.0/?utm_source=openai
- 3: https://402.ecma-international.org/3.0/?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString?utm_source=openai
DateTime formatting should use toLocaleString for time components
toLocaleDateString restrices options to date-only components per ECMAScript spec; time options (hour, minute, second) are ignored. Since the comment indicates the intended output is "15 Jan 2026 12:34:56" (including time), use toLocaleString instead:
Suggested change
- return date.toLocaleDateString("en-GB", {
+ return date.toLocaleString("en-GB", {
day: "numeric",
month: "short",
year: "numeric",
hour: "2-digit",
minute: "2-digit",
second: "2-digit",
});📝 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.
| // Handle DateTime types - format for display | |
| if (isDateTimeType(column.type)) { | |
| if (typeof value === "string") { | |
| try { | |
| const date = new Date(value); | |
| // Format as a searchable string: "15 Jan 2026 12:34:56" | |
| return date.toLocaleDateString("en-GB", { | |
| day: "numeric", | |
| month: "short", | |
| year: "numeric", | |
| hour: "2-digit", | |
| minute: "2-digit", | |
| second: "2-digit", | |
| }); | |
| } catch { | |
| return String(value); | |
| } | |
| // Handle DateTime types - format for display | |
| if (isDateTimeType(column.type)) { | |
| if (typeof value === "string") { | |
| try { | |
| const date = new Date(value); | |
| // Format as a searchable string: "15 Jan 2026 12:34:56" | |
| return date.toLocaleString("en-GB", { | |
| day: "numeric", | |
| month: "short", | |
| year: "numeric", | |
| hour: "2-digit", | |
| minute: "2-digit", | |
| second: "2-digit", | |
| }); | |
| } catch { | |
| return String(value); | |
| } |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/code/TSQLResultsTable.tsx` around lines 105 - 121,
The datetime formatting branch in TSQLResultsTable uses toLocaleDateString but
passes time options (hour, minute, second) which are ignored; update the code in
the isDateTimeType handling (where value is parsed into const date = new
Date(value)) to call date.toLocaleString("en-GB", { day: "numeric", month:
"short", year: "numeric", hour: "2-digit", minute: "2-digit", second: "2-digit"
}) instead of toLocaleDateString, preserving the try/catch and the fallback
return String(value).
| // Empty state | ||
| if (rows.length === 0) { | ||
| return ( | ||
| <div | ||
| className="h-full min-h-0 w-full overflow-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600" | ||
| style={{ position: "relative" }} | ||
| > | ||
| <table style={{ display: "grid" }}> | ||
| <thead | ||
| className="bg-background-dimmed" | ||
| style={{ | ||
| display: "grid", | ||
| position: "sticky", | ||
| top: 0, | ||
| zIndex: 1, | ||
| }} | ||
| > | ||
| {table.getHeaderGroups().map((headerGroup) => ( | ||
| <tr key={headerGroup.id} style={{ display: "flex", width: "100%" }}> | ||
| {headerGroup.headers.map((header) => { | ||
| const meta = header.column.columnDef.meta as ColumnMeta | undefined; | ||
| return ( | ||
| <th | ||
| key={header.id} | ||
| className="group/header relative" | ||
| style={{ | ||
| display: "flex", | ||
| width: header.getSize(), | ||
| }} | ||
| > | ||
| <HeaderCellContent | ||
| alignment={meta?.alignment ?? "left"} | ||
| tooltip={meta?.outputColumn.description} | ||
| onFilterClick={() => { | ||
| if (!showFilters) { | ||
| setFocusFilterColumn(header.id); | ||
| } else { | ||
| setColumnFilters([]); | ||
| } | ||
| setShowFilters(!showFilters); | ||
| }} | ||
| showFilters={showFilters} | ||
| hasActiveFilter={!!header.column.getFilterValue()} | ||
| sortDirection={header.column.getIsSorted()} | ||
| onSortClick={header.column.getToggleSortingHandler()} | ||
| canSort={header.column.getCanSort()} | ||
| > | ||
| {flexRender(header.column.columnDef.header, header.getContext())} | ||
| </HeaderCellContent> | ||
| {/* Column resizer */} | ||
| <div | ||
| onDoubleClick={() => header.column.resetSize()} | ||
| onMouseDown={header.getResizeHandler()} | ||
| onTouchStart={header.getResizeHandler()} | ||
| className={cn( | ||
| "absolute right-0 top-0 h-full w-0.5 cursor-col-resize touch-none select-none", | ||
| "opacity-0 group-hover/header:opacity-100", | ||
| "bg-charcoal-600 hover:bg-indigo-500", | ||
| header.column.getIsResizing() && "bg-indigo-500 opacity-100" | ||
| )} | ||
| /> | ||
| </th> | ||
| ); | ||
| })} | ||
| </tr> | ||
| ))} | ||
| {/* Filter row - shown when filters are toggled */} | ||
| {showFilters && ( | ||
| <tr style={{ display: "flex", width: "100%" }}> | ||
| {table.getHeaderGroups()[0]?.headers.map((header) => ( | ||
| <FilterCell | ||
| key={`filter-${header.id}`} | ||
| column={header.column} | ||
| width={header.getSize()} | ||
| shouldFocus={focusFilterColumn === header.id} | ||
| onFocused={() => setFocusFilterColumn(null)} | ||
| /> | ||
| ))} | ||
| </tr> | ||
| )} | ||
| </thead> | ||
| <tbody style={{ display: "grid" }}> | ||
| <tr style={{ display: "flex" }}> | ||
| <td> | ||
| <Paragraph variant="extra-small" className="p-4 text-text-dimmed"> | ||
| No results | ||
| </Paragraph> | ||
| </td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Empty state ignores filtered results
This only checks rows.length. When filters reduce tableRows to zero, the table renders blank without the “No results” state. Consider checking tableRows.length instead.
💡 Suggested change
- if (rows.length === 0) {
+ if (tableRows.length === 0) {🤖 Prompt for AI Agents
In `@apps/webapp/app/components/code/TSQLResultsTable.tsx` around lines 956 -
1049, The empty-state check currently uses rows.length which ignores active
table filters; change the condition to check the table's current row model
instead (e.g. replace rows.length === 0 with table.getRowModel().rows.length ===
0 or the existing tableRows variable if present) so the "No results" UI is shown
when filters reduce the visible rows to zero; update the if condition where rows
is checked in the component (around the top of the empty-state JSX block) to use
table.getRowModel().rows.length (or tableRows.length) and keep the rest of the
rendering logic unchanged.
| // Get current data for the legend based on hover state | ||
| const currentData = useMemo(() => { | ||
| if (!highlight.activePayload?.length) return totals; | ||
|
|
||
| // If we have activePayload data from hovering over a bar | ||
| const hoverData = highlight.activePayload.reduce((acc, item) => { | ||
| if (item.dataKey && item.value !== undefined) { | ||
| acc[item.dataKey] = item.value; | ||
| } | ||
| return acc; | ||
| }, {} as Record<string, number>); | ||
|
|
||
| // Return a merged object - totals for keys not in the hover data | ||
| return { | ||
| ...totals, | ||
| ...hoverData, | ||
| }; | ||
| }, [highlight.activePayload, totals]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the ChartLegendCompound.tsx file
cat -n apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxRepository: triggerdotdev/trigger.dev
Length of output: 10885
🏁 Script executed:
# Search for AnimatedNumber component to understand what type it expects
rg "AnimatedNumber" apps/webapp/app/components/primitives/charts/ -A 3 -B 3Repository: triggerdotdev/trigger.dev
Length of output: 5463
🏁 Script executed:
# Look for how highlight.activePayload is used elsewhere in the codebase
rg "activePayload" apps/webapp/app/components/primitives/charts/ -A 2 -B 2Repository: triggerdotdev/trigger.dev
Length of output: 8346
🏁 Script executed:
# Check Recharts types to understand activePayload structure
rg -t js -t ts "activePayload" apps/webapp/app --max-count 20 -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 8346
🏁 Script executed:
# Find the AnimatedNumber component definition
find apps/webapp/app/components/primitives -name "AnimatedNumber*" -type fRepository: triggerdotdev/trigger.dev
Length of output: 125
🏁 Script executed:
# Read the AnimatedNumber component
cat -n apps/webapp/app/components/primitives/AnimatedNumber.tsxRepository: triggerdotdev/trigger.dev
Length of output: 711
Coerce hovered values to numbers in the currentData object
activePayload values can be strings depending on the chart data source; AnimatedNumber expects numeric values. Currently, currentTotal already coerces values using Number(item.value) || 0 (line 53), but currentData (line 81) stores raw values without coercion. Apply consistent coercion to currentData to prevent type mismatches when rendering legend items.
Suggested change
const hoverData = highlight.activePayload.reduce((acc, item) => {
if (item.dataKey && item.value !== undefined) {
- acc[item.dataKey] = item.value;
+ acc[item.dataKey] = Number(item.value) || 0;
}
return acc;
}, {} as Record<string, number>);🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/charts/ChartLegendCompound.tsx` around
lines 74 - 91, The currentData computed in the useMemo (referencing
highlight.activePayload and totals) stores raw hover values which can be
strings; coerce hovered values to numbers when building hoverData so legend
rendering matches currentTotal/AnimatedNumber expectations—replace use of
item.value with Number(item.value) || 0 inside the reduce that constructs
hoverData (in the currentData useMemo) so the returned merged object contains
numeric values only.
| type LegacyChartLineProps = { | ||
| config: ChartConfig; | ||
| data: any[]; | ||
| dataKey: string; | ||
| state?: ChartState; | ||
| /** @deprecated Use onZoomChange callback instead */ | ||
| useGlobalDateRange?: boolean; | ||
| lineType?: CurveType; | ||
| /** Series keys to render (if not provided, derived from config keys) */ | ||
| series?: string[]; | ||
| /** Custom X-axis props to merge with defaults */ | ||
| xAxisProps?: Partial<XAxisProps>; | ||
| /** Custom Y-axis props to merge with defaults */ | ||
| yAxisProps?: Partial<YAxisProps>; | ||
| /** Render as stacked area chart instead of line chart */ | ||
| stacked?: boolean; | ||
| /** Custom tooltip label formatter */ | ||
| tooltipLabelFormatter?: (label: string, payload: any[]) => string; | ||
| /** Show legend (default false) */ | ||
| showLegend?: boolean; | ||
| /** Enable zoom selection (default false for line charts) */ | ||
| enableZoom?: boolean; | ||
| /** Callback when zoom range changes */ | ||
| onZoomChange?: (range: ZoomRange) => void; | ||
| /** Additional className for the container */ | ||
| className?: string; | ||
| /** Minimum height for the chart */ | ||
| minHeight?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Legacy ChartLine component for backward compatibility. | ||
| * Uses the new compound component system internally. | ||
| * | ||
| * For new code, prefer the compound component API: | ||
| * ```tsx | ||
| * <Chart.Root config={config} data={data} dataKey="day"> | ||
| * <Chart.Line type="step" /> | ||
| * <Chart.Legend simple /> | ||
| * </Chart.Root> | ||
| * ``` | ||
| */ | ||
| export function ChartLine({ | ||
| config, | ||
| data, | ||
| dataKey, | ||
| state, | ||
| useGlobalDateRange = false, | ||
| lineType = "step", | ||
| series, | ||
| xAxisProps, | ||
| yAxisProps, | ||
| stacked = false, | ||
| tooltipLabelFormatter, | ||
| showLegend = false, | ||
| enableZoom = false, | ||
| onZoomChange, | ||
| className, | ||
| minHeight, | ||
| }: LegacyChartLineProps) { | ||
| return ( | ||
| <ChartRoot | ||
| config={config} | ||
| data={data} | ||
| dataKey={dataKey} | ||
| series={series} | ||
| state={state} | ||
| enableZoom={enableZoom} | ||
| onZoomChange={onZoomChange} | ||
| minHeight={minHeight} | ||
| className={className} | ||
| showLegend={showLegend} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "useGlobalDateRange" --type ts --type tsxRepository: triggerdotdev/trigger.dev
Length of output: 96
🏁 Script executed:
rg -n "ChartLine\s*\(" --type ts --type tsx -A 5Repository: triggerdotdev/trigger.dev
Length of output: 96
🏁 Script executed:
rg -n "useGlobalDateRange"Repository: triggerdotdev/trigger.dev
Length of output: 579
🏁 Script executed:
rg -n "ChartLine\s*\(" -A 5Repository: triggerdotdev/trigger.dev
Length of output: 542
🏁 Script executed:
# Check the full ChartLine function to see if useGlobalDateRange is actually used
sed -n '277,350p' apps/webapp/app/components/primitives/charts/ChartLine.tsxRepository: triggerdotdev/trigger.dev
Length of output: 898
🏁 Script executed:
# Search for calls to ChartLine that might pass useGlobalDateRange
rg "ChartLine\s*\({1}" -A 20 | head -100Repository: triggerdotdev/trigger.dev
Length of output: 1667
🏁 Script executed:
# Search for places where ChartLine is imported
rg "import.*ChartLine" -A 2Repository: triggerdotdev/trigger.dev
Length of output: 1081
🏁 Script executed:
# Search for JSX calls to ChartLine that might include useGlobalDateRange
rg "<ChartLine" -A 15Repository: triggerdotdev/trigger.dev
Length of output: 6896
🏁 Script executed:
# Look at the storybook file that imports ChartLine
cat apps/webapp/app/routes/storybook.charts/route.tsx | head -200Repository: triggerdotdev/trigger.dev
Length of output: 7606
🏁 Script executed:
# Search for where useGlobalDateRange is passed as an argument/prop
rg "useGlobalDateRange\s*=" -B 5 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 1255
🏁 Script executed:
# Search for all usages of ChartLine component with more context
rg "ChartLine\s*\(" apps/webapp -A 20 | grep -A 20 "useGlobalDateRange"Repository: triggerdotdev/trigger.dev
Length of output: 1306
🏁 Script executed:
# Also check the storybook file more carefully for ChartLine usage
sed -n '200,400p' apps/webapp/app/routes/storybook.charts/route.tsxRepository: triggerdotdev/trigger.dev
Length of output: 9289
🏁 Script executed:
# Search for any JSX/TSX that passes useGlobalDateRange as a prop
rg "useGlobalDateRange\s*:" --type ts --type tsx -A 2 -B 2Repository: triggerdotdev/trigger.dev
Length of output: 96
🏁 Script executed:
# Search more broadly for useGlobalDateRange being passed (with various spacing)
rg "useGlobalDateRange\s*\{|useGlobalDateRange\s*=" -A 2 -B 2Repository: triggerdotdev/trigger.dev
Length of output: 830
Remove the unused useGlobalDateRange prop or implement its functionality.
The useGlobalDateRange parameter is accepted in the function signature but never used or forwarded to any child components. If this prop is no longer needed, remove it from the type definition and function signature. If it should still be supported, plumb it through to the appropriate child component. Either way, update any code paths to avoid the misleading API contract where a parameter is silently ignored.
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/charts/ChartLine.tsx` around lines 235
- 306, The LegacyChartLineProps and ChartLine function currently declare a
useGlobalDateRange prop that is never used; remove useGlobalDateRange from the
LegacyChartLineProps type, delete the parameter and its default value from the
ChartLine signature, and remove any mention in the JSDoc comment so the API is
not misleading; also search for and update any callers/tests that pass
useGlobalDateRange (or forward it to ChartRoot if you instead choose to
implement the behavior) so the codebase remains consistent.
| function ChartBarLoadingBackground() { | ||
| return ( | ||
| <motion.div | ||
| className="flex h-full w-full flex-col" | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| transition={{ duration: 0.5 }} | ||
| > | ||
| <motion.div | ||
| className="flex flex-1 items-end gap-1" | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| transition={{ duration: 0.6, delay: 0.1 }} | ||
| > | ||
| {Array.from({ length: 30 }).map((_, i) => { | ||
| const height = Math.max(15, Math.floor(Math.random() * 100)); | ||
| return ( | ||
| <motion.div | ||
| key={i} | ||
| className="flex-1 rounded-sm bg-charcoal-750/50" | ||
| style={{ height: `${height}%` }} | ||
| initial={{ height: 0, opacity: 0 }} | ||
| animate={{ height: `${height}%`, opacity: 1 }} | ||
| transition={{ | ||
| type: "spring", | ||
| stiffness: 120, | ||
| damping: 14, | ||
| mass: 1, | ||
| delay: 0.1 + i * 0.02, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Avoid non‑deterministic render output (SSR hydration risk).
Line 130–145 and Line 195–197 use Math.random() during render, which can produce different DOM/SVG between server and client. This can trigger hydration warnings and visible flicker. Prefer deterministic values (or generate after mount).
🔧 Proposed deterministic fix
function ChartBarLoadingBackground() {
+ const barHeights = useMemo(
+ () => Array.from({ length: 30 }, (_, i) => 15 + ((i * 37) % 85)),
+ []
+ );
+
return (
<motion.div
className="flex h-full w-full flex-col"
@@
- {Array.from({ length: 30 }).map((_, i) => {
- const height = Math.max(15, Math.floor(Math.random() * 100));
+ {barHeights.map((height, i) => {
return (
<motion.div- const change = Math.random() * 6 - 3; // Range from -3 to +3
+ const change = Math.sin(i * 1.7) * 3; // Deterministic, smooth variationAlso applies to: 183-197
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/primitives/charts/ChartLoading.tsx` around lines
116 - 146, ChartBarLoadingBackground (and the other loading component that uses
Math.random between lines 183–197) is producing non‑deterministic DOM during
render causing SSR/client hydration mismatch; fix by moving any Math.random()
calls out of the render path — generate the bar/line heights after mount (e.g.,
compute the array of random heights inside a useEffect/useLayoutEffect and store
in state or useRef seeded on mount) and render deterministically from that
state/ref, ensuring initial server render uses a stable placeholder and updates
on the client.
| interface AITimeFilter { | ||
| period?: string; | ||
| from?: string; | ||
| to?: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "interface AITimeFilter|type AITimeFilter" --type=ts --type=tsx -g '!*.test.*'Repository: triggerdotdev/trigger.dev
Length of output: 96
🏁 Script executed:
rg -n "interface AITimeFilter|type AITimeFilter" -g '*.ts' -g '*.tsx'Repository: triggerdotdev/trigger.dev
Length of output: 853
🏁 Script executed:
# Check the definitions in each location
echo "=== aiQueryService.server.ts (exported) ===" && sed -n '14,18p' apps/webapp/app/v3/services/aiQueryService.server.ts
echo "=== route.tsx ===" && sed -n '467,471p' apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
echo "=== AITabContent.tsx ===" && sed -n '5,9p' apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
echo "=== QueryHelpSidebar.tsx ===" && sed -n '14,18p' apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsx
echo "=== AIQueryInput.tsx ===" && sed -n '23,27p' apps/webapp/app/components/code/AIQueryInput.tsxRepository: triggerdotdev/trigger.dev
Length of output: 731
🏁 Script executed:
# Find the directory
fd -t d "_app.orgs" apps/webapp/app/routes/
# List contents to get full path
ls -la apps/webapp/app/routes/ | grep -i "_app"Repository: triggerdotdev/trigger.dev
Length of output: 11732
🏁 Script executed:
ls -la "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/"Repository: triggerdotdev/trigger.dev
Length of output: 748
🏁 Script executed:
# Read AITabContent.tsx
echo "=== AITabContent.tsx ===" && cat -n "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/AITabContent.tsx" | head -20
echo ""
echo "=== QueryHelpSidebar.tsx ===" && cat -n "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/QueryHelpSidebar.tsx" | head -25
echo ""
echo "=== route.tsx ===" && cat -n "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/route.tsx" | head -10 && echo "..." && cat -n "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/route.tsx" | sed -n '465,475p'
echo ""
echo "=== AIQueryInput.tsx ===" && cat -n "apps/webapp/app/components/code/AIQueryInput.tsx" | head -30Repository: triggerdotdev/trigger.dev
Length of output: 4024
🏁 Script executed:
# Check if these files import from aiQueryService.server
echo "=== Checking current imports in files ===" && \
grep -n "import.*aiQueryService" \
"apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/AITabContent.tsx" \
"apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/QueryHelpSidebar.tsx" \
"apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/route.tsx" \
"apps/webapp/app/components/code/AIQueryInput.tsx" 2>&1
echo ""
echo "=== Checking if aiQueryService.server is importable in route context ===" && \
grep -n "from.*v3/services" \
"apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query/route.tsx" | head -5Repository: triggerdotdev/trigger.dev
Length of output: 184
🏁 Script executed:
# Check if aiQueryService.server has server-only marker
echo "=== aiQueryService.server.ts top lines ===" && cat -n "apps/webapp/app/v3/services/aiQueryService.server.ts" | head -25
echo ""
echo "=== How ai-generate.tsx imports from aiQueryService.server ===" && cat -n "apps/webapp/app/routes/resources.orgs.\$organizationSlug.projects.\$projectParam.env.\$envParam.query.ai-generate.tsx" | head -25Repository: triggerdotdev/trigger.dev
Length of output: 2208
Consolidate duplicated AITimeFilter type and apply TypeScript guidelines.
This interface is defined in 6 locations (not 3):
aiQueryService.server.ts(exported, lines 14-18)ai-generate.tsx(correctly imports fromaiQueryService.server)route.tsx(lines 467-471)AITabContent.tsx(lines 5-9)QueryHelpSidebar.tsx(lines 14-18)AIQueryInput.tsx(lines 23-27)
Two issues:
- Duplication: Extract to a shared types file. Client components (AITabContent, QueryHelpSidebar, AIQueryInput) shouldn't import from
.serverfiles per Remix conventions, so create a types file in the query route folder that both client and server code can import. - Type guideline violation: Convert from
interfacetotypeper the coding guidelines.
🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsx
around lines 14 - 18, The AITimeFilter interface is duplicated across server and
client files; extract it into a single exported type alias (change from
"interface AITimeFilter" to "export type AITimeFilter = { period?: string;
from?: string; to?: string }") in a new shared types file colocated with the
query route (so both server and client code can import it), then update all
usages/imports in aiQueryService.server.ts, ai-generate.tsx, route.tsx,
AITabContent.tsx, QueryHelpSidebar.tsx, and AIQueryInput.tsx to import the named
type from that new file (and remove any imports from .server files in client
components). Ensure the type is exported so server and client modules can
reference it.
| // Check if the query contains triggered_at in a WHERE clause | ||
| // This disables the time filter UI since the user is filtering in their query | ||
| const queryHasTriggeredAt = /\bWHERE\b[\s\S]*\btriggered_at\b/i.test(query); | ||
|
|
There was a problem hiding this comment.
Regex for triggered_at detection has limitations.
The regex /\bWHERE\b[\s\S]*\btriggered_at\b/i will match triggered_at appearing anywhere after WHERE, including in string literals or comments. While this is only used for UI hints (disabling the time filter), it could cause minor UX confusion.
For example, a query like SELECT * FROM runs WHERE status = 'triggered_at test' would incorrectly disable the time filter UI.
🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
around lines 368 - 371, The current detection assigned to queryHasTriggeredAt
uses the regex /\bWHERE\b[\s\S]*\btriggered_at\b/i which can match occurrences
inside string literals or comments; update the logic to first remove/strip SQL
string literals and comments (single-quoted, double-quoted, backtick, -- line
comments and /* */ block comments) from the raw query, then run the existing
/\bWHERE\b[\s\S]*\btriggered_at\b/i test against the cleaned query; implement
this as a small helper (e.g., stripSqlLiteralsOrComments) and use it where
queryHasTriggeredAt is computed so the UI only disables the time filter for real
column references.
| const readRows = parseInt(stats.read_rows, 10); | ||
| const readBytes = parseInt(stats.read_bytes, 10); | ||
| const elapsedNs = parseInt(stats.elapsed_ns, 10); | ||
| const byteSeconds = parseFloat(stats.byte_seconds); | ||
|
|
||
| const elapsedMs = elapsedNs / 1_000_000; | ||
| const formattedTime = | ||
| elapsedMs < 1000 ? `${elapsedMs.toFixed(1)}ms` : `${(elapsedMs / 1000).toFixed(2)}s`; | ||
| const formattedBytes = formatBytes(readBytes); | ||
|
|
||
| return `${readRows.toLocaleString()} rows read · ${formattedBytes} · ${formattedTime} · ${formatBytes( | ||
| byteSeconds | ||
| )}s`; |
There was a problem hiding this comment.
Guard against NaN/precision loss in stats parsing.
If any stat is non‑numeric, the UI will show NaN values; large counters can also lose precision. Consider finite checks (and BigInt if values can exceed Number.MAX_SAFE_INTEGER) with a safe fallback.
🛠️ Suggested hardening
- const readRows = parseInt(stats.read_rows, 10);
- const readBytes = parseInt(stats.read_bytes, 10);
- const elapsedNs = parseInt(stats.elapsed_ns, 10);
- const byteSeconds = parseFloat(stats.byte_seconds);
+ const readRows = Number(stats.read_rows);
+ const readBytes = Number(stats.read_bytes);
+ const elapsedNs = Number(stats.elapsed_ns);
+ const byteSeconds = Number(stats.byte_seconds);
+
+ if (![readRows, readBytes, elapsedNs, byteSeconds].every(Number.isFinite)) {
+ return "—";
+ }🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts
around lines 7 - 19, The parsing code in utils.ts that computes readRows,
readBytes, elapsedNs, and byteSeconds should guard against non-numeric inputs
and potential precision loss: validate parsed values with Number.isFinite and
fall back to a safe default (e.g., 0 or a placeholder) when parseInt/parseFloat
yields NaN, and when counters may exceed Number.MAX_SAFE_INTEGER, parse as
BigInt and branch to use BigInt-safe formatting (or coerce to string) for
readRows/readBytes/byteSeconds before calling formatBytes; ensure formattedTime
calculation checks for finite elapsedNs and handles BigInt or very large values
safely so the UI never displays "NaN" or loses precision.
| export function formatBytes(bytes: number): string { | ||
| if (bytes === 0) return "0 B"; | ||
| if (bytes < 0) return "-" + formatBytes(-bytes); | ||
| const k = 1024; | ||
| const sizes = ["B", "KB", "MB", "GB"]; | ||
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(k)), sizes.length - 1); | ||
| return `${parseFloat((bytes / Math.pow(k, i)).toFixed(1))} ${sizes[i]}`; |
There was a problem hiding this comment.
Fix unit selection for sub‑1 byte values.
For 0 < bytes < 1 (possible with byte_seconds), Math.log(bytes) is negative and produces an invalid unit index (sizes[-1]), yielding "undefined".
🛠️ Proposed fix
- const i = Math.min(Math.floor(Math.log(bytes) / Math.log(k)), sizes.length - 1);
+ const i = Math.max(
+ 0,
+ Math.min(Math.floor(Math.log(bytes) / Math.log(k)), sizes.length - 1)
+ );📝 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.
| export function formatBytes(bytes: number): string { | |
| if (bytes === 0) return "0 B"; | |
| if (bytes < 0) return "-" + formatBytes(-bytes); | |
| const k = 1024; | |
| const sizes = ["B", "KB", "MB", "GB"]; | |
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(k)), sizes.length - 1); | |
| return `${parseFloat((bytes / Math.pow(k, i)).toFixed(1))} ${sizes[i]}`; | |
| export function formatBytes(bytes: number): string { | |
| if (bytes === 0) return "0 B"; | |
| if (bytes < 0) return "-" + formatBytes(-bytes); | |
| const k = 1024; | |
| const sizes = ["B", "KB", "MB", "GB"]; | |
| const i = Math.max( | |
| 0, | |
| Math.min(Math.floor(Math.log(bytes) / Math.log(k)), sizes.length - 1) | |
| ); | |
| return `${parseFloat((bytes / Math.pow(k, i)).toFixed(1))} ${sizes[i]}`; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts
around lines 22 - 28, formatBytes computes a negative unit index for 0 < bytes <
1 (e.g. byte_seconds) because Math.log(bytes) < 0; update the unit index
calculation in formatBytes to clamp the computed index to a minimum of 0 and
maximum of sizes.length - 1 (e.g. use Math.max(0, Math.floor(Math.log(bytes) /
Math.log(k))) then Math.min(..., sizes.length - 1)) so sub‑1 values resolve to
the "B" unit and avoid sizes[-1].
| "react-use": "17.5.1", | ||
| "react-window-splitter": "^0.4.1", | ||
| "recharts": "^2.12.6", | ||
| "recharts": "^2.15.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify latest recharts version and check for breaking changes
echo "=== Latest Recharts Version ==="
curl -s https://registry.npmjs.org/recharts/latest | jq -r '.version'
echo -e "\n=== Recharts 2.15.2 Release Info ==="
curl -s https://registry.npmjs.org/recharts/2.15.2 | jq -r '.version, .description'
echo -e "\n=== Check for versions between 2.12.6 and 2.15.2 ==="
curl -s https://registry.npmjs.org/recharts | jq -r '.releases | to_entries[] | select(.key >= "2.12.6" and .key <= "2.15.2") | .key' | sort -VRepository: triggerdotdev/trigger.dev
Length of output: 285
🏁 Script executed:
# Check package.json file formatting and content
cd apps/webapp 2>/dev/null && {
echo "=== Package.json file exists ==="
ls -lh package.json
echo -e "\n=== Lines around 194 (recharts) ==="
sed -n '190,198p' package.json
echo -e "\n=== Check if file is Prettier formatted ==="
npx prettier --check package.json 2>/dev/null || echo "prettier not available in sandbox"
} || {
echo "apps/webapp directory not found, checking root"
ls -lh package.json 2>/dev/null || echo "No package.json at root"
}Repository: triggerdotdev/trigger.dev
Length of output: 595
🏁 Script executed:
# Find the package.json file
fd -t f "package.json" | head -20Repository: triggerdotdev/trigger.dev
Length of output: 789
🌐 Web query:
recharts 2.15.2 vs 2.12.6 breaking changes changelog
💡 Result:
Short answer: There are no breaking changes between recharts v2.12.6 and v2.15.2 — both are 2.x bugfix releases. v2.12.6 and v2.15.2 only contain fixes/chores (no documented breaking changes).
Notable items:
- v2.12.6: fixed Tooltip rendering glitch when animation disabled; added .js suffixes in package.json paths (CI/build). [1]
- v2.15.2: several bugfixes/backports (Bar/Rectangle key fix to avoid duplicate keys; re-added points in Dot props; Tooltip type and activeBar fixes; allow data-* props on SVG). [2]
Sources:
[1] Recharts v2.12.6 changelog / release notes.
[2] Recharts v2.15.2 changelog / release notes.
🏁 Script executed:
# Check git diff for package.json to see actual changes
cd apps/webapp && git diff HEAD~5..HEAD package.json 2>/dev/null | head -100 || echo "git diff not available"Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
# Check for new chart component files in the webapp
find apps/webapp -type f \( -name "*chart*" -o -name "*Chart*" \) 2>/dev/null | head -20Repository: triggerdotdev/trigger.dev
Length of output: 721
Recharts upgrade to 2.15.2 contains bugfixes with no breaking changes.
The version bump from ^2.12.6 to ^2.15.2 includes bugfixes (Bar/Rectangle key handling, Tooltip type fixes, data-* props support on SVG elements) with no breaking changes. This is safe for existing chart usage.
Ensure the file is formatted with Prettier before committing per project guidelines.
🤖 Prompt for AI Agents
In `@apps/webapp/package.json` at line 194, Bump the dependency "recharts" in
package.json to "^2.15.2" (replace the existing "recharts" entry) and then run
the project's Prettier formatting before committing; ensure the package.json
line containing the "recharts" key is updated and formatted to match the repo's
Prettier rules.
What changed