Conversation
|
|
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 end-to-end error analytics and optional error fingerprinting: a fingerprinting utility and normalization; optional fingerprint computation in the runs replication service; schema migrations and new ClickHouse tables/materialized views (error aggregations and occurrences) plus query/utility APIs; server presenters for errors list and error-group detail; new Remix routes and UI for Errors index and detail pages; RunFilters, TaskRunsTable, and search input updates to support errorId filtering; path helpers, semver and time-granularity utilities; ClickHouse query builder HAVING support; tests, benchmarks, and related type/interface updates. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 |
![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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/components/primitives/DateTime.tsx (1)
377-390: Avoid initial blank text and duplicate update path.At Line 377 the state starts as
"", then Line 388-390 immediately sets it. Consider initializing state fromgetRelativeText(realDate)and collapsing to a single effect to remove the empty first paint and extra update cycle.♻️ Proposed simplification
- const [relativeText, setRelativeText] = useState(""); + const [relativeText, setRelativeText] = useState(() => getRelativeText(realDate)); - // Every 60s refresh - useEffect(() => { - const interval = setInterval(() => { - setRelativeText(getRelativeText(realDate)); - }, 60_000); - return () => clearInterval(interval); - }, [realDate]); - - // On first render - useEffect(() => { - setRelativeText(getRelativeText(realDate)); - }, [realDate]); + useEffect(() => { + setRelativeText(getRelativeText(realDate)); + const interval = setInterval(() => { + setRelativeText(getRelativeText(realDate)); + }, 60_000); + return () => clearInterval(interval); + }, [realDate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/primitives/DateTime.tsx` around lines 377 - 390, Initialize the relativeText state using getRelativeText(realDate) instead of an empty string and replace the two effects with a single useEffect that (1) sets setRelativeText(getRelativeText(realDate)) immediately when realDate changes and (2) starts the 60s interval that updates setRelativeText(getRelativeText(realDate)) and is cleaned up on unmount; update references to relativeText, setRelativeText, getRelativeText and the interval logic accordingly so there is no initial blank render or duplicated update path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/components/primitives/DateTime.tsx`:
- Around line 377-390: Initialize the relativeText state using
getRelativeText(realDate) instead of an empty string and replace the two effects
with a single useEffect that (1) sets setRelativeText(getRelativeText(realDate))
immediately when realDate changes and (2) starts the 60s interval that updates
setRelativeText(getRelativeText(realDate)) and is cleaned up on unmount; update
references to relativeText, setRelativeText, getRelativeText and the interval
logic accordingly so there is no initial blank render or duplicated update path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee2d7330-486f-49ed-96f8-1dcc497045c6
📒 Files selected for processing (1)
apps/webapp/app/components/primitives/DateTime.tsx
📜 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). (28)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 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 (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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}: In TypeScript SDK usage, always import from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or use deprecated client.defineJob
Import from@trigger.dev/coresubpaths only, never from the root
Use the Run Engine 2.0 (@internal/run-engine) and redis-worker for all new work, not legacy V1 MarQS queue or deprecated V1 functions
Files:
apps/webapp/app/components/primitives/DateTime.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/primitives/DateTime.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/primitives/DateTime.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/primitives/DateTime.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/primitives/DateTime.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/primitives/DateTime.tsx
apps/**/*.{ts,tsx,js}
📄 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/components/primitives/DateTime.tsx
🧠 Learnings (2)
📚 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: Use `useRun`, `useRealtimeRun` and other SWR/realtime hooks from `trigger.dev/react-hooks` for data fetching
Applied to files:
apps/webapp/app/components/primitives/DateTime.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/primitives/DateTime.tsx
🔇 Additional comments (1)
apps/webapp/app/components/primitives/DateTime.tsx (1)
379-385: Nice fix for live relative-time updates.The interval refresh with proper cleanup keeps
RelativeDateTimecurrent and avoids the previously frozen output issue.
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.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| CREATE MATERIALIZED VIEW trigger_dev.errors_mv_v1 | ||
| TO trigger_dev.errors_v1 | ||
| AS | ||
| SELECT | ||
| organization_id, | ||
| project_id, | ||
| environment_id, | ||
| task_identifier, | ||
| error_fingerprint, | ||
|
|
||
| any(coalesce(nullIf(toString(error.data.type), ''), nullIf(toString(error.data.name), ''), 'Error')) as error_type, | ||
| any(coalesce(nullIf(substring(toString(error.data.message), 1, 500), ''), 'Unknown error')) as error_message, | ||
| any(coalesce(substring(toString(error.data.stack), 1, 2000), '')) as sample_stack_trace, | ||
|
|
||
| toDateTime(max(created_at)) as last_seen_date, | ||
|
|
||
| min(created_at) as first_seen, | ||
| max(created_at) as last_seen, | ||
| sumState(toUInt64(1)) as occurrence_count, | ||
| uniqState(task_version) as affected_task_versions, | ||
|
|
||
| anyState(run_id) as sample_run_id, | ||
| anyState(friendly_id) as sample_friendly_id, | ||
|
|
||
| sumMapState([status], [toUInt64(1)]) as status_distribution | ||
| FROM trigger_dev.task_runs_v2 | ||
| WHERE | ||
| error_fingerprint != '' | ||
| AND status IN ('SYSTEM_FAILURE', 'CRASHED', 'INTERRUPTED', 'COMPLETED_WITH_ERRORS', 'TIMED_OUT') | ||
| AND _is_deleted = 0 | ||
| GROUP BY | ||
| organization_id, | ||
| project_id, | ||
| environment_id, | ||
| task_identifier, | ||
| error_fingerprint; |
There was a problem hiding this comment.
🚩 ClickHouse MV over-counting risk with ReplacingMergeTree source
The materialized views errors_mv_v1 and error_occurrences_mv_v1 fire on every INSERT into task_runs_v2. Since task_runs_v2 uses ReplacingMergeTree, the same run can be inserted multiple times (once per replication event/update). Each insert triggers the MV independently, before ReplacingMergeTree deduplication occurs. This means if a run in COMPLETED_WITH_ERRORS status gets updated (e.g., tags added, metadata changed), the MV fires again and increments occurrence_count. The ConcurrentFlushScheduler deduplication (internal-packages/clickhouse/schema/022_create_errors_v1_table.sql:39-74) mitigates this within flush windows, but updates across different batches will still cause over-counting. This is a known ClickHouse MV limitation and the counts are already approximate, but worth documenting as a known trade-off.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is good to know, not much we can do about it.
All of these statuses are "final" so the run shouldn't be updated in theory
3 participants
A top-level Errors page that aggregates errors from failed runs with occurrences metrics.
feature-errors-mvp-compressed-louder.mp4
Errors are “fingerprinted” so similar errors are grouped together (e.g. has an ID in the error message).
You can view an individual error to view a timeline of when it fired, the runs, and bulk replay them.