Query improvements #2905
Query improvements #2905
Conversation
|
WalkthroughAdds a compound charting system and related primitives: ChartCompound (Root, Bar, Line, Legend, Zoom), ChartContext, ChartRoot, ChartLegendCompound, ChartBar/ChartLine renderers, chart loading/no-data components, hooks (useZoomSelection, useHighlightState), Card and BigNumber primitives, and DateRangeContext. Reworks QueryResultsChart to use the new chart compound API. Replaces TSQLResultsTable with a virtualized TanStack table including fuzzy filtering, copyable cells, and dynamic column sizing. Introduces AITimeFilter across AI services, SSE events, UI callbacks, and query route 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 (1 warning, 2 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.
.../_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsx
Outdated
Show resolved
Hide resolved
| // 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.
...p/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/routes/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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/v3/services/aiQueryService.server.ts`:
- Around line 107-140: The setTimeFilter tool is duplicated in streamQuery() and
call(); extract the shared tool definition into a single private helper (e.g., a
method or getter like buildSetTimeFilterTool or this.setTimeFilterTool) and have
both streamQuery() and call() reference that helper so behavior stays consistent
and DRY; ensure the helper returns the tool object with description, parameters,
and execute which updates this.pendingTimeFilter and returns the same
success/message shape.
♻️ Duplicate comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts (1)
7-19: Guard against non‑numeric stats to avoid NaN in the UI.This is the same issue as previously flagged:
parseInt/parseFloatcan yieldNaN, which then propagates intotoLocaleString()/formatBytes(). Please add finite checks (and consider large‑number safety) before formatting.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
369-371: Regex fortriggered_atdetection has limitations.The regex
/\bWHERE\b[\s\S]*\btriggered_at\b/iwill matchtriggered_atanywhere afterWHERE, including in string literals or comments. This could cause minor UX confusion (e.g.,WHERE status = 'triggered_at test'would incorrectly disable the time filter UI).Consider this a known limitation for now, or strip SQL string literals/comments before testing.
🧹 Nitpick comments (2)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx (1)
128-140: Unsafe type assertion onpart.args.The cast
part.args as { period?: string; from?: string; to?: string }bypasses type safety. While the tool's Zod schema should ensure valid args, the SDK typespart.argsasunknown. Consider using Zod parsing for runtime validation to maintain type safety.♻️ Suggested fix using Zod validation
+ // 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 argsSchema = z.object({ + period: z.string().optional(), + from: z.string().optional(), + to: z.string().optional(), + }); + const args = argsSchema.parse(part.args); sendEvent({ type: "time_filter", filter: {apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
403-409: Empty string handling may cause unexpected behavior.The hidden inputs pass empty strings (
value={period ?? ""}) when values are null. However, on the server side (lines 256-258), the code usesperiod ?? undefined, which only convertsnulltoundefined, not empty strings. An empty string passed toparse("")could returnnullor cause unexpected behavior.Consider converting empty strings to undefined explicitly:
♻️ Suggested fix
const timeFilter = timeFilters({ - period: period ?? undefined, - from: from ?? undefined, - to: to ?? undefined, + period: period || undefined, + from: from || undefined, + to: to || undefined, defaultPeriod: DEFAULT_PERIOD, });This uses
||instead of??to treat empty strings as falsy and convert them toundefined.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/v3/services/aiQueryService.server.tspackage.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsx
- apps/webapp/app/components/code/AIQueryInput.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/AITabContent.tsx
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
package.jsonapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
**/*.{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/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
{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/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
**/*.{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/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
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/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
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/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/services/aiQueryService.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/v3/services/aiQueryService.server.ts
apps/webapp/app/v3/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Organize services in the webapp following the pattern
app/v3/services/*/*.server.ts
Files:
apps/webapp/app/v3/services/aiQueryService.server.ts
🧠 Learnings (11)
📚 Learning: 2026-01-15T10:48:02.673Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.673Z
Learning: Use pnpm as the package manager (version 10.23.0 or later) and Node.js 20.20.0
Applied to files:
package.json
📚 Learning: 2025-11-26T14:40:07.146Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2710
File: packages/schema-to-json/package.json:0-0
Timestamp: 2025-11-26T14:40:07.146Z
Learning: Node.js 24+ has native TypeScript support and can execute .ts files directly without tsx or ts-node for scripts that use only erasable TypeScript syntax (type annotations, interfaces, etc.). The trigger.dev repository uses Node.js 24.11.1+ and scripts like updateVersion.ts can be run with `node` instead of `tsx`.
Applied to files:
package.json
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Access environment variables via `env` export from `apps/webapp/app/env.server.ts`, never use `process.env` directly
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code in the webapp, never import env.server.ts in test files - pass configuration as options instead
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx
📚 Learning: 2026-01-08T15:57:09.323Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/otel-metrics.mdc:0-0
Timestamp: 2026-01-08T15:57:09.323Z
Learning: Applies to **/*.ts : Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts
📚 Learning: 2026-01-08T15:57:09.323Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/otel-metrics.mdc:0-0
Timestamp: 2026-01-08T15:57:09.323Z
Learning: Applies to **/*.ts : When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
📚 Learning: 2025-06-10T09:31:01.040Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2158
File: internal-packages/clickhouse/src/client/queryBuilder.ts:77-92
Timestamp: 2025-06-10T09:31:01.040Z
Learning: ClickHouse SQL syntax requires GROUP BY clause to come before ORDER BY clause, following standard SQL clause ordering: SELECT, FROM, WHERE, GROUP BY, HAVING, ORDER BY, LIMIT.
Applied to files:
apps/webapp/app/v3/services/aiQueryService.server.ts
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to internal-packages/clickhouse/schema/**/*.sql : Follow ClickHouse naming conventions: `raw_` prefix for input tables, `_v1`, `_v2` suffixes for versioning, `_mv_v1` suffix for materialized views
Applied to files:
apps/webapp/app/v3/services/aiQueryService.server.ts
🧬 Code graph analysis (3)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.ts (1)
AITimeFilter(5-9)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.ts (1)
apps/webapp/app/v3/services/aiQueryService.server.ts (1)
AITimeFilter(13-13)
apps/webapp/app/v3/services/aiQueryService.server.ts (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.ts (1)
AITimeFilter(5-9)
⏰ 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 / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- 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: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/utils.ts (1)
22-31: LGTM.Clamping the unit index and handling negatives keeps formatting safe and predictable.
package.json (1)
87-87: Format package.json with Prettier before committing.The package.json file has code style issues that violate the formatting guideline. Run
npx prettier --write package.jsonto fix.⛔ Skipped due to learnings
Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-01-15T10:48:02.673Z Learning: Applies to **/*.{js,ts,jsx,tsx,json,md,yaml,yml} : Format code using Prettier before committingLearnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-01-15T10:48:02.673Z Learning: Use pnpm as the package manager (version 10.23.0 or later) and Node.js 20.20.0Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-15T11:50:06.044Z Learning: Applies to {packages,integrations}/**/* : Add a changeset when modifying any public package in `packages/*` or `integrations/*` using `pnpm run changeset:add`Learnt from: ericallam Repo: triggerdotdev/trigger.dev PR: 2710 File: packages/schema-to-json/package.json:0-0 Timestamp: 2025-11-26T14:40:07.146Z Learning: Node.js 24+ has native TypeScript support and can execute .ts files directly without tsx or ts-node for scripts that use only erasable TypeScript syntax (type annotations, interfaces, etc.). The trigger.dev repository uses Node.js 24.11.1+ and scripts like updateVersion.ts can be run with `node` instead of `tsx`.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/types.ts (1)
1-9: LGTM!The type definition is clean, well-documented, and follows the coding guideline to use
typeoverinterface. The optional fields appropriately model the flexible time filter configuration.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query.ai-generate.tsx (1)
151-163: LGTM!The integration of
timeFilterinto the final result correctly retrieves the pending time filter from the service and includes it when a query is successfully extracted.apps/webapp/app/v3/services/aiQueryService.server.ts (2)
54-68: LGTM!The
pendingTimeFilterstate management is well-designed. Resetting the state at the start of eachstreamQuery/callensures clean state for each request. This works correctly since a newAIQueryServiceinstance is created per request in the route handler.
153-158: LGTM!The
getPendingTimeFilter()method provides clean access to the time filter set during query generation, enabling the caller to include it in the response.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (3)
254-276: LGTM!The time filter fallback logic correctly handles all cases: absolute ranges (both
fromandto), partial ranges (only one bound), and relative periods. The double null-coalescing on line 274 properly handles both undefined periods and invalid parse results.
441-452: LGTM!The conditional rendering provides good UX - showing "Set in query" when the user has manually added
triggered_atto their query prevents confusion about which filter takes precedence. TheTimeFiltercomponent'sonApplycallback correctly triggers form submission.
493-506: LGTM!The
handleTimeFilterChangecallback correctly propagates AI-generated time filters to the URL search params and appropriately clears pagination state (cursor,direction) when the time filter changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| setTimeFilter: tool({ | ||
| description: | ||
| "Set the time filter for the query page UI instead of adding triggered_at conditions to the query. ALWAYS use this tool when the user wants to filter by time (e.g., 'last 7 days', 'past hour', 'yesterday'). The UI will apply this filter automatically. Do NOT add triggered_at to the WHERE clause - use this tool instead.", | ||
| parameters: z.object({ | ||
| period: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| "Relative time period like '1m', '5m', '30m', '1h', '6h', '12h', '1d', '3d', '7d', '14d', '30d', '90d'. Use this for 'last X days/hours/minutes' requests." | ||
| ), | ||
| from: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| "ISO 8601 timestamp for the start of an absolute date range. Use with 'to' for specific date ranges." | ||
| ), | ||
| to: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| "ISO 8601 timestamp for the end of an absolute date range. Use with 'from' for specific date ranges." | ||
| ), | ||
| }), | ||
| execute: async ({ period, from, to }) => { | ||
| // Store the time filter so we can include it in the result | ||
| this.pendingTimeFilter = { period, from, to }; | ||
| return { | ||
| success: true, | ||
| message: period | ||
| ? `Time filter set to: last ${period}` | ||
| : `Time filter set to: ${from ?? "start"} - ${to ?? "now"}`, | ||
| }; | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate tool definition violates DRY principle.
The setTimeFilter tool is defined identically in both streamQuery() (lines 107-140) and call() (lines 206-239). Extract this to a shared private method or property to avoid duplication and ensure consistent behavior.
♻️ Suggested refactor to extract shared tool definition
export class AIQueryService {
private pendingTimeFilter: AITimeFilter | undefined;
constructor(
private readonly tableSchema: TableSchema[],
private readonly model: LanguageModelV1 = openai("gpt-4o-mini")
) {}
+ /**
+ * Build the shared tools for query generation
+ */
+ private buildTools() {
+ return {
+ validateTSQLQuery: tool({
+ description:
+ "Validate a TSQL query for syntax errors and schema compliance. Always use this tool to verify your query before returning it to the user.",
+ parameters: z.object({
+ query: z.string().describe("The TSQL query to validate"),
+ }),
+ execute: async ({ query }) => {
+ return this.validateQuery(query);
+ },
+ }),
+ getTableSchema: tool({
+ description:
+ "Get detailed schema information about available tables and columns. Use this to understand what data is available and how to query it.",
+ parameters: z.object({
+ tableName: z
+ .string()
+ .optional()
+ .describe("Optional: specific table name to get details for"),
+ }),
+ execute: async ({ tableName }) => {
+ return this.getSchemaInfo(tableName);
+ },
+ }),
+ setTimeFilter: tool({
+ description:
+ "Set the time filter for the query page UI instead of adding triggered_at conditions to the query. ALWAYS use this tool when the user wants to filter by time (e.g., 'last 7 days', 'past hour', 'yesterday'). The UI will apply this filter automatically. Do NOT add triggered_at to the WHERE clause - use this tool instead.",
+ parameters: z.object({
+ period: z
+ .string()
+ .optional()
+ .describe(
+ "Relative time period like '1m', '5m', '30m', '1h', '6h', '12h', '1d', '3d', '7d', '14d', '30d', '90d'. Use this for 'last X days/hours/minutes' requests."
+ ),
+ from: z
+ .string()
+ .optional()
+ .describe(
+ "ISO 8601 timestamp for the start of an absolute date range. Use with 'to' for specific date ranges."
+ ),
+ to: z
+ .string()
+ .optional()
+ .describe(
+ "ISO 8601 timestamp for the end of an absolute date range. Use with 'from' for specific date ranges."
+ ),
+ }),
+ execute: async ({ period, from, to }) => {
+ this.pendingTimeFilter = { period, from, to };
+ return {
+ success: true,
+ message: period
+ ? `Time filter set to: last ${period}`
+ : `Time filter set to: ${from ?? "start"} - ${to ?? "now"}`,
+ };
+ },
+ }),
+ };
+ }
streamQuery(prompt: string, options: AIQueryOptions = {}) {
// ... existing code ...
return streamText({
model: this.model,
system: systemPrompt,
prompt: userPrompt,
- tools: {
- validateTSQLQuery: tool({ ... }),
- getTableSchema: tool({ ... }),
- setTimeFilter: tool({ ... }),
- },
+ tools: this.buildTools(),
maxSteps: 5,
// ...
});
}
async call(prompt: string, options: AIQueryOptions = {}): Promise<AIQueryResult> {
// ... existing code ...
const result = await streamText({
model: this.model,
system: systemPrompt,
prompt: userPrompt,
- tools: {
- validateTSQLQuery: tool({ ... }),
- getTableSchema: tool({ ... }),
- setTimeFilter: tool({ ... }),
- },
+ tools: this.buildTools(),
maxSteps: 5,
// ...
});
// ...
}🤖 Prompt for AI Agents
In `@apps/webapp/app/v3/services/aiQueryService.server.ts` around lines 107 - 140,
The setTimeFilter tool is duplicated in streamQuery() and call(); extract the
shared tool definition into a single private helper (e.g., a method or getter
like buildSetTimeFilterTool or this.setTimeFilterTool) and have both
streamQuery() and call() reference that helper so behavior stays consistent and
DRY; ensure the helper returns the tool object with description, parameters, and
execute which updates this.pendingTimeFilter and returns the same
success/message shape.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/code/TSQLResultsTable.tsx`:
- Around line 63-69: Remove the orphaned doc comment block "Fuzzy filter
function using match-sorter ranking" that is sitting above the CellValue
formatting comment; it is a leftover and should be deleted so docs match code,
leaving the actual fuzzyFilter implementation (the function named fuzzyFilter
further down) as the single source of truth. Locate the stray comment near the
top of the file and delete it, ensuring the real fuzzyFilter function remains
unchanged.
♻️ Duplicate comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
370-372: Regex can disable the time filter whentriggered_atappears in strings/comments.The
/\bWHERE\b[\s\S]*\btriggered_at\b/itest will matchtriggered_atinside string literals or comments and hide the filter UI. Consider stripping SQL strings/comments before the regex.
🧹 Nitpick comments (3)
apps/webapp/app/components/code/TSQLResultsTable.tsx (2)
206-212: Consider stabilizingonChangereference to prevent debounce timer resets.Including
onChangein the dependency array means the debounce timer resets whenever the parent re-renders with a new callback reference. SinceFilterCellpasses an inline arrow function(value) => column.setFilterValue(value), this could reset the timer on unrelated re-renders.A common fix is to use a ref to capture the latest callback without triggering the effect:
Suggested fix
const DebouncedInput = forwardRef< HTMLInputElement, { value: string; onChange: (value: string) => void; debounce?: number; } & Omit<React.InputHTMLAttributes<HTMLInputElement>, "onChange"> >(function DebouncedInput({ value: initialValue, onChange, debounce = 300, ...props }, ref) { const [value, setValue] = useState(initialValue); + const onChangeRef = useRef(onChange); + onChangeRef.current = onChange; useEffect(() => { setValue(initialValue); }, [initialValue]); useEffect(() => { const timeout = setTimeout(() => { - onChange(value); + onChangeRef.current(value); }, debounce); return () => clearTimeout(timeout); - }, [value, debounce, onChange]); + }, [value, debounce]); return <input ref={ref} {...props} value={value} onChange={(e) => setValue(e.target.value)} />; });
218-221: Usetypeinstead ofinterfaceper coding guidelines.The coding guidelines specify to use types over interfaces for TypeScript.
Suggested fix
-interface ColumnMeta { - outputColumn: OutputColumnMetadata; - alignment: "left" | "right"; -} +type ColumnMeta = { + outputColumn: OutputColumnMetadata; + alignment: "left" | "right"; +};apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
255-261: Normalize empty-string time filter inputs beforetimeFilters.Hidden inputs submit
"", andperiod ?? undefinedwon’t convert that, sotimeFiltersmay see empty strings as explicit values. Consider trimming and normalizing toundefinedfirst.Proposed normalization
- const timeFilter = timeFilters({ - period: period ?? undefined, - from: from ?? undefined, - to: to ?? undefined, - defaultPeriod: DEFAULT_PERIOD, - }); + const normalizedPeriod = period?.trim() ? period : undefined; + const normalizedFrom = from?.trim() ? from : undefined; + const normalizedTo = to?.trim() ? to : undefined; + const timeFilter = timeFilters({ + period: normalizedPeriod, + from: normalizedFrom, + to: normalizedTo, + defaultPeriod: DEFAULT_PERIOD, + });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/components/code/TSQLResultsTable.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/components/code/TSQLResultsTable.tsx (3)
106-124: DateTime formatting correctly usestoLocaleString.The previous review flagged that
toLocaleDateStringignores time options per ECMAScript spec. This has been correctly updated to usetoLocaleString, which includes time components as intended.
1089-1095: Verify: hiding filter row clears all column filters.When toggling off the filter row visibility,
setColumnFilters([])clears all active filters. This means users lose their filter state when they hide the filter row. If this is intentional (to reset filters on toggle), this is fine. If users expect to hide the UI but keep filters active, consider removing thesetColumnFilters([])call.
886-957: Solid virtualized table implementation.The table setup with TanStack Table and TanStack Virtual is well-structured:
- Memoized column definitions with dynamic width calculation
- Proper row virtualization with reasonable overscan
- Filtering and sorting integration
- Column resizing support
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
291-293: ConfirmwhereClauseFallbackis ignored when the query already filterstriggered_at.
whereClauseFallbackis always passed. IfexecuteQuerydoesn’t guard against existingtriggered_atpredicates, users will get an extra constraint. Please verify that behavior (or add a server-side check before passing the fallback).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /** | ||
| * Fuzzy filter function using match-sorter ranking | ||
| */ | ||
| /** | ||
| * Get the formatted display string for a value based on its column type | ||
| * This mirrors the formatting logic in CellValue component | ||
| */ |
There was a problem hiding this comment.
Remove orphaned doc comment.
Lines 63-65 contain a doc comment for "Fuzzy filter function using match-sorter ranking" but no function follows it. The actual fuzzyFilter function is defined later at line 149. This appears to be a leftover from refactoring.
Suggested fix
-/**
- * Fuzzy filter function using match-sorter ranking
- */
/**
* Get the formatted display string for a value based on its column type📝 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.
| /** | |
| * Fuzzy filter function using match-sorter ranking | |
| */ | |
| /** | |
| * Get the formatted display string for a value based on its column type | |
| * This mirrors the formatting logic in CellValue component | |
| */ | |
| /** | |
| * Get the formatted display string for a value based on its column type | |
| * This mirrors the formatting logic in CellValue component | |
| */ |
🤖 Prompt for AI Agents
In `@apps/webapp/app/components/code/TSQLResultsTable.tsx` around lines 63 - 69,
Remove the orphaned doc comment block "Fuzzy filter function using match-sorter
ranking" that is sitting above the CellValue formatting comment; it is a
leftover and should be deleted so docs match code, leaving the actual
fuzzyFilter implementation (the function named fuzzyFilter further down) as the
single source of truth. Locate the stray comment near the top of the file and
delete it, ensuring the real fuzzyFilter function remains unchanged.
What changed