Query improvements #2905
Query improvements #2905
Conversation
|
WalkthroughAdds a compound charting system (ChartCompound) with ChartRoot, Chart.Bar, Chart.Line, Chart.Legend, Chart.Zoom, ChartContext, and related hooks (useZoomSelection, useHighlightState). Introduces Chart primitives, loading/no-data visuals, Card and BigNumber components, DateRangeContext, and a charts storybook. Reworks QueryResultsChart to a unified Chart.Root system. Replaces TSQLResultsTable with a virtualized TanStack table and adds fuzzy filtering and copyable cells. Extends AI query flow with AITimeFilter across service, SSE events, UI callbacks, and server actions. Adds where-clause fallback support to tsql/ClickHouse with tests and updates dependencies. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 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
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
✏️ 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 |
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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/routes/storybook.charts/route.tsx`:
- Around line 694-711: Remove the unused constant barChartConfig (and its
trailing type assertion "satisfies ChartConfig") from the file; locate the
declaration of barChartConfig and delete the entire object literal and the
"satisfies ChartConfig" suffix, and if ChartConfig is no longer referenced
anywhere in this module, also remove its import/usage to avoid unused symbol
warnings.
🧹 Nitpick comments (5)
apps/webapp/app/components/primitives/charts/ChartBar.tsx (2)
40-55: Consider using a more specific type forpayloadin tooltip formatter.The
tooltipLabelFormatterusesany[]for the payload parameter, which loses type safety.♻️ Suggested improvement
+import type { Payload } from "recharts/types/component/DefaultTooltipContent"; + export type ChartBarRendererProps = { // ... - tooltipLabelFormatter?: (label: string, payload: any[]) => string; + tooltipLabelFormatter?: (label: string, payload: Payload<number, string>[]) => string; // ... };
194-218: Parameter shadowing in onClick handler.The
indexparameter in theonClickcallback (line 211) shadows the outerindexfrom themapcallback (line 194). While functionally correct, this can be confusing. Consider renaming one of them for clarity.♻️ Suggested fix
- {dataKeys.map((key, index, array) => { + {dataKeys.map((key, keyIndex, array) => { return ( <Bar key={key} dataKey={key} stackId={stackId} fill={config[key]?.color} radius={ [ - index === array.length - 1 ? 2 : 0, - index === array.length - 1 ? 2 : 0, - index === 0 ? 2 : 0, - index === 0 ? 2 : 0, + keyIndex === array.length - 1 ? 2 : 0, + keyIndex === array.length - 1 ? 2 : 0, + keyIndex === 0 ? 2 : 0, + keyIndex === 0 ? 2 : 0, ] as [number, number, number, number] } activeBar={false} fillOpacity={1} - onClick={(data, index, e) => handleBarClick(data, e)} + onClick={(data, _cellIndex, e) => handleBarClick(data, e)}apps/webapp/app/components/primitives/charts/ChartLine.tsx (3)
25-37: Consider exportingCurveTypefor external use.The
CurveTypeunion is a useful type that consumers might want to use for type-safe curve specification. Currently it's not exported.♻️ Suggested improvement
-type CurveType = +export type CurveType = | "basis" | "basisClosed" // ...
126-130: Consider memoizinghandleMouseLeavefor performance.Unlike
ChartBarRendererwhich usesuseCallbackfor itshandleMouseLeave, this implementation creates a new function on every render. For consistency and to prevent unnecessary re-renders, consider memoizing it.♻️ Suggested improvement
+import React, { useCallback } from "react"; // ... - const handleMouseLeave = () => { + const handleMouseLeave = useCallback(() => { highlight.setTooltipActive(false); highlight.reset(); - }; + }, [highlight]);
132-179: Stacked area chart uses hardcodedstackId.The
stackId="stack"is hardcoded on line 173. If multiple stacked area charts need different stacking groups, this could cause issues. Consider making it configurable via props, similar to howChartBarRendereraccepts astackIdprop.♻️ Suggested improvement
export type ChartLineRendererProps = { // ... stacked?: boolean; + /** Stack ID for stacked area charts (default: "stack") */ + stackId?: string; // ... }; export function ChartLineRenderer({ // ... stacked = false, + stackId = "stack", // ... }: ChartLineRendererProps) { // ... <Area key={key} type={lineType} dataKey={key} stroke={config[key]?.color} fill={config[key]?.color} fillOpacity={0.6} strokeWidth={2} - stackId="stack" + stackId={stackId} isAnimationActive={false} />
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/primitives/charts/ChartBar.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/routes/storybook.charts/route.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/storybook.charts/route.tsxapps/webapp/app/components/primitives/charts/ChartLine.tsxapps/webapp/app/components/primitives/charts/ChartBar.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/primitives/charts/ChartBar.tsx (6)
apps/webapp/app/components/primitives/charts/ChartContext.tsx (1)
useChartContext(40-46)apps/webapp/app/components/primitives/charts/ChartRoot.tsx (1)
useHasNoData(147-160)apps/webapp/app/components/primitives/charts/ChartZoom.tsx (2)
useZoomHandlers(173-244)ZoomTooltip(105-167)apps/webapp/app/components/primitives/charts/ChartLoading.tsx (3)
ChartBarLoading(8-18)ChartBarNoData(20-39)ChartBarInvalid(41-60)apps/webapp/app/components/primitives/charts/hooks/useHighlightState.ts (1)
getBarOpacity(95-115)apps/webapp/app/components/primitives/charts/hooks/useZoomSelection.ts (1)
ZoomRange(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- 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 (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/components/primitives/charts/ChartBar.tsx (2)
1-27: LGTM - Imports and type definitions are well-structured.The imports are properly organized, separating external dependencies (recharts, React) from internal modules. The type import for
ZoomRangecorrectly usesimport typesyntax.
165-181: Tooltip content logic may be inverted.The tooltip renders
ChartTooltipContentwhentooltipLabelFormatteris provided, but usesZoomTooltipwhen it's not. However,ChartTooltipContentis typically the default content component, whileZoomTooltipis specialized for zoom interactions. This logic seems inverted - you'd expect the custom formatter to work with the zoom tooltip, not the standard one.Verify this is the intended behavior. If
tooltipLabelFormattercustomizes label formatting, it should likely apply to theZoomTooltipas well, or the condition should be checking for zoom enablement instead.apps/webapp/app/components/primitives/charts/ChartLine.tsx (2)
1-24: LGTM - Clean imports with proper separation.Imports are well-organized. The comment on line 22 clarifies the legend rendering architecture. Note that
ChartRootis imported but onlyuseHasNoDataappears to be used from it.
181-224: LGTM - LineChart implementation is clean and consistent.The
LineChartrendering follows the same patterns as theAreaChartbranch. Good use ofaccessibilityLayerprop for a11y, disabled animations for performance, and consistent axis/tooltip configuration.apps/webapp/app/routes/storybook.charts/route.tsx (5)
1-18: LGTM - Imports are well-organized.Good separation of UI primitives, chart components, and context providers. The imports follow the project conventions.
28-43: Edge case:filterDataByDateRangemay return incorrect results with duplicate keys.If the data array contains duplicate values for
dataKey,findIndexreturns only the first match, which could lead to unexpected slicing behavior. This is likely fine for date-keyed data where each date is unique, but worth noting.
51-57: Zoom handler is a no-op (intentional for demo).The
handleZoomChangefunction only logs and doesn't update the date range context. The commented-outdateRange?.setDateRangecall and the explanatory comments make it clear this is intentional scaffolding for the demo. Just confirming this is expected behavior for a storybook route.
713-786: LGTM - Rich mock data with JSX labels.The
barChartBigDatasetConfigdemonstrates the chart system's support for JSX labels (React nodes), which is a nice capability showcase. The data structure is consistent and comprehensive.
280-284: The current implementation is correct; no type mismatch exists.The
DateRangeProvideracceptsDateobjects as props (not strings), and internally converts them to ISO strings viatoISODateString()before storing in context. The code at lines 280-284 is correctly typed and requires no changes.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const barChartConfig = { | ||
| completed: { | ||
| label: "Completed", | ||
| color: "#28BF5C", | ||
| }, | ||
| "in-progress": { | ||
| label: "In Progress", | ||
| color: "#3B82F6", | ||
| }, | ||
| canceled: { | ||
| label: "Canceled", | ||
| color: "#878C99", | ||
| }, | ||
| failed: { | ||
| label: "Failed", | ||
| color: "#E11D48", | ||
| }, | ||
| } satisfies ChartConfig; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if barChartConfig is used anywhere in the file
rg -n "barChartConfig[^B]" apps/webapp/app/routes/storybook.charts/route.tsxRepository: triggerdotdev/trigger.dev
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of barChartConfig in the file
rg -n "barChartConfig" apps/webapp/app/routes/storybook.charts/route.tsxRepository: triggerdotdev/trigger.dev
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for all references to barChartConfig across the entire webapp
rg "barChartConfig" apps/webapp/Repository: triggerdotdev/trigger.dev
Length of output: 143
Remove the unused barChartConfig constant.
The barChartConfig constant defined at lines 694-711 is not used anywhere in the file or codebase. Remove it unless there's a planned use case for it.
🤖 Prompt for AI Agents
In `@apps/webapp/app/routes/storybook.charts/route.tsx` around lines 694 - 711,
Remove the unused constant barChartConfig (and its trailing type assertion
"satisfies ChartConfig") from the file; locate the declaration of barChartConfig
and delete the entire object literal and the "satisfies ChartConfig" suffix, and
if ChartConfig is no longer referenced anywhere in this module, also remove its
import/usage to avoid unused symbol warnings.
What changed