TRQL/Query improvements #2870
TRQL/Query improvements #2870
Conversation
They were showing as [Object Object] before.
|
WalkthroughThe PR adds Redis-backed query concurrency limiting and related env-configured limits; admin-gated EXPLAIN support that returns generated SQL and execution plans; ClickHouse settings defaults for query protection; a RedisConcurrencyLimiter implementation and singleton exporter; changes to TSQL execution to support explain mode and surface hiddenColumns, explainOutput, and generatedSql; coreColumn support in schemas and propagation to AI prompts and query printer (hiddenColumns); error sanitization utilities; quote-aware and enum-aware SQL completion; JSON cell rendering refactor; various test updates and public API exports. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
🧰 Additional context used📓 Path-based instructions (9)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,md,css,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*.test.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-01-12T11:01:34.777ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
🔇 Additional comments (2)
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 |
PR Review: TRQL/Query improvementsThis is a well-structured PR that adds important safety and UX improvements to the query feature. Below is my detailed review. Overall AssessmentStrengths:
Code Quality & Best PracticesExcellent Patterns
Minor Suggestions
Potential Bugs or Issues1. Concurrency Limiter Release on Error (Low Risk)In finally {
try {
await queryConcurrencyLimiter.release({ key: organizationId, requestId });
} catch (releaseError) {
logger.error('Failed to release concurrency slot', { organizationId, requestId, error: releaseError });
}
}2. Missing Cleanup for AutoSubmit Ref (Low Risk)In 3. Autocomplete Timing with setTimeoutIn Performance ConsiderationsGood Decisions
Potential Concerns
const [estimateResult, pipelineResult] = await Promise.all([
runExplainQuery('ESTIMATE'),
runExplainQuery('PIPELINE'),
]);
Security ConcernsStrong Points
RecommendationConsider rate limiting the AI query endpoint in addition to the concurrency limiting for the query execution itself. The AI endpoint could be abused separately. Test CoverageWell Tested:
Could Use More Coverage:
SummaryThis is a solid PR that adds meaningful safety improvements to the query feature. The concurrency limiting, error sanitization, and ClickHouse query safeguards are all well-implemented. The code follows the project's patterns and has good test coverage for the new utilities. Recommendations before merge:
Overall: Approve with minor suggestions ✓ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/webapp/app/services/redisConcurrencyLimiter.server.ts:
- Around line 56-66: The custom Redis command fails in Cluster mode because
acquireConcurrency (and the release() pipeline) are called with two keys that
map to different hash slots (keyKey vs globalKey); change to a
single-cluster-safe key strategy by either (a) collapsing both scopes into one
key passed to acquireConcurrency (e.g., store global counters under a namespaced
field inside the per-key key) or (b) ensure both keys hash to the same slot by
using Redis hash tags (make keyKey and globalKey include the same {tag} token).
Update the call sites in acquireConcurrency and the release() pipeline so only
the single slot-safe key is used (or both keys share the same {tag}), and adjust
any Lua/script logic to reference the unified key name or tagged keys
accordingly.
🧹 Nitpick comments (12)
apps/webapp/app/components/code/TSQLResultsTable.tsx (1)
372-392: Useunknowninstead ofanyand handleundefinededge case.Two issues:
- Per TypeScript best practices, prefer
unknownoveranyfor better type safety.JSON.stringify(undefined)returnsundefined(not a string), which would causejsonString.lengthto throw a TypeError.Suggested fix
-function JSONCellValue({ value }: { value: any }) { - const jsonString = JSON.stringify(value); +function JSONCellValue({ value }: { value: unknown }) { + if (value === undefined) { + return <span className="font-mono text-xs text-text-dimmed">undefined</span>; + } + const jsonString = JSON.stringify(value);apps/webapp/app/components/code/tsql/tsqlCompletion.ts (1)
200-207: Usetypeinstead ofinterfaceper coding guidelines.The coding guidelines specify using types over interfaces for TypeScript definitions.
Suggested change
-interface ContextResult { +type ContextResult = { type: CompletionContextType; tablePrefix?: string; /** Column being compared (for value context) */ columnName?: string; /** Table alias for the column (for value context) */ columnTableAlias?: string; -} +};apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (1)
831-852: Consider:aiFixRequestalways takes precedence over example prompts.Once "Try fix error" is clicked,
aiFixRequestis set and will always be used (aiFixRequest ?? examplePromptRequest). Clicking example prompts afterward won't have any effect becauseaiFixRequestis never cleared.Consider clearing
aiFixRequestafter the AI query completes or when the user interacts with example prompts:♻️ Suggested approach
In
Pagecomponent, pass a callback to clearaiFixRequest:<QueryHelpSidebar ... aiFixRequest={aiFixRequest} + onAiFixComplete={() => setAiFixRequest(null)} />Then in
AITabContent, call it after the AI generates a query (via a callback fromAIQueryInput).internal-packages/tsql/src/query/schema.ts (1)
799-806: Low ReDoS risk - schema values are trusted.Static analysis flagged these dynamically constructed regexes. However, the risk is low because:
columnsToStripcomes from schema definitions (developer-controlled, not user input)- The regex patterns (
\s*=\s*'[^']*') are linear and don't contain nested quantifiers- Error messages are typically short strings
The
escapeRegexhelper properly escapes special characters. If schema values ever become user-controllable, consider adding length limits or using string operations instead.apps/webapp/app/v3/querySchemas.ts (1)
2-2: Consider movingautoFormatSQLto a shared utility.Importing from a UI component (
TSQLEditor.tsx) into a schema definition file creates a coupling that could cause issues:
- Schemas might be imported in server-only contexts
- Creates a circular dependency risk if TSQLEditor ever imports from querySchemas
Consider extracting
autoFormatSQLto a shared utility file (e.g.,~/utils/sqlFormatter.ts) that both files can import from.Suggested refactor
Create
apps/webapp/app/utils/sqlFormatter.ts:import { format as formatSQL } from "sql-formatter"; export function autoFormatSQL(sql: string) { return formatSQL(sql, { language: "sql", keywordCase: "upper", indentStyle: "standard", linesBetweenQueries: 2, }); }Then update both files to import from the shared location.
internal-packages/tsql/src/query/validator.ts (1)
201-226: LGTM! Informative SELECT * handling.The implementation:
- Detects SELECT * patterns in the query
- Collects core columns from all tables in the FROM clause
- Emits an
info-level issue (not blocking) with helpful guidance- Includes
suggestedColumnsfor UI consumptionUsing "info" severity is the right choice - it informs users about the behavior without blocking their query.
One minor note: if the same table appears with different aliases,
coreColumnsmight contain duplicates. Consider deduplicating:Optional: Deduplicate core columns
// Collect core columns from all tables in context -const coreColumns: string[] = []; +const coreColumns = new Set<string>(); for (const tableSchema of context.tables.values()) { const tableCoreColumns = getCoreColumns(tableSchema); - coreColumns.push(...tableCoreColumns); + tableCoreColumns.forEach(col => coreColumns.add(col)); } +const uniqueCoreColumns = Array.from(coreColumns);apps/webapp/app/v3/services/aiQueryService.server.ts (1)
71-104: Consider extracting duplicate tool definitions.The
validateTSQLQueryandgetTableSchematools are duplicated betweenstreamQueryandcallmethods. Consider extracting them to a private method to reduce duplication.♻️ Optional refactor to reduce duplication
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); }, }), }; }Then use
tools: this.buildTools()in both methods.Also applies to: 126-159
apps/webapp/app/services/queryService.server.ts (1)
140-153: Consider moving database queries outside the concurrency slot.The
prisma.project.findManyandprisma.runtimeEnvironment.findManycalls are executed while holding the concurrency slot. If these queries are slow, they unnecessarily consume the slot time. Consider fetching field mappings before acquiring the slot.♻️ Move DB queries before concurrency acquisition
export async function executeQuery<TOut extends z.ZodSchema>( options: ExecuteQueryOptions<TOut> ): Promise<TSQLQueryResult<z.output<TOut>>> { const { scope, organizationId, projectId, environmentId, history, customOrgConcurrencyLimit, ...baseOptions } = options; + // Build field mappings BEFORE acquiring concurrency slot + const projects = await prisma.project.findMany({ + where: { organizationId }, + select: { id: true, externalRef: true }, + }); + + const environments = await prisma.runtimeEnvironment.findMany({ + where: { project: { organizationId } }, + select: { id: true, slug: true }, + }); + + const fieldMappings: FieldMappings = { + project: Object.fromEntries(projects.map((p) => [p.id, p.externalRef])), + environment: Object.fromEntries(environments.map((e) => [e.id, e.slug])), + }; // Generate unique request ID for concurrency tracking const requestId = crypto.randomUUID(); // ... acquire slot and executeinternal-packages/tsql/src/query/printer.ts (1)
682-700: Consider deduplicating hiddenColumns when multiple tables are involved.When expanding
SELECT *across multiple tables, the same column name could appear in multiple schemas. The current implementation pushes tohiddenColumnswithout deduplication.♻️ Consider using a Set for hiddenColumns
If column name uniqueness matters for the user notification, consider using a Set internally:
- private hiddenColumns: string[] = []; + private hiddenColumnsSet: Set<string> = new Set(); print(node: SelectQuery | SelectSetQuery): PrintResult { this.outputColumns = []; - this.hiddenColumns = []; + this.hiddenColumnsSet = new Set(); const sql = this.visit(node); const result: PrintResult = { sql, params: this.context.getParams(), columns: this.outputColumns, }; - if (this.hiddenColumns.length > 0) { - result.hiddenColumns = this.hiddenColumns; + if (this.hiddenColumnsSet.size > 0) { + result.hiddenColumns = Array.from(this.hiddenColumnsSet); } return result; }And in
getSelectableColumnsFromSchema:- this.hiddenColumns.push(columnName); + this.hiddenColumnsSet.add(columnName);internal-packages/clickhouse/src/client/tsql.ts (2)
180-207: Consider parallelizing additional EXPLAIN queries.The ESTIMATE and PIPELINE explain queries are executed sequentially. Since they're independent, running them in parallel could improve response time for the EXPLAIN feature.
♻️ Parallelize additional explain queries
- for (const explainType of explainTypes) { - try { - const additionalQueryFn = reader.queryWithStats({ - name: `${options.name}-explain-${explainType.name.toLowerCase()}`, - query: explainType.query, - params: z.record(z.any()), - schema: z.object({ explain: z.string() }), - settings: options.clickhouseSettings, - }); - - const [additionalError, additionalResult] = await additionalQueryFn(params); - - if (!additionalError && additionalResult) { - const additionalRows = additionalResult.rows as Array<{ explain: string }>; - const output = additionalRows.map((r) => r.explain).join("\n"); - additionalOutputs.push(`── ${explainType.name} ──\n${output}`); - } - } catch { - // Ignore errors from additional explain queries - } - } + const explainPromises = explainTypes.map(async (explainType) => { + try { + const additionalQueryFn = reader.queryWithStats({ + name: `${options.name}-explain-${explainType.name.toLowerCase()}`, + query: explainType.query, + params: z.record(z.any()), + schema: z.object({ explain: z.string() }), + settings: options.clickhouseSettings, + }); + + const [additionalError, additionalResult] = await additionalQueryFn(params); + + if (!additionalError && additionalResult) { + const additionalRows = additionalResult.rows as Array<{ explain: string }>; + const output = additionalRows.map((r) => r.explain).join("\n"); + return `── ${explainType.name} ──\n${output}`; + } + } catch { + // Ignore errors from additional explain queries + } + return null; + }); + + const results = await Promise.all(explainPromises); + additionalOutputs.push(...results.filter((r): r is string => r !== null));
204-206: Consider logging suppressed errors for debugging.The empty catch block silently swallows errors from additional explain queries. While this is intentional (best-effort), logging at debug level could help diagnose issues.
♻️ Add debug logging for suppressed errors
} catch { - // Ignore errors from additional explain queries + // Best-effort: additional explain queries may fail for certain query types + logger.debug(`[TSQL] ${explainType.name} explain query failed`, { + name: options.name, + }); }apps/webapp/app/services/redisConcurrencyLimiter.server.ts (1)
84-91: Consider adding error handling for pipeline execution.The
releasemethod uses a pipeline but doesn't check for execution errors. While ZREM is idempotent, logging failures could help debug issues.♻️ Add error handling for release pipeline
async release(options: { key: string; requestId: string }): Promise<void> { const { key, requestId } = options; const keyKey = this.#getKeyKey(key); const globalKey = this.#getGlobalKey(); // Remove from both sets in a single round trip - await this.redis.pipeline().zrem(keyKey, requestId).zrem(globalKey, requestId).exec(); + const results = await this.redis.pipeline().zrem(keyKey, requestId).zrem(globalKey, requestId).exec(); + + // Check for errors (results is [[error, reply], [error, reply]]) + if (results) { + for (const [error] of results) { + if (error) { + // Log but don't throw - release should be best-effort + console.warn(`[ConcurrencyLimiter] Release error: ${error.message}`); + } + } + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/code/tsql/tsqlCompletion.test.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.tsinternal-packages/clickhouse/src/client/tsql.tsinternal-packages/clickhouse/src/index.tsinternal-packages/tsql/src/index.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/schema.test.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/validator.ts
💤 Files with no reviewable changes (1)
- apps/webapp/app/components/code/tsql/tsqlCompletion.test.ts
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{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 insteadEvery Trigger.dev task must be exported; use task() function with unique id and run async function
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/printer.tsapps/webapp/app/components/code/AIQueryInput.tsxinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsinternal-packages/tsql/src/index.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/tsql/src/query/validator.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.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/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/printer.tsapps/webapp/app/components/code/AIQueryInput.tsxinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsinternal-packages/tsql/src/index.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/tsql/src/query/validator.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.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 webappIn webapp development, access environment variables via env export from apps/webapp/app/env.server.ts; never use process.env directly
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/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 webapp
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/printer.tsapps/webapp/app/components/code/AIQueryInput.tsxinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsinternal-packages/tsql/src/index.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/tsql/src/query/validator.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from @trigger.dev/core using subpaths only; never import from root of @trigger.dev/core
Always import task definitions from @trigger.dev/sdk, never from @trigger.dev/sdk/v3 or deprecated client.defineJob pattern
Files:
apps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/services/queryConcurrencyLimiter.server.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/printer.tsapps/webapp/app/components/code/AIQueryInput.tsxinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsinternal-packages/tsql/src/index.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/tsql/src/query/validator.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.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/services/queryConcurrencyLimiter.server.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/printer.tsinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsinternal-packages/tsql/src/index.tsapps/webapp/app/env.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/tsql/src/query/validator.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
internal-packages/tsql/src/query/schema.test.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Avoid mocks or stubs in tests; use helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for unit tests
Files:
internal-packages/tsql/src/query/schema.test.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use vitest exclusively for testing and never mock anything; use testcontainers instead
Files:
internal-packages/tsql/src/query/schema.test.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place test files next to source files with .test.ts naming convention (e.g., MyService.ts → MyService.test.ts)
Use testcontainers helpers (redisTest, postgresTest, containerTest from @internal/testcontainers) for Redis/PostgreSQL testing instead of mocks
Files:
internal-packages/tsql/src/query/schema.test.tsapps/webapp/test/components/code/tsql/tsqlCompletion.test.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
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Test files should only import classes and functions from
app/**/*.tsfiles and should not importenv.server.tsdirectly or indirectly; pass configuration through options instead
Files:
apps/webapp/test/components/code/tsql/tsqlCompletion.test.ts
🧠 Learnings (13)
📚 Learning: 2025-11-14T16:03:06.917Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2681
File: apps/webapp/app/services/platform.v3.server.ts:258-302
Timestamp: 2025-11-14T16:03:06.917Z
Learning: In `apps/webapp/app/services/platform.v3.server.ts`, the `getDefaultEnvironmentConcurrencyLimit` function intentionally throws an error (rather than falling back to org.maximumConcurrencyLimit) when the billing client returns undefined plan limits. This fail-fast behavior prevents users from receiving more concurrency than their plan entitles them to. The org.maximumConcurrencyLimit fallback is only for self-hosted deployments where no billing client exists.
Applied to files:
apps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/env.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Control concurrency using the `queue` property with `concurrencyLimit` option
Applied to files:
apps/webapp/app/services/queryConcurrencyLimiter.server.tsapps/webapp/app/services/redisConcurrencyLimiter.server.tsapps/webapp/app/services/queryService.server.ts
📚 Learning: 2026-01-12T11:01:34.777Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.777Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Edit database schema in internal-packages/database/prisma/schema.prisma; remove extraneous lines from migrations related to BackgroundWorker, TaskRun tags, waitpoints, and SecretStore indexes
Applied to files:
internal-packages/tsql/src/query/schema.tsinternal-packages/tsql/src/query/schema.test.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/components/code/tsql/tsqlCompletion.tsinternal-packages/clickhouse/src/client/tsql.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsxapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/services/aiQueryService.server.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/services/queryService.server.ts
📚 Learning: 2026-01-12T11:01:34.777Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.777Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Database indexes must use CONCURRENTLY to avoid table locks; CONCURRENTLY indexes must be in separate migration files and cannot be combined with other schema changes
Applied to files:
apps/webapp/app/services/queryService.server.ts
📚 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/env.server.ts
📚 Learning: 2026-01-12T11:01:34.777Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:01:34.777Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : In webapp development, access environment variables via env export from apps/webapp/app/env.server.ts; never use process.env directly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-06-14T08:07:46.625Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2175
File: apps/webapp/app/services/environmentMetricsRepository.server.ts:202-207
Timestamp: 2025-06-14T08:07:46.625Z
Learning: In apps/webapp/app/services/environmentMetricsRepository.server.ts, the ClickHouse methods (getTaskActivity, getCurrentRunningStats, getAverageDurations) intentionally do not filter by the `tasks` parameter at the ClickHouse level, even though the tasks parameter is accepted by the public methods. This is done on purpose as there is not much benefit from adding that filtering at the ClickHouse layer.
Applied to files:
apps/webapp/app/env.server.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 {packages/core,apps/webapp}/**/*.{ts,tsx} : Use zod for validation in packages/core and apps/webapp
Applied to files:
internal-packages/clickhouse/src/client/tsql.ts
📚 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:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schemaTask()` from `trigger.dev/sdk/v3` with Zod schema for payload validation
Applied to files:
apps/webapp/app/v3/querySchemas.ts
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
Applied to files:
apps/webapp/app/v3/querySchemas.ts
📚 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
🧬 Code graph analysis (7)
internal-packages/tsql/src/query/printer.ts (2)
internal-packages/tsql/src/index.ts (2)
PrintResult(109-109)TableSchema(94-94)internal-packages/tsql/src/query/schema.ts (1)
TableSchema(285-303)
internal-packages/tsql/src/query/schema.test.ts (2)
internal-packages/tsql/src/query/schema.ts (3)
TableSchema(285-303)column(318-330)sanitizeErrorMessage(747-875)apps/webapp/app/v3/querySchemas.ts (1)
runsSchema(27-420)
apps/webapp/app/components/code/TSQLResultsTable.tsx (2)
internal-packages/tsql/src/index.ts (1)
column(62-62)apps/webapp/app/components/primitives/Tooltip.tsx (1)
SimpleTooltip(141-141)
apps/webapp/app/services/redisConcurrencyLimiter.server.ts (1)
apps/webapp/app/redis.server.ts (2)
RedisWithClusterOptions(4-13)RedisClient(15-15)
internal-packages/tsql/src/query/validator.ts (2)
internal-packages/tsql/src/query/ast.ts (1)
Field(468-472)internal-packages/tsql/src/query/schema.ts (1)
getCoreColumns(712-716)
internal-packages/clickhouse/src/client/tsql.ts (4)
internal-packages/tsql/src/index.ts (3)
compileTSQL(262-281)sanitizeErrorMessage(82-82)transformResults(125-125)internal-packages/tsql/src/query/schema.ts (1)
sanitizeErrorMessage(747-875)internal-packages/tsql/src/query/errors.ts (1)
QueryError(43-45)internal-packages/tsql/src/query/results.ts (1)
transformResults(73-88)
apps/webapp/app/v3/querySchemas.ts (3)
internal-packages/tsql/src/index.ts (1)
column(62-62)internal-packages/tsql/src/query/schema.ts (1)
column(318-330)apps/webapp/app/components/code/TSQLEditor.tsx (1)
autoFormatSQL(282-289)
🪛 ast-grep (0.40.4)
internal-packages/tsql/src/query/schema.ts
[warning] 772-772: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${escapeRegex(table.name)}\\s+AS\\s+${escapeRegex(table.name)}\\b, "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 801-801: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\(${escaped}\\s*=\\s*'[^']*'\\)\\s*AND\\s*, "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 803-803: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*AND\\s*\\(${escaped}\\s*=\\s*'[^']*'\\), "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 805-805: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\(${escaped}\\s*=\\s*'[^']*'\\), "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 891-891: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((?<![a-zA-Z0-9_])${escapeRegex(search)}(?![a-zA-Z0-9_]), "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (24)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 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: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (48)
apps/webapp/app/components/code/TSQLResultsTable.tsx (2)
137-140: Consider handling null/undefined before the JSON type check for consistency.In non-pretty mode, JSON columns bypass the
nullcheck on line 141, but this is handled correctly sinceJSON.stringify(null)returns"null". The inconsistency with pretty mode (which checks null/undefined first at lines 160-166) is minor but worth noting for maintainability.
283-285: LGTM!Good centralization of JSON rendering logic. The null/undefined checks above (lines 160-166) ensure this path is safe.
apps/webapp/app/components/code/AIQueryInput.tsx (3)
35-36: LGTM!The new optional
autoSubmitKeyprop with clear documentation is a clean way to force re-submission for identical prompts.
56-56: LGTM!Composite object type correctly tracks both prompt and key for the re-submission check.
203-218: LGTM!The auto-submit logic correctly handles the composite (prompt, key) comparison:
- Early return guards against empty prompts and concurrent submissions
- The
isDifferentcheck properly handles null, prompt changes, and key changes- Dependency array is complete
apps/webapp/app/components/code/tsql/tsqlCompletion.ts (6)
3-4: LGTM!Using
typekeyword for type-only imports is the correct approach.
91-96: LGTM!The ternary formatting for argument hints is cleaner and produces correct output for function signatures.
Also applies to: 110-115
213-245: LGTM!The extended regex patterns correctly handle partial string values after operators (e.g.,
column = 'val). The patterns appropriately match:
- Comparison operators (
=,!=,<>) with optional partial quoted stringsINclauses including after commasThis enables autocomplete to work mid-value entry.
328-342: LGTM!The
createEnumValueCompletionsfunction cleanly derives completions fromallowedValues, properly formats them with surrounding quotes, and assigns the highest boost priority for value context.
393-413: LGTM!The value context handling correctly:
- Looks up the column schema to get enum values
- Detects auto-paired closing quotes after the cursor
- Extends the replacement range to include the closing quote, avoiding doubled quotes like
'Completed''This is a thoughtful UX improvement for editor auto-pairing behavior.
469-478: LGTM!Conditionally setting
toonly when needed keeps the result clean and avoids unnecessary property assignments.apps/webapp/test/components/code/tsql/tsqlCompletion.test.ts (3)
6-28: LGTM!Good addition of
sliceStringto the mock context—this is necessary to support the new quote-detection logic in the implementation.
31-67: LGTM!The test schema is well-structured with realistic enum columns (
status,machine,environment_type) usingLowCardinality(String)type and properallowedValuesarrays. This provides good coverage for testing enum value completions.
192-326: Excellent test coverage for enum value completions.The test suite comprehensively covers:
- Basic enum suggestions after
=,!=,IN (operators- Quote-aware completion (opening quote, partial input)
- Multiple enum columns (
status,machine,environment_type)- Edge cases: auto-paired quote replacement range extension, no extension when no closing quote, empty results for non-enum columns
The tests at lines 292-312 specifically validate the quote handling behavior which is crucial for good UX with editor auto-pairing.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx (9)
43-43: LGTM!The
Calloutcomponent import is correctly added to support the hidden columns warning UI.
118-161: LGTM!The loader correctly:
- Passes admin/impersonation flags to
hasQueryAccessfor authorization- Computes
isAdminfor downstream UI gating (Explain button)- Returns
isAdminin the loader response for client consumption
163-167: LGTM!The
explainfield uses a string enum which is appropriate for form data handling. Usingz.enum(["true", "false"]).nullable().optional()correctly handles the optional nature of the explain parameter.
248-270: LGTM!The explain logic correctly:
- Gates the feature to admin/impersonating users only (line 251)
- Passes the
explainflag toexecuteQuery(line 264)- Skips saving to query history for impersonating users (line 268), preventing audit trail pollution during support sessions
385-395: LGTM!The Explain button is correctly implemented:
- Conditionally rendered for admin users only
- Uses form submission with
name="explain" value="true"to pass the flag- Appropriately disabled during loading or when query is empty
- Uses a secondary variant to visually differentiate from the primary Query button
424-433: LGTM!The AI fix request state management is well designed:
- Uses a
keyproperty to force re-execution when the same error message needs to be retriedhandleTryFixErrorcorrectly switches to the AI tab and triggers a new fix request- The callback has stable dependencies (state setters are stable)
524-577: LGTM!The results rendering logic is well structured:
- Error state includes a helpful "Try fix error" button that triggers the AI assistant
- Explain output displays both generated SQL and execution plan with proper scrolling
- Hidden columns callout clearly explains
SELECT *limitations and how to include omitted columns- The conditional rendering chain correctly prioritizes error → explain → results → empty state
751-797: LGTM!The sidebar refactoring correctly implements controlled tabs:
activeTabandonTabChangeprops enable programmatic tab switching (e.g., when clicking "Try fix error")aiFixRequestis threaded through toAITabContentfor AI-assisted error fixing- The controlled
ClientTabspattern maintains a single source of truth for tab state in the parent
622-635: LGTM!The
QueryHelpSidebarinvocation correctly passes:
activeTabandonTabChangefor controlled tab stateaiFixRequestfor the error-fixing flowThe component integration is clean and maintains unidirectional data flow.
apps/webapp/app/components/code/TSQLEditor.tsx (1)
107-123: LGTM! Quote-triggered autocomplete for enum values.The implementation correctly triggers autocomplete after quote characters are typed, enabling users to get suggestions for enum values in WHERE clauses. The 50ms timeout ensures the quote character is inserted before the completion popup appears.
Minor observation:
event.code === "Quote"may be redundant sinceevent.key === "'"should cover the single quote case, but it doesn't hurt to have it as a fallback for different keyboard layouts.internal-packages/tsql/src/query/schema.ts (3)
136-152: LGTM! Well-documented core column property.The
coreColumnproperty is clearly documented with its purpose (marking essential columns for default queries) and includes a helpful usage example.
702-716: LGTM!Clean implementation that correctly extracts core column names from the table schema.
747-875: Complex but necessary error sanitization logic.The function handles multiple sanitization concerns in a well-ordered sequence. A few observations:
- The step-by-step approach (tenant filters → cleanup → name replacement → alias removal) is logical
- Sorting replacements by length (longest first) prevents partial matches
- The extensive cleanup in Step 2 handles various edge cases from tenant filter removal
Consider adding unit tests specifically for edge cases like:
- Multiple tenant filters being removed
- Nested parentheses after filter removal
- Empty WHERE clauses at end of query
apps/webapp/app/v3/querySchemas.ts (2)
46-47: Good selection of core columns.The four columns marked as core (
run_id,status,task_identifier,triggered_at) represent the essential information users typically need when querying runs. This aligns well with the PR objective of guiding users away fromSELECT *.Also applies to: 92-93, 109-113, 186-193
430-433: LGTM! Default query is now formatted.The default query is wrapped with
autoFormatSQLfor better readability in the editor.apps/webapp/app/env.server.ts (1)
1178-1188: LGTM! Well-structured environment variables for query limits.The new environment variables follow the established patterns in this file:
- Uses
z.coerce.number().int()for type-safe integer parsing- Sensible defaults: 10s execution time, 1GB memory, 3 concurrent queries per org, 50 global
- Clear naming convention with
QUERY_prefix- Properly uses Zod for validation as per coding guidelines
internal-packages/clickhouse/src/index.ts (2)
1-2: LGTM! Type-only import for ClickHouseSettings.Correctly uses
import typeandexport typeto re-export the ClickHouse settings type without runtime overhead.
48-49: LGTM! QueryError export.Exporting
QueryErrorfrom the public API allows consumers to properly type-check and handle query-specific errors.internal-packages/tsql/src/index.ts (1)
67-68: LGTM! Clean public API exports.The new exports (
getCoreColumns,sanitizeErrorMessage) are properly added with descriptive comments that group related functionality. This maintains the organized structure of the module's public API.Also applies to: 81-82
internal-packages/tsql/src/query/validator.ts (2)
175-186: LGTM! Correct SELECT * detection.The
isSelectStarhelper correctly identifies both unqualified (SELECT *) and qualified (SELECT table.*) asterisk patterns by checking the field chain structure.
40-50: Type definition follows coding guidelines.The
ValidationIssuetype is correctly extended with the new"select_star"discriminant and optionalsuggestedColumnsfield. Using a type (via interface) aligns with the TypeScript conventions in the codebase.apps/webapp/app/services/queryConcurrencyLimiter.server.ts (1)
1-29: LGTM! Clean singleton initialization following project conventions.The file correctly:
- Accesses environment variables via the
envexport from~/env.server.tsas per coding guidelines- Uses the singleton pattern for Redis client reuse
- Separates configuration from the testable
RedisConcurrencyLimiterserviceinternal-packages/tsql/src/query/schema.test.ts (1)
461-781: Comprehensive test coverage for error sanitization.The test suite thoroughly covers:
- Fully qualified table.column replacements
- Standalone table/column name replacements
- Quoted and backtick identifier handling
- Tenant isolation filter removal
- Redundant alias cleanup
- Required filters (e.g.,
engine = 'V2')- Edge cases with field mappings
The tests are well-organized and descriptive, following vitest conventions.
apps/webapp/app/v3/services/aiQueryService.server.ts (2)
261-277: Good addition of core column guidance for AI schema awareness.The core column identification and annotation help the AI understand which columns to prefer when users don't specify explicit columns. The
[CORE]suffix in descriptions provides clear guidance.
366-377: Well-structured rules for query safety.The updated rules appropriately:
- Prohibit
SELECT *for ClickHouse columnar performance- Guide toward core columns for ambiguous selections
- Add default 7-day time filtering for safety
apps/webapp/app/services/queryService.server.ts (3)
31-56: Well-structured ClickHouse query protection settings.The default settings appropriately enforce:
- Execution time limits
- Memory usage limits
- AST complexity limits to prevent expensive queries
- Read-only mode for safety
Good reference to PostHog's HogQL settings for battle-tested defaults.
101-119: Good concurrency control with clear error messaging.The acquisition flow properly:
- Uses UUID for unique request tracking
- Returns user-friendly error messages distinguishing org vs global limits
- Fails fast when limits are exceeded
188-194: Proper resource cleanup with finally block.The concurrency slot is always released regardless of success or failure, preventing slot leakage.
internal-packages/tsql/src/query/printer.ts (2)
73-78: Good extension of PrintResult for hidden columns notification.The optional
hiddenColumnsfield allows callers to notify users which columns were omitted whenSELECT *was expanded to core columns only.
619-643: Core-column expansion with appropriate fallback.The
SELECT *expansion correctly:
- Only returns core columns when defined
- Falls back to all columns when no core columns exist
- Tracks hidden columns for user notification
internal-packages/clickhouse/src/client/tsql.ts (2)
70-76: Good admin-only EXPLAIN mode for query debugging.The
explainoption is well-documented and appropriately scoped for admin use only.
167-170: Good error sanitization for user-facing messages.Sanitizing ClickHouse error messages to use TSQL names instead of internal identifiers improves the user experience and avoids leaking implementation details.
apps/webapp/app/services/redisConcurrencyLimiter.server.ts (2)
15-33: Well-designed two-level concurrency limiter.The class design appropriately:
- Uses sorted sets with timestamps for active request tracking
- Implements automatic expiry as a safety net
- Configures sensible defaults (5-minute expiry)
101-142: Robust Lua script for atomic two-level limiting.The script correctly:
- Purges expired entries before checking limits
- Checks global limit first (more restrictive)
- Then checks key-specific limit
- Atomically adds to both sets on success
- Sets expiry on both keys
The implementation is thread-safe due to Lua script atomicity.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
PR Review: TRQL/Query ImprovementsThis is a well-structured PR that adds several security and usability improvements to the query page. Here's my detailed review: Code Quality ✅Strengths:
Minor Suggestions:
interface RedisWithConcurrencyCommand extends RedisClient {
acquireConcurrency: (...args: string[]) => Promise<number>;
}
Potential Bugs/Issues
|
Summary
Changes