fix: Autofill startedAt and completedAt#284
Conversation
WalkthroughThe pull request introduces enhanced timestamp management for job updates in the job dispatch system. Two new functions, Changes
Sequence DiagramsequenceDiagram
participant Job as Existing Job
participant Update as Job Update Request
participant Timestamp as Timestamp Computation
participant Database as Job Database
Job->>Update: Receive update request
Update->>Timestamp: Compute startedAt
Timestamp-->>Update: Determined startedAt
Update->>Timestamp: Compute completedAt
Timestamp-->>Update: Determined completedAt
Update->>Database: Save updated job with timestamps
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/job-dispatch/src/job-update.ts (2)
52-62: Add type safety for status comparisons.While the logic is sound, consider adding type safety for the status comparisons:
- const isPreviousStatusPending = jobBeforeUpdate.status === JobStatus.Pending; - const isCurrentStatusInProgress = updates.status === JobStatus.InProgress; + const isPreviousStatusPending = jobBeforeUpdate.status as JobStatus === JobStatus.Pending; + const isCurrentStatusInProgress = updates.status != null && updates.status as JobStatus === JobStatus.InProgress;The function correctly implements a priority-based approach for determining the startedAt timestamp:
- Uses provided timestamp if available
- Preserves existing timestamp if present
- Sets timestamp on Pending -> InProgress transition
64-79: Strengthen null checks for status transitions.The function implements a robust completion timestamp logic. Consider making the null check more explicit:
const isPreviousStatusExited = exitedStatus.includes( - jobBeforeUpdate.status as JobStatus, + jobBeforeUpdate.status as JobStatus ); const isCurrentStatusExited = updates.status != null && - exitedStatus.includes(updates.status as JobStatus); + exitedStatus.includes(updates.status as JobStatus);The function correctly:
- Prioritizes explicit timestamps
- Preserves existing completion time
- Sets timestamp on transition to exited status
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/job-update.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/job-dispatch/src/job-update.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (2)
packages/job-dispatch/src/job-update.ts (2)
5-5: LGTM!The import statement correctly includes the
exitedStatusenum which is used for job status transition checks.
93-99: Verify impact on job completion handlers.The timestamp computation and update logic look correct. However, let's verify that the job completion handlers (
onJobCompletionandonJobFailure) are compatible with the new timestamp management:✅ Verification successful
Timestamp management is consistent across job updates
The webhook handlers and job update logic consistently manage timestamps, with proper computation of
completedAtbased on job completion status and correct propagation through the update interface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timestamp usage in completion handlers echo "Checking onJobCompletion usage of timestamps:" rg -A 5 "onJobCompletion.*startedAt|completedAt" --type ts echo "Checking onJobFailure usage of timestamps:" rg -A 5 "onJobFailure.*startedAt|completedAt" --type tsLength of output: 7088
Summary by CodeRabbit