feat: add Model Registry feature with catalog pipeline, dashboard pages, and TSQL schema#3270
Merged
feat: add Model Registry feature with catalog pipeline, dashboard pages, and TSQL schema#3270
Conversation
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new internal llm-model-catalog package (catalog, registry, scripts, tests), replaces llm-pricing usage with the catalog, and seeds/syncs catalog and pricing into Prisma. Extends Prisma schema with catalog fields and adds migrations for base model and ClickHouse (base_response_model column and a new llm_model_aggregates_v1 table plus materialized view). Introduces ClickHouse query builders and aggregates, a ModelRegistry presenter, new Remix routes (models index, detail, compare) and UI bits (side menu gating, path builders, formatters), feature-flag keys and access helpers, enriched LLM metric payloads, and related tooling/scripts. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ericallam
force-pushed
the
feature/tri-7941-ai-models
branch
from
6f5703e to
5478539
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/webapp/app/routes/admin.api.v1.llm-models.seed.ts (1)
18-21:⚠️ Potential issue | 🟠 MajorReject unknown
actionvalues; don’t fall through to seeding.At Line 19, any value other than
"sync"currently executes the seed path. A typo like?action=sycnstill mutates data. Validateactionexplicitly and return400for invalid values.Proposed fix
+import { z } from "zod"; import { type ActionFunctionArgs, json } from "@remix-run/server-runtime"; import { seedLlmPricing, syncLlmCatalog } from "@internal/llm-model-catalog"; @@ const url = new URL(request.url); - const action = url.searchParams.get("action") ?? "seed"; + const ActionSchema = z.enum(["seed", "sync"]); + const parsedAction = ActionSchema.safeParse(url.searchParams.get("action") ?? "seed"); + if (!parsedAction.success) { + return json({ error: "Invalid action" }, { status: 400 }); + } + const action = parsedAction.data;As per coding guidelines:
Use zod for validation in packages/core and apps/webapp.Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.api.v1.llm-models.seed.ts` around lines 18 - 21, Replace the permissive action handling that defaults unknown values to "seed" with explicit validation using zod: create a zod validator (e.g., z.enum(["sync","seed"])) and validate the value obtained from url.searchParams.get("action") (currently stored in the action variable); if validation fails, return a 400 response and do not execute the seed or sync branches. Apply the same zod validation check where action is read/used later (the block around the lines referenced) so typos like "?action=sycn" are rejected rather than falling through to seeding.
🧹 Nitpick comments (5)
internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh (3)
77-77: Unused variableMODEL_TO_BASE.The associative array is populated but never read. Either use it or remove the declaration to reduce noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh` at line 77, The script populates the associative array symbol MODEL_TO_BASE (via MODEL_TO_BASE["$model"]="$base") but never reads it; remove the unused declaration and any lines that populate MODEL_TO_BASE to eliminate dead code, or alternatively wire the array into the intended consumer (e.g., replace direct uses of $base with lookups like ${MODEL_TO_BASE[$model]} in the code paths that need model->base mapping); reference the array name MODEL_TO_BASE and the assignment MODEL_TO_BASE["$model"]="$base" to locate and remove or integrate the usage.
113-113: Unused variableLOCK_FILE.Declared for thread-safe catalog writes but never used (no
flockor similar locking). Either implement locking or remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh` at line 113, The LOCK_FILE variable is declared but never used; either remove the declaration or implement proper locking around catalog writes in generate-model-catalog.sh: if you choose locking, create the lock file under LOG_DIR using LOCK_FILE and wrap the critical section that writes the catalog with a flock-based lock (e.g., open a file descriptor on LOCK_FILE and use flock -n / flock to acquire/release) so concurrent runs are serialized; otherwise delete the LOCK_FILE assignment and any related comments to avoid dead/unused variables.
240-240: Quote the array assignment to prevent word splitting issues.If any model name ever contains spaces or glob characters, this expansion could break.
🛡️ Proposed fix
-MODEL_LIST=($MODELS_TO_RESEARCH) +mapfile -t MODEL_LIST <<< "$MODELS_TO_RESEARCH"Or if
mapfileisn't available:-MODEL_LIST=($MODELS_TO_RESEARCH) +IFS=$'\n' read -r -d '' -a MODEL_LIST <<< "$MODELS_TO_RESEARCH" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh` at line 240, The array assignment MODEL_LIST=($MODELS_TO_RESEARCH) can suffer word-splitting and glob expansion; change it so MODEL_LIST is populated safely from the MODELS_TO_RESEARCH value (e.g., use a quoted expansion or use mapfile/readarray to split into elements) to prevent spaces or glob characters from breaking the array; update the assignment that produces MODEL_LIST to use a safe quoted/splitting method and ensure MODELS_TO_RESEARCH is read without word-splitting or globbing.internal-packages/llm-model-catalog/src/types.ts (1)
1-1: Remove unused import.The
Decimaltype is imported but never used in this file.🧹 Proposed fix
-import type { Decimal } from "@trigger.dev/database"; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/src/types.ts` at line 1, Remove the unused import of Decimal from "@trigger.dev/database" in the types.ts file: delete the import statement or remove Decimal from the import list so there are no unused imports (reference symbol: Decimal, file symbol: import line in types.ts).apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts (1)
7-10: Potential precision loss in DateTime formatting.The regex
replace(/\.\d{3}Z$/, "")assumes exactly 3 millisecond digits. JavaScript'stoISOString()always produces 3 digits, so this is safe, but consider a more explicit format for clarity and to handle any edge cases:function formatDateForCH(date: Date): string { return date.toISOString().slice(0, 19).replace("T", " "); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts` around lines 7 - 10, The current formatDateForCH function strips fractional seconds using a regex that assumes three millisecond digits; change it to explicitly take the ISO string up to the seconds portion to avoid relying on a regex and make intent clearer: in formatDateForCH, call date.toISOString().slice(0,19) and replace "T" with a space so the returned string is "YYYY-MM-DD HH:MM:SS" suitable for ClickHouse DateTime64 parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/webapp/app/routes/admin.api.v1.llm-models.seed.ts`:
- Around line 18-21: Replace the permissive action handling that defaults
unknown values to "seed" with explicit validation using zod: create a zod
validator (e.g., z.enum(["sync","seed"])) and validate the value obtained from
url.searchParams.get("action") (currently stored in the action variable); if
validation fails, return a 400 response and do not execute the seed or sync
branches. Apply the same zod validation check where action is read/used later
(the block around the lines referenced) so typos like "?action=sycn" are
rejected rather than falling through to seeding.
---
Nitpick comments:
In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts`:
- Around line 7-10: The current formatDateForCH function strips fractional
seconds using a regex that assumes three millisecond digits; change it to
explicitly take the ISO string up to the seconds portion to avoid relying on a
regex and make intent clearer: in formatDateForCH, call
date.toISOString().slice(0,19) and replace "T" with a space so the returned
string is "YYYY-MM-DD HH:MM:SS" suitable for ClickHouse DateTime64 parameters.
In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh`:
- Line 77: The script populates the associative array symbol MODEL_TO_BASE (via
MODEL_TO_BASE["$model"]="$base") but never reads it; remove the unused
declaration and any lines that populate MODEL_TO_BASE to eliminate dead code, or
alternatively wire the array into the intended consumer (e.g., replace direct
uses of $base with lookups like ${MODEL_TO_BASE[$model]} in the code paths that
need model->base mapping); reference the array name MODEL_TO_BASE and the
assignment MODEL_TO_BASE["$model"]="$base" to locate and remove or integrate the
usage.
- Line 113: The LOCK_FILE variable is declared but never used; either remove the
declaration or implement proper locking around catalog writes in
generate-model-catalog.sh: if you choose locking, create the lock file under
LOG_DIR using LOCK_FILE and wrap the critical section that writes the catalog
with a flock-based lock (e.g., open a file descriptor on LOCK_FILE and use flock
-n / flock to acquire/release) so concurrent runs are serialized; otherwise
delete the LOCK_FILE assignment and any related comments to avoid dead/unused
variables.
- Line 240: The array assignment MODEL_LIST=($MODELS_TO_RESEARCH) can suffer
word-splitting and glob expansion; change it so MODEL_LIST is populated safely
from the MODELS_TO_RESEARCH value (e.g., use a quoted expansion or use
mapfile/readarray to split into elements) to prevent spaces or glob characters
from breaking the array; update the assignment that produces MODEL_LIST to use a
safe quoted/splitting method and ensure MODELS_TO_RESEARCH is read without
word-splitting or globbing.
In `@internal-packages/llm-model-catalog/src/types.ts`:
- Line 1: Remove the unused import of Decimal from "@trigger.dev/database" in
the types.ts file: delete the import statement or remove Decimal from the import
list so there are no unused imports (reference symbol: Decimal, file symbol:
import line in types.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cdd7076-b662-4f5c-8c62-116755a41e71
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.compare/route.tsxapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.tsapps/webapp/app/routes/admin.llm-models.$modelId.tsxapps/webapp/app/routes/admin.llm-models._index.tsxapps/webapp/app/routes/admin.llm-models.new.tsxapps/webapp/app/services/queryService.server.tsapps/webapp/app/utils/modelFormatters.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/canAccessAi.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/llmPricingRegistry.server.tsapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/utils/enrichCreatableEvents.server.tsapps/webapp/package.jsonapps/webapp/seed-ai-spans.mtsinternal-packages/clickhouse/schema/026_add_base_response_model_to_llm_metrics_v1.sqlinternal-packages/clickhouse/schema/027_create_llm_model_aggregates_v1.sqlinternal-packages/clickhouse/src/client/queryBuilder.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/llmMetrics.tsinternal-packages/clickhouse/src/llmModelAggregates.tsinternal-packages/database/prisma/migrations/20260323104144_add_catalog_columns_to_llm_models/migration.sqlinternal-packages/database/prisma/migrations/20260324142824_add_base_model_name_to_llm_models/migration.sqlinternal-packages/database/prisma/schema.prismainternal-packages/llm-model-catalog/package.jsoninternal-packages/llm-model-catalog/scripts/.gitignoreinternal-packages/llm-model-catalog/scripts/generate-model-catalog.shinternal-packages/llm-model-catalog/scripts/generate.mjsinternal-packages/llm-model-catalog/scripts/research-model.shinternal-packages/llm-model-catalog/scripts/sync-model-prices.shinternal-packages/llm-model-catalog/src/default-model-prices.jsoninternal-packages/llm-model-catalog/src/defaultPrices.tsinternal-packages/llm-model-catalog/src/index.tsinternal-packages/llm-model-catalog/src/model-catalog.jsoninternal-packages/llm-model-catalog/src/modelCatalog.tsinternal-packages/llm-model-catalog/src/registry.test.tsinternal-packages/llm-model-catalog/src/registry.tsinternal-packages/llm-model-catalog/src/seed.tsinternal-packages/llm-model-catalog/src/sync.tsinternal-packages/llm-model-catalog/src/types.tsinternal-packages/llm-model-catalog/tsconfig.jsoninternal-packages/llm-pricing/package.jsoninternal-packages/llm-pricing/src/types.tsinternal-packages/tsql/src/index.test.tsinternal-packages/tsql/src/query/functions.tsinternal-packages/tsql/src/query/schema.ts
💤 Files with no reviewable changes (3)
- internal-packages/tsql/src/index.test.ts
- internal-packages/llm-pricing/package.json
- internal-packages/llm-pricing/src/types.ts
✅ Files skipped from review due to trivial changes (12)
- internal-packages/llm-model-catalog/scripts/.gitignore
- internal-packages/database/prisma/migrations/20260324142824_add_base_model_name_to_llm_models/migration.sql
- internal-packages/database/prisma/migrations/20260323104144_add_catalog_columns_to_llm_models/migration.sql
- internal-packages/llm-model-catalog/src/defaultPrices.ts
- apps/webapp/package.json
- internal-packages/llm-model-catalog/package.json
- internal-packages/clickhouse/schema/026_add_base_response_model_to_llm_metrics_v1.sql
- internal-packages/llm-model-catalog/src/modelCatalog.ts
- apps/webapp/app/utils/modelFormatters.ts
- apps/webapp/app/v3/llmPricingRegistry.server.ts
- internal-packages/database/prisma/schema.prisma
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.compare/route.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- internal-packages/clickhouse/src/llmMetrics.ts
- internal-packages/llm-model-catalog/src/index.ts
- apps/webapp/seed-ai-spans.mts
- apps/webapp/app/routes/admin.api.v1.llm-models.ts
- apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts
- apps/webapp/app/v3/featureFlags.server.ts
- apps/webapp/app/routes/admin.llm-models._index.tsx
- apps/webapp/app/env.server.ts
- apps/webapp/app/routes/admin.llm-models.new.tsx
- internal-packages/llm-model-catalog/scripts/research-model.sh
- apps/webapp/app/v3/canAccessAi.server.ts
- internal-packages/llm-model-catalog/scripts/sync-model-prices.sh
- internal-packages/llm-model-catalog/scripts/generate.mjs
- apps/webapp/app/v3/querySchemas.ts
- apps/webapp/app/v3/utils/enrichCreatableEvents.server.ts
- internal-packages/llm-model-catalog/src/sync.ts
- internal-packages/llm-model-catalog/src/model-catalog.json
- internal-packages/tsql/src/query/functions.ts
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsx
- internal-packages/clickhouse/schema/027_create_llm_model_aggregates_v1.sql
- apps/webapp/app/routes/admin.llm-models.$modelId.tsx
- internal-packages/llm-model-catalog/src/seed.ts
📜 Review details
⏰ 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). (13)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 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 / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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}: For apps and internal packages (apps/*,internal-packages/*), usepnpm run typecheck --filter <package>for verification, never usebuildas it proves almost nothing about correctness
Use testcontainers helpers (redisTest,postgresTest,containerTestfrom@internal/testcontainers) for integration tests with Redis and PostgreSQL instead of mocking
When writing Trigger.dev tasks, always import from@trigger.dev/sdk- never use@trigger.dev/sdk/v3or deprecatedclient.defineJob
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.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/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Use pnpm for package management in this monorepo (version 10.23.0) with Turborepo for orchestration - run commands from root withpnpm run
Add crumbs as you write code for debug tracing using//@Crumbscomments or `// `#region` `@crumbsblocks - they stay on the branch throughout development and are stripped viaagentcrumbs stripbefore merge
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.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/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.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/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.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/v3/eventRepository/eventRepository.types.tsinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
apps/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components (
apps/webapp/,apps/supervisor/, etc.) with no package changes, add a.server-changes/file instead of a changeset
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
apps/webapp/app/v3/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (
@internal/run-engine) and redis-worker, not legacy V1 engine code
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.ts
apps/webapp/app/**/*.{ts,tsx,server.ts}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Access environment variables via
envexport fromapp/env.server.ts. Never useprocess.envdirectly
Files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
apps/webapp/app/routes/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Use Remix flat-file route convention with dot-separated segments where
api.v1.tasks.$taskId.trigger.tsmaps to/api/v1/tasks/:taskId/trigger
Files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.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/queryService.server.ts
🧠 Learnings (42)
📓 Common learnings
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.ts:52-68
Timestamp: 2026-03-13T13:42:57.512Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.ts` (and related admin LLM model routes), multiple `LlmPricingTier` entries with the same `priority` under the same model are valid and intentional — tiers are matched in priority order. Duplicate-tier validation via Zod `superRefine` is not warranted as this is admin-only tooling with low concurrency.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.new.tsx:65-91
Timestamp: 2026-03-13T13:42:25.092Z
Learning: In `apps/webapp/app/routes/admin.llm-models.new.tsx`, sequential Prisma writes for model/tier creation are intentionally not wrapped in a transaction. The form is admin-only with low concurrency risk, and the blast radius is considered minimal for admin tooling.
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/components/navigation/SideMenu.tsxinternal-packages/tsql/src/query/schema.tsinternal-packages/clickhouse/src/client/queryBuilder.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/services/queryService.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/utils/pathBuilder.tsinternal-packages/clickhouse/src/llmModelAggregates.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T13:45:36.346Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/navigation/SideMenu.tsx:460-489
Timestamp: 2026-03-22T13:45:36.346Z
Learning: In triggerdotdev/trigger.dev, sidebar navigation items (SideMenu.tsx) are intentionally NOT gated behind feature-flag or permission checks at the nav level. Authorization is enforced at the route/loader level instead. Hiding nav items based on access checks is considered confusing UX. This applies to items like "AI Metrics" (v3BuiltInDashboardPath) and other dashboard links — they are always rendered in the sidebar regardless of hasQueryAccess or similar flags.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-02-03T18:27:49.039Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:49.039Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (like the Edit button with PencilSquareIcon) intentionally have no text labels - only icons are shown in the TableCellMenu. This is a deliberate UI design pattern for compact icon-only menu items.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsx
📚 Learning: 2026-02-11T16:50:14.167Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:14.167Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-21T21:23:35.117Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/runs/v3/ai/extractAISummarySpanData.ts:149-150
Timestamp: 2026-03-21T21:23:35.117Z
Learning: In `apps/webapp/app/components/runs/v3/ai/extractAISummarySpanData.ts` (and related AI span extraction files in `apps/webapp/app/components/runs/v3/ai/`), manual JSON.parse with typeof guards and type assertions is intentional. These functions are on the hot path for span rendering, so Zod validation is deliberately avoided for performance reasons. Do not suggest replacing manual JSON parsing with Zod schemas in these files.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-10T17:56:26.581Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3201
File: apps/webapp/app/v3/services/setSeatsAddOn.server.ts:25-29
Timestamp: 2026-03-10T17:56:26.581Z
Learning: In the `triggerdotdev/trigger.dev` webapp, service classes such as `SetSeatsAddOnService` and `SetBranchesAddOnService` do NOT need to perform their own userId-to-organizationId authorization checks. Auth is enforced at the route layer: `requireUserId(request)` authenticates the user, and the `_app.orgs.$organizationSlug` layout route enforces that the authenticated user is a member of the org. Any `userId` and `organizationId` reaching these services from org-scoped routes are already validated. This is the consistent pattern used across all org-scoped services in the codebase.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-13T13:42:25.092Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.new.tsx:65-91
Timestamp: 2026-03-13T13:42:25.092Z
Learning: In `apps/webapp/app/routes/admin.llm-models.new.tsx`, sequential Prisma writes for model/tier creation are intentionally not wrapped in a transaction. The form is admin-only with low concurrency risk, and the blast radius is considered minimal for admin tooling.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-13T13:42:57.512Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.ts:52-68
Timestamp: 2026-03-13T13:42:57.512Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.ts` (and related admin LLM model routes), multiple `LlmPricingTier` entries with the same `priority` under the same model are valid and intentional — tiers are matched in priority order. Duplicate-tier validation via Zod `superRefine` is not warranted as this is admin-only tooling with low concurrency.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsinternal-packages/llm-model-catalog/src/types.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-13T13:42:47.903Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:47.903Z
Learning: Relaxed date validation is acceptable only for admin-only API routes protected by PATs. When reviewing similarly structured routes, prefer not to enforce strict ISO datetime validation (e.g., using z.string().optional() or z.string().nullable().optional()) if the business logic stores/validates dates at the DB layer and access is restricted. This guidance applies specifically to the files matching apps/webapp/app/routes/admin.api.v1.llm-models*.ts. If similar fields exist in user-facing or publicly accessible endpoints, require strict ISO8601 validation or a proper date type. Ensure automated checks verify that admin routes remain protected and that any deviation from strict validation is justified by security/access-control guarantees.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.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:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-13T13:45:39.411Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.missing.$model.tsx:19-21
Timestamp: 2026-03-13T13:45:39.411Z
Learning: In `apps/webapp/app/routes/admin.llm-models.missing.$model.tsx`, the `decodeURIComponent(params.model ?? "")` call is intentionally unguarded. Remix route params are decoded at the routing layer before reaching the loader, so malformed percent-encoding is rejected upstream. The page is also admin-only, so the risk is minimal and no try-catch is warranted.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2025-08-14T18:35:44.370Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2390
File: apps/webapp/app/env.server.ts:764-765
Timestamp: 2025-08-14T18:35:44.370Z
Learning: The BoolEnv helper in apps/webapp/app/utils/boolEnv.ts uses z.preprocess with inconsistent default value types across the codebase - some usages pass boolean defaults (correct) while others pass string defaults (incorrect), leading to type confusion. The helper should enforce boolean-only defaults or have clearer documentation.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-22T13:49:25.960Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: internal-packages/llm-pricing/src/default-model-prices.json:3787-3898
Timestamp: 2026-03-22T13:49:25.960Z
Learning: `internal-packages/llm-pricing/src/default-model-prices.json` in the triggerdotdev/trigger.dev repository is auto-generated by syncing from Langfuse. Manual edits to this file will be overwritten on the next sync. Any missing or incorrect pricing data (e.g., missing reasoning token price aliases) must be fixed upstream in Langfuse, not in this file directly.
Applied to files:
apps/webapp/app/routes/admin.api.v1.llm-models.seed.tsinternal-packages/llm-model-catalog/src/types.tsinternal-packages/llm-model-catalog/scripts/generate-model-catalog.sh
📚 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} : Export every task, including subtasks
Applied to files:
internal-packages/clickhouse/src/index.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} : Use `tasks.trigger()` with type-only imports to trigger tasks from backend code without importing the task implementation
Applied to files:
internal-packages/clickhouse/src/index.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 **/*.{ts,tsx,js,jsx} : Use function declarations instead of default exports
Applied to files:
internal-packages/clickhouse/src/index.ts
📚 Learning: 2026-01-28T14:15:15.011Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2953
File: apps/webapp/app/components/runs/v3/SharedFilters.tsx:441-452
Timestamp: 2026-01-28T14:15:15.011Z
Learning: In apps/webapp/app/components/runs/v3/SharedFilters.tsx, the maxPeriodDays limit for date ranges should only check the from date (via dateRangeToDays(fromValue)) because it enforces data retention limits—how far back in history queries can reach. The to date is irrelevant for retention-based limits.
Applied to files:
apps/webapp/app/services/queryService.server.ts
📚 Learning: 2026-03-23T06:24:25.029Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.029Z
Learning: Applies to apps/webapp/app/routes/**/*.ts : Use Remix flat-file route convention with dot-separated segments where `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/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/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/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: The webapp at apps/webapp is a Remix 2.1 application using Node.js v20
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-13T13:42:59.104Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:59.104Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts` and `apps/webapp/app/routes/admin.api.v1.llm-models.ts`, the `startDate` field in `UpdateModelSchema` and `CreateModelSchema` intentionally uses `z.string().optional()` (or `.nullable().optional()`) without strict ISO datetime validation. Invalid date strings are rejected at the Prisma/DB layer. This is acceptable because these are admin-only API routes protected by Personal Access Token (PAT) authentication and are not user-facing.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2025-09-03T14:35:52.384Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2464
File: apps/webapp/app/utils/pathBuilder.ts:144-146
Timestamp: 2025-09-03T14:35:52.384Z
Learning: In the trigger.dev codebase, organization slugs are safe for URL query parameters and don't require URL encoding, as confirmed by the maintainer in apps/webapp/app/utils/pathBuilder.ts.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-22T13:32:43.689Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/metrics/PromptsFilter.tsx:74-98
Timestamp: 2026-03-22T13:32:43.689Z
Learning: In triggerdotdev/trigger.dev, filter components (e.g. PromptsFilter, ModelsFilter, QueuesFilter in apps/webapp/app/components/metrics/) use a `FilterMenuProvider` render-prop pattern that wraps Ariakit's ComboboxProvider. The render props `(search, setSearch)` represent live combobox search state; `searchValue` passed to the inner `*Dropdown` component is always up-to-date on each keystroke. Do not flag these components for "unconnected search state" or "broken filtering" — the wiring is correct by design and consistent across all filter components.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-22T13:32:47.004Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/metrics/ProvidersFilter.tsx:74-96
Timestamp: 2026-03-22T13:32:47.004Z
Learning: In the triggerdotdev/trigger.dev codebase, `FilterMenuProvider` (in `apps/webapp/app/components/runs/v3/SharedFilters.tsx`) wraps Ariakit's `ComboboxProvider` and exposes `(search, setSearch)` as render props. Filter components like `ProvidersFilter`, `OperationsFilter`, `PromptsFilter`, `ModelsFilter`, and `QueuesFilter` all use this pattern: `searchValue` (from the render prop) is passed into the dropdown child and used in a `useMemo` to filter options. Because Ariakit's ComboboxProvider re-renders the render prop on every keystroke, `searchValue` is reactive and the filtered list updates correctly. Do not flag these patterns as broken/unconnected search state — the wiring is intentional and correct.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-22T13:32:46.525Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/metrics/OperationsFilter.tsx:85-110
Timestamp: 2026-03-22T13:32:46.525Z
Learning: In triggerdotdev/trigger.dev, `FilterMenuProvider` (from `~/components/runs/v3/SharedFilters`) wraps Ariakit's `ComboboxProvider` and exposes the current search string and setter via render props `(search, setSearch) => …`. Components like `OperationsFilter`, `ModelsFilter`, `QueuesFilter`, and `ProvidersFilter` all use this pattern: they pass `searchValue={search}` and `clearSearchValue={() => setSearch("")}` into their inner dropdown component, and the `filtered` useMemo is correctly reactive. Do not flag this as a missing/broken wiring between the ComboBox input and the search state — the state is managed by Ariakit's provider and surfaced through the render-prop tuple.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
📚 Learning: 2026-03-23T06:24:14.566Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to apps/webapp/app/v3/**/*.{ts,tsx} : In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 engine code
Applied to files:
apps/webapp/app/utils/pathBuilder.ts
📚 Learning: 2026-03-23T06:24:25.029Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.029Z
Learning: Applies to apps/webapp/app/v3/services/**/*.server.ts : Only modify V2 code paths when editing services that branch on `RunEngineVersion` to support both V1 and V2 (e.g., `cancelTaskRun.server.ts`, `batchTriggerV3.server.ts`)
Applied to files:
apps/webapp/app/utils/pathBuilder.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/v3/services/**/*.server.{ts,tsx} : Organize services in the webapp following the pattern `app/v3/services/*/*.server.ts`
Applied to files:
apps/webapp/app/utils/pathBuilder.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/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
apps/webapp/app/utils/pathBuilder.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/v3/presenters/**/*.server.{ts,tsx} : Organize presenters in the webapp following the pattern `app/v3/presenters/*/*.server.ts` to move complex loader code into classes
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T19:34:22.737Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors._index/route.tsx:99-103
Timestamp: 2026-03-22T19:34:22.737Z
Learning: In `apps/webapp/app/presenters/v3/ErrorsListPresenter.server.ts`, the `statuses` filter (UNRESOLVED | RESOLVED | IGNORED) is applied in-memory after the ClickHouse query and a batch Postgres lookup via `getErrorGroupStates`. This is intentional: `status` lives in the Postgres `ErrorGroupState` table, not in ClickHouse, so post-query filtering is the correct approach. Do not flag this as a missing predicate or a no-op filter during code review.
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.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/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T13:51:25.797Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/presenters/v3/PromptPresenter.server.ts:100-141
Timestamp: 2026-03-22T13:51:25.797Z
Learning: In the triggerdotdev/trigger.dev codebase, the ClickHouse server is configured with UTC as its timezone. Therefore, `toStartOfHour(start_time)` (without an explicit timezone argument) in ClickHouse queries correctly returns UTC-formatted strings that align with JavaScript `toISOString()`-derived UTC bucket keys (e.g., in `apps/webapp/app/presenters/v3/PromptPresenter.server.ts`). Do not flag this as a timezone mismatch bug.
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T19:32:16.231Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx:82-151
Timestamp: 2026-03-22T19:32:16.231Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts` and `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx`, the `errorAlertConfig` field on `ProjectAlertChannel` is intentionally `Json?` (nullable). The `ErrorAlertEvaluator.computeMinInterval()` in `apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts` uses `ErrorAlertConfig.safeParse(ch.errorAlertConfig)` and falls back to `DEFAULT_INTERVAL_MS = 300_000` when `errorAlertConfig` is null. No UI currently collects this value — it is scaffolding for a future per-channel evaluation interval feature. Do not flag the absence of `errorAlertConfig` in `CreateAlertChannelOptions` or the action handler as a bug; null configs are safe and expected.
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-03-22T13:50:20.911Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/runs/v3/ai/extractAISummarySpanData.ts:15-34
Timestamp: 2026-03-22T13:50:20.911Z
Learning: In `apps/webapp/app/components/runs/v3/ai/extractAISummarySpanData.ts`, the `rec()` helper always returns `{}` for non-object values and `str()` returns `undefined` for non-strings. The truthiness guard on `ai.operationId` (Line 15) is intentionally loose — a non-string truthy value simply results in `str()` returning `undefined` and `operationName` defaulting to `""`. The real early-exit guard is the model check (`if (!model) return undefined`). An empty `operationName` is considered harmless in this code path.
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
📚 Learning: 2026-02-06T19:53:38.843Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/presenters/v3/DeploymentListPresenter.server.ts:233-237
Timestamp: 2026-02-06T19:53:38.843Z
Learning: When constructing Vercel dashboard URLs from deployment IDs, always strip the dpl_ prefix from the ID. Implement this by transforming the ID with .replace(/^dpl_/, "") before concatenating into the URL: https://vercel.com/${teamSlug}/${projectName}/${cleanedDeploymentId}. Consider centralizing this logic in a small helper (e.g., getVercelDeploymentId(id) or a URL builder) and add tests to verify both prefixed and non-prefixed inputs.
Applied to files:
apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts
🪛 Shellcheck (0.11.0)
internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh
[warning] 77-77: MODEL_TO_BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 113-113: LOCK_FILE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 240-240: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…es, and TSQL schema - Add llm-model-catalog package (renamed from llm-pricing) with Claude CLI research pipeline - Add Prisma schema: catalog columns + baseModelName on LlmModel - Add ClickHouse: llm_model_aggregates MV + base_response_model column - Add TSQL llm_models schema for query page integration - Add ModelRegistryPresenter with catalog, metrics, and comparison queries - Add 3 dashboard pages: catalog (cards+table+filters), detail (overview+metrics+cost estimator), compare - Add sidebar navigation under AI section with hasAiAccess feature flag - Add admin dashboard sync/seed for catalog metadata - Add model variant grouping (dated snapshots under base models) - Add shared formatters and design system component usage refs TRI-7941
ericallam
force-pushed
the
feature/tri-7941-ai-models
branch
from
dc2736c to
b20e7e5
Compare
myftija
approved these changes
Mar 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh (2)
131-146:⚠️ Potential issue | 🟠 MajorBase-family lookups still miss variant-only catalog entries.
The skip and diff paths still read
d['$base'], but the merge only writes variant keys. Any family that exists only asfoo-latest/foo-20250301will be treated as missing on every run, so it never becomes fresh andCHANGEDis computed againstnull. Please make those lookups scanBASE_TO_VARIANTS[$base], or persist a canonical base entry alongside the variants.Also applies to: 288-293, 317-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh` around lines 131 - 146, The script currently checks for a base entry using d['$base'] (used when computing SKIP_REASON and RESOLVED_DATE from EXISTING_CATALOG), which misses catalogs that exist only as variant keys; update those node JSON lookups to first consult BASE_TO_VARIANTS[$base] and scan EXISTING_CATALOG for any variant keys mapped from that base (e.g., iterate BASE_TO_VARIANTS[$base] and pick the first existing variant entry) or, alternatively, persist and read a canonical base entry alongside variants; apply the same change to every JSON read of d['$base'] (including the blocks that set SKIP_REASON, RESOLVED_DATE and the other occurrences around the diff sections) so families represented only by variant keys are recognized as present.
304-313:⚠️ Potential issue | 🟡 MinorPass JSON through environment variables instead of splicing into string literals.
The code directly embeds
$OLD_ENTRYand$NEW_FOR_DIFFinto single-quoted JavaScript strings (JSON.parse('$OLD_ENTRY')). Sincedescriptionfields come from external API responses and may contain apostrophes, a single'character breaks the string literal, causingJSON.parse()to fail silently (hidden by2>/dev/null || true). Change detection then produces no output.Use environment variables to pass the JSON safely:
💡 Proposed fix
- node -e " - const old = JSON.parse('$OLD_ENTRY'); - const cur = JSON.parse('$NEW_FOR_DIFF'); + OLD_ENTRY_JSON="$OLD_ENTRY" NEW_ENTRY_JSON="$NEW_FOR_DIFF" node -e ' + const old = JSON.parse(process.env.OLD_ENTRY_JSON); + const cur = JSON.parse(process.env.NEW_ENTRY_JSON); const changes = []; for (const k of new Set([...Object.keys(old), ...Object.keys(cur)])) { const o = JSON.stringify(old[k]); const n = JSON.stringify(cur[k]); if (o !== n) changes.push(k + ': ' + o + ' → ' + n); } if (changes.length) console.log(' CHANGED: ' + changes.join(', ')); - " 2>/dev/null || true + ' 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh` around lines 304 - 313, The inline JS currently uses JSON.parse('$OLD_ENTRY') and JSON.parse('$NEW_FOR_DIFF') which breaks when values contain apostrophes; instead read the payloads from environment variables inside the Node snippet (e.g., use JSON.parse(process.env.OLD_ENTRY) and JSON.parse(process.env.NEW_FOR_DIFF')) and invoke the node command with those env vars supplied (export or prefix the node invocation so the shell passes the raw JSON rather than embedding it into the single-quoted literal); update the node -e block that builds changes to reference process.env.OLD_ENTRY and process.env.NEW_FOR_DIFF and ensure the surrounding shell sets those env vars when calling node.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsx (2)
214-243:⚠️ Potential issue | 🟡 MinorClamp the estimator inputs to zero or above.
Negative values currently produce negative token counts and a negative estimated cost.
Suggested change
<Input id="input-tokens" variant="medium" fullWidth type="number" + min={0} value={inputTokens} - onChange={(e) => setInputTokens(parseInt(e.target.value) || 0)} + onChange={(e) => + setInputTokens(Math.max(0, parseInt(e.target.value, 10) || 0)) + } /> @@ <Input id="output-tokens" variant="medium" fullWidth type="number" + min={0} value={outputTokens} - onChange={(e) => setOutputTokens(parseInt(e.target.value) || 0)} + onChange={(e) => + setOutputTokens(Math.max(0, parseInt(e.target.value, 10) || 0)) + } /> @@ <Input id="num-calls" variant="medium" fullWidth type="number" + min={0} value={numCalls} - onChange={(e) => setNumCalls(parseInt(e.target.value) || 0)} + onChange={(e) => + setNumCalls(Math.max(0, parseInt(e.target.value, 10) || 0)) + } />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsx around lines 214 - 243, The three numeric inputs (ids "input-tokens", "output-tokens", "num-calls") allow negative values because their onChange handlers use parseInt(e.target.value) || 0 without clamping; update the handlers that call setInputTokens, setOutputTokens and setNumCalls to parse the value and then clamp to zero or above (e.g. const v = Math.max(0, parseInt(... ) || 0)) before passing to the state setters so estimated token counts/costs cannot become negative.
430-483:⚠️ Potential issue | 🟠 MajorInclude variant traffic in these model widgets.
Every query is pinned to
response_model = .... For grouped/base model pages that drops traffic stored under dated variant names, even thoughbase_response_modelnow exists for the rollup.Also applies to: 527-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsx around lines 430 - 483, The queries in the MetricWidget instances are filtering only on response_model, which excludes traffic stored under variant names; update each WHERE clause used in the model widgets (e.g., the MetricWidget instances with widgetKey patterns like `${modelName}-calls`, `-ttfc-p50`, `-ttfc-p90`, `-tps`, `-calls-time`, `-ttfc-time`) to include both response_model and base_response_model (e.g., WHERE (response_model = '${escapeTSQL(modelName)}' OR base_response_model = '${escapeTSQL(modelName)}')) so variant traffic is included; apply the same change to the other, duplicate MetricWidget queries later in the file that target the same metrics.apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts (3)
235-239:⚠️ Potential issue | 🟠 MajorInclude
providerin the variant-group key.Grouping by normalized base/model name alone collapses same-named models across providers, so one provider can absorb another provider’s variants and representative card.
Suggested change
- const groupKey = normalizeForGrouping(item._baseModelName ?? item.modelName); + const groupKey = `${item.provider}:${normalizeForGrouping( + item._baseModelName ?? item.modelName + )}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts` around lines 235 - 239, The grouping key currently uses only normalizeForGrouping(item._baseModelName ?? item.modelName) which merges models with the same name across providers; include the provider in the key so variants are grouped per-provider. Change the key construction (referencing groupKey, items, normalizeForGrouping, variantGroups and the item._baseModelName/item.modelName) to incorporate the provider (e.g., combine item.provider with the normalized name, handling missing provider values consistently) before using variantGroups.set/get so models with the same name but different providers remain separate.
307-315:⚠️ Potential issue | 🟠 MajorKeep detail lookups scoped to browsable models.
getModelCatalog()only surfaces global, non-hidden rows, but this lookup drops those constraints. A direct detail URL can still resolve rows that never appear in the registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts` around lines 307 - 315, The detail lookup using this._replica.llmModel.findUnique(friendlyId, include: pricingTiers) is missing the same browsable constraints used by getModelCatalog(), which allows hidden or non-global models to be resolved directly; update the findUnique where clause to include the same filters as getModelCatalog() (e.g., hidden: false and the appropriate scope/visibility constraint such as scope: 'global' or browsable flag) so only models surfaced in getModelCatalog() can be fetched by detail URL.
378-385:⚠️ Potential issue | 🟠 MajorDon’t synthesize zero-valued percentiles.
0here reads as real telemetry, not “missing data”. Return nullable/omitted values or extend the aggregate query to emit the actual percentiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts` around lines 378 - 385, The mapping is synthesizing zero-valued percentiles (tpsP95, tpsP99, durationP95, durationP99) which misrepresents missing telemetry; either extend the aggregate query that populates r to compute and return the real percentile fields (e.g. r.tps_p95, r.tps_p99, r.duration_p95, r.duration_p99) and map those, or change the presenter mapping (where tpsP50/tpsP90/durationP50/durationP90 are set) to emit nullable/omitted values instead of 0 (set to null or omit the key when r lacks the percentile fields) so consumers can distinguish missing data from actual zeroes.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx (2)
423-427:⚠️ Potential issue | 🟡 MinorValidate
viewandfeaturesbefore using them.
viewcurrently falls through to table on any unexpected string, andfeaturesis only type-asserted. Invalid URL params can silently break layout/filtering.As per coding guidelines: "Use zod for validation in packages/core and apps/webapp".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx around lines 423 - 427, Validate the incoming URL params for view and features using zod before using them: define a zod schema/enums for allowed view values (e.g., "cards"|"table") and for FeatureKey, validate searchParams.value("view") against that enum and default to "cards" for invalid values, and validate searchParams.values("features") as an array of allowed FeatureKey strings instead of a raw type assertion; update the code paths that read view and selectedFeatures (the variables view and selectedFeatures/selectedFeatures usage in this route.tsx) to use the parsed/validated results so unexpected URL params can't break layout or filtering.
507-518:⚠️ Potential issue | 🟠 MajorLabel the view-toggle buttons.
These are icon-only controls with no accessible name or pressed state, so screen readers can’t tell which button switches to cards vs. table or which view is active.
Suggested change
<button + type="button" onClick={() => searchParams.replace({ view: "cards" })} + aria-label="Cards view" + title="Cards view" + aria-pressed={view === "cards"} className={`p-1.5 transition-colors ${view === "cards" ? "bg-charcoal-700 text-text-bright" : "text-text-dimmed hover:text-text-bright"}`} > <Squares2X2Icon className="h-4 w-4" /> </button> <button + type="button" onClick={() => searchParams.replace({ view: "table" })} + aria-label="Table view" + title="Table view" + aria-pressed={view === "table"} className={`p-1.5 transition-colors ${view === "table" ? "bg-charcoal-700 text-text-bright" : "text-text-dimmed hover:text-text-bright"}`} > <ListBulletIcon className="h-4 w-4" /> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx around lines 507 - 518, The two icon-only view-toggle buttons in route.tsx (the button elements that call searchParams.replace and reference view === "cards" / view === "table") lack accessible names and pressed state; add an accessible label (e.g., aria-label="Cards view" and aria-label="Table view" or use visually-hidden text) and expose the active state with aria-pressed={view === "cards"} and aria-pressed={view === "table"} (or aria-current="true" when active) so screen readers know which control is which and which view is selected; keep the existing onClick handlers and visual styling unchanged.apps/webapp/app/components/navigation/SideMenu.tsx (1)
461-504:⚠️ Potential issue | 🟠 MajorDon’t gate the AI section in the sidebar.
This hides both the section and the Models item at render time. In this codebase, discoverability stays in the sidebar and the access check happens in the route/loaders.
Based on learnings: sidebar navigation items in
apps/webapp/app/components/navigation/SideMenu.tsxare intentionally not gated behind feature-flag or permission checks at the nav level; authorization is enforced at the route/loader level instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/SideMenu.tsx` around lines 461 - 504, Remove the feature/permission gating around the AI navigation so the section and its items always render: delete the outer conditional expression that wraps the SideMenuSection (the "(user.admin || user.isImpersonating || featureFlags.hasAiAccess) &&" check) and also remove the inner guard around the Models SideMenuItem (the "(user.admin || user.isImpersonating || featureFlags.hasAiModelsAccess) &&" check) so SideMenuSection and all SideMenuItem entries (Prompts, Models, AI Metrics) render unconditionally; keep existing props like isSideMenuCollapsed, initialCollapsed (getSectionCollapsed), onCollapseToggle (handleSectionToggle("ai")), and isCollapsed unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts`:
- Around line 397-449: The summaryQuery and taskQuery currently filter only on
response_model, which misses rows where a variant's base_response_model should
be rolled up; update both queries' WHERE clauses to match either response_model
= {responseModel: String} OR base_response_model = {responseModel: String}
(i.e., use a predicate like (response_model = {responseModel} OR
base_response_model = {responseModel})), keeping the existing params object the
same and ensuring the condition appears in both the modelRegistryUserSummary and
modelRegistryUserTasks queries.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx:
- Around line 372-380: The UI uses model.modelName as a key for popularity
lookups, compareSet, and the compare URL which is wrong when the same modelName
exists under different providers; modify the code to use a provider-qualified
key (e.g., const providerKey = `${model.provider}:${model.modelName}` or use
model.providerQualifiedId if available) everywhere instead of model.modelName:
replace popularMap.get(model.modelName) with popularMap.get(providerKey), change
compareSet.has(model.modelName) and onToggleCompare(model.modelName) to use
providerKey, and update any URL/route helpers (v3ModelDetailPath/compare URL
builders) to incorporate the provider-qualified key so rows and metrics remain
distinct across providers.
- Around line 65-91: In the loader function (exported as loader) add a
feature-gate check using canAccessAiModels before loading the model catalog:
call canAccessAiModels(request) right after requireUserId(request) (or at top of
loader) and if it returns false short-circuit the loader by returning/throwing
the same response used elsewhere for disabled features (e.g. a 404/redirect
consistent with the project’s existing pattern); then continue with the existing
ModelRegistryPresenter and catalog/popularModels logic only when the check
passes. Ensure you reference canAccessAiModels and loader so the change is
applied in this route and mirror the same check in sibling model routes if they
have their own loaders.
---
Duplicate comments:
In `@apps/webapp/app/components/navigation/SideMenu.tsx`:
- Around line 461-504: Remove the feature/permission gating around the AI
navigation so the section and its items always render: delete the outer
conditional expression that wraps the SideMenuSection (the "(user.admin ||
user.isImpersonating || featureFlags.hasAiAccess) &&" check) and also remove the
inner guard around the Models SideMenuItem (the "(user.admin ||
user.isImpersonating || featureFlags.hasAiModelsAccess) &&" check) so
SideMenuSection and all SideMenuItem entries (Prompts, Models, AI Metrics)
render unconditionally; keep existing props like isSideMenuCollapsed,
initialCollapsed (getSectionCollapsed), onCollapseToggle
(handleSectionToggle("ai")), and isCollapsed unchanged.
In `@apps/webapp/app/presenters/v3/ModelRegistryPresenter.server.ts`:
- Around line 235-239: The grouping key currently uses only
normalizeForGrouping(item._baseModelName ?? item.modelName) which merges models
with the same name across providers; include the provider in the key so variants
are grouped per-provider. Change the key construction (referencing groupKey,
items, normalizeForGrouping, variantGroups and the
item._baseModelName/item.modelName) to incorporate the provider (e.g., combine
item.provider with the normalized name, handling missing provider values
consistently) before using variantGroups.set/get so models with the same name
but different providers remain separate.
- Around line 307-315: The detail lookup using
this._replica.llmModel.findUnique(friendlyId, include: pricingTiers) is missing
the same browsable constraints used by getModelCatalog(), which allows hidden or
non-global models to be resolved directly; update the findUnique where clause to
include the same filters as getModelCatalog() (e.g., hidden: false and the
appropriate scope/visibility constraint such as scope: 'global' or browsable
flag) so only models surfaced in getModelCatalog() can be fetched by detail URL.
- Around line 378-385: The mapping is synthesizing zero-valued percentiles
(tpsP95, tpsP99, durationP95, durationP99) which misrepresents missing
telemetry; either extend the aggregate query that populates r to compute and
return the real percentile fields (e.g. r.tps_p95, r.tps_p99, r.duration_p95,
r.duration_p99) and map those, or change the presenter mapping (where
tpsP50/tpsP90/durationP50/durationP90 are set) to emit nullable/omitted values
instead of 0 (set to null or omit the key when r lacks the percentile fields) so
consumers can distinguish missing data from actual zeroes.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx:
- Around line 423-427: Validate the incoming URL params for view and features
using zod before using them: define a zod schema/enums for allowed view values
(e.g., "cards"|"table") and for FeatureKey, validate searchParams.value("view")
against that enum and default to "cards" for invalid values, and validate
searchParams.values("features") as an array of allowed FeatureKey strings
instead of a raw type assertion; update the code paths that read view and
selectedFeatures (the variables view and selectedFeatures/selectedFeatures usage
in this route.tsx) to use the parsed/validated results so unexpected URL params
can't break layout or filtering.
- Around line 507-518: The two icon-only view-toggle buttons in route.tsx (the
button elements that call searchParams.replace and reference view === "cards" /
view === "table") lack accessible names and pressed state; add an accessible
label (e.g., aria-label="Cards view" and aria-label="Table view" or use
visually-hidden text) and expose the active state with aria-pressed={view ===
"cards"} and aria-pressed={view === "table"} (or aria-current="true" when
active) so screen readers know which control is which and which view is
selected; keep the existing onClick handlers and visual styling unchanged.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsx:
- Around line 214-243: The three numeric inputs (ids "input-tokens",
"output-tokens", "num-calls") allow negative values because their onChange
handlers use parseInt(e.target.value) || 0 without clamping; update the handlers
that call setInputTokens, setOutputTokens and setNumCalls to parse the value and
then clamp to zero or above (e.g. const v = Math.max(0, parseInt(... ) || 0))
before passing to the state setters so estimated token counts/costs cannot
become negative.
- Around line 430-483: The queries in the MetricWidget instances are filtering
only on response_model, which excludes traffic stored under variant names;
update each WHERE clause used in the model widgets (e.g., the MetricWidget
instances with widgetKey patterns like `${modelName}-calls`, `-ttfc-p50`,
`-ttfc-p90`, `-tps`, `-calls-time`, `-ttfc-time`) to include both response_model
and base_response_model (e.g., WHERE (response_model =
'${escapeTSQL(modelName)}' OR base_response_model = '${escapeTSQL(modelName)}'))
so variant traffic is included; apply the same change to the other, duplicate
MetricWidget queries later in the file that target the same metrics.
In `@internal-packages/llm-model-catalog/scripts/generate-model-catalog.sh`:
- Around line 131-146: The script currently checks for a base entry using
d['$base'] (used when computing SKIP_REASON and RESOLVED_DATE from
EXISTING_CATALOG), which misses catalogs that exist only as variant keys; update
those node JSON lookups to first consult BASE_TO_VARIANTS[$base] and scan
EXISTING_CATALOG for any variant keys mapped from that base (e.g., iterate
BASE_TO_VARIANTS[$base] and pick the first existing variant entry) or,
alternatively, persist and read a canonical base entry alongside variants; apply
the same change to every JSON read of d['$base'] (including the blocks that set
SKIP_REASON, RESOLVED_DATE and the other occurrences around the diff sections)
so families represented only by variant keys are recognized as present.
- Around line 304-313: The inline JS currently uses JSON.parse('$OLD_ENTRY') and
JSON.parse('$NEW_FOR_DIFF') which breaks when values contain apostrophes;
instead read the payloads from environment variables inside the Node snippet
(e.g., use JSON.parse(process.env.OLD_ENTRY) and
JSON.parse(process.env.NEW_FOR_DIFF')) and invoke the node command with those
env vars supplied (export or prefix the node invocation so the shell passes the
raw JSON rather than embedding it into the single-quoted literal); update the
node -e block that builds changes to reference process.env.OLD_ENTRY and
process.env.NEW_FOR_DIFF and ensure the surrounding shell sets those env vars
when calling node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6825fc2-f92d-40b3-aff6-8e6a288052a9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/ModelRegistryPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.$modelId/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models.compare/route.tsxapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.tsapps/webapp/app/routes/admin.llm-models.$modelId.tsxapps/webapp/app/routes/admin.llm-models._index.tsxapps/webapp/app/routes/admin.llm-models.new.tsxapps/webapp/app/services/queryService.server.tsapps/webapp/app/utils/modelFormatters.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/canAccessAi.server.tsapps/webapp/app/v3/canAccessAiModels.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/eventRepository.types.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/llmPricingRegistry.server.tsapps/webapp/app/v3/querySchemas.tsapps/webapp/app/v3/utils/enrichCreatableEvents.server.tsapps/webapp/package.jsonapps/webapp/seed-ai-spans.mtsinternal-packages/clickhouse/schema/026_add_base_response_model_to_llm_metrics_v1.sqlinternal-packages/clickhouse/schema/027_create_llm_model_aggregates_v1.sqlinternal-packages/clickhouse/src/client/queryBuilder.tsinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/llmMetrics.tsinternal-packages/clickhouse/src/llmModelAggregates.tsinternal-packages/database/prisma/migrations/20260323104144_add_catalog_columns_to_llm_models/migration.sqlinternal-packages/database/prisma/migrations/20260324142824_add_base_model_name_to_llm_models/migration.sqlinternal-packages/database/prisma/schema.prismainternal-packages/llm-model-catalog/package.jsoninternal-packages/llm-model-catalog/scripts/.gitignoreinternal-packages/llm-model-catalog/scripts/generate-model-catalog.shinternal-packages/llm-model-catalog/scripts/generate.mjsinternal-packages/llm-model-catalog/scripts/research-model.shinternal-packages/llm-model-catalog/scripts/sync-model-prices.shinternal-packages/llm-model-catalog/src/default-model-prices.jsoninternal-packages/llm-model-catalog/src/defaultPrices.tsinternal-packages/llm-model-catalog/src/index.tsinternal-packages/llm-model-catalog/src/model-catalog.jsoninternal-packages/llm-model-catalog/src/modelCatalog.tsinternal-packages/llm-model-catalog/src/registry.test.tsinternal-packages/llm-model-catalog/src/registry.tsinternal-packages/llm-model-catalog/src/seed.tsinternal-packages/llm-model-catalog/src/sync.tsinternal-packages/llm-model-catalog/src/types.tsinternal-packages/llm-model-catalog/tsconfig.jsoninternal-packages/llm-pricing/package.jsoninternal-packages/llm-pricing/src/types.tsinternal-packages/tsql/src/index.test.tsinternal-packages/tsql/src/query/functions.tsinternal-packages/tsql/src/query/schema.ts
💤 Files with no reviewable changes (3)
- internal-packages/tsql/src/index.test.ts
- internal-packages/llm-pricing/package.json
- internal-packages/llm-pricing/src/types.ts
✅ Files skipped from review due to trivial changes (18)
- internal-packages/llm-model-catalog/scripts/.gitignore
- internal-packages/database/prisma/migrations/20260324142824_add_base_model_name_to_llm_models/migration.sql
- internal-packages/database/prisma/migrations/20260323104144_add_catalog_columns_to_llm_models/migration.sql
- apps/webapp/package.json
- internal-packages/clickhouse/src/llmMetrics.ts
- internal-packages/llm-model-catalog/src/defaultPrices.ts
- internal-packages/llm-model-catalog/package.json
- internal-packages/clickhouse/src/index.ts
- apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts
- internal-packages/clickhouse/schema/026_add_base_response_model_to_llm_metrics_v1.sql
- apps/webapp/app/utils/modelFormatters.ts
- internal-packages/llm-model-catalog/src/types.ts
- internal-packages/llm-model-catalog/src/modelCatalog.ts
- internal-packages/llm-model-catalog/src/index.ts
- apps/webapp/app/v3/eventRepository/eventRepository.types.ts
- internal-packages/llm-model-catalog/src/model-catalog.json
- internal-packages/clickhouse/schema/027_create_llm_model_aggregates_v1.sql
- internal-packages/tsql/src/query/functions.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- apps/webapp/app/v3/utils/enrichCreatableEvents.server.ts
- apps/webapp/app/services/queryService.server.ts
- apps/webapp/app/routes/admin.api.v1.llm-models.seed.ts
- internal-packages/tsql/src/query/schema.ts
- apps/webapp/app/v3/canAccessAiModels.server.ts
- internal-packages/llm-model-catalog/src/seed.ts
- apps/webapp/app/v3/llmPricingRegistry.server.ts
- internal-packages/llm-model-catalog/scripts/sync-model-prices.sh
- apps/webapp/app/v3/canAccessAi.server.ts
- apps/webapp/app/routes/admin.llm-models.new.tsx
- apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts
- apps/webapp/app/utils/pathBuilder.ts
- apps/webapp/app/v3/featureFlags.server.ts
- internal-packages/llm-model-catalog/src/sync.ts
- apps/webapp/app/routes/admin.llm-models._index.tsx
- apps/webapp/seed-ai-spans.mts
- apps/webapp/app/routes/admin.api.v1.llm-models.ts
- internal-packages/clickhouse/src/llmModelAggregates.ts
- internal-packages/database/prisma/schema.prisma
- internal-packages/llm-model-catalog/scripts/research-model.sh
- apps/webapp/app/v3/querySchemas.ts
- internal-packages/llm-model-catalog/scripts/generate.mjs
...tes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
Show resolved
Hide resolved
...tes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs TRI-7941