Feat: Improved run start timeline visibility #1732
Conversation
|
WalkthroughThis update introduces extensive improvements across multiple modules. The changes include enhancements in socket event handling by updating payloads to include timing metrics, modifications to environment variables in Docker and Kubernetes providers, and new React components along with refactored timeline views for run and span events. Legacy inspector components have been removed while hooks and presenters now incorporate impersonation and timeline metrics. New SQL migrations and Prisma schema updates add execution timestamps. Moreover, the CLI and core packages have been updated to integrate a comprehensive run timeline metrics API, supporting detailed performance tracking throughout task execution. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (7)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
apps/webapp/app/components/primitives/DateTime.tsx (2)
74-109: NewSmartDateTimecomponent enhances date-time context.This component conditionally shows only time if the date hasn’t changed. The approach is useful, but be sure to avoid confusion in logs or analytics if the date label is omitted. Also note the static analysis hint near line 108 suggests removing unnecessary
<Fragment>if this is a single child.- return <Fragment>{formattedDateTime.replace(/\s/g, String.fromCharCode(32))}</Fragment>; + return formattedDateTime.replace(/\s/g, String.fromCharCode(32));🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
203-215:DateTimeShortextends flexibility for simpler formatting.This offers a shorter alternative to full date-time. The static analysis suggests removing the
<Fragment>if it’s redundant around line 214.- return <Fragment>{formattedDateTime.replace(/\s/g, String.fromCharCode(32))}</Fragment>; + return formattedDateTime.replace(/\s/g, String.fromCharCode(32));🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts (1)
5-47:StandardRunTimelineMetricsManagereffectively collects metrics.Storing metrics in
_metricsis simple and direct. Use caution if the number of metrics can grow large; consider a flush-to-storage mechanism when it might exceed memory constraints. Also be mindful of potential clock skew issues aroundDate.now()for time calculations.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (1)
924-925: Use optional chaining for safer property access
To avoid potential runtime errors when checkingtimelineEventsand its first element, consider optional chaining:- node.data.timelineEvents && - node.data.timelineEvents[0] && + node.data.timelineEvents?.[0] &&🧰 Tools
🪛 Biome (1.9.4)
[error] 924-925: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/v3/runTimelineMetrics/index.ts (1)
19-23: Avoid usingthisin static context
Static analysis points out that referencingthisin static methods can be confusing. Consider using the class name directly, e.g.,RunTimelineMetricsAPI._instance, for clarity:- if (!this._instance) { - this._instance = new RunTimelineMetricsAPI(); - } - return this._instance; + if (!RunTimelineMetricsAPI._instance) { + RunTimelineMetricsAPI._instance = new RunTimelineMetricsAPI(); + } + return RunTimelineMetricsAPI._instance;🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
apps/webapp/app/components/run/RunTimeline.tsx (3)
60-89: Consider extracting “buildTimelineItems” logic into smaller, dedicated helper functions.The
RunTimelinecomponent callsbuildTimelineItems(run)which implements a fairly long and detailed conditional logic in one place. This can be refactored into smaller functions representing each stage (e.g., triggered, waiting, executing, etc.) to improve readability and maintainability.
255-266: Check potential redundancy between “Expired” and final states.Lines 257-266 handle a finished event if
run.isFinishedand no expiration, while lines 269-279 handle expiration. If these conditions were to overlap (e.g., a run is both expired and finished), ensure logic remains coherent and does not display conflicting timeline items.
356-446: Consider adding unit tests for “SpanTimeline” rendering logic.The
SpanTimelinecomponent processes multiple events, sorting them chronologically, then inserts additional “Started” and “Finished” events. It’s an ideal candidate for a focused unit test to confirm correct interpretation of edge cases (e.g., no events, partial events, out-of-order timestamps, etc.).Do you want help writing a suite of tests for this component?
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (2)
530-554: Consider security implications for passing “dequeuedAt” into message handlers.Line 530 onward introduces an additional parameter
dequeuedAtinto each handler. While this helps with auditing run times, confirm whether any untrusted data can overwrite or forge these timestamps. If so, consider sanitizing or validating the passed date.
1341-1386: Consider a more direct approach to “RESUME_AFTER_DURATION” & “FAIL” checkpoints.Lines 1341-1386 restore a checkpoint or mark the run as failed. If accumulative logic grows (e.g., partial successes, multiple resumed states), consider a more granular checkpoint system or state machine approach. This can decouple complex concurrency states for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/coordinator/src/index.ts(1 hunks)apps/docker-provider/src/index.ts(1 hunks)apps/kubernetes-provider/src/index.ts(2 hunks)apps/webapp/app/components/primitives/DateTime.tsx(3 hunks)apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/components/runs/v3/InspectorTimeline.tsx(0 hunks)apps/webapp/app/components/runs/v3/RunInspector.tsx(0 hunks)apps/webapp/app/components/runs/v3/SpanEvents.tsx(1 hunks)apps/webapp/app/components/runs/v3/SpanInspector.tsx(0 hunks)apps/webapp/app/hooks/useUser.ts(2 hunks)apps/webapp/app/presenters/v3/RunPresenter.server.ts(2 hunks)apps/webapp/app/presenters/v3/SpanPresenter.server.ts(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx(8 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx(0 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx(5 hunks)apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx(1 hunks)apps/webapp/app/routes/storybook/route.tsx(1 hunks)apps/webapp/app/v3/marqs/devQueueConsumer.server.ts(2 hunks)apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts(9 hunks)apps/webapp/app/v3/services/createTaskRunAttempt.server.ts(1 hunks)internal-packages/database/prisma/migrations/20250225164413_add_executed_at_to_task_run/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(2 hunks)packages/cli-v3/src/dev/workerRuntime.ts(3 hunks)packages/cli-v3/src/entryPoints/deploy-run-controller.ts(5 hunks)packages/cli-v3/src/entryPoints/deploy-run-worker.ts(5 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(6 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(3 hunks)packages/core/src/v3/apps/provider.ts(2 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/run-timeline-metrics-api.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/index.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/types.ts(1 hunks)packages/core/src/v3/schemas/messages.ts(4 hunks)packages/core/src/v3/schemas/schemas.ts(3 hunks)packages/core/src/v3/semanticInternalAttributes.ts(1 hunks)packages/core/src/v3/taskContext/index.ts(1 hunks)packages/core/src/v3/tracer.ts(4 hunks)packages/core/src/v3/utils/globals.ts(2 hunks)packages/core/src/v3/workers/index.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(10 hunks)references/test-tasks/src/trigger/helpers.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/webapp/app/components/runs/v3/InspectorTimeline.tsx
- apps/webapp/app/components/runs/v3/RunInspector.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx
- apps/webapp/app/components/runs/v3/SpanInspector.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/v3/run-timeline-metrics-api.ts
- packages/core/src/v3/taskContext/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/components/primitives/DateTime.tsx
[error] 108-108: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 214-214: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/core/src/v3/runTimelineMetrics/index.ts
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx
[error] 924-925: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (102)
packages/core/src/v3/workers/index.ts (1)
19-19: Export for StandardRunTimelineMetricsManager looks good.This export follows the established pattern in this file and enables the run timeline metrics functionality to be consumed by other modules.
internal-packages/database/prisma/migrations/20250225164413_add_executed_at_to_task_run/migration.sql (1)
1-5: Migration to add executedAt timestamp field is clean and appropriate.The migration adds a nullable TIMESTAMP column which allows tracking when a task run was executed. This is essential for the timeline visibility improvements.
packages/core/src/v3/index.ts (1)
17-17: Export of run-timeline-metrics-api is correctly placed.This export follows the existing pattern in the file and makes the timeline metrics API available to consumers of the core package.
packages/core/src/v3/utils/globals.ts (2)
6-6: Import of RunTimelineMetricsManager is appropriate.This import is necessary for the type declaration added below.
65-65: Global registration for run-timeline-metrics follows existing patterns.Adding the run-timeline-metrics manager to the global API follows the established pattern for other managers and enables global access to timeline metrics functionality.
apps/webapp/app/routes/storybook/route.tsx (1)
119-122: Good addition to storybook entries.The new "Run & Span timeline" story entry follows the established pattern and naming convention.
packages/core/src/v3/semanticInternalAttributes.ts (1)
55-55: Appropriate addition to semantic attributes.The new
METRIC_EVENTSattribute follows the established naming pattern and will allow for tracking metric events consistently throughout the system.packages/core/src/v3/runTimelineMetrics/types.ts (1)
1-13: Well-structured type definitions for the new timeline metrics feature.The type definitions are clear, concise, and provide a solid foundation for implementing run timeline metrics. The interface includes essential methods for registering and retrieving metrics.
apps/docker-provider/src/index.ts (2)
125-125: Better naming for scheduling timestamp environment variable.Renaming to
TRIGGER_POD_SCHEDULED_AT_MSmakes the purpose of this environment variable clearer and more consistent with other naming patterns.
133-135: Good addition of dequeued timestamp tracking.Adding the
TRIGGER_RUN_DEQUEUED_AT_MSenvironment variable when available improves the timeline metrics by capturing when a run was dequeued from the queue.references/test-tasks/src/trigger/helpers.ts (1)
1-1: Added debug logging to improve run visibility.The addition of the logger import and the debug logging statement in the
genericChildTaskfunction enhances observability by providing visibility into when the task starts executing.Also applies to: 191-192
packages/core/src/v3/apps/provider.ts (2)
53-53: Added timestamp tracking for task dequeuing.The new optional
dequeuedAtproperty enhances the interface to track when a task was dequeued from the queue, which is important for measuring queue waiting times.
155-155: Properly propagating dequeue timestamp to task creation.The change correctly passes the dequeued timestamp from the message data to the task creation function, ensuring the timing data flows through the system.
apps/coordinator/src/index.ts (2)
666-680: Enhanced lazyPayload to include timing metrics.This change creates a structured way to track execution metrics by adding a metrics array that captures the start time and duration of lazy payload processing. This is valuable for monitoring performance and identifying bottlenecks in the execution flow.
684-684: Updated emit call to use the enhanced payload.This line properly uses the newly constructed lazyPayload object with metrics in the socket emit call.
apps/webapp/app/v3/marqs/devQueueConsumer.server.ts (2)
319-319: Added timestamp tracking for message dequeuing.Capturing the exact moment when a message is dequeued provides an important reference point for performance analysis.
477-485: Added metrics array with dequeue timing information.This enhancement adds structured timing metrics to the payload, including the dequeue timestamp and duration calculation. This data will be valuable for understanding queue processing performance and identifying potential bottlenecks.
apps/webapp/app/hooks/useUser.ts (2)
6-6: Good addition of impersonation functionalityThe import of
useIsImpersonatingfrom./useOrganizationsis a well-structured addition that prepares for the enhanced permission handling.
31-35: Enhanced admin access control with impersonationThis change correctly extends the admin access logic to include users who are impersonating others. This is a logical enhancement that ensures support staff or admins using impersonation have the necessary permissions while maintaining the original admin check.
apps/webapp/app/presenters/v3/RunPresenter.server.ts (2)
3-4: Good import organizationProperly importing the required functions and modules for the timeline feature implementation.
142-145: Enhanced run events with timeline dataThis addition enriches each event with timeline information by transforming the existing events data using the root start time as a reference point. This provides valuable timing context for visualizing event sequences in the UI.
packages/cli-v3/src/dev/workerRuntime.ts (3)
9-9: Good addition of metrics typeAdding the
TaskRunExecutionMetricstype import prepares for the metrics tracking implementation.
317-318: Performance metric capture - starting timestampGood practice to capture the exact timestamp when the attempt creation starts. This enables accurate performance tracking.
331-343: Comprehensive metrics implementationThis change correctly implements run timeline metrics by:
- Capturing the duration of attempt creation
- Including the metric in a structured format with event type, name, and timestamp
- Preserving any existing metrics from the payload
This enables detailed performance tracking throughout task execution.
apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (2)
155-163: Improved task run status update with execution timestampThis change streamlines the status update logic while adding crucial execution timing information. The code now:
- Sets the status to "EXECUTING" conditionally in a more concise way
- Records the execution timestamp, preserving any existing value or setting a new one
This enhances the run timeline visibility by tracking when execution actually begins.
165-167: Better code organization for TTL handlingMoving the TTL acknowledgment check outside the update block improves code readability while maintaining the same functionality.
apps/kubernetes-provider/src/index.ts (3)
42-49: Well-structured environment variable parsing.The addition of these constants correctly handles parsing string environment variables to integers, with appropriate fallbacks when variables are not defined.
205-207: Good implementation of conditional environment variable assignment.This change properly integrates dequeued timestamp information when available, improving the task execution metrics collection. The spread operator usage with conditional array is a clean pattern.
524-524: Improved environment variable naming for better clarity.Renaming from
SCHEDULED_AT_MStoTRIGGER_POD_SCHEDULED_AT_MSprovides better context and consistency with the naming convention used forTRIGGER_RUN_DEQUEUED_AT_MS.apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx (1)
1-154: Well-structured storybook component for timeline visualization.This new file provides comprehensive test cases for both
SpanTimelineandRunTimelinecomponents, covering various states including in-progress, error, and completed states with different event configurations.The mock data sets are well-designed to showcase the components' capabilities, including edge cases like admin-only events.
apps/webapp/app/components/runs/v3/SpanEvents.tsx (2)
21-25: Improved event filtering for more relevant UI display.Filtering out events that start with "trigger.dev/" makes the UI cleaner by removing internal system events that aren't directly relevant to users. The early return when no displayable events exist improves rendering efficiency.
29-29: Consistent application of filtering logic.Updated the rendering loop to use filtered events, ensuring internal events are consistently hidden from the UI.
packages/cli-v3/src/entryPoints/deploy-run-controller.ts (4)
42-49: Consistent environment variable parsing.Matches the approach used in the Kubernetes provider, ensuring consistent handling of timing metrics across the application.
437-439: Enhanced logging with timing information.Including
startTimein the logs improves debugging capabilities and helps track performance metrics.
844-845: Added performance measurement for attempt creation.Captures the start time of the attempt creation process, which is useful for performance monitoring.
891-923: Comprehensive metrics collection for run timeline.The enhanced payload structure now includes detailed timing metrics covering:
- Attempt creation duration
- Pod scheduling time
- Time between dequeue and pod scheduling
This provides a complete picture of the run's startup phases, significantly improving observability.
packages/cli-v3/src/executions/taskRunProcess.ts (3)
127-127: LGTM: Adding timestamp capture for process fork start timeThis environment variable captures the exact time when the task process is forked, enabling precise performance tracking of the process initialization phase.
218-218: LGTM: Adding metrics to payload destructuringAdding metrics to the destructured payload allows the task run process to receive and handle metrics data from the execution context.
236-236: LGTM: Passing metrics to worker processIncluding the metrics in the EXECUTE_TASK_RUN message enables proper metrics propagation to the worker process.
packages/cli-v3/src/entryPoints/dev-run-worker.ts (4)
20-20: LGTM: Adding necessary imports for timeline metricsThese imports are required for implementing the run timeline metrics functionality.
Also applies to: 40-40
82-83: LGTM: Initializing metrics manager and seeding from environmentThe setup follows the standard pattern used for other global managers in this codebase, and seeding from environment ensures metrics data is populated from the beginning of the execution lifecycle.
Also applies to: 108-108
189-191: LGTM: Registering metrics from executionThis change allows the worker to receive and register metrics from the execution context, properly integrating the metrics collection with the task execution flow.
244-253: LGTM: Measuring import performance with metricsWrapping the task import process in a metric measurement call provides valuable performance data about the import phase, which can be useful for identifying slow-loading tasks.
packages/cli-v3/src/entryPoints/deploy-run-worker.ts (4)
20-20: LGTM: Adding necessary imports for timeline metricsThese imports mirror the changes in dev-run-worker.ts, maintaining consistency across environments.
Also applies to: 41-41
122-124: LGTM: Setting up metrics manager in deployment environmentProperly initializing the metrics manager and seeding from environment, ensuring consistent metrics collection in deployment scenarios.
209-211: LGTM: Registering metrics from execution in deployment workerThis ensures metrics are properly captured in the deployment environment, maintaining feature parity with the development environment.
268-283: LGTM: Comprehensive import performance trackingThis implementation not only measures the import duration but also logs it, providing visibility into task loading performance. The approach nicely combines the new metrics system with existing logging.
packages/core/src/v3/schemas/messages.ts (4)
16-16: LGTM: Adding metrics schema importIncluding the TaskRunExecutionMetrics schema is necessary for type checking and validation of metrics data.
56-56: LGTM: Adding dequeuedAt timestamp fieldThis optional field enhances the scheduling information by tracking when a task was dequeued, which is valuable for queue wait time analysis.
207-207: LGTM: Adding metrics to execution message schemaMaking the metrics field optional ensures backward compatibility while allowing the new metrics functionality to be used when available.
678-678: LGTM: Adding startTime to lazy attempt messagesThis optional field allows tracking when lazy attempts are initiated, providing better visibility into the execution timeline for lazily loaded tasks.
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (6)
43-43: Good use of a separated private method call.Calling
#getRun(spanId)helps streamline this section, and the subsequentif (run)block is clear. Ensure you have test coverage confirming that no errors bubble up unexpectedly if the method returnsundefined.
52-52: Verify the fallback logic when#getRunreturns undefined.After fetching using
#getRun, you fetch a span with#getSpan. Confirm that there's no scenario where both methods return valid data simultaneously, which might produce conflicting results. Otherwise, this fallback is solid.
64-64: Private method encapsulation looks good.Making
#getRunprivate improves encapsulation. Consider ensuring it’s thoroughly tested, as it performs a complex data selection. Also watch out for performance overhead due to the large SELECT operation.
91-91: Confirm the newexecutedAtfield is always available.The query now selects
executedAt. If any older runs are missing this column or it’s sometimes null, ensure no downstream assumptions break. If needed, handle null gracefully in the returned object.
253-253: ExposingexecutedAtin the returned object.This line consistently adds the new field, preserving the rest of the schema. Verify that clients consuming this object can handle runs that may not have
executedAtpopulated (for instance, older data).
329-329: Good conversion to a private method.
#getSpanbeing private aligns with the same pattern used by#getRun. This maintains consistent design. Keep an eye on any external usage that may have relied on the public method signature.apps/webapp/app/components/primitives/DateTime.tsx (7)
10-10: OptionalpreviousDateis helpful for comparative displays.Great addition for controlling how date changes are shown. Ensure type checks handle string vs. null correctly, and confirm all usage is tested.
111-119:isSameDayhelper is straightforward and readable.This utility function is well-defined and the naming is clear.
120-132:formatSmartDateTimeprovides fine-grained formatting.The usage of fractional seconds is beneficial for high-precision timestamps. Make sure to confirm that older browsers or environments gracefully degrade if
fractionalSecondDigitsis not supported.
134-144:formatTimeOnlyoffers concise time formatting.This effectively isolates time-only presentation. Same note regarding
fractionalSecondDigitsbrowser support applies here.
146-185:DateTimeAccurateusesSmartDateTimelogic well.Switching to time-only if
previousDateis provided and it’s the same day is a convenient user experience improvement. Verify that all states (nopreviousDate, different day, same day) are tested to avoid edge cases.🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
188-201:formatDateTimeAccurateensures precise timestamp display.The approach is consistent with the rest of the file. Watch for potential locale differences in second decimal rendering, which sometimes vary in certain languages or regions.
217-229:formatDateTimeShortencapsulates minimal date-time output.A helpful function for quick date-time representations. Keep the code consistent with the other format functions for maintainability.
packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts (2)
1-4: Imports and type definitions are well-structured.All required imports come from local modules, which keeps the file self-contained and consistent.
49-57:NoopRunTimelineMetricsManageris a practical fallback.This pattern is helpful for scenarios that don’t require metrics. No concerns found.
packages/core/src/v3/workers/taskExecutor.ts (5)
8-8: Enhancing task execution observability with metrics tracking.The addition of the
runTimelineMetricsimport introduces a structured way to measure and track execution metrics throughout the task lifecycle.
94-98: Good implementation of execution metrics for payload parsing.Wrapping the payload parsing operation with
runTimelineMetrics.measureMetricenables precise tracking of how long this operation takes, which can help identify potential bottlenecks.
231-231: Enhancing span observability with execution metrics.Setting span attributes based on metrics in the cleanup phase ensures that all collected metrics are properly recorded before the span ends.
240-247: Conditional metrics recording based on attempt number.This approach of only attaching metrics to span attributes and events on the first attempt is efficient, as it prevents duplicate metrics in retry scenarios while still maintaining complete observability.
264-277: Performance tracking for run function execution.The implementation correctly handles both the direct run function case and the middleware path, ensuring metrics are captured regardless of the execution path.
internal-packages/database/prisma/schema.prisma (2)
1706-1709: Adding crucial timestamps for run lifecycle tracking.The addition of
startedAtandexecutedAtfields provides important visibility into the timing of task runs, particularly for performance analysis and debugging. The clear comments explain the purpose of each field.
1921-1922: Minor formatting improvement in TaskRunDependency model.The spacing change maintains consistency with other datetime field declarations in the schema.
packages/core/src/v3/schemas/schemas.ts (4)
11-22: Well-structured metrics schema definition.The
TaskRunExecutionMetricschema provides a clear, typed structure for capturing timing metrics with all necessary fields: name, event, timestamp, and duration. The array wrapperTaskRunExecutionMetricsallows for collecting multiple metrics in sequence.
28-28: Adding metrics to TaskRunExecutionPayload.Making the metrics field optional ensures backward compatibility while enabling enhanced telemetry.
52-52: Adding metrics to ProdTaskRunExecutionPayload.Consistent application of the metrics field across related schemas maintains a uniform data model.
265-265: Adding metrics to TaskRunExecutionLazyAttemptPayload.Including metrics in the lazy attempt payload completes the metrics integration across all relevant execution payloads.
packages/core/src/v3/tracer.ts (4)
2-7: Adding necessary imports for enhanced tracing.The additional imports support the expanded tracing functionality with proper types for time tracking and attribute management.
31-39: Well-designed types for span event tracking.The
TriggerTracerSpanEventandTriggerTracerSpanOptionstypes provide a clear structure for attaching multiple events to spans with appropriate metadata.
73-73: Updated parameter type for enhanced span options.Changing the
optionsparameter type toTriggerTracerSpanOptionsenables the method to accept event data without breaking existing functionality.
101-127: Comprehensive implementation of span event tracking.The implementation handles both partial spans and the main span, ensuring events are properly recorded in all contexts. The conditional checks prevent unnecessary processing when no events are provided.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (4)
34-34: Import statement looks good
This addition ofDateTimeShortis consistent with the rest of the imports and doesn't introduce any issues.
68-68: Admin access import
Bringing inuseHasAdminAccessallows for clearer permissions management in the UI. No concerns here.
744-744: isAdmin check
Usingconst isAdmin = useHasAdminAccess();in the timeline context appears sound, enabling selective rendering for admin-only features.
779-779: CurrentTimeIndicator usage
PassingrootStartedAtalongsidetotalDurationis a neat way to display live-running metrics. This addition looks well-implemented.packages/core/src/v3/runTimelineMetrics/index.ts (7)
1-12: Initial imports and constants
Imports from OpenTelemetry and internal modules look properly organized. DefiningAPI_NAMEand initializingNOOP_MANAGERis clear for fallback usage.
13-18: Class definition and singleton pattern
DefiningRunTimelineMetricsAPIas implementingRunTimelineMetricsManagersets a clear contract for metric operations. The private constructor ensures singleton usage.
26-33: Metric registration and retrieval
TheregisterMetric()andgetMetrics()methods delegate to the global manager or a no-op manager, making the API flexible and robust.
34-103: Async timing inmeasureMetric
Capturing duration, handling exceptions, and still registering the metric on error is a solid approach. The JSDoc annotation is helpful for clarity.
105-120: Converting metrics to span events
convertMetricsToSpanEvents()neatly translates collected metrics into tracer events. It ensures important attributes are retained, promoting good observability.
122-194: Aggregating metrics into span attributes
Group-by logic and flattening attributes inconvertMetricsToSpanAttributes()effectively organizes metrics. The optional usage of durations and timestamps appears well-thought-out.
196-203: Global manager delegation
setGlobalManager()and#getManager()effectively leverage shared global references, falling back to the noop manager to avoid breakages.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (5)
14-14: useEffect import
The addition ofuseEffectindicates a possible new data-fetch flow. No issues found.
39-44: New timeline imports
Importing functions and components such ascreateTimelineSpanEventsFromSpanEventsandSpanTimelinecentralizes timeline features in one place.
54-54: useHasAdminAccess import
This import aligns with the file’s usage of admin-based filtering for timeline events.
173-173: isAdmin usage
GrabbinguseHasAdminAccess()ensures you can conditionally display admin-only event data.
313-313: Admin-only events filtering
PassingshowAdminOnlyEvents={isAdmin}to<SpanTimeline>provides controlled conditional rendering, preventing unauthorized event visibility. Nicely done.apps/webapp/app/components/run/RunTimeline.tsx (2)
180-196: Verify date handling in “waiting-to-execute” and “Started” event sequences.When
run.executedAtis notnull, the code calculates waiting time before execution (line 182) and then immediately marks the run as “Started” (line 190). Ensure that theexecutedAttimestamp is always available and consistent with upstream logic, so that the computed durations accurately reflect reality.
210-221: Confirm spinner state for ongoing “executing” phase.For runs that have an
executedAttimestamp but are not yet finished, the code displays a spinner and a live timer. Verify that this state correctly transitions to “complete” or “error” when the run’s final status is determined, preventing any lingering spinners.apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (3)
442-442: Ensure consistent usage of timezone or UTC.Storing
const dequeuedAt = new Date()captures the current local time. Confirm whether the rest of the system expects times to be in UTC or local time. A mismatch across environments may lead to inconsistent date calculations.
718-718: Double-check the “startedAt” fallback logic.Using
existingTaskRun.startedAt ?? dequeuedAtensures “startedAt” is set only if not previously set. If a partial setup previously stored a time for “startedAt,” it won’t be overridden. Confirm this is desired behavior, especially in scenarios where “startedAt” might need updating if the run was re-queued.
892-896: Validate machine preset computation.Here, the machine preset is derived from either
machinePresetFromRun(lockedTaskRun)or fallback tomachinePresetFromConfig(...). If the run or config is ever missing or outdated, consider handling null/undefined returns.
…ld start metrics as span events on attempt spans and display them in the run dashboard
ericallam
force-pushed
the
feat/run-timeline-metrics
branch
from
dbb6c6a to
7f7cdc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
packages/cli-v3/src/entryPoints/deploy-run-controller.ts (1)
891-923: Comprehensive metrics collection with conditional logic.This implementation effectively aggregates metrics from multiple sources and adds timing data about the run lifecycle. The conditional logic for pod scheduling and dequeuing metrics is particularly valuable for production debugging.
However, the nested conditional array structure is a bit complex. Consider extracting this logic to a separate helper function to improve readability.
- const payload = { - ...createAttempt.result.executionPayload, - metrics: [ - ...(createAttempt.result.executionPayload.metrics ?? []), - ...(message.lazyPayload.metrics ?? []), - { - name: "start", - event: "create_attempt", - timestamp: createAttemptStart, - duration: Date.now() - createAttemptStart, - }, - ...(TRIGGER_POD_SCHEDULED_AT_MS && TRIGGER_RUN_DEQUEUED_AT_MS - ? [ - ...(TRIGGER_POD_SCHEDULED_AT_MS !== TRIGGER_RUN_DEQUEUED_AT_MS - ? [ - { - name: "start", - event: "pod_scheduled", - timestamp: TRIGGER_POD_SCHEDULED_AT_MS, - duration: Date.now() - TRIGGER_POD_SCHEDULED_AT_MS, - }, - ] - : []), - { - name: "start", - event: "dequeue", - timestamp: TRIGGER_RUN_DEQUEUED_AT_MS, - duration: TRIGGER_POD_SCHEDULED_AT_MS - TRIGGER_RUN_DEQUEUED_AT_MS, - }, - ] - : []), - ], - }; + const payload = { + ...createAttempt.result.executionPayload, + metrics: buildMetricsArray({ + existingMetrics: createAttempt.result.executionPayload.metrics, + lazyPayloadMetrics: message.lazyPayload.metrics, + createAttemptStart, + podScheduledAtMs: TRIGGER_POD_SCHEDULED_AT_MS, + runDequeuedAtMs: TRIGGER_RUN_DEQUEUED_AT_MS + }), + }; // Then define this helper function elsewhere in the file: // function buildMetricsArray({ existingMetrics, lazyPayloadMetrics, createAttemptStart, podScheduledAtMs, runDequeuedAtMs }) { // const metrics = [ // ...(existingMetrics ?? []), // ...(lazyPayloadMetrics ?? []), // { // name: "start", // event: "create_attempt", // timestamp: createAttemptStart, // duration: Date.now() - createAttemptStart, // } // ]; // // if (podScheduledAtMs && runDequeuedAtMs) { // if (podScheduledAtMs !== runDequeuedAtMs) { // metrics.push({ // name: "start", // event: "pod_scheduled", // timestamp: podScheduledAtMs, // duration: Date.now() - podScheduledAtMs, // }); // } // // metrics.push({ // name: "start", // event: "dequeue", // timestamp: runDequeuedAtMs, // duration: podScheduledAtMs - runDequeuedAtMs, // }); // } // // return metrics; // }packages/core/src/v3/runTimelineMetrics/index.ts (2)
13-24: Refactor static methods to avoidthisin static context.The singleton pattern implementation works but using
thisin static methods can be confusing.public static getInstance(): RunTimelineMetricsAPI { - if (!this._instance) { - this._instance = new RunTimelineMetricsAPI(); + if (!RunTimelineMetricsAPI._instance) { + RunTimelineMetricsAPI._instance = new RunTimelineMetricsAPI(); } - return this._instance; + return RunTimelineMetricsAPI._instance; }🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
161-162: Remove duplicate comment.There's a duplicated comment line "Calculate duration for each metric type".
// Calculate duration for each metric type - // Calculate duration for each metric type for (const [metricName, metricEvents] of Object.entries(metricsByName)) {apps/webapp/app/components/primitives/DateTime.tsx (3)
108-108: Remove unnecessary Fragment.
A single top-level element (string in this case) doesn't need a Fragment wrapper. You can return the element directly:- return <Fragment>{formattedDateTime.replace(/\s/g, String.fromCharCode(32))}</Fragment>; + return formattedDateTime.replace(/\s/g, String.fromCharCode(32));🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
120-132: Consider compatibility fallback for fractionalSecondDigits.
Your usage offractionalSecondDigitsis helpful for precision, but it’s not universally supported in older browsers. Consider providing a fallback or polyfill to avoid possible formatting issues in unsupported environments.
214-214: Remove unnecessary Fragment here too.
Similar to line 108, you can remove this Fragment for a single-child return:- return <Fragment>{formattedDateTime.replace(/\s/g, String.fromCharCode(32))}</Fragment>; + return formattedDateTime.replace(/\s/g, String.fromCharCode(32));🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (2)
893-965: Filtering admin-only events is correct, but confirm performance.
The filter step?.filter((event) => !event.adminOnly || isAdmin)is succinct. However, if the event array grows large, consider lazy loading or incremental rendering. Also, test for edge cases whereevent.adminOnlymight be undefined to avoid runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 924-925: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
924-925: Leverage optional chaining for readability.
You can avoid multiple chained conditions by using optional chaining:- if (node.data.timelineEvents && node.data.timelineEvents[0] && node.data.timelineEvents[0].offset < node.data.offset) { + if (node.data.timelineEvents?.[0]?.offset < node.data.offset) {🧰 Tools
🪛 Biome (1.9.4)
[error] 924-925: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/webapp/app/components/run/RunTimeline.tsx (6)
80-80: Consider updating type usage to avoid type assertion.The type assertion
state={item.state as "complete" | "error"}suggests a mismatch betweenTimelineEventState(which includes "inprogress" and "delayed") and the expected prop types. This could lead to runtime issues if an unhandled state is passed.- state={item.state as "complete" | "error"} + state={item.state === "inprogress" || item.state === "delayed" ? "complete" : item.state}
92-282: Consider refactoring the lengthy buildTimelineItems function.While the function is well-commented and organized into logical sections, its length (almost 200 lines) makes it harder to maintain. Consider breaking it down into smaller functions for each timeline section/state.
// Example refactoring approach function buildTimelineItems(run: TimelineSpanRun): TimelineItem[] { const items: TimelineItem[] = []; // Add triggered event addTriggeredEvent(items, run); // Add waiting/dequeuing section addWaitingToDequeueSection(items, run); // Add dequeued event if applicable addDequeuedEvent(items, run); // Add execution section addExecutionSection(items, run); // Add finished or expired events addCompletionEvents(items, run); return items; } // Then implement each helper function separately function addTriggeredEvent(items: TimelineItem[], run: TimelineSpanRun) { items.push({ type: "event", id: "triggered", title: "Triggered", date: run.createdAt, previousDate: undefined, state: "complete", shouldRender: true, }); } // Additional helper functions...
174-253: Consider using a React hook to memoize timeline data.The conditional logic for handling runs with and without
executedAttimestamps adds complexity. Using React's useMemo hook could optimize performance by preventing unnecessary recalculations when the run hasn't changed.// In the RunTimeline component: -const timelineItems = buildTimelineItems(run); +const timelineItems = React.useMemo(() => buildTimelineItems(run), [ + run.createdAt, + run.startedAt, + run.executedAt, + run.updatedAt, + run.expiredAt, + run.completedAt, + run.isFinished, + run.isError +]);
317-345: Extract inline styles to improve maintainability.The inline styles for the
"inprogress"state are quite complex. Consider extracting them to a CSS class or styled component for better maintainability.// Add this to your CSS module or styled component +const progressLineStyles = { + width: "1px", + height: "100%", + background: "repeating-linear-gradient(to bottom, #3B82F6 0%, #3B82F6 50%, transparent 50%, transparent 100%)", + backgroundSize: "1px 6px", + maskImage: "linear-gradient(to bottom, black 50%, transparent 100%)", + WebkitMaskImage: "linear-gradient(to bottom, black 50%, transparent 100%)", +}; // Then in your component - style={ - state === "inprogress" - ? { - width: "1px", - height: "100%", - background: - "repeating-linear-gradient(to bottom, #3B82F6 0%, #3B82F6 50%, transparent 50%, transparent 100%)", - backgroundSize: "1px 6px", - maskImage: "linear-gradient(to bottom, black 50%, transparent 100%)", - WebkitMaskImage: "linear-gradient(to bottom, black 50%, transparent 100%)", - } - : undefined - } + style={state === "inprogress" ? progressLineStyles : undefined}
425-436: Extract repeated date calculation to a variable.The date calculation
new Date(startTime.getTime() + nanosecondsToMilliseconds(duration))is repeated multiple times. Extract it to a variable to improve readability and prevent potential inconsistencies.+ const finishTime = new Date(startTime.getTime() + nanosecondsToMilliseconds(duration)); <RunTimelineLine - title={formatDuration( - startTime, - new Date(startTime.getTime() + nanosecondsToMilliseconds(duration)) - )} + title={formatDuration(startTime, finishTime)} state={"complete"} /> <RunTimelineEvent title="Finished" subtitle={ <DateTimeAccurate - date={new Date(startTime.getTime() + nanosecondsToMilliseconds(duration))} + date={finishTime} previousDate={startTime} /> }
525-595: Consider consolidating event metadata into a single source of truth.The three utility functions (
getFriendlyNameForEvent,getAdminOnlyForEvent,getHelpTextForEvent) share similar switch cases. Consider consolidating this metadata into a single object structure for better maintainability.+const EVENT_METADATA = { + dequeue: { + friendlyName: "Dequeued", + adminOnly: false, + helpText: "The task was dequeued from the queue" + }, + fork: { + friendlyName: "Launched", + adminOnly: false, + helpText: "The process was created to run the task" + }, + create_attempt: { + friendlyName: "Attempt created", + adminOnly: true, + helpText: "An attempt was created for the task" + }, + // ... other events +}; function getFriendlyNameForEvent(event: string): string { + return EVENT_METADATA[event]?.friendlyName || event; - switch (event) { - case "dequeue": { - return "Dequeued"; - } - // ... other cases - } } function getAdminOnlyForEvent(event: string): boolean { + return EVENT_METADATA[event]?.adminOnly ?? true; - switch (event) { - // ... cases - } } function getHelpTextForEvent(event: string): string | undefined { + return EVENT_METADATA[event]?.helpText; - switch (event) { - // ... cases - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/coordinator/src/index.ts(1 hunks)apps/docker-provider/src/index.ts(1 hunks)apps/kubernetes-provider/src/index.ts(2 hunks)apps/webapp/app/components/primitives/DateTime.tsx(3 hunks)apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/components/runs/v3/InspectorTimeline.tsx(0 hunks)apps/webapp/app/components/runs/v3/RunInspector.tsx(0 hunks)apps/webapp/app/components/runs/v3/SpanEvents.tsx(1 hunks)apps/webapp/app/components/runs/v3/SpanInspector.tsx(0 hunks)apps/webapp/app/hooks/useUser.ts(2 hunks)apps/webapp/app/presenters/v3/RunPresenter.server.ts(2 hunks)apps/webapp/app/presenters/v3/SpanPresenter.server.ts(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx(8 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx(0 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx(5 hunks)apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx(1 hunks)apps/webapp/app/routes/storybook/route.tsx(1 hunks)apps/webapp/app/v3/marqs/devQueueConsumer.server.ts(2 hunks)apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts(9 hunks)apps/webapp/app/v3/services/createTaskRunAttempt.server.ts(1 hunks)internal-packages/database/prisma/migrations/20250225164413_add_executed_at_to_task_run/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(2 hunks)packages/cli-v3/src/dev/workerRuntime.ts(3 hunks)packages/cli-v3/src/entryPoints/deploy-run-controller.ts(5 hunks)packages/cli-v3/src/entryPoints/deploy-run-worker.ts(5 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(6 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(3 hunks)packages/core/src/v3/apps/provider.ts(2 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/run-timeline-metrics-api.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/index.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts(1 hunks)packages/core/src/v3/runTimelineMetrics/types.ts(1 hunks)packages/core/src/v3/schemas/messages.ts(4 hunks)packages/core/src/v3/schemas/schemas.ts(3 hunks)packages/core/src/v3/semanticInternalAttributes.ts(1 hunks)packages/core/src/v3/taskContext/index.ts(1 hunks)packages/core/src/v3/tracer.ts(4 hunks)packages/core/src/v3/utils/globals.ts(2 hunks)packages/core/src/v3/workers/index.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(10 hunks)references/test-tasks/src/trigger/helpers.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx
- apps/webapp/app/components/runs/v3/RunInspector.tsx
- apps/webapp/app/components/runs/v3/InspectorTimeline.tsx
- apps/webapp/app/components/runs/v3/SpanInspector.tsx
🚧 Files skipped from review as they are similar to previous changes (28)
- packages/core/src/v3/semanticInternalAttributes.ts
- apps/coordinator/src/index.ts
- internal-packages/database/prisma/migrations/20250225164413_add_executed_at_to_task_run/migration.sql
- packages/core/src/v3/run-timeline-metrics-api.ts
- packages/core/src/v3/workers/index.ts
- packages/core/src/v3/utils/globals.ts
- packages/core/src/v3/taskContext/index.ts
- packages/core/src/v3/index.ts
- apps/webapp/app/routes/storybook/route.tsx
- apps/webapp/app/v3/services/createTaskRunAttempt.server.ts
- packages/core/src/v3/apps/provider.ts
- references/test-tasks/src/trigger/helpers.ts
- apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx
- apps/webapp/app/v3/marqs/devQueueConsumer.server.ts
- apps/webapp/app/hooks/useUser.ts
- apps/docker-provider/src/index.ts
- apps/webapp/app/components/runs/v3/SpanEvents.tsx
- packages/cli-v3/src/dev/workerRuntime.ts
- apps/webapp/app/presenters/v3/RunPresenter.server.ts
- packages/cli-v3/src/executions/taskRunProcess.ts
- packages/core/src/v3/schemas/messages.ts
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts
- apps/kubernetes-provider/src/index.ts
- packages/core/src/v3/workers/taskExecutor.ts
- packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts
- packages/core/src/v3/runTimelineMetrics/types.ts
- internal-packages/database/prisma/schema.prisma
- apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx
[error] 924-925: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/webapp/app/components/primitives/DateTime.tsx
[error] 108-108: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 214-214: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/core/src/v3/runTimelineMetrics/index.ts
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (45)
packages/cli-v3/src/entryPoints/deploy-run-controller.ts (3)
42-49: Well-structured environment variable parsing for timing metrics.The implementation properly handles environment variables by checking if they're strings before parsing, with appropriate fallback to undefined. This is a good defensive programming practice.
437-440: Enhanced logging with timing information.Adding
startTimeto the log improves observability by capturing the exact time when the system is ready for a lazy attempt, which will help with debugging and performance analysis.
844-845: Good timing instrumentation for attempt creation.Capturing a timestamp before the attempt creation allows precise measurement of this operation's duration.
packages/cli-v3/src/entryPoints/dev-run-worker.ts (6)
19-21: Adding runTimelineMetrics to imports.Important addition of runTimelineMetrics to the imports from core, which is necessary for the timeline metrics implementation.
39-41: Import StandardRunTimelineMetricsManager from workers.Good addition of the StandardRunTimelineMetricsManager import, maintaining consistency with the existing pattern of importing standardized implementations.
82-84: Initialize and configure global timeline metrics manager.Creating a StandardRunTimelineMetricsManager instance and setting it as the global manager follows the same pattern used for other managers in the codebase.
108-109: Seed metrics from environment.This call properly initializes metrics from environment variables, ensuring consistent initial state.
189-191: Update EXECUTE_TASK_RUN handler to register metrics.The handler signature is updated to receive metrics and registers them with the metrics manager. This ensures incoming metrics from the execution context are properly tracked.
244-254: Instrument task import with metrics tracking.The code now measures the duration of the task import operation using the metrics system, which provides valuable performance insights about the import phase.
packages/cli-v3/src/entryPoints/deploy-run-worker.ts (5)
19-21: Add runTimelineMetrics to imports.This addition ensures the deploy-run-worker has access to the timeline metrics functionality, maintaining consistency with other parts of the system.
40-42: Import StandardRunTimelineMetricsManager.Appropriate import of the StandardRunTimelineMetricsManager to match the architecture used in other parts of the system.
122-125: Setup of timeline metrics manager.The code correctly instantiates the StandardRunTimelineMetricsManager, sets it as the global manager, and seeds it with metrics from the environment, ensuring consistency with the rest of the application.
209-211: Update EXECUTE_TASK_RUN handler to receive and register metrics.The handler is updated to accept metrics as a parameter and register them with the metrics manager, ensuring continuity of metrics collection throughout the execution flow.
268-283: Enhanced task import with metrics tracking.The implementation wraps the import process in a metrics measurement, while retaining the original functionality including timestamp logging. This provides visibility into import performance without disrupting existing behavior.
packages/core/src/v3/schemas/schemas.ts (4)
11-23: Well-defined schemas for execution metrics.The TaskRunExecutionMetric schema properly defines the required properties for tracking metrics (name, event, timestamp, duration) and the TaskRunExecutionMetrics array schema enables consistent collection of multiple metrics.
28-29: Add metrics to TaskRunExecutionPayload schema.Appropriate addition of an optional metrics field to the TaskRunExecutionPayload schema, ensuring that execution payloads can include performance metrics.
52-53: Add metrics to ProdTaskRunExecutionPayload schema.Properly adds the optional metrics field to the ProdTaskRunExecutionPayload schema, maintaining consistency with the regular execution payload.
265-266: Add metrics to TaskRunExecutionLazyAttemptPayload schema.This addition ensures that lazy attempt payloads can also include metrics, completing the integration of metrics across all relevant payload types.
packages/core/src/v3/tracer.ts (4)
2-7: LGTM: Import additions support the new timeline features.The added imports of
HrTimeandTimeInputfrom@opentelemetry/apiappropriately support the new span event capabilities.
31-39: Well-structured new type definitions.The new types are well-designed with clear purposes:
TriggerTracerSpanEventdefines events with optional timestamps and attributesTriggerTracerSpanOptionsextends existingSpanOptionsto include eventsThis design follows good typing practices and maintains backward compatibility.
73-73: API signature change is properly specialized.Updating the parameter type from
SpanOptionstoTriggerTracerSpanOptionsallows for the new event tracking functionality while maintaining compatibility with existing code through type extension.
101-127: Solid implementation for span event handling.The implementation correctly handles the new
eventsproperty by:
- Creating a
partialSpanwith the necessary attributes- Adding events to the partial span when provided
- Adding the same events to the main span
This approach ensures comprehensive event tracking while maintaining the existing functionality.
packages/core/src/v3/runTimelineMetrics/index.ts (3)
34-104: Well-implemented metric measurement with robust error handling.The
measureMetricmethod is thoughtfully designed:
- Clear documentation with JSDoc comments
- Flexible function signature with proper overload handling
- Robust error handling that captures metrics even when errors occur
- Appropriate propagation of errors after capturing metrics
This implementation provides a solid foundation for performance tracking.
105-194: Well-structured metric conversion methods.The conversion methods effectively transform metrics into formats suitable for tracing:
convertMetricsToSpanEvents: Cleanly maps metrics to span eventsconvertMetricsToSpanAttributes: Properly groups metrics by name and calculates durationsThe implementation handles edge cases (empty metrics) and uses TypeScript's type system effectively.
196-202: Good pattern for global manager registration.The manager methods:
setGlobalManager: Properly registers the manager with the global registry#getManager: Correctly retrieves the manager with a fallback to NOOP_MANAGERThis approach provides flexibility while ensuring the API always behaves predictably.
apps/webapp/app/components/primitives/DateTime.tsx (7)
10-10: Optional property addition confirmed.
AddingpreviousDate?: Date | string | null;is a clean approach for optional date comparisons. Just be sure to consistently handle all three nullable forms (Date, string, and null) throughout your codebase to avoid unexpected parsing errors.
74-109: SmartDateTime component looks good, but watch out for SSR.
Your reliance onIntl.DateTimeFormat().resolvedOptions()is valid for client-side rendering, but be aware that you might encounter issues if this runs server-side, as some Node.js environments handle time zones differently. Consider fallback logic in SSR contexts, if needed.🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
111-119: Day comparison is straightforward.
TheisSameDayfunction reads well and should handle daily boundary checks correctly for simple date comparisons.
134-144: Time-only formatting is well-defined.
The function logic forformatTimeOnlymirrorsformatSmartDateTime; no issues detected.
146-185: DateTimeAccurate integrates well withpreviousDate.
The conditional handling ofpreviousDateis coherent and ensures consistent output format across different states.🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
203-215: DateTimeShort addition looks solid.
This concise view should be convenient in UIs that only need short date-time representations. Continue to ensure consistent locale/time zone usage across all date/time components.🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
217-229: Short date-time format is consistent.
This helper provides a concise representation, matching the approach in your other date-time helpers. Keep an eye on fractional second compatibility as well.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (4)
34-34: New import of DateTimeShort is coherent with the updated components.
The addition ofDateTimeShortaligns with the new short-format usage in the timeline. Ensure consistent usage to prevent confusion with other date-time components.
68-68: Admin access hook usage.
ImportinguseHasAdminAccessis a good step for conditionally rendering admin-only data. Confirm relevant tests exist to exercise admin gating logic and avoid regressions.
744-744: StoringisAdminis straightforward.
Initializingconst isAdmin = useHasAdminAccess();centralizes the access check. Make sure the rest of the timeline logic gracefully handles the non-admin path.
778-779: CurrentTimeIndicator usage.
Integrating the newrootStartedAtprop ensures accurate real-time timeline markers. Confirm there's no mismatch with the timeline scale to avoid visual off-sync.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (5)
14-14: Redundant import caution.
Check ifimport { useEffect } from "react";is necessary. If you’re using it solely in the same block as other specialized hooks, see if you can rely on them or confirm it’s used in the new logic for reloading the data. Removing unused imports can simplify code.
39-44: Consolidating timeline functionality.
ImportingcreateTimelineSpanEventsFromSpanEvents,RunTimeline, andSpanTimelinefrom~/components/run/RunTimelineis a clean approach to unify timeline logic. Ensuring each context uses consistent data shapes is key to maintainable timeline components.
54-54: Admin gating logic remains consistent.
useHasAdminAccessensures consistent gating of admin-only timeline events across multiple routes. This is good for uniform user experience.
173-174:isAdminusage for SpanBody.
Assigningconst isAdmin = useHasAdminAccess();is straightforward. Confirm that partial or failed data loads don’t cause timing issues with the gate logic in the UI—e.g., incomplete data vs. admin checks.
313-314: Conditional rendering of admin events.
SettingshowAdminOnlyEvents={isAdmin}effectively toggles restricted content. This ensures non-admin users cannot see these events, abiding by principle of least privilege.apps/webapp/app/components/run/RunTimeline.tsx (4)
60-89: Strong component structure with good separation of concerns.The RunTimeline component effectively separates data transformation (using buildTimelineItems) from rendering, making the code easier to understand and maintain.
458-523: The createTimelineSpanEventsFromSpanEvents function has good sorting behavior.The function correctly sorts events first by time and then by name when times are equal, ensuring a consistent and predictable order in the timeline.
1-13: Imports are well organized.The imports are cleanly organized with clear separation between external dependencies and internal components.
14-58: Well-defined TypeScript types with good documentation.The type definitions are clear, well-structured, and include helpful comments explaining their purpose, particularly for the
TimelineSpanRuntype.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/tailwind.config.js (1)
270-270: Fix extra space in gradient-radial-secondary key name.There's an extra space at the end of the key name which could cause issues with style application.
- "gradient-radial-secondary ": `radial-gradient(hsl(271 91 65), hsl(221 83 53))`, + "gradient-radial-secondary": `radial-gradient(hsl(271 91 65), hsl(221 83 53))`,apps/webapp/app/components/run/RunTimeline.tsx (3)
391-402: Consider extracting repeated animation styling to a utility function.There's significant duplication in the animation styling code across multiple places where the tile background is applied.
You could extract this to a utility function:
+function getInProgressStyle() { + return { + height: "100%", + backgroundImage: `url(${tileBgPath})`, + backgroundSize: "8px 8px", + }; +} // Then use it in place of the duplicated objects: -style={ - state === "inprogress" - ? { - height: "100%", - backgroundImage: `url(${tileBgPath})`, - backgroundSize: "8px 8px", - } - : undefined -} +style={state === "inprogress" ? getInProgressStyle() : undefined}Also applies to: 424-435, 458-469
785-809: Consider using a data-driven approach for event visibility settings.The
getAdminOnlyForEventfunction uses a switch statement that could potentially become unwieldy as more event types are added.Consider using a map-based approach:
const ADMIN_ONLY_EVENTS = { dequeue: false, fork: false, create_attempt: true, import: true, lazy_payload: true, pod_scheduled: true, // Add other events here }; function getAdminOnlyForEvent(event: string): boolean { return event in ADMIN_ONLY_EVENTS ? ADMIN_ONLY_EVENTS[event as keyof typeof ADMIN_ONLY_EVENTS] : true; }This approach would be more maintainable and easier to update.
759-783: Consider unifying the event metadata handling functions.The functions
getFriendlyNameForEvent,getAdminOnlyForEvent, andgetHelpTextForEventall operate on the same event types but are implemented as separate functions with switch statements.You could consolidate these into a single data structure:
type EventMetadata = { friendlyName: string; adminOnly: boolean; helpText?: string; }; const EVENT_METADATA: Record<string, EventMetadata> = { dequeue: { friendlyName: "Dequeued", adminOnly: false, helpText: "The task was dequeued from the queue", }, fork: { friendlyName: "Launched", adminOnly: false, helpText: "The process was created to run the task", }, // Add other events here }; function getEventMetadata(event: string): EventMetadata { return EVENT_METADATA[event] || { friendlyName: event, adminOnly: true, helpText: undefined, }; }Then you could replace the individual functions with:
const metadata = getEventMetadata(event); const friendlyName = metadata.friendlyName; const isAdminOnly = metadata.adminOnly; const helpText = metadata.helpText;Also applies to: 811-829
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx(1 hunks)apps/webapp/tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/webapp/tailwind.config.js (2)
254-257: Good addition of a new keyframe animation for the timeline visualization.The new
tile-move-offsetkeyframe animation provides a slightly different animation pattern from the existingtile-movewith an offset starting position, which will be useful for the new RunTimeline component.
263-263: Great integration with existing animation patterns.The new
tile-scroll-offsetanimation correctly links to the newly defined keyframe with appropriate duration and timing functions, maintaining consistency with other animations.apps/webapp/app/components/run/RunTimeline.tsx (8)
16-16: Well-structured type definitions that clearly describe the timeline components.The type definitions are comprehensive and strongly typed, which will help prevent errors during component usage and future development.
Also applies to: 29-48
51-71: Comprehensive run data model with well-documented properties.The
TimelineSpanRuntype is well-defined with clear documentation for each property, making it easy for other developers to understand what data is needed for the timeline visualization.
73-110: Well-implemented main RunTimeline component with clean rendering logic.The
RunTimelinecomponent has a clean implementation that filters visible items and conditionally renders the appropriate components based on item type.
113-304: Comprehensive timeline item building logic with excellent state handling.The
buildTimelineItemsfunction handles all possible run states including triggered, dequeued, executing, finished, and expired scenarios. The code is well-structured with clear comments explaining each section of the timeline.
334-484: Well-structured EventMarker component with comprehensive variant support.The
EventMarkercomponent handles all possible timeline event marker variants with appropriate styling for each state. The code is well-organized and uses thecnutility function effectively for conditional class names.
487-568: Effective implementation of the RunTimelineLine component.The implementation provides clear visual differentiation between normal and light variants, with appropriate styling for different states.
581-668: Well-designed SpanTimeline component with good filtration of admin-only events.The
SpanTimelinecomponent effectively handles filtering of admin-only events and provides a comprehensive visualization of span timeline events with appropriate timestamps and durations.
681-757: Comprehensive event conversion with good sorting and normalization.The
createTimelineSpanEventsFromSpanEventsfunction effectively converts span events to timeline events, with proper handling of timestamps, sorting, and friendly name conversion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/webapp/app/components/runs/v3/SpanTitle.tsx (2)
147-175: Consider unifying border and background class logic.
This neweventBorderClassNamefunction closely mirrors the existingeventBackgroundClassName. To avoid duplication, you could merge shared logic into a helper that returns both border and background classes based on a parameter.
202-214: Reduce duplication.
Similarly,borderClassNameForVariantre-implements much of the pattern inbackgroundClassNameForVariant. Combining them under a single function or configuration object would improve maintainability.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (1)
175-184: Avoid unsafe casting.
Castingrun.environmentto a specific shape can lead to runtime errors if the underlying data changes. Consider a typed interface or runtime checks to ensure reliability.apps/webapp/app/components/run/RunTimeline.tsx (4)
115-312: Comprehensive timeline assembly inbuildTimelineItems.
This function covers all run states thoroughly, but consider unit tests to confirm correct sequence of items (especially edge cases likeexpiredAt).
355-505: EventMarker variants.
The approach for distinct marker shapes is well-structured, but ensure you test combinations ofstateandvariant(e.g.,inprogresswithdot-solid) for visual correctness.
526-589: LineMarker animation.
Tiled backgrounds forinprogressstates are a nice indication. Keep an eye on performance if you plan to scale up usage.
819-843: Admin-only event logic.
getAdminOnlyForEventmight grow as new events appear. Monitor for correctness or refactor to a dictionary-based approach if it expands significantly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webapp/app/components/primitives/Badge.tsx(1 hunks)apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/components/runs/v3/SpanTitle.tsx(3 hunks)apps/webapp/app/presenters/v3/RunPresenter.server.ts(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx(11 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx(6 hunks)apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/components/primitives/Badge.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/presenters/v3/RunPresenter.server.ts
- apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx
[error] 942-943: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
apps/webapp/app/components/runs/v3/SpanTitle.tsx (1)
78-78: Use of custom size utility looks good.
Ensuring"size-4"is properly defined in your CSS or utility classes prevents inconsistent styling.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (5)
34-34: Consistent date formatting import.
Good addition ofDateTimeShortto provide a concise date display.
58-62: Aligned imports for event styling.
These imports (SpanTitle,eventBackgroundClassName,eventBorderClassName) are used to maintain consistency across runs' visuals. The usage seems appropriate.
72-72: New admin hook import.
Bringing inuseHasAdminAccessenables more granular timeline filtering based on admin privileges.
757-757: Conditional logic for isAdmin usage.
This new variable supports admin-only features; ensure it’s consistently handled throughout the run timeline code.
1131-1135: Verify custom sizing classes.
Using"size-2"classes is consistent with your styling convention, but confirm they’re defined and tested across all breakpoints.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (4)
14-14: Use of React useEffect.
AddinguseEffectis suitable for loading span data once the ID is known.
54-54: Admin logic import.
New importuseHasAdminAccesswill help tailor UI for admin users.
173-173: Applying admin logic.
CallinguseHasAdminAccess()here provides a clear approach for filtering events.
313-313: Admin filtering for timeline events.
PassingisAdmintocreateTimelineSpanEventsFromSpanEventsensures non-admin users don’t see admin-only events.apps/webapp/app/components/run/RunTimeline.tsx (10)
1-15: Imports are well-organized.
Great job keeping important utility imports (e.g., durations, tooltips) close at hand for timeline features.
16-50: Clear type definitions.
Defining separate event and line variants makes the timeline’s data structure easy to understand and expand.
53-74: Minimal run interface for timeline rendering.
TimelineSpanRuneffectively captures essential fields. Ensure future fields remain optional to avoid bloat in timeline logic.
75-113: Main RunTimeline component.
Cleanly maps out timeline items and delegates rendering to smaller components. Great separation of concerns.
314-353: Granular timeline event display.
RunTimelineEventneatly separates the marker, text, and optional help text. Good detail control.
507-524: Line-based timeline items.
SeparatingRunTimelineLinefrom the event logic clarifies how durations or pending states appear in the UI.
591-688: SpanTimeline specialized flow.
This custom timeline for spans reuses the same marker/line style. Good code reuse.
691-791: Filtering admin-only events.
createTimelineSpanEventsFromSpanEventselegantly handles admin logic. Confirm that all event types are consistently recognized ingetAdminOnlyForEvent.
793-816: User-friendly naming.
getFriendlyNameForEventensures clearer display for internal event codes; continue updating as new events are introduced.
845-884: User hints for each event.
Providing descriptive help text greatly improves developer understanding. This is a nice approach to reduce reliance on separate docs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (1)
933-935: Use optional chaining for safer access to timeline eventsThe current code could be improved by using optional chaining to safely access nested properties.
- {node.data.timelineEvents && - node.data.timelineEvents[0] && - node.data.timelineEvents[0].offset < node.data.offset ? ( + {node.data.timelineEvents?.[0]?.offset < node.data.offset ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 933-934: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/webapp/app/components/run/RunTimeline.tsx (2)
377-410: Consider refactoring repeated state color logicThere's similar color mapping logic for states in both
EventMarkerandLineMarker. This could be extracted into a utility function for better maintainability.You could create a helper function like:
function getColorClassesForState(state: TimelineEventState | undefined, style: TimelineStyle = "normal") { if (state === "complete") return { bg: "bg-success", border: "border-success" }; if (state === "error") return { bg: "bg-error", border: "border-error" }; if (state === "delayed") return { bg: "bg-text-dimmed", border: "border-text-dimmed" }; if (state === "inprogress") { return style === "normal" ? { bg: "bg-pending", border: "border-pending" } : { bg: "bg-text-dimmed", border: "border-text-dimmed" }; } return { bg: "bg-text-dimmed", border: "border-text-dimmed" }; }And then use it in both marker components.
778-869: Consider consolidating event metadata functionsThe three functions
getFriendlyNameForEvent,getAdminOnlyForEvent, andgetHelpTextForEventall have similar switch statements for the same event types. Consider combining them into a single function that returns all metadata for an event.You could create a function like:
function getEventMetadata(event: string): { friendlyName: string; isAdminOnly: boolean; helpText?: string; } { switch (event) { case "dequeue": return { friendlyName: "Dequeued", isAdminOnly: false, helpText: "The run was dequeued from the queue" }; case "fork": return { friendlyName: "Launched", isAdminOnly: false, helpText: "The process was created to run the task" }; // Add other cases... default: return { friendlyName: event, isAdminOnly: true, helpText: undefined }; } }This would improve maintainability by keeping all related information for each event type in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx(11 hunks)apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/storybook.run-and-span-timeline/route.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx
[error] 933-934: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (5)
72-72: Good security enhancement with useHasAdminAccessAdding the
useHasAdminAccessimport will enable proper permission checks for admin-only timeline events.
748-748: Appropriate use of admin check for timeline visibilityUsing
useHasAdminAccess()here properly controls what timeline events are visible to different user roles.
897-959: Improved timeline visualization with detailed event markersThe timeline visualization has been significantly enhanced with more detailed event markers and lines that better represent the state of each span.
🧰 Tools
🪛 Biome (1.9.4)
[error] 933-934: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1169-1227: Enhanced CurrentTimeIndicator with both duration and actual timestampThe new
CurrentTimeIndicatorimplementation improves user experience by showing both the relative duration and the actual timestamp, making it easier to understand when events occurred.
1122-1126: Simplified and consistent size styling for PulsingDotUpdated to use the modern Tailwind
size-Xutility instead of separate width and height declarations.apps/webapp/app/components/run/RunTimeline.tsx (5)
15-74: Well-structured type definitions for timeline componentsThe type definitions are comprehensive and provide a strong foundation for the timeline visualization system. They clearly define the data structures needed for representing different timeline states and events.
76-114: Clean implementation of RunTimeline componentThe
RunTimelinecomponent has a clean implementation that filters items and renders appropriate components based on item type. It correctly handles the display of events and connecting lines.
117-323: Comprehensive timeline state handling in buildTimelineItemsThe
buildTimelineItemsfunction effectively handles all possible run states and transitions, including:
- Triggered state
- Waiting/delayed states
- Dequeued state
- Execution state
- Completion state
- Error handling
- Expiration scenarios
This provides a complete visual representation of the run's lifecycle.
688-776: Robust implementation for creating timeline events from span eventsThe
createTimelineSpanEventsFromSpanEventsfunction effectively:
- Filters for relevant events
- Sorts them chronologically
- Respects admin-only visibility permissions
- Calculates proper offsets and durations
- Maps to appropriate visualization variants
The proper handling of admin-only events is particularly important for security.
581-676: Well-structured SpanTimeline componentThe
SpanTimelinecomponent provides a clear visualization of span events with proper handling of in-progress vs. completed states. The conditional rendering based on state is clean and maintainable.
Summary by CodeRabbit
New Features
DateTimecomponent with a new optional property for comparing dates.Storycomponent for showcasing span and run timelines.Improvements
Removed Components
InspectorTimelineandRunInspectorcomponents, consolidating their functionality into new timelines.