feat: Run metadata #1357
feat: Run metadata #1357
Conversation
🦋 Changeset detectedLatest commit: 5d39775 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a feature for managing run metadata within the Trigger.dev SDK, enabling users to store and manipulate metadata associated with task runs. This includes the addition of new properties, methods, and API endpoints for efficient metadata handling. The updates span multiple files, enhancing both backend and frontend components to support this functionality. Changes
Possibly related PRs
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 (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (40)
.changeset/tasty-rats-rhyme.md (1)
6-6: Consider enhancing the feature description.While the current description is good, it could be slightly improved to better reflect the full scope of the feature as described in the PR objectives.
Consider updating the description to:
-Add Run metadata to allow for storing up to 4KB of data on a run and update it during the run +Add Run metadata to allow for storing and updating up to 4KB of JSON-serializable data on a run, accessible via SDK and dashboardThis change provides more context about the data type (JSON-serializable) and mentions the accessibility improvements (SDK and dashboard), which are key aspects of the feature.
packages/database/prisma/migrations/20240925092304_add_metadata_and_output_to_task_run/migration.sql (1)
1-5: LGTM! Consider using JSONB for better performance.The migration script successfully implements the metadata feature described in the PR objectives. The addition of "metadata" and "metadataType" columns aligns with the requirement to attach JSON-serializable metadata to task runs. The "output" and "outputType" columns, while not explicitly mentioned in the objectives, follow a similar pattern and could be useful for storing run results.
Consider using the JSONB type instead of TEXT for the "metadata" and "output" columns if your database supports it (e.g., PostgreSQL). JSONB offers better performance for JSON data, including indexing and querying capabilities. Here's a suggested modification:
-- AlterTable ALTER TABLE "TaskRun" - ADD COLUMN "metadata" TEXT, + ADD COLUMN "metadata" JSONB, ADD COLUMN "metadataType" TEXT NOT NULL DEFAULT 'application/json', - ADD COLUMN "output" TEXT, + ADD COLUMN "output" JSONB, ADD COLUMN "outputType" TEXT NOT NULL DEFAULT 'application/json';This change would provide better performance and more flexibility when working with JSON data in your database.
apps/webapp/app/utils/packets.ts (1)
5-10: LGTM: Well-implemented custom error class.The
MetadataTooLargeErrorclass is well-structured and follows good practices for custom error types. The use of the 413 status code (Payload Too Large) is appropriate for this error scenario.Consider adding a readonly
statusCodeproperty to the class for better type safety and to make the status code easily accessible:export class MetadataTooLargeError extends ServiceValidationError { + readonly statusCode = 413; constructor(message: string) { - super(message, 413); + super(message, this.statusCode); this.name = "MetadataTooLargeError"; } }apps/webapp/app/v3/testTask.ts (2)
14-26: Improved error handling for payload parsing.The new error handling structure enhances the granularity of error reporting by distinguishing between
Errorinstances and other types of errors. This approach provides more specific error messages when possible, which is beneficial for debugging and user feedback.Consider removing or replacing the
console.log("parsing error", e);statement with a more appropriate logging mechanism for production code. If it's intended for debugging, consider using a debug logging library or removing it before merging to production.
Line range hint
27-43: Metadata parsing added with consistent error handling.The addition of metadata parsing aligns with the PR objectives and uses the same error handling approach as the payload parsing, ensuring consistency across the schema.
Consider refactoring the JSON parsing logic for both payload and metadata into a separate function to reduce code duplication. This would make the code more maintainable and easier to update in the future. Here's a suggested implementation:
function parseJSON(input: string, ctx: z.RefinementCtx) { try { return JSON.parse(input) as any; } catch (e) { console.log("parsing error", e); if (e instanceof Error) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: e.message, }); } else { ctx.addIssue({ code: z.ZodIssueCode.custom, message: "This is invalid JSON", }); } } } // Usage: payload: z.string().transform((payload, ctx) => parseJSON(payload, ctx)), metadata: z.string().transform((metadata, ctx) => parseJSON(metadata, ctx)),This refactoring would make the code more DRY and easier to maintain.
apps/webapp/app/components/primitives/Tabs.tsx (1)
100-100: Approve the addition oftype="button"with a minor suggestion.The addition of
type="button"to the button element is a good practice. It explicitly prevents the button from submitting forms by default, which can prevent unintended behavior in complex applications.For consistency, consider adding
type="button"to other custom button components in your application if they don't already have it. This ensures uniform behavior across all button elements.- type="button" + type="button" // Prevents form submissionAdding a comment can help other developers understand the purpose of this attribute.
apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (3)
131-132: LGTM: SQL query updated to include metadata columns.The addition of
metadataandmetadataTypecolumns to the SELECT statement aligns with the PR objective of retrieving metadata for task runs. This change ensures that the metadata information is fetched from the database along with other run details.Consider adding a condition to only fetch these columns when they are not null, which could potentially improve query performance for runs without metadata:
COALESCE(taskr.metadata, '') as metadata, COALESCE(taskr."metadataType", '') as "metadataType",This change would ensure that empty strings are returned instead of NULL values, which might simplify handling in the application code.
168-170: LGTM: Metadata processing added to run transformation.The addition of metadata processing in the run transformation logic is consistent with the PR's objective. It ensures that metadata, when present, is formatted consistently with other run data using
prettyPrintPacket.Consider adding error handling for the
prettyPrintPacketcall on metadata:metadata: r.metadata ? await prettyPrintPacket(r.metadata, r.metadataType).catch(error => { console.error(`Error processing metadata for run ${r.id}:`, error); return JSON.stringify(r.metadata); }) : undefined,This change would ensure that an error in processing the metadata doesn't break the entire run transformation, and provides a fallback of stringifying the raw metadata.
Line range hint
175-195: Ensure metadata handling for SCHEDULED trigger source.While the implementation for the "STANDARD" trigger source correctly handles the new metadata fields, the "SCHEDULED" case doesn't appear to process these fields. This inconsistency could lead to metadata being ignored for scheduled tasks.
Consider updating the "SCHEDULED" case to handle metadata similarly to the "STANDARD" case:
case "SCHEDULED": const possibleTimezones = getTimezones(); return { triggerSource: "SCHEDULED", task: taskWithEnvironment, possibleTimezones, runs: ( await Promise.all( latestRuns.map(async (r) => { const number = Number(r.number); const payload = await getScheduleTaskRunPayload(r); if (payload.success) { return { ...r, number, payload: payload.data, metadata: r.metadata ? await prettyPrintPacket(r.metadata, r.metadataType).catch(error => { console.error(`Error processing metadata for scheduled run ${r.id}:`, error); return JSON.stringify(r.metadata); }) : undefined, }; } }) ) ).filter(Boolean), };This change ensures consistent handling of metadata across both trigger sources.
packages/core/src/v3/schemas/common.ts (1)
226-226: LGTM: Consistent omission of metadata in TaskRunContext.The omission of the
metadataproperty from therunproperty in theTaskRunContextschema is consistent with howpayloadandpayloadTypeare handled. This ensures that potentially sensitive or large metadata is not included in the context of a task run.For improved readability, consider using an array for the omitted properties:
- run: TaskRun.omit({ payload: true, payloadType: true, metadata: true }), + run: TaskRun.omit(["payload", "payloadType", "metadata"]),This change makes it easier to add or remove properties in the future and improves code clarity.
docs/tasks/overview.mdx (2)
197-204: LGTM! Consider adding a comment about theprojectproperty.The implementation of the
onStartfunction in thetrigger.config.tsfile looks good. It aligns with the PR objective of enhancing task run functionality.Consider adding a brief comment explaining the purpose and significance of the
projectproperty, as it wasn't mentioned in the PR description:export default defineConfig({ + // Unique identifier for the project project: "proj_1234", onStart: async (payload, { ctx }) => { console.log("Task started", ctx.task.id); }, });
226-233: LGTM! Consider using template literals for consistency.The implementation of the
onSuccessfunction in thetrigger.config.tsfile looks good and aligns with the PR objective.For consistency with the
onStartfunction, consider using template literals in the console.log statement:export default defineConfig({ project: "proj_1234", onSuccess: async (payload, output, { ctx }) => { - console.log("Task succeeded", ctx.task.id); + console.log(`Task succeeded ${ctx.task.id}`); }, });docs/runs/metadata.mdx (5)
129-161: LGTM: Clear examples of metadata usage in lifecycle hooks.This section effectively demonstrates how metadata methods can be used within task lifecycle hooks, both for specific tasks and in global hooks defined in the configuration file. The examples are clear and illustrative.
Consider adding a brief explanation of what lifecycle hooks are and their purpose at the beginning of this section. This would provide more context for users who might be unfamiliar with the concept.
163-177: LGTM: Clear explanation of metadata propagation.This section effectively explains that metadata is not automatically propagated to child tasks and provides a clear example of how to explicitly pass metadata to a child task.
Consider adding a brief explanation of what child tasks are and when they might be used. This would provide more context for users who might be unfamiliar with the concept of task hierarchies in Trigger.dev.
179-247: LGTM: Comprehensive guide on type-safe metadata handling.This section effectively explains the current loose typing of metadata APIs and provides clear examples of valid and invalid metadata inputs. The recommendation to use Zod for type-safe metadata handling is a good practice, and the provided example is helpful. The note about Date serialization is important for preventing potential issues.
Consider adding a brief explanation of why type safety is important in this context. This would help users understand the benefits of using a schema validation library like Zod for metadata handling.
249-269: LGTM: Clear explanation of metadata inspection methods.This section effectively covers both dashboard and API methods for inspecting metadata. The inclusion of a visual example for the dashboard view is particularly helpful. The API example using
runs.retrieve()is clear and concise.Consider adding a brief explanation of when and why a user might need to inspect metadata programmatically using the API, as opposed to using the dashboard. This would provide more context for the API example.
271-273: LGTM: Clear explanation of metadata size limits.This section effectively explains the default 4KB size limit for metadata and provides information on how to increase this limit when self-hosting Trigger.dev.
Consider adding a brief explanation of why there's a size limit on metadata and the potential implications of increasing it. This would help users make informed decisions about adjusting the limit in self-hosted environments.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx (1)
78-84: Approve changes with a minor optimization suggestionThe changes to determine the loading state of tasks look good. They effectively prevent unnecessary loading states when only certain parameters change, which aligns with the PR objectives.
A minor optimization suggestion:
Consider memoizing the
URLSearchParamsobjects to avoid recreating them on every render:const locationSearchParams = useMemo(() => new URLSearchParams(location.search), [location.search]); const navigationSearchParams = useMemo( () => new URLSearchParams(navigation.location?.search), [navigation.location?.search] );This optimization would be particularly beneficial if this component re-renders frequently.
apps/webapp/app/env.server.ts (1)
215-215: LGTM! Consider enhancing the comment for clarity.The addition of
TASK_RUN_METADATA_MAXIMUM_SIZEaligns well with the PR objectives and is implemented consistently with other properties in the schema. The default value of 4KB is reasonable for metadata storage.Consider enhancing the comment for added clarity:
- TASK_RUN_METADATA_MAXIMUM_SIZE: z.coerce.number().int().default(4_096), // 4KB + TASK_RUN_METADATA_MAXIMUM_SIZE: z.coerce.number().int().default(4_096), // 4KB (maximum size for JSON-serializable metadata per task run)This change provides more context about the purpose of the property directly in the code.
packages/core/src/v3/workers/taskExecutor.ts (1)
75-77: LGTM: Metadata handling implementationThe new conditional block effectively implements the metadata handling functionality as per the PR objectives. It's well-placed within the
executemethod, ensuring that metadata is set up early in the execution process. The condition prevents unnecessary calls when metadata is not present, which is a good practice.Consider adding a comment explaining the purpose of this block for better code readability:
+ // Set up run metadata if present if (execution.run.metadata) { runMetadata.enterWithMetadata(execution.run.metadata); }packages/core/src/v3/schemas/api.ts (2)
85-86: LGTM: Metadata fields added to TriggerTaskRequestBody.The addition of
metadataandmetadataTypefields aligns with the PR objective. However, consider using a more specific type formetadatainstead ofz.any()to improve type safety. For example, you could usez.record(z.unknown())to ensure it's an object without restricting its contents.-metadata: z.any(), +metadata: z.record(z.unknown()),This change would maintain flexibility while providing some structure to the metadata.
623-627: LGTM: UpdateMetadataResponseBody schema added.The new
UpdateMetadataResponseBodyschema is well-structured and consistent with the PR objective. It provides good type safety by usingDeserializedJsonSchemafor the metadata field.Consider adding a
metadataTypefield to the response schema if it would be useful for clients to know the type of metadata being returned:export const UpdateMetadataResponseBody = z.object({ metadata: z.record(DeserializedJsonSchema), + metadataType: z.string().optional(), });This addition would make the response schema symmetrical with the request schema and potentially provide more information to the client.
apps/webapp/app/v3/services/triggerTask.server.ts (2)
103-109: LGTM: Metadata handling logicThe implementation of metadata handling is correct and follows good practices:
- It checks for the existence of metadata before processing.
- Uses optional chaining to safely access nested properties.
- Provides a sensible default for the metadata type.
Consider extracting the default metadata type to a constant for better maintainability:
+const DEFAULT_METADATA_TYPE = "application/json"; + const metadataPacket = body.options?.metadata ? handleMetadataPacket( body.options?.metadata, - body.options?.metadataType ?? "application/json" + body.options?.metadataType ?? DEFAULT_METADATA_TYPE ) : undefined;
352-353: LGTM: Addition of metadata fields in TaskRun creationThe inclusion of
metadataandmetadataTypefields in the TaskRun creation is correct and completes the metadata handling process by persisting the data.For consistency with the rest of the code, consider using null coalescing operator (
??) instead of optional chaining (?.) for themetadataType:metadata: metadataPacket?.data, -metadataType: metadataPacket?.dataType, +metadataType: metadataPacket?.dataType ?? undefined,This change ensures that
undefinedis explicitly set whenmetadataPacketisundefined, maintaining consistency with how other fields are handled in this object.docs/triggering.mdx (2)
737-739: New 'tags' section added.The addition of the 'tags' section with a reference to the tags documentation is helpful. Consider adding a brief description of what tags are used for in this context to give users a quick overview.
Consider adding a one-sentence description of tags, e.g.:
### `tags` +Tags allow you to categorize and filter your task runs. View our [tags doc](/tags) for more information.
741-743: New 'metadata' section added.The addition of the 'metadata' section with a reference to the metadata documentation is helpful and consistent with the PR objectives. For consistency with the 'tags' section, consider adding a brief description of metadata's purpose in this context.
Consider adding a one-sentence description of metadata, e.g.:
### `metadata` +Metadata allows you to attach additional information to your task runs. View our [metadata doc](/runs/metadata) for more information.packages/trigger-sdk/src/v3/shared.ts (2)
500-504: LGTM! Consider adding an example for clarity.The addition of the
metadataproperty toTaskRunOptionsis well-implemented and aligns with the PR objectives. The type definition ensures type safety and serialization compatibility.Consider adding a brief example in the comment to illustrate how to use the metadata property. This could help developers quickly understand its usage. For instance:
/** * Metadata to attach to the run. Metadata can be used to store additional information about the run. Limited to 4KB. * @example * { * userId: "123", * source: "api", * version: "1.0.0" * } */ metadata?: Record<string, SerializableJson>;
873-873: LGTM! Consider consistent formatting for readability.The
metadataproperty has been correctly added to the options in bothbatchTrigger_internalandbatchTriggerAndWait_internalfunctions. This implementation properly handles metadata for batch operations.For consistency and readability, consider aligning the
metadataproperty with the other properties in the options object. For example:options: { queue: item.options?.queue ?? queue, concurrencyKey: item.options?.concurrencyKey, test: taskContext.ctx?.run.isTest, payloadType: payloadPacket.dataType, idempotencyKey: await makeKey(item.options?.idempotencyKey), delay: item.options?.delay, ttl: item.options?.ttl, tags: item.options?.tags, maxAttempts: item.options?.maxAttempts, parentAttempt: taskContext.ctx?.attempt.id, metadata: item.options?.metadata, },This minor adjustment would improve code readability and maintain consistency throughout the file.
Also applies to: 1047-1047
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (1)
1037-1041: Metadata handling implemented correctlyThe implementation of metadata parsing and inclusion in the execution payload aligns well with the PR objectives. The
parsePacketfunction is used to process the metadata from thetaskRunobject, and the result is correctly added to therunobject in the execution payload.A minor suggestion for improvement:
Consider adding a null check before calling
parsePacketto handle cases wheretaskRun.metadatais null. This can be done using the nullish coalescing operator:const metadata = await parsePacket({ - data: taskRun.metadata ?? undefined, + data: taskRun.metadata ?? null, dataType: taskRun.metadataType, });This ensures that
nullis passed toparsePacketwhentaskRun.metadataisnull, which might be more explicit thanundefined.Also applies to: 1069-1069
packages/database/prisma/schema.prisma (1)
1754-1760: LGTM! Consider adding comments for clarity.The addition of
metadata,metadataType,output, andoutputTypefields to theTaskRunmodel successfully implements the metadata feature described in the PR objectives. This change will allow for attaching and retrieving metadata and output for each task run.For consistency and clarity, consider adding brief comments explaining the purpose of these new fields, similar to other commented fields in the schema. For example:
model TaskRun { // ... (previous fields) + /// Metadata associated with the task run metadata String? + /// The content type of the metadata, defaults to JSON metadataType String @default("application/json") + /// Output produced by the task run output String? + /// The content type of the output, defaults to JSON outputType String @default("application/json") // ... (remaining fields) }This will help other developers quickly understand the purpose of these new fields.
references/v3-catalog/src/trigger/runMetadata.ts (4)
21-21: Unusedctxparameter in function signaturesThe
ctxparameter is not used within therunfunctions ofrunMetadataChildTaskandrunMetadataChildTask2. If it's not needed, consider removing it to simplify the function signatures.Apply the following changes:
export const runMetadataChildTask = task({ id: "run-metadata-child-task", - run: async (payload: any, { ctx }) => { + run: async (payload: any) => { await metadata.set("child", "task");export const runMetadataChildTask2 = task({ id: "run-metadata-child-task-2", - run: async (payload: any, { ctx }) => {}, + run: async (payload: any) => {}, });Also applies to: 56-56
24-24: Inconsistent logging keys in metadata logsThe logging statements use different keys for the metadata object (
metadataandcurrent). This inconsistency could lead to confusion when analyzing logs. Consider using a consistent key name across all logging statements.Apply the following changes to make the logging keys consistent:
logger.info("metadata", { metadata: metadata.current() }); logger.info("metadata", { metadata: metadata.current() }); logger.info("metadata", { metadata: metadata.current() }); logger.info("metadata", { metadata: metadata.current() }); logger.info("metadata", { metadata: metadata.current() });Alternatively, if you prefer to use a different key, ensure it is consistently applied in all instances.
Also applies to: 28-28, 32-32, 47-47, 50-50
59-62: Empty function body inmyTaskThe
runfunction ofmyTaskis empty. If this task is a placeholder, consider adding a TODO comment to indicate future implementation plans. If not needed, you might consider removing it to keep the codebase clean.
5-5: Specify the payload type instead of usinganyUsing
anyfor thepayloadparameter can undermine TypeScript's type safety features. Consider defining a specific type or interface for the payload to leverage TypeScript's benefits.Example:
interface PayloadType { // Define the structure of your payload here } export const runMetadataTask = task({ id: "run-metadata-task", run: async (payload: PayloadType) => { // ... }, });Apply similar changes to other tasks where
payload: anyis used.Also applies to: 61-61
packages/core/src/v3/runMetadata/index.ts (1)
20-22: Initializestoreto an empty object to prevent potentialundefinedaccessCurrently,
storeis typed asRecord<string, DeserializedJson> | undefined. Accessing it without ensuring it's defined could lead to undefined behavior. Consider initializingstoreto an empty object to simplify the logic.Apply this diff to initialize
store:private store: Record<string, DeserializedJson> | undefined; - private constructor() {} + private constructor() { + this.store = {}; + } public enterWithMetadata(metadata: Record<string, DeserializedJson>): void { this.store = metadata; } public current(): Record<string, DeserializedJson> | undefined { - return this.store; + return this.store ?? {}; } public getKey(key: string): DeserializedJson | undefined { - return this.store?.[key]; + return this.store[key]; }Also applies to: 28-30
apps/webapp/app/routes/api.v1.runs.$runId.metadata.ts (4)
49-51: Enhance error message for invalid metadata.The error message "Invalid metadata" may not provide enough information for the client to correct the issue. Consider providing more detailed feedback or including validation errors if available.
67-69: Clarify error when updating metadata for completed runs.The message "Cannot update metadata for a completed run" is helpful but could be more informative by specifying the run's current status. This can aid in debugging and provide clearer feedback to the client.
Apply this diff to enhance the error message:
- return json({ error: "Cannot update metadata for a completed run" }, { status: 400 }); + return json( + { error: `Cannot update metadata for a run in status '${taskRun.status}'` }, + { status: 400 } + );
85-86: Log errors for improved debugging.In the catch block, errors are returned to the client but not logged on the server. Logging errors can greatly assist in debugging and monitoring.
Apply this diff to log the error:
+ console.error("Error in updating run metadata:", error);
15-95: Add unit tests for the new API endpoint.To ensure reliability and prevent regressions, consider adding unit tests covering various scenarios like successful updates, invalid inputs, authentication failures, and edge cases.
Would you like assistance in creating unit tests for this endpoint or opening a GitHub issue to track this task?
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (1)
Line range hint
156-167: Refactor duplicated packet processing logicThe code for processing
output,payload, andmetadatashares similar patterns. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a reusable helper function.Here's an example of how you might refactor the code:
Create a helper function:
async function processPacket( packet: any, packetType: string | null | undefined, runtimeEnvironmentId: string ): Promise<any> { if (packetType === "application/store") { return `/resources/packets/${runtimeEnvironmentId}/${packet}`; } else if (typeof packet !== "undefined" && packet !== null) { return await prettyPrintPacket(packet, packetType ?? undefined); } else { return undefined; } }Update the code using the helper function:
// Processing output - const output = - finishedAttempt === null - ? undefined - : finishedAttempt.outputType === "application/store" - ? `/resources/packets/${run.runtimeEnvironment.id}/${finishedAttempt.output}` - : typeof finishedAttempt.output !== "undefined" && finishedAttempt.output !== null - ? await prettyPrintPacket(finishedAttempt.output, finishedAttempt.outputType ?? undefined) - : undefined; + const output = finishedAttempt + ? await processPacket( + finishedAttempt.output, + finishedAttempt.outputType, + run.runtimeEnvironment.id + ) + : undefined; // Processing payload - const payload = - run.payloadType === "application/store" - ? `/resources/packets/${run.runtimeEnvironment.id}/${run.payload}` - : typeof run.payload !== "undefined" && run.payload !== null - ? await prettyPrintPacket(run.payload, run.payloadType ?? undefined) - : undefined; + const payload = await processPacket( + run.payload, + run.payloadType, + run.runtimeEnvironment.id + ); // Processing metadata - const metadata = run.metadata - ? await prettyPrintPacket(run.metadata, run.metadataType) - : undefined; + const metadata = await processPacket( + run.metadata, + run.metadataType, + run.runtimeEnvironment.id + );This refactor reduces code duplication and centralizes the packet processing logic, making it easier to maintain and update in the future.
Also applies to: 195-198, 250-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
docs/images/run-metadata.pngis excluded by!**/*.png
📒 Files selected for processing (37)
- .changeset/tasty-rats-rhyme.md (1 hunks)
- apps/webapp/app/components/code/JSONEditor.tsx (1 hunks)
- apps/webapp/app/components/primitives/Tabs.tsx (1 hunks)
- apps/webapp/app/env.server.ts (1 hunks)
- apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (5 hunks)
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts (4 hunks)
- apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (4 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx (6 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx (1 hunks)
- apps/webapp/app/routes/api.v1.runs.$runId.metadata.ts (1 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (2 hunks)
- apps/webapp/app/routes/resources.runs.$runParam.ts (1 hunks)
- apps/webapp/app/utils/packets.ts (1 hunks)
- apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (3 hunks)
- apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (3 hunks)
- apps/webapp/app/v3/services/replayTaskRun.server.ts (2 hunks)
- apps/webapp/app/v3/services/testTask.server.ts (1 hunks)
- apps/webapp/app/v3/services/triggerTask.server.ts (3 hunks)
- apps/webapp/app/v3/testTask.ts (1 hunks)
- docs/runs/metadata.mdx (1 hunks)
- docs/tasks/overview.mdx (3 hunks)
- docs/triggering.mdx (2 hunks)
- docs/v3-openapi.yaml (1 hunks)
- packages/core/src/v3/apiClient/index.ts (2 hunks)
- packages/core/src/v3/apiClientManager/index.ts (2 hunks)
- packages/core/src/v3/index.ts (1 hunks)
- packages/core/src/v3/run-metadata-api.ts (1 hunks)
- packages/core/src/v3/runMetadata/index.ts (1 hunks)
- packages/core/src/v3/schemas/api.ts (4 hunks)
- packages/core/src/v3/schemas/common.ts (3 hunks)
- packages/core/src/v3/workers/taskExecutor.ts (2 hunks)
- packages/database/prisma/migrations/20240925092304_add_metadata_and_output_to_task_run/migration.sql (1 hunks)
- packages/database/prisma/schema.prisma (1 hunks)
- packages/trigger-sdk/src/v3/index.ts (1 hunks)
- packages/trigger-sdk/src/v3/metadata.ts (1 hunks)
- packages/trigger-sdk/src/v3/shared.ts (6 hunks)
- references/v3-catalog/src/trigger/runMetadata.ts (1 hunks)
🧰 Additional context used
LanguageTool
docs/runs/metadata.mdx
[misspelling] ~7-~7: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e additional, structured information on an run. For example, you could store your ...(EN_A_VS_AN)
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
Biome
packages/core/src/v3/runMetadata/index.ts
[error] 13-13: 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] 14-14: 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] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments not posted (62)
.changeset/tasty-rats-rhyme.md (1)
1-6: LGTM! Changeset looks good.The changeset file is correctly formatted and the patch version bump for both packages is appropriate for this feature addition. The description aligns well with the PR objectives.
packages/core/src/v3/run-metadata-api.ts (2)
1-5: LGTM! Well-structured module for run metadata API.The file is well-organized and follows good practices:
- It provides clear comments explaining the purpose of the file.
- It uses a single responsibility principle by only handling the run metadata API instance.
- It facilitates tree-shaking, which can lead to better performance in the final bundle.
This implementation aligns well with the PR objectives of enhancing task runs with metadata functionality.
5-5: LGTM! Singleton pattern used for RunMetadataAPI.The use of
RunMetadataAPI.getInstance()suggests a singleton pattern, which is appropriate for managing a global instance of the run metadata API. This approach ensures consistency across the application when working with run metadata.apps/webapp/app/utils/packets.ts (3)
1-3: LGTM: Imports are appropriate and well-structured.The imports are relevant to the functionality being implemented in this file. The use of relative imports (~/...) is consistent with the project structure.
1-36: Overall, good implementation aligning with PR objectives.This new file successfully implements the core functionality for handling metadata in task runs, as described in the PR objectives. The
MetadataTooLargeErrorclass andhandleMetadataPacketfunction provide the necessary tools to attach, validate, and process up to 4KB of JSON-serializable metadata when triggering a task.The implementation correctly enforces the size limit defined in the environment variable
TASK_RUN_METADATA_MAXIMUM_SIZE, throwing an appropriate error when exceeded. This aligns with the PR's goal of enhancing task run functionality while maintaining control over metadata size.While the current implementation is functional, the suggested improvements to the
handleMetadataPacketfunction would further enhance its robustness and error handling, contributing to a more reliable system overall.Remember to update any related components (such as the run dashboard and SDK functions) to utilize this new functionality for a complete implementation of the "Run metadata" feature.
12-36:⚠️ Potential issueGood implementation, but consider some improvements for robustness.
The
handleMetadataPacketfunction handles different types of metadata input well and implements the size check correctly. However, there are a few areas where it could be improved:
The function doesn't handle non-string, non-JSON metadata types. It silently returns undefined in these cases, which might lead to unexpected behavior.
There's no type checking for the
metadataTypeparameter, which could lead to runtime errors if an invalid type is passed.The function could benefit from more explicit error handling for JSON parsing.
Consider the following improvements:
export function handleMetadataPacket(metadata: any, metadataType: string): IOPacket | undefined { let metadataPacket: IOPacket | undefined = undefined; if (typeof metadata === "string") { metadataPacket = { data: metadata, dataType: metadataType }; } else if (metadataType === "application/json") { try { metadataPacket = { data: JSON.stringify(metadata), dataType: "application/json" }; } catch (error) { throw new Error("Failed to stringify metadata: " + (error as Error).message); } } else { throw new Error(`Unsupported metadata type: ${metadataType}`); } if (!metadataPacket || !metadataPacket.data) { throw new Error("Invalid metadata: empty or undefined"); } const byteLength = Buffer.byteLength(metadataPacket.data, "utf8"); if (byteLength > env.TASK_RUN_METADATA_MAXIMUM_SIZE) { throw new MetadataTooLargeError( `Metadata exceeds maximum size of ${env.TASK_RUN_METADATA_MAXIMUM_SIZE} bytes` ); } return metadataPacket; }These changes will:
- Handle unsupported metadata types explicitly.
- Catch and rethrow JSON stringify errors with a more informative message.
- Throw an error for invalid (empty or undefined) metadata instead of silently returning undefined.
To ensure that these changes don't break existing functionality, please run the following verification script:
If this script returns any results, we may need to adjust the error handling to maintain backwards compatibility.
✅ Verification successful
Shell script adjustment needed to correctly identify TypeScript files.
The previous script failed because
ripgrepdoes not recognizetypescriptas a valid file type. To fix this, we can specify the correct TypeScript file extensions.Please run the following verification script to check for instances where
handleMetadataPacketis explicitly compared toundefined:This script searches all
.tsand.tsxfiles for the patternhandleMetadataPacket === undefined. If any results are returned, please review them to ensure that the proposed changes do not break existing functionality.
Verified the review comment. No conflicting usages of
handleMetadataPacketreturningundefinedwere found.The proposed changes to
handleMetadataPacketcan be safely implemented as there are no existing checks forundefinedreturns that would be affected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usage of handleMetadataPacket function # Expected: No instances where undefined is explicitly checked as a return value rg --type typescript 'handleMetadataPacket.*===\s*undefined'Length of output: 97
Script:
#!/bin/bash # Description: Check for existing usage of handleMetadataPacket function # Expected: No instances where undefined is explicitly checked as a return value rg --glob "*.ts" --glob "*.tsx" 'handleMetadataPacket\s*===\s*undefined'Length of output: 74
apps/webapp/app/v3/services/testTask.server.ts (1)
22-22: LGTM! Consider extending metadata support and adding type checks.The addition of
metadata: data.metadata,to the options object correctly implements the PR objective of allowing metadata attachment to task runs.To ensure consistency, please verify if metadata support should also be added to the
SCHEDULEDcase. Run the following script to check for any references to metadata in scheduled tasks:Consider adding type checking and validation for the metadata to ensure it meets the 4KB JSON-serializable requirement mentioned in the PR objectives. You could implement this as follows:
import { z } from 'zod'; const metadataSchema = z.object({}).passthrough().refine( (data) => JSON.stringify(data).length <= 4096, { message: "Metadata must be JSON-serializable and not exceed 4KB" } ); // In the call method const validatedMetadata = metadataSchema.parse(data.metadata); // Then use validatedMetadata instead of data.metadataThis ensures type safety and validates the metadata size before processing.
packages/trigger-sdk/src/v3/index.ts (1)
10-10: LGTM: New metadata module export aligns with PR objectives.The addition of
export * from "./metadata.js";is consistent with the existing code structure and directly supports the PR objective of enhancing task runs with metadata functionality. This export will make the metadata-related features accessible to other parts of the SDK.packages/core/src/v3/index.ts (1)
14-14: LGTM! New run metadata API exported.The addition of
export * from "./run-metadata-api.js";aligns with the PR objectives to enhance task runs with metadata functionality. This export makes the new run metadata API accessible to other parts of the codebase.Let's verify the existence and contents of the new module:
This script will help us confirm that the new module exists and contains the expected metadata-related functionality.
apps/webapp/app/v3/testTask.ts (1)
Line range hint
1-43: Verify integration with the rest of the system.The changes successfully implement the metadata feature and improve error handling for both payload and metadata parsing. However, it's important to ensure that these changes are properly integrated with the rest of the system.
Please run the following script to verify the usage of
TestTaskDataacross the codebase:This will help ensure that all components using
TestTaskDataare updated to handle the new metadata field correctly.apps/webapp/app/v3/services/replayTaskRun.server.ts (1)
68-73: Metadata handling implementation looks good, but consider a few improvements.The implementation of metadata handling aligns well with the PR objectives. However, there are a few suggestions to enhance robustness and maintainability:
- Consider adding metadata size validation to ensure it doesn't exceed the 4KB limit mentioned in the PR objectives.
- Implement explicit error handling for metadata parsing to gracefully handle potential issues.
Here's a suggested improvement for metadata handling:
import { MAX_METADATA_SIZE } from "@trigger.dev/core/v3/constants"; // Assume this constant is defined // ... let metadata: unknown; if (existingTaskRun.metadata) { try { const parsedMetadata = await parsePacket({ data: existingTaskRun.metadata, dataType: existingTaskRun.metadataType, }); if (JSON.stringify(parsedMetadata).length > MAX_METADATA_SIZE) { logger.warn("Metadata exceeds maximum size limit, it will be truncated", { taskRunId: existingTaskRun.id, taskRunFriendlyId: existingTaskRun.friendlyId, }); // Implement truncation logic here } else { metadata = parsedMetadata; } } catch (error) { logger.error("Failed to parse metadata", { taskRunId: existingTaskRun.id, taskRunFriendlyId: existingTaskRun.friendlyId, error, }); // Optionally, you might want to set metadata to a default value or leave it undefined } }Let's verify the usage of the new
metadatafield in theTriggerTaskService:Also applies to: 95-95
apps/webapp/app/components/code/JSONEditor.tsx (1)
116-117: LGTM! Class ordering change improves customization.The reordering of class names in the
cnfunction call is a good practice. By movingopts.classNameto the end of the list, it allows any custom classes passed through props to override the default classes. This change enhances the component's flexibility for styling customization without affecting its core functionality.apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (1)
2-2: LGTM: Import and type updates align with PR objectives.The addition of
TaskRunStatusimport and the new optionalmetadataandmetadataTypeproperties in theRawRuntype are consistent with the PR's goal of introducing metadata support for task runs. These changes provide the necessary type definitions for handling the new metadata feature.Also applies to: 49-50
apps/webapp/app/routes/resources.runs.$runParam.ts (1)
75-76: LGTM! Consider enhancing metadata handling.The addition of
metadataandmetadataTypefields aligns well with the PR objectives. However, to fully implement the metadata feature, consider the following suggestions:
- Include the retrieved metadata in the returned data structure of the
typedjsonfunction at the end of the loader.- Add type checking for the metadata to ensure it's properly handled, especially considering the 4KB JSON-serializable constraint mentioned in the PR objectives.
- Implement proper serialization/deserialization of the metadata if needed.
Here's a suggested modification to include metadata in the returned structure:
return typedjson({ // ... existing properties metadata: run.metadata, metadataType: run.metadataType, // ... remaining properties });To ensure the complete implementation of the metadata feature, please verify:
- The run dashboard has been updated to display the metadata in the sidebar panel.
- All triggering methods include the
metadataoption.You can use the following script to check for the dashboard update:
apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (4)
1-1: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
parsePacketfrom "@trigger.dev/core/v3", which is used later in the file for metadata parsing.
160-163: LGTM: Metadata parsing implemented correctly.The metadata parsing is implemented using the
parsePacketfunction, correctly handling potential undefined values and using the appropriate fields from thetaskRunobject. The asynchronous nature of the operation is properly managed with theawaitkeyword.
194-194: LGTM: Metadata correctly integrated into execution object.The parsed metadata is appropriately added to the
runproperty of theexecutionobject. This placement is logical and maintains the structure of theTaskRunExecutiontype.
Line range hint
1-194: Summary: Changes align well with PR objectives.The implemented changes successfully add metadata support to the task run creation process, as outlined in the PR objectives. The modifications are minimal and focused, including:
- Importing the necessary
parsePacketfunction.- Implementing metadata parsing using
parsePacket.- Integrating the parsed metadata into the
TaskRunExecutionobject.These changes enhance the functionality of task runs by allowing the attachment and retrieval of metadata, as intended. The implementation is clean and maintains the existing structure of the code.
packages/core/src/v3/schemas/common.ts (2)
Line range hint
1-290: Summary: Metadata feature implementation is well-executed.The changes in this file successfully implement the metadata feature for task runs as outlined in the PR objectives. The
metadataproperty is appropriately added to theTaskRunschema and consistently omitted in theTaskRunContext. The implementation allows for flexible, JSON-serializable metadata while maintaining consistency with existing patterns in the codebase.A minor suggestion for code readability was provided, but overall, the changes are well-implemented and ready for integration.
146-146: LGTM: Metadata property added as per PR objectives.The addition of the
metadataproperty to theTaskRunschema is well-implemented. It's defined as an optional record ofDeserializedJsonSchema, which aligns with the PR objective of allowing up to 4KB of JSON-serializable metadata to be attached to task runs.To ensure consistency, let's verify that this change is reflected in other relevant parts of the codebase:
docs/tasks/overview.mdx (1)
255-262: LGTM! Consider using template literals and adding metadata example.The implementation of the
onFailurefunction in thetrigger.config.tsfile looks good and aligns with the PR objective.For consistency with the previous suggestions, consider using template literals in the console.log statement:
export default defineConfig({ project: "proj_1234", onFailure: async (payload, error, { ctx }) => { - console.log("Task failed", ctx.task.id); + console.log(`Task failed ${ctx.task.id}`); }, });The PR objectives mention the ability to attach and access metadata for task runs. Could you provide an example of how to access and use this metadata within these lifecycle functions? This would help demonstrate the new functionality described in the PR.
To verify the implementation of metadata access, we can run the following script:
docs/runs/metadata.mdx (3)
1-33: LGTM: Clear introduction and usage examples.The introduction effectively explains the concept of run metadata, its size limitations, and potential use cases. The usage examples demonstrate how to add metadata when triggering a task and how to access it within the run function, providing a solid foundation for users to understand the basic functionality.
🧰 Tools
LanguageTool
[misspelling] ~7-~7: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e additional, structured information on an run. For example, you could store your ...(EN_A_VS_AN)
35-122: LGTM: Comprehensive explanation of metadata update methods.This section effectively covers various methods for updating metadata during a run, including
metadata.set(),metadata.update(), andmetadata.del(). The examples are clear and illustrative. The emphasis on these methods only working within the context of a run is crucial for preventing misuse. The note about the behavior ofmetadata.current()andmetadata.get()outside the run function is particularly helpful for developers.🧰 Tools
LanguageTool
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
1-273: Excellent documentation on run metadata.This documentation file provides a comprehensive and well-structured guide to using run metadata in the Trigger.dev SDK. It covers all essential aspects, including attaching, accessing, updating, and inspecting metadata, with clear explanations and helpful code examples. The inclusion of type safety recommendations and information on size limits adds valuable depth to the documentation.
The minor suggestions provided in the review comments are aimed at enhancing clarity and providing additional context in specific areas. Overall, this is a high-quality documentation file that will be very helpful for users working with run metadata in Trigger.dev.
🧰 Tools
LanguageTool
[misspelling] ~7-~7: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e additional, structured information on an run. For example, you could store your ...(EN_A_VS_AN)
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx (1)
Line range hint
1-284: Overall assessment: Changes align with PR objectivesThe modifications to the
Pagecomponent effectively enhance the task run functionality by optimizing the loading state determination. This improvement aligns well with the PR objectives of allowing metadata attachment and improving the usability of task runs.The changes are focused and don't introduce any apparent issues. They contribute to a better user experience, especially when dealing with different environments.
packages/core/src/v3/workers/taskExecutor.ts (2)
6-6: LGTM: New import forrunMetadataThe addition of the
runMetadataimport aligns with the PR objective of implementing metadata functionality for task runs. Grouping it with the existingTriggerConfigimport from the same file is a good practice for maintaining clean and organized imports.
Line range hint
1-577: Overall assessment: Changes successfully implement metadata functionalityThe modifications to
taskExecutor.tseffectively implement the metadata functionality for task runs as outlined in the PR objectives. The changes are minimal, focused, and integrate well with the existing code structure. They add the ability to attach and access metadata during task execution without disrupting the current logic.Key points:
- The new import for
runMetadatais appropriately placed.- The conditional block for handling metadata is well-positioned within the
executemethod.- The implementation is consistent with the PR objectives.
Minor suggestions have been made to improve code readability and error handling, which would further enhance the robustness of the implementation.
packages/core/src/v3/apiClient/index.ts (3)
25-26: LGTM: New types imported for metadata update functionalityThe addition of
UpdateMetadataRequestBodyandUpdateMetadataResponseBodytypes is consistent with the PR objectives and follows the existing import pattern.
503-518: LGTM: Well-implementedupdateRunMetadatamethodThe
updateRunMetadatamethod is well-implemented and consistent with other methods in theApiClientclass. It correctly uses the provided types, allows for custom request options, and adheres to the PR objectives by providing a way to update run metadata.
25-26: Summary: Excellent implementation of run metadata update functionalityThe changes in this file successfully implement the ability to update run metadata, which aligns perfectly with the PR objectives. The new imports and the
updateRunMetadatamethod are well-integrated into the existingApiClientclass, following established patterns and best practices. This implementation provides a solid foundation for attaching and modifying JSON-serializable metadata for task runs.Also applies to: 503-518
packages/core/src/v3/schemas/api.ts (4)
5-5: LGTM: Import statement for JSON schemas added.The new import statement for
DeserializedJsonSchemaandSerializableJsonSchemais correctly added and necessary for the metadata-related schemas introduced in this PR.
511-511: LGTM: Metadata field added to CommonRunFields.The addition of the optional
metadatafield as a record of any type is consistent with the PR objective and provides a good balance between structure and flexibility.
616-621: LGTM: UpdateMetadataRequestBody schema added.The new
UpdateMetadataRequestBodyschema is well-structured and consistent with the PR objective. It provides good type safety by usingDeserializedJsonSchemafor the metadata field and includes an optionalmetadataTypefor flexibility.
Line range hint
1-627: Overall assessment: Metadata support successfully implemented.The changes in this file effectively implement metadata support as per the PR objectives. The new schemas and modifications to existing ones are well-structured and consistent. A few minor suggestions have been made to improve type safety and consistency, but overall, the implementation is solid and ready for integration.
apps/webapp/app/v3/services/triggerTask.server.ts (2)
23-23: LGTM: Import ofhandleMetadataPacketThe import of
handleMetadataPacketfrom~/utils/packetsis correctly placed and follows the existing import structure.
Line range hint
1-686: Overall assessment: Metadata handling implementation is well-integratedThe changes to implement metadata handling in the
TriggerTaskServiceare well-designed and correctly integrated into the existing code. The implementation follows good practices such as:
- Proper import of the new utility function
- Careful handling of optional fields
- Consistent use of TypeScript features like optional chaining
The metadata is processed and stored appropriately, fulfilling the PR objective of allowing the attachment of metadata when triggering a task.
To ensure the completeness of the implementation, please verify that the
handleMetadataPacketfunction in~/utils/packetscorrectly processes the metadata and handles potential errors. Also, confirm that the database schema has been updated to include the newmetadataandmetadataTypefields for the TaskRun table.✅ Verification successful
Verification Successful: Metadata Handling is Properly Implemented
The
handleMetadataPacketfunction correctly processes metadata and includes necessary error handling for oversized metadata. Additionally, the database schema has been appropriately updated to include themetadataandmetadataTypefields in theTaskRuntable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of handleMetadataPacket and database schema updates # Check the implementation of handleMetadataPacket echo "Checking handleMetadataPacket implementation:" cat $(fd --type f --full-path "utils/packets" | grep -v "test") | sed 's/^/ /' # Check for database schema updates echo "Checking for database schema updates:" fd --type f --full-path "prisma" | xargs grep -n "metadata" | sed 's/^/ /'Length of output: 9871
docs/triggering.mdx (2)
223-223: Improved readability with added blank line.The addition of a blank line after the code block enhances the overall readability of the documentation.
Line range hint
1-744: Overall documentation improvements.The additions to the triggering documentation, including sections on tags and metadata, align well with the PR objectives. These changes provide users with quick access to information about new features while maintaining the document's readability. The suggested minor improvements to add brief descriptions will further enhance the usefulness of these new sections.
packages/trigger-sdk/src/v3/shared.ts (2)
801-801: LGTM! Metadata correctly incorporated.The
metadataproperty has been properly added to the options passed toapiClient.triggerTask. This change is consistent with the modifications made to theTaskRunOptionstype and correctly implements the new feature.
Line range hint
1-1191: Overall implementation of metadata feature is well-executed.The addition of the
metadataproperty toTaskRunOptionsand its incorporation into various internal functions has been implemented consistently and correctly. These changes align well with the PR objectives of allowing the attachment of up to 4KB of JSON-serializable metadata when triggering a task.Key points:
- The
TaskRunOptionstype has been updated with a well-definedmetadataproperty.- The
metadataproperty is correctly passed to the API client intrigger_internal.- Batch operations (
batchTrigger_internalandbatchTriggerAndWait_internal) properly handle metadata for each item.The implementation maintains type safety and is consistent with the existing codebase structure. The minor suggestions for improvement (adding an example in the comment and consistent formatting) would further enhance the code quality and developer experience.
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (2)
424-434: New "Metadata" tab added to the run details view.The implementation looks good. The new tab is consistently implemented with the existing tabs, including the use of a shortcut key.
Line range hint
1-1002: Summary: Metadata tab successfully implementedThe changes in this file effectively implement a new "Metadata" tab in the run details view. The implementation is consistent with the existing code structure and follows good practices. Here's a summary of the changes:
- A new TabButton for "Metadata" has been added to the tab navigation.
- The RunBody component has been updated to handle the "metadata" tab selection.
- When the "Metadata" tab is active, it displays the run metadata using a CodeBlock component if metadata exists, or a Callout component with a link to documentation if no metadata is present.
These changes enhance the functionality of the run details view by providing users with easy access to run metadata, which aligns well with the PR objectives.
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (2)
12-12: New import added for metadata parsingThe
parsePacketfunction has been imported from "@trigger.dev/core/v3". This aligns with the PR objective of enhancing task runs with metadata functionality.
12-12: Changes align well with PR objectivesThe implemented changes successfully add metadata functionality to task runs, as outlined in the PR objectives. The addition of the
parsePacketimport and its usage in thegetExecutionPayloadFromAttemptmethod effectively processes and includes metadata in the execution payload.The changes are minimal and focused, integrating well with the existing code structure. This implementation enhances the task run functionality by allowing the attachment and retrieval of metadata, which can be valuable for various use cases in the system.
Also applies to: 1037-1041, 1069-1069
docs/v3-openapi.yaml (1)
1900-1903: LGTM: Metadata property added successfully.The
metadataproperty has been added to theCommonRunObjectschema as requested in the PR objectives. The implementation looks correct and consistent with the rest of the schema. A few observations:
- The property is correctly typed as an object.
- The description is clear and includes a reference to additional documentation.
- An example is provided, which is helpful for API users.
This addition enhances the API by allowing the attachment of JSON-serializable metadata to task runs, as specified in the PR objectives.
references/v3-catalog/src/trigger/runMetadata.ts (2)
46-51: Ensure correct usage of lifecycle hooksThe
onStartandonSuccesshooks are defined without parameters and only log the current metadata. Verify that this aligns with the intended functionality. If additional context or parameters are needed, adjust the function signatures accordingly.
8-8:⚠️ Potential issuePotential serialization issue with
Dateobject in metadataThe
datefield in the metadata is set tonew Date(). JavaScriptDateobjects may not serialize to JSON as expected, potentially causing issues when the metadata is transmitted or stored. Consider converting the date to an ISO string.Apply the following change:
hello: "world", - date: new Date(), + date: new Date().toISOString(), anotherThing: {Likely invalid or redundant comment.
packages/core/src/v3/runMetadata/index.ts (1)
84-88: Check for the presence oftaskContext.ctxbefore accessingrun.idIn the
updatemethod, you're accessingtaskContext.ctx?.run.id. IftaskContext.ctxcan beundefined, ensure that this is the desired behavior and that it won't cause unexpected issues downstream.Run the following script to confirm that
taskContext.ctxis properly initialized before use:packages/trigger-sdk/src/v3/metadata.ts (9)
1-9: Import statements are correctly definedAll necessary modules and types are properly imported, ensuring that dependencies are correctly managed.
11-20: Comprehensive documentation for themetadatanamespaceThe JSDoc comments accurately and thoroughly describe the
metadatanamespace and its associated functions, which enhances maintainability and developer understanding.
21-28: Exporting themetadataobject with all intended methodsThe
metadataobject is correctly exported with all its methods (current,get,set,del,update), making it readily accessible for external modules.
29-30: Appropriate type definition forRunMetadataThe
RunMetadatatype is correctly defined as aRecord<string, DeserializedJson>, ensuring type safety when handling metadata objects.
31-43: Correct implementation ofcurrentMetadatafunctionThe
currentMetadatafunction properly retrieves the current run's metadata by delegating torunMetadata.current().
45-58: Accurate retrieval ingetMetadataKeyfunctionThe
getMetadataKeyfunction effectively accesses a specific key in the metadata, ensuring that it returns the correct value orundefinedwhen necessary.
72-99: Proper setting of metadata insetMetadataKeyfunctionThe
setMetadataKeyfunction correctly updates or adds a key-value pair in the run's metadata. The request options are appropriately merged, and the attributes are accurately flattened for logging purposes.
101-134: Effective deletion indeleteMetadataKeyfunctionThe
deleteMetadataKeyfunction successfully removes a specified key from the metadata. The function correctly handles request options and attributes.
136-164: Successful replacement inupdateMetadatafunctionThe
updateMetadatafunction replaces the entire metadata object as intended. It correctly handles the merging of request options and flattening of attributes.apps/webapp/app/presenters/v3/SpanPresenter.server.ts (4)
1-6: Imports updated to support metadata processingThe added imports
parsePacket,prettyPrintPacket, andTaskRunErrorare necessary for handling metadata and error processing.
121-122: Includemetadatafields in database queryAdding
metadataandmetadataTypeto the select clause ensures these fields are retrieved from the database for further processing.
195-198: Process and formatmetadatafor outputThe logic correctly processes
run.metadatausingprettyPrintPacket, ensuring metadata is formatted appropriately for inclusion in the response.
286-286: Includemetadatain the returned objectIncluding
metadatain the returned object allows the frontend to access and display the metadata as intended.apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (1)
163-163: Ensure all calls tocreateCommonRunStructureare awaitedSince
createCommonRunStructureis now asynchronous, it's important toawaitits result in all calls to prevent unresolved promises and ensure correct execution flow. It appears that within this file, all calls have been appropriately updated.Also applies to: 195-196, 198-199, 201-202
| @@ -0,0 +1,5 @@ | |||
| // Split module-level variable definition into separate files to allow | |||
| // tree-shaking on each api instance. | |||
| import { RunMetadataAPI } from "./runMetadata/index.js"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Incorrect import path for RunMetadataAPI.
The file packages/core/src/v3/runMetadata/index.js does not exist, and RunMetadataAPI is not found exported in the codebase. Please verify and update the import path accordingly.
🔗 Analysis chain
Verify the correct path for the imported module.
The import statement looks correct, but it's crucial to ensure that the ./runMetadata/index.js file exists and contains the RunMetadataAPI class.
Let's verify the existence and content of the imported file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the imported file
if [ -f "packages/core/src/v3/runMetadata/index.js" ]; then
echo "File exists. Checking its content:"
# Check if the file exports RunMetadataAPI
grep -n "export.*RunMetadataAPI" packages/core/src/v3/runMetadata/index.js
else
echo "Error: File 'packages/core/src/v3/runMetadata/index.js' does not exist."
fi
Length of output: 213
Script:
#!/bin/bash
# Search for the export of RunMetadataAPI across the codebase
ast-grep --lang javascript --pattern $'export $_ RunMetadataAPI'
Length of output: 512
Script:
#!/bin/bash
# Search for exports of RunMetadataAPI with corrected ast-grep pattern
ast-grep --lang javascript --pattern $'export { RunMetadataAPI }'
Length of output: 66
docs/runs/metadata.mdx
Outdated
| description: "Attach a small amount of data to a run and update it as the run progresses." | ||
| --- | ||
|
|
||
| You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on an run. For example, you could store your user’s full name and corresponding unique identifier from your system on every task that is associated with that user. |
There was a problem hiding this comment.
Minor grammatical correction needed.
There's a small grammatical issue in this sentence. Replace "an run" with "a run" for correct usage of the indefinite article.
Apply this change:
-You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on an run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user.
+You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on an run. For example, you could store your user’s full name and corresponding unique identifier from your system on every task that is associated with that user. | |
| You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user. |
🧰 Tools
LanguageTool
[misspelling] ~7-~7: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e additional, structured information on an run. For example, you could store your ...(EN_A_VS_AN)
| ) : tab === "metadata" ? ( | ||
| <div className="flex flex-col gap-4 py-3"> | ||
| {run.metadata ? ( | ||
| <CodeBlock code={run.metadata} showLineNumbers={false} /> | ||
| ) : ( | ||
| <Callout to="https://trigger.dev/docs/runs/metadata" variant="docs"> | ||
| No metadata set for this run. View our metadata documentation to learn more. | ||
| </Callout> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Metadata display implementation looks good.
The implementation handles both cases (with and without metadata) effectively. The use of CodeBlock for displaying metadata and Callout for providing guidance when no metadata is present is a good approach.
A minor suggestion for improvement:
Consider adding a loading state for the metadata display. This could be useful if fetching metadata is an asynchronous operation. Here's a possible implementation:
) : tab === "metadata" ? (
<div className="flex flex-col gap-4 py-3">
- {run.metadata ? (
+ {run.metadata === undefined ? (
+ <Spinner />
+ ) : run.metadata ? (
<CodeBlock code={run.metadata} showLineNumbers={false} />
) : (
<Callout to="https://trigger.dev/docs/runs/metadata" variant="docs">
No metadata set for this run. View our metadata documentation to learn more.
</Callout>
)}
</div>
) : (This change assumes that run.metadata could be undefined while loading. Adjust according to your actual data fetching logic if needed.
Committable suggestion was skipped due to low confidence.
| await metadata.update({ | ||
| there: { | ||
| is: { | ||
| something: "here", | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deeply nested metadata structure
The metadata update introduces a deeply nested object. While nested structures are acceptable, they can become difficult to manage and may impact performance if they grow large. Consider flattening the metadata structure if possible.
| async function createCommonRunStructure(run: CommonRelatedRun) { | ||
| const metadata = await parsePacket({ | ||
| data: run.metadata ?? undefined, | ||
| dataType: run.metadataType, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add error handling for parsePacket in createCommonRunStructure
If parsePacket throws an error while parsing metadata, it could result in an unhandled exception, potentially affecting the API response. Consider adding error handling to gracefully manage any parsing errors and provide a default value if necessary.
Here’s a suggested modification:
async function createCommonRunStructure(run: CommonRelatedRun) {
+ let metadata;
+ try {
metadata = await parsePacket({
data: run.metadata ?? undefined,
dataType: run.metadataType,
});
+ } catch (error) {
+ // Optionally log the error
+ metadata = undefined; // or assign a default value
+ }
return {
// ... other properties
metadata,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function createCommonRunStructure(run: CommonRelatedRun) { | |
| const metadata = await parsePacket({ | |
| data: run.metadata ?? undefined, | |
| dataType: run.metadataType, | |
| }); | |
| async function createCommonRunStructure(run: CommonRelatedRun) { | |
| let metadata; | |
| try { | |
| metadata = await parsePacket({ | |
| data: run.metadata ?? undefined, | |
| dataType: run.metadataType, | |
| }); | |
| } catch (error) { | |
| // Optionally log the error | |
| metadata = undefined; // or assign a default value | |
| } | |
...routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
docs/runs/metadata.mdx (6)
7-7: Minor grammatical correction needed.There's a small grammatical issue in this sentence. Replace "on a run" with "to a run" for better clarity and consistency with the previous sentence.
Apply this change:
-You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user. +You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information to a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user.
55-72: Clarify the difference betweenmetadata.get()andmetadata.current().The documentation shows two methods to retrieve metadata:
metadata.get()andmetadata.current(). However, it's not clear what the difference is between these two methods. Consider adding a brief explanation of when to use each method and how they differ.For example:
- Does
metadata.get()retrieve a specific key whilemetadata.current()retrieves the entire metadata object?- Are there any performance implications of using one over the other?
Providing this clarity will help developers choose the appropriate method for their use case.
129-161: Consider adding information about hook execution order.The examples demonstrate how to use metadata in task lifecycle hooks, both at the task level and globally. However, it would be beneficial to include information about the order in which these hooks are executed. For example:
- Are global hooks executed before or after task-specific hooks?
- What is the execution order of
onStart,run, andonSuccess?Adding this information would help developers understand how metadata changes propagate through the lifecycle of a task.
163-177: Enhance the metadata propagation section with use cases and best practices.The section clearly explains that metadata is not automatically propagated to child tasks and provides an example of manual propagation. To make this section more valuable, consider adding:
- Common use cases for propagating metadata to child tasks.
- Best practices for deciding what metadata to propagate.
- Potential pitfalls or considerations when propagating large amounts of metadata.
This additional information would help developers make informed decisions about metadata propagation in their task hierarchies.
216-247: Consider mentioning alternative type validation libraries.The section recommends using Zod for type-safe metadata handling, which is a great suggestion. To make this section more comprehensive, consider:
- Mentioning other popular type validation libraries (e.g., Joi, Yup) as alternatives to Zod.
- Briefly explaining why Zod is the recommended choice (if it is).
- Providing a link to the Zod documentation for users who want to learn more.
This addition would give developers a broader perspective on type validation options while still emphasizing your recommended approach.
271-273: Add a note about performance considerations for increased metadata size.The section mentions that self-hosted instances can increase the metadata size limit. To provide a more complete picture, consider adding a note about potential performance implications of increasing this limit. For example:
- How might larger metadata objects impact storage requirements?
- Are there any potential effects on API response times when dealing with larger metadata?
- Are there any best practices for managing larger metadata objects efficiently?
This additional information would help users make informed decisions when considering increasing the metadata size limit.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx (1)
Line range hint
1-563: Summary of changes and recommendationsThe changes to this file significantly enhance the StandardTaskForm component by introducing a tabbed interface for separate payload and metadata editing. This improvement allows for better organization and management of task data. The implementation is generally well-structured and functional.
However, there are a few areas for improvement:
- Optimize rendering of JSONEditor components.
- Handle potential undefined values in editor content comparisons.
- Fix invalid HTML attributes on JSONEditor components.
- Improve autoFocus behavior for JSONEditor components.
- Update the dependency array in the submitForm useCallback hook.
Addressing these points will further improve the component's performance, reliability, and user experience. Overall, the changes represent a positive step forward in the functionality of the task form.
apps/webapp/app/v3/services/finalizeTaskRun.server.ts (2)
63-64: Simplify Variable InitializationThe variables
outputandoutputTypeare initialized toundefined, which is the default value for uninitialized variables in TypeScript. You can simplify the declarations by removing the explicit assignment:-let output: string | undefined = undefined; -let outputType: string | undefined = undefined; +let output: string | undefined; +let outputType: string | undefined;
78-79: Redundant Nullish Coalescing OperatorIn line 78, using the nullish coalescing operator with
undefinedis unnecessary sinceattempt.outputis of typestring | null, andoutputexpects astring | undefined. Assigningattempt.outputdirectly suffices:-output = attempt.output ?? undefined; +output = attempt.output;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx (6 hunks)
- apps/webapp/app/v3/services/finalizeTaskRun.server.ts (1 hunks)
- docs/runs/metadata.mdx (1 hunks)
- references/v3-catalog/src/trigger/runMetadata.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- references/v3-catalog/src/trigger/runMetadata.ts
🧰 Additional context used
LanguageTool
docs/runs/metadata.mdx
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
🔇 Additional comments not posted (11)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx (9)
30-30: LGTM: New imports and hook usage for tabbed interfaceThe addition of TabButton and TabContainer imports, along with the useSearchParams hook, provides the necessary components and state management for implementing a tabbed interface. This change enhances the user experience by allowing easy switching between payload and metadata editors.
Also applies to: 34-34, 135-137
144-164: LGTM: Extended state management and form submission for metadataThe changes appropriately extend the existing state management to include metadata, with new state variables and setters. The form submission logic has been correctly updated to incorporate the new metadata state in the payload. These modifications enable the handling of metadata alongside the existing payload data.
Also applies to: 171-172
206-226: LGTM: Tabbed interface implementationThe implementation of the tabbed interface using TabContainer and TabButton components, along with the useSearchParams hook for state management, is well-structured and functional. This allows users to switch between payload and metadata editors seamlessly.
281-284: LGTM: Handling metadata in RecentPayloads componentThe update to the
onSelectedcallback in the RecentPayloads component now correctly handles setting metadata when available. This change ensures that both payload and metadata are consistently updated when a user selects a recent payload, providing a more complete and accurate representation of the selected run.
183-183:⚠️ Potential issueIncorrect dependency array in useCallback hook
The useCallback hook for submitForm includes
currentPayloadJsonandcurrentMetadataJsonin its dependency array. However, these are ref objects created with useRef, and changes to their .current property don't cause re-renders or affect the identity of the ref itself. Including them in the dependency array doesn't have the intended effect.Consider removing these dependencies from the array:
- [currentPayloadJson, currentMetadataJson] + []This change will ensure that the submitForm callback is only recreated when necessary, improving performance.
227-272: 🛠️ Refactor suggestionOptimize rendering by conditionally rendering JSONEditor components
Currently, both JSONEditor components are always rendered, with one hidden using the
hiddenclass. This approach can be inefficient, especially if JSONEditor is a heavy component. Consider conditionally rendering the editors to improve performance.Here's how you can modify the code:
<div> {(!tab || tab === "payload") && ( <JSONEditor // ...payload editor props /> )} {tab === "metadata" && ( <JSONEditor // ...metadata editor props /> )} </div>This change will ensure that only the active editor is rendered, potentially improving performance.
237-240:⚠️ Potential issueHandle potential undefined values when comparing editor content
When comparing the current editor value
vwithselectedCodeSamplePayloadorselectedCodeSampleMetadata, there's a possibility that these selected sample values areundefined. This can lead to unintended behavior where the example is deselected even if the content hasn't changed.Consider providing default values to handle potential
undefinedvalues:- if (v !== selectedCodeSamplePayload) { + if (v !== (selectedCodeSamplePayload ?? startingJson)) {And for the metadata editor:
- if (v !== selectedCodeSampleMetadata) { + if (v !== (selectedCodeSampleMetadata ?? "{}")) {These changes will ensure that the comparison is always made against a valid string, preventing unintended deselection of examples.
Also applies to: 259-262
243-245:⚠️ Potential issueInvalid attributes
min-heightandmax-heighton JSONEditor componentsThe attributes
min-heightandmax-heightare not valid HTML attributes and will have no effect. If you intend to set these styles, you should use thestyleprop or include them in your CSS classes.Apply this diff to fix the issue:
- min-height="100%" - max-height="100%" + style={{ minHeight: "100%", maxHeight: "100%" }}Make sure to apply this change to both
JSONEditorinstances.Also applies to: 265-267
246-246:⚠️ Potential issueAvoid setting
autoFocuson both JSONEditor componentsBoth
JSONEditorcomponents have theautoFocusprop set. This can cause unexpected behavior because both editors may attempt to focus simultaneously, even though one is hidden. It's better to conditionally setautoFocusonly on the visible editor.Apply this diff to conditionally set
autoFocus:For the payload editor:
- autoFocus + autoFocus={!tab || tab === "payload"}For the metadata editor:
- autoFocus + autoFocus={tab === "metadata"}This change ensures that only the visible editor receives focus when the component mounts or updates.
Also applies to: 268-268
apps/webapp/app/v3/services/finalizeTaskRun.server.ts (2)
68-75: Handle Potential Absence of Completed AttemptsWhen querying for the latest completed attempt with a non-null output, there might be cases where no such attempt exists. Ensure that the rest of the code handles
outputandoutputTypebeingundefinedappropriately when updating the task run.
63-85: Overall Code Integration Looks GoodThe added logic correctly retrieves and assigns the
outputandoutputTypefrom the most recent completed task run attempt when the task run completes successfully. This enhancement aligns with the PR objectives by ensuring that metadata is appropriately captured and stored.
- Add metadata to testing - Make using metadata outside of runs a no-op - Add docs
ericallam
force-pushed
the
feat/run-metadata
branch
from
c26b6f9 to
5832de8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
packages/trigger-sdk/src/v3/tags.ts (1)
17-17: Approve the simplified API client retrieval with a minor suggestion.The change to use
apiClientManager.clientOrThrow()is a good improvement. It simplifies the code by encapsulating the null check and error throwing into a single method call. This makes the code more concise and potentially more maintainable.For consistency with the rest of the codebase, consider renaming the variable from
apiClienttoclient:- const apiClient = apiClientManager.clientOrThrow(); + const client = apiClientManager.clientOrThrow();This would maintain consistency with other parts of the code that might still be using
clientas the variable name.apps/webapp/app/v3/taskStatus.ts (1)
56-58: LGTM! Consider adding a helper function for consistency.The new
FAILED_ATTEMPT_STATUSESconstant and its corresponding type are well-defined and consistent with the existing patterns in the file. They provide a clear categorization for failed attempt statuses and allow for type-safe usage.For consistency with other status categories in this file, consider adding a helper function to check if a status is a failed attempt status. Here's a suggested implementation:
export function isFailedAttemptStatus(status: TaskRunAttemptStatus): boolean { return FAILED_ATTEMPT_STATUSES.includes(status); }This addition would align with the pattern established for other status categories and provide a convenient way to check for failed attempt statuses.
apps/webapp/app/v3/services/finalizeTaskRun.server.ts (2)
69-71: LGTM: Error finalization added for failed runs.The addition of error finalization for failed runs is a good improvement. It ensures that run errors are properly handled and stored.
Consider adding a debug log statement before calling
finalizeRunErrorto improve traceability:if (isFailedRunStatus(run.status)) { + logger.debug("Finalizing run error", { runId: run.id, status: run.status }); await this.finalizeRunError(run, error); }
91-102: LGTM: New method for finalizing run errors.The
finalizeRunErrormethod is a good addition for handling run errors. It properly checks for the existence of an error and sanitizes it before storage.Consider adding a debug log statement after updating the TaskRun to improve traceability:
async finalizeRunError(run: TaskRun, error?: TaskRunError) { if (!error) { return; } - await this._prisma.taskRun.update({ + const updatedRun = await this._prisma.taskRun.update({ where: { id: run.id }, data: { error: sanitizeError(error), }, }); + logger.debug("Finalized run error", { runId: run.id, error: updatedRun.error }); }packages/trigger-sdk/src/v3/runs.ts (1)
Line range hint
157-184: LGTM: Improved error handling and URL resolutionThe changes improve the function in two ways:
- Using
apiClientManager.clientOrThrow()simplifies error handling and makes it consistent with other functions.- The addition of
resolvePayloadAndOutputUrlsenhances the handling of payload and output URLs.These changes improve the function's robustness and functionality.
Consider adding a comment explaining the purpose of
resolvePayloadAndOutputUrlsfor better code documentation.docs/runs/metadata.mdx (4)
7-7: Minor grammatical correction needed.There's a small grammatical issue in this sentence. Replace "on a run" with "to a run" for better readability.
Apply this change:
-You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information on a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user. +You can attach up to 4KB (4,096 bytes) of metadata to a run, which you can then access from inside the run function, via the API, and in the dashboard. You can use metadata to store additional, structured information to a run. For example, you could store your user's full name and corresponding unique identifier from your system on every task that is associated with that user.
74-127: LGTM! Consider a minor style improvement.This section clearly explains how to manipulate metadata and the behavior of metadata methods. The examples are accurate and helpful.
As a minor style suggestion, consider rephrasing line 111 for conciseness:
-If you call any of the metadata methods outside of the run function, they will have no effect: +If you call any of the metadata methods outside the run function, they will have no effect:This change removes the redundant "of" while maintaining the meaning.
🧰 Tools
LanguageTool
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
179-247: LGTM! Comprehensive guide on type-safe metadata.This section effectively explains the current loose typing of metadata APIs and provides clear examples of valid and invalid metadata usage. The recommendation to use Zod for type-safe metadata handling is valuable, and the implementation example is comprehensive.
Consider adding a brief note about error handling when using Zod's
parsemethod. For example:function getMetadata() { try { return Metadata.parse(metadata.current()); } catch (error) { console.error("Invalid metadata:", error); // Handle the error or return a default value } }This addition would help users understand how to handle potential parsing errors in a production environment.
249-273: LGTM! Comprehensive coverage of metadata inspection and size limits.This section effectively explains how to view metadata in the dashboard and retrieve it via API. The information about the default 4KB size limit and instructions for increasing it in self-hosted instances is valuable.
Consider adding a note about the potential performance implications of increasing the metadata size limit, such as:
"Note: Increasing the metadata size limit may impact performance and storage requirements. Carefully consider your use case and system capabilities before adjusting this limit."
This addition would help users make informed decisions about modifying the size limit.
packages/trigger-sdk/src/v3/shared.ts (1)
500-504: LGTM! Consider adding an example to the comment.The addition of the
metadataproperty toTaskRunOptionsis a valuable enhancement. It allows users to attach additional context to task runs, which can be useful for various purposes.Consider adding a brief example to the comment to illustrate how the metadata can be used. For instance:
/** * Metadata to attach to the run. Metadata can be used to store additional information about the run. Limited to 4KB. * @example * { * userId: "123", * source: "api", * version: "1.0.0" * } */ metadata?: Record<string, SerializableJson>;docs/v3-openapi.yaml (2)
363-441: Consider adding rate limiting information.While the endpoint is well-defined, it might be beneficial to include information about rate limiting for this endpoint, if applicable. This could help users understand any constraints on how frequently they can update metadata.
Consider adding a note about rate limiting, for example:
x-rateLimit: limit: 100 period: 60 description: Maximum of 100 requests per minute for updating run metadata.
1980-1983: Consider adding size limit information for metadata.While the metadata property is well-defined, it might be helpful to include information about any size limits for the metadata object. This could prevent issues with excessively large metadata payloads.
Consider adding a note about size limits, for example:
metadata: type: object description: The metadata of the run. See [Metadata](/runs/metadata) for more information. Maximum size is 4KB. example: { "foo": "bar" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
docs/images/run-metadata.pngis excluded by!**/*.png
📒 Files selected for processing (47)
- .changeset/tasty-rats-rhyme.md (1 hunks)
- apps/webapp/app/components/code/JSONEditor.tsx (1 hunks)
- apps/webapp/app/components/primitives/Tabs.tsx (1 hunks)
- apps/webapp/app/env.server.ts (1 hunks)
- apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (5 hunks)
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts (4 hunks)
- apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts (4 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx (6 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx (1 hunks)
- apps/webapp/app/routes/api.v1.runs.$runId.metadata.ts (1 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (2 hunks)
- apps/webapp/app/routes/resources.runs.$runParam.ts (1 hunks)
- apps/webapp/app/utils/packets.ts (1 hunks)
- apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (3 hunks)
- apps/webapp/app/v3/services/completeAttempt.server.ts (2 hunks)
- apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (3 hunks)
- apps/webapp/app/v3/services/finalizeTaskRun.server.ts (3 hunks)
- apps/webapp/app/v3/services/replayTaskRun.server.ts (2 hunks)
- apps/webapp/app/v3/services/testTask.server.ts (1 hunks)
- apps/webapp/app/v3/services/triggerTask.server.ts (3 hunks)
- apps/webapp/app/v3/taskStatus.ts (1 hunks)
- apps/webapp/app/v3/testTask.ts (1 hunks)
- docs/management/runs/update-metadata.mdx (1 hunks)
- docs/mint.json (9 hunks)
- docs/runs/metadata.mdx (1 hunks)
- docs/tasks/overview.mdx (3 hunks)
- docs/triggering.mdx (2 hunks)
- docs/v3-openapi.yaml (2 hunks)
- packages/core/src/v3/apiClient/index.ts (2 hunks)
- packages/core/src/v3/apiClientManager/index.ts (2 hunks)
- packages/core/src/v3/index.ts (1 hunks)
- packages/core/src/v3/run-metadata-api.ts (1 hunks)
- packages/core/src/v3/runMetadata/index.ts (1 hunks)
- packages/core/src/v3/schemas/api.ts (4 hunks)
- packages/core/src/v3/schemas/common.ts (3 hunks)
- packages/core/src/v3/workers/taskExecutor.ts (2 hunks)
- packages/database/prisma/migrations/20240925092304_add_metadata_and_output_to_task_run/migration.sql (1 hunks)
- packages/database/prisma/migrations/20240925205409_add_error_to_task_run/migration.sql (1 hunks)
- packages/database/prisma/schema.prisma (1 hunks)
- packages/trigger-sdk/src/v3/envvars.ts (6 hunks)
- packages/trigger-sdk/src/v3/index.ts (1 hunks)
- packages/trigger-sdk/src/v3/metadata.ts (1 hunks)
- packages/trigger-sdk/src/v3/runs.ts (6 hunks)
- packages/trigger-sdk/src/v3/schedules/index.ts (8 hunks)
- packages/trigger-sdk/src/v3/shared.ts (10 hunks)
- packages/trigger-sdk/src/v3/tags.ts (1 hunks)
- references/v3-catalog/src/trigger/runMetadata.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/mint.json
🚧 Files skipped from review as they are similar to previous changes (33)
- .changeset/tasty-rats-rhyme.md
- apps/webapp/app/components/code/JSONEditor.tsx
- apps/webapp/app/components/primitives/Tabs.tsx
- apps/webapp/app/env.server.ts
- apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts
- apps/webapp/app/presenters/v3/TestTaskPresenter.server.ts
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test.tasks.$taskParam/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx
- apps/webapp/app/routes/api.v1.runs.$runId.metadata.ts
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx
- apps/webapp/app/routes/resources.runs.$runParam.ts
- apps/webapp/app/utils/packets.ts
- apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts
- apps/webapp/app/v3/services/createTaskRunAttempt.server.ts
- apps/webapp/app/v3/services/replayTaskRun.server.ts
- apps/webapp/app/v3/services/testTask.server.ts
- apps/webapp/app/v3/services/triggerTask.server.ts
- apps/webapp/app/v3/testTask.ts
- docs/tasks/overview.mdx
- docs/triggering.mdx
- packages/core/src/v3/apiClient/index.ts
- packages/core/src/v3/apiClientManager/index.ts
- packages/core/src/v3/index.ts
- packages/core/src/v3/run-metadata-api.ts
- packages/core/src/v3/schemas/api.ts
- packages/core/src/v3/schemas/common.ts
- packages/core/src/v3/workers/taskExecutor.ts
- packages/database/prisma/migrations/20240925092304_add_metadata_and_output_to_task_run/migration.sql
- packages/database/prisma/schema.prisma
- packages/trigger-sdk/src/v3/index.ts
- packages/trigger-sdk/src/v3/metadata.ts
- references/v3-catalog/src/trigger/runMetadata.ts
🧰 Additional context used
LanguageTool
docs/runs/metadata.mdx
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...If you call any of the metadata methods outside of the run function, they will have no eff...(OUTSIDE_OF)
Biome
packages/core/src/v3/runMetadata/index.ts
[error] 13-13: 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] 14-14: 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] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments not posted (35)
packages/database/prisma/migrations/20240925205409_add_error_to_task_run/migration.sql (1)
1-2: LGTM: Addition of error column enhances error tracking capabilities.The addition of the
errorcolumn of type JSONB to the "TaskRun" table is a good improvement. This change allows for storing structured error information for each task run, which can greatly enhance error tracking and debugging capabilities. Using the JSONB type provides flexibility in storing various error structures, aligning with best practices for error handling and logging in database systems.packages/trigger-sdk/src/v3/tags.ts (1)
Line range hint
1-62: Overall impact of the change is positive and minimal.The modification to use
apiClientManager.clientOrThrow()is a good refactoring that simplifies the code without changing its core functionality. This change:
- Encapsulates the API client retrieval and error handling in a single method call.
- Removes the need for an explicit null check and separate error throwing.
- Correctly removes the unused import of
apiClientMissingError.The rest of the function remains unchanged, preserving the existing logic for adding tags. This change improves code maintainability without introducing any apparent risks or alterations to the function's behavior.
apps/webapp/app/v3/services/finalizeTaskRun.server.ts (3)
5-6: LGTM: Import statements reorganized and updated.The import statements have been reorganized, and
isFailedRunStatushas been added to the import fromtaskStatus. These changes align with the new functionality introduced in the class.
Line range hint
1-103: Overall, good improvements to error handling, with one point for clarification.The changes in this file align well with the PR objectives of enhancing error handling in task runs. The new
finalizeRunErrormethod and the modifications to thecallmethod are good improvements that ensure proper handling and storage of run errors.However, there's an uncertainty regarding the changes to the
finalizeAttemptmethod mentioned in the AI summary but not visible in the provided code. Clarification on these changes would be helpful for a comprehensive review.To get a complete picture of the changes, please run the following command and provide the output:
✅ Verification successful
Please run the following corrected command to retrieve the full diff of
finalizeTaskRun.server.ts:
Finalized Review: Enhancements to Error Handling Confirmed
The changes in
finalizeTaskRun.server.tssuccessfully enhance error handling as intended. The introduction of thefinalizeRunErrormethod and the removal of unnecessaryoutputfields streamline the service's error management without altering thefinalizeAttemptmethod.
- No issues found with the
finalizeAttemptmethod.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Show the full diff of the file # Test: Display the full diff of the file git diff HEAD~1 apps/webapp/app/v3/services/finalizeTaskRun.server.tsLength of output: 3188
Script:
#!/bin/bash # Description: Show the full diff of the specified file git diff HEAD~1 apps/webapp/app/v3/services/finalizeTaskRun.server.tsLength of output: 3035
103-103: Clarification needed on changes tofinalizeAttemptmethod.The AI summary mentioned modifications to the
finalizeAttemptmethod, but these changes are not visible in the provided code snippet. Could you please clarify what specific changes were made to this method? This information would be helpful for a comprehensive review.packages/trigger-sdk/src/v3/runs.ts (5)
60-60: LGTM: Improved error handlingThe change to use
apiClientManager.clientOrThrow()simplifies the error handling and makes it consistent with other functions in the file. This approach is more maintainable and reduces code duplication.
211-211: LGTM: Consistent error handling improvementThe change to use
apiClientManager.clientOrThrow()aligns with the error handling approach used in other functions, improving consistency and maintainability.
241-241: LGTM: Consistent error handling improvementThe change to use
apiClientManager.clientOrThrow()maintains consistency with the error handling approach used in other functions, enhancing overall code maintainability.
Line range hint
1-324: Overall assessment: Improved error handling and consistencyThe changes made to this file consistently improve error handling across multiple functions by replacing direct
apiClientchecks withapiClientManager.clientOrThrow(). This approach enhances code consistency, reduces duplication, and improves maintainability. The modifications don't alter the core functionality of the functions but make the code more robust and easier to manage.Key improvements:
- Consistent error handling across functions
- Simplified code by removing repetitive checks
- Potential for better error messages and handling through the
apiClientManagerThese changes align well with good coding practices and should make the codebase more maintainable in the long run.
272-272: LGTM: Consistent error handling improvementThe change to use
apiClientManager.clientOrThrow()aligns with the error handling approach used in other functions, improving consistency and maintainability.To ensure that this change doesn't have unintended consequences, let's verify the usage of
apiClientManager.clientOrThrow()across the codebase:This script will help us confirm that the error handling approach is consistent across the codebase and that no direct
apiClientchecks remain.✅ Verification successful
LGTM: Consistent error handling verified
The usage of
apiClientManager.clientOrThrow()is consistent across the codebase, and no directapiClientchecks remain. This change improves maintainability without introducing unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of apiClientManager.clientOrThrow() # Test: Search for apiClientManager.clientOrThrow() usage echo "Checking for apiClientManager.clientOrThrow() usage:" rg --type typescript "apiClientManager\.clientOrThrow\(\)" -A 2 -B 2 # Test: Search for any remaining direct apiClient checks echo "\nChecking for any remaining direct apiClient checks:" rg --type typescript "if\s*\(\s*!?\s*apiClient\s*\)" -A 2 -B 2Length of output: 437
Script:
#!/bin/bash # Description: Check for consistent usage of apiClientManager.clientOrThrow() # Test: Search for apiClientManager.clientOrThrow() usage in TypeScript files echo "Checking for apiClientManager.clientOrThrow() usage:" rg "apiClientManager\.clientOrThrow\(\)" -t ts -A 2 -B 2 # If the above command still fails, use file globbing as a fallback if [ $? -ne 0 ]; then echo "\nFallback: Searching TypeScript files using file globbing:" rg "apiClientManager\.clientOrThrow\(\)" -g "*.ts" -g "*.tsx" -A 2 -B 2 fi # Test: Search for any remaining direct apiClient checks in TypeScript files echo "\nChecking for any remaining direct apiClient checks:" rg "if\s*\(\s*!?\s*apiClient\s*\)" -t ts -A 2 -B 2 # If the above command still fails, use file globbing as a fallback if [ $? -ne 0 ]; then echo "\nFallback: Searching TypeScript files using file globbing:" rg "if\s*\(\s*!?\s*apiClient\s*\)" -g "*.ts" -g "*.tsx" -A 2 -B 2 fiLength of output: 9974
packages/trigger-sdk/src/v3/schedules/index.ts (6)
123-123: LGTM! Consistent error handling improvement.The use of
apiClientManager.clientOrThrow()is consistent with the changes in thecreatefunction. This simplification improves code readability and maintainability.
164-164: LGTM! Consistent error handling across functions.The use of
apiClientManager.clientOrThrow()in theupdatefunction maintains consistency with thecreateandretrievefunctions. This unified approach to error handling is a good practice.
198-198: LGTM! Consistent error handling implemented across all API functions.The use of
apiClientManager.clientOrThrow()has been consistently applied to thedel,deactivate, andactivatefunctions. This uniform approach to error handling across all API-related functions improves code consistency and maintainability.Also applies to: 232-232, 266-266
303-303: LGTM! Consistent error handling completed for all functions.The use of
apiClientManager.clientOrThrow()in thelistfunction completes the consistent implementation of centralized error handling across all API-related functions in this file. This refactoring has significantly improved code consistency, readability, and maintainability throughout the entire module.
Line range hint
1-339: Overall improvements with one point needing clarification.This refactoring significantly improves the code by centralizing error handling across all API-related functions using
apiClientManager.clientOrThrow(). This change enhances code consistency, readability, and maintainability throughout the module.However, there's a discrepancy between the AI-generated summary and the actual code for the
timezonesfunction that needs to be addressed. Please clarify whether the error handling forbaseUrlshould be removed from thetimezonesfunction or if the summary should be updated.Once this is resolved, the changes in this file will represent a solid improvement to the codebase.
89-89: LGTM! Consider verifying error message clarity.The use of
apiClientManager.clientOrThrow()simplifies the code and centralizes error handling. This is a good improvement.To ensure a good developer experience, please verify that the error message from
clientOrThrow()is clear and helpful. Run the following script to check the implementation:docs/runs/metadata.mdx (2)
129-161: LGTM! Comprehensive coverage of metadata in lifecycle hooks.This section effectively demonstrates how to use metadata methods in both task-specific and global lifecycle hooks. The examples are clear and accurate, providing users with a good understanding of how to implement metadata operations in different contexts.
163-177: LGTM! Critical information about metadata propagation.This section effectively communicates an important aspect of metadata handling: it's not automatically propagated to child tasks. The explanation is clear, and the example demonstrates the correct way to manually pass metadata to a child task. This information is crucial for users to understand to avoid potential issues with metadata in nested task structures.
packages/trigger-sdk/src/v3/envvars.ts (6)
78-80: LGTM: Simplified API client retrievalThe change to use
apiClientManager.clientOrThrow()simplifies the code and improves error handling. This is consistent across all functions in the file and aligns with the PR objectives.
119-121: LGTM: Consistent API client retrievalThe change to use
apiClientManager.clientOrThrow()is consistent with the other functions in this file. It simplifies the code and improves error handling.
181-183: LGTM: Consistent API client retrievalThe change to use
apiClientManager.clientOrThrow()maintains consistency with the other functions in this file. It simplifies the code and improves error handling.
228-230: LGTM: Consistent API client retrievalThe change to use
apiClientManager.clientOrThrow()maintains consistency with the other functions in this file. It simplifies the code and improves error handling.
275-277: LGTM: Consistent API client retrievalThe change to use
apiClientManager.clientOrThrow()maintains consistency with the other functions in this file. It simplifies the code and improves error handling.
344-346: LGTM: Consistent API client retrieval and overall improvementThe change to use
apiClientManager.clientOrThrow()maintains consistency with the other functions in this file. It simplifies the code and improves error handling.Overall, these changes across all functions in the file (
upload,list,create,retrieve,del, andupdate) improve the code by:
- Simplifying error handling
- Reducing code duplication
- Improving consistency in API client retrieval
These modifications align well with the PR objectives of enhancing usability and flexibility.
packages/trigger-sdk/src/v3/shared.ts (5)
797-797: LGTM! Metadata correctly incorporated.The
metadataproperty has been properly added to the options passed toapiClient.triggerTask. This change ensures that any metadata provided by the user is correctly passed along to the API client and stored with the task run.
865-865: LGTM! Metadata correctly incorporated for batch operations.The
metadataproperty has been properly added to the options for each item in the batch trigger operation. This change ensures consistency with the single trigger operation and allows metadata to be provided for each individual task in a batch.
938-938: LGTM! Metadata correctly incorporated for triggerAndWait operations.The
metadataproperty has been properly added to the options passed toapiClient.triggerTaskin thetriggerAndWait_internalfunction. This change ensures consistency with other trigger operations and allows metadata to be provided for tasks that are triggered and waited upon.
1031-1031: LGTM! Metadata feature consistently implemented across all trigger functions.The
metadataproperty has been properly added to the options for each item in the batch in thebatchTriggerAndWait_internalfunction. This change completes the implementation of the metadata feature across all trigger functions (single, batch, wait, and batch-wait).Overall, the implementation of the metadata feature is consistent and well-integrated into the existing codebase. It provides users with the ability to attach additional information to task runs in all triggering scenarios, enhancing the flexibility and usefulness of the SDK.
Line range hint
500-1031: Great addition of the metadata feature across the SDK!The implementation of the metadata feature in the Trigger.dev SDK is well-executed and consistent. Here's a summary of the changes:
- A new
metadataproperty has been added to theTaskRunOptionstype.- The
metadataproperty is correctly incorporated into all relevant trigger functions:trigger_internal,batchTrigger_internal,triggerAndWait_internal, andbatchTriggerAndWait_internal.- The changes are minimal and focused, ensuring that existing functionality remains intact while adding this new capability.
These changes enhance the SDK's flexibility by allowing users to attach additional context to task runs. This can be particularly useful for debugging, tracking, or providing extra information for task execution.
Consider adding some documentation or examples in the README or user guide to showcase how this new metadata feature can be used effectively in different scenarios.
docs/v3-openapi.yaml (3)
363-441: New endpoint for updating run metadata looks good.The new endpoint
/api/v1/runs/{runId}/metadatais well-defined and consistent with the rest of the API. It includes:
- Proper use of HTTP PUT method for updating resources
- Clear description and summary
- Well-structured request body and response schemas
- Appropriate error responses
- Security requirements
- A code sample for usage
The endpoint adheres to RESTful principles and provides a clear way to update run metadata.
1980-1983: Addition of metadata to CommonRunObject schema is appropriate.The
metadataproperty has been correctly added to theCommonRunObjectschema. It's defined as an object type with a clear description and example, which is consistent with best practices for API documentation.
Line range hint
363-1983: Overall, the changes for adding run metadata functionality are well-implemented.The new endpoint for updating run metadata and the corresponding schema changes are well-designed and consistent with the rest of the API. They provide a clear and RESTful way to manage run metadata. The documentation is comprehensive, including appropriate error responses and code samples.
While the implementation is solid, consider the minor suggestions for adding rate limiting information and metadata size limits to further improve the API documentation and prevent potential issues.
packages/core/src/v3/runMetadata/index.ts (2)
13-17: Avoid usingthisin static methods; use the class name insteadUsing
thisin static methods can be confusing because it refers to the class itself, not an instance. Replacethis._instancewithRunMetadataAPI._instancefor clarity.🧰 Tools
Biome
[error] 13-13: 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] 14-14: 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] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
37-41: Add error handling for API calls to ensure robustnessCurrently, if the API calls to
apiClient.updateRunMetadatafail, the errors will propagate unhandled. Consider wrapping these calls intry-catchblocks to handle potential errors gracefully, such as logging the error or implementing a retry mechanism.Also applies to: 50-57, 71-78, 92-95
apps/webapp/app/v3/services/completeAttempt.server.ts (1)
131-136: Verify the nested update syntax for updating relatedtaskRunfieldsThe nested update within the
taskRunAttempt.updatecall aims to update theoutputandoutputTypefields of the relatedtaskRun. Ensure that this nested update syntax is correctly used in Prisma and that it effectively updates the associatedtaskRunrecord. Misuse of nested updates might result in thetaskRunnot being updated as intended.Run the following script to confirm the correct usage of nested updates:
| public async setKey( | ||
| key: string, | ||
| value: DeserializedJson, | ||
| requestOptions?: ApiRequestOptions | ||
| ): Promise<void> { | ||
| const runId = taskContext.ctx?.run.id; | ||
|
|
||
| if (!runId) { | ||
| return; | ||
| } | ||
|
|
||
| const apiClient = apiClientManager.clientOrThrow(); | ||
|
|
||
| const nextStore = { | ||
| ...(this.store ?? {}), | ||
| [key]: value, | ||
| }; | ||
|
|
||
| const response = await apiClient.updateRunMetadata( | ||
| runId, | ||
| { metadata: nextStore }, | ||
| requestOptions | ||
| ); | ||
|
|
||
| this.store = response.metadata; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in metadata update methods
The methods setKey, deleteKey, and update contain similar code for retrieving the run ID, checking its existence, obtaining the API client, and updating the metadata. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring the shared logic into a private helper method.
Apply this diff to refactor the code:
+ private async updateMetadataStore(
+ nextStore: Record<string, DeserializedJson>,
+ requestOptions?: ApiRequestOptions
+ ): Promise<void> {
+ const runId = taskContext.ctx?.run.id;
+
+ if (!runId) {
+ return;
+ }
+
+ const apiClient = apiClientManager.clientOrThrow();
+
+ try {
+ const response = await apiClient.updateRunMetadata(
+ runId,
+ { metadata: nextStore },
+ requestOptions
+ );
+ this.store = response.metadata;
+ } catch (error) {
+ // Handle error appropriately, e.g., log the error or rethrow
+ console.error("Failed to update run metadata:", error);
+ throw error;
+ }
+ }
+
public async setKey(
key: string,
value: DeserializedJson,
requestOptions?: ApiRequestOptions
): Promise<void> {
- const runId = taskContext.ctx?.run.id;
-
- if (!runId) {
- return;
- }
-
- const apiClient = apiClientManager.clientOrThrow();
-
const nextStore = {
...(this.store ?? {}),
[key]: value,
};
-
- const response = await apiClient.updateRunMetadata(
- runId,
- { metadata: nextStore },
- requestOptions
- );
-
- this.store = response.metadata;
+ await this.updateMetadataStore(nextStore, requestOptions);
}
public async deleteKey(
key: string,
requestOptions?: ApiRequestOptions
): Promise<void> {
const nextStore = { ...(this.store ?? {}) };
delete nextStore[key];
-
- const runId = taskContext.ctx?.run.id;
-
- if (!runId) {
- return;
- }
-
- const apiClient = apiClientManager.clientOrThrow();
-
- const response = await apiClient.updateRunMetadata(
- runId,
- { metadata: nextStore },
- requestOptions
- );
-
- this.store = response.metadata;
+ await this.updateMetadataStore(nextStore, requestOptions);
}
public async update(
metadata: Record<string, DeserializedJson>,
requestOptions?: ApiRequestOptions
): Promise<void> {
- const runId = taskContext.ctx?.run.id;
-
- if (!runId) {
- return;
- }
-
- const apiClient = apiClientManager.clientOrThrow();
-
- const response = await apiClient.updateRunMetadata(runId, { metadata }, requestOptions);
-
- this.store = response.metadata;
+ await this.updateMetadataStore(metadata, requestOptions);
}This refactoring reduces code duplication and centralizes error handling, making the code more maintainable.
Also applies to: 59-78, 80-95
Allows attaching up to 4KB of json-serializable metadata to a run when triggering a task and allows getting and mutating the metadata during a run execution.
metadataandmetadataTypecolumns to the TaskRun tableruns.retrieve()SDK function/endpointmetadataoption to all triggering methods.Summary by CodeRabbit
Release Notes
New Features
StandardTaskFormwith a tabbed interface for managing payload and metadata.Improvements
Schema Updates