-
Notifications
You must be signed in to change notification settings - Fork 4
fix: [PRODUCT-592], [ENG-2326] build createParams to elide implicit NOW
#218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Changed Files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This PR properly implements the fix for issue #215 (ENG-2326) by avoiding race conditions when creating reserved nodes with start="NOW".
Key Changes:
- When
options.start === "NOW", the code now leavescreateParams.start_atundefined (instead of setting it to the client's current timestamp), allowing the API server to use its own current time and avoid race conditions - Fixed
endDatecalculation to use the actual start time: when start is "NOW", it usesnew Date()instead of the non-existentcreateParams.start_at - Added user feedback showing the inferred start time when prompting for end time selection (src/lib/nodes/create.ts:299-311)
- Updated to nodes-sdk alpha.22 which supports
start_atas nullable - Cleaned up lock file with dependency updates
The changes are well-structured and address the root cause: confirmation steps were causing client-side timestamps to become stale by the time they reached the API.
Confidence Score: 5/5
- This PR is safe to merge with no identified issues
- The fix correctly addresses the race condition by delegating "NOW" timestamp generation to the API server. The logic properly handles both code paths (NOW vs explicit date), calculates end dates correctly, and improves UX with clear messaging. Dependency updates are standard maintenance.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| deno.lock | 5/5 | Lock file regeneration with dependency updates (nodes-sdk alpha.22, @std/assert, @types/node) and cleanup of unused dependencies |
| package.json | 5/5 | Dependency update to nodes-sdk alpha.22, no issues |
| src/lib/nodes/create.ts | 5/5 | Correctly fixes race condition by not passing start_at for "NOW", calculates endDate from actual start time, and improves user messaging |
Sequence Diagram
sequenceDiagram
participant User
participant CLI
participant Validation
participant API
User->>CLI: Create reserved node with start="NOW"
CLI->>Validation: Validate start time
alt start is "NOW"
Note over CLI: Skip setting createParams.start_at
Note over CLI: Use current time for end calculations
CLI->>CLI: Calculate endDate from new Date()
else start is Date object
CLI->>CLI: Set createParams.start_at = timestamp
CLI->>CLI: Calculate endDate from options.start
end
CLI->>CLI: Validate end time on hour boundary
alt end time invalid AND start was valid
CLI->>User: Show inferred start time
CLI->>User: Prompt for end time selection
User->>CLI: Select end time
end
CLI->>API: POST /nodes/create with params
Note over API: API uses current server time for start_at=undefined
API->>CLI: Return created nodes
CLI->>User: Display success
2 files reviewed, no comments
createParams to elide implicit NOW
Follow up to #215 to correctly build/implement the intended functionality
Also improves UI to show startDate when selecting endDate
