Replace DelayRequestByMs with SleepAfterMs
#126
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the delay mechanism from an HTTP-request-specific delay that occurred before requests to a more general sleep that occurs after any step execution. The name change from DelayRequestByMs to SleepAfterMs reflects this behavioral change.
Key changes:
- Moved delay functionality from HTTP request actions to the step level
- Changed timing from "delay before request" to "sleep after step"
- Added UI rendering for sleep steps with appropriate time formatting
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| version/version.go | Refactored const declarations to use grouped syntax |
| render/render.go | Grouped var declarations and added handler for new SleepMsg to display wait status |
| messages/messages.go | Added new SleepMsg type for communicating sleep duration |
| cmd/login.go | Added blank line for improved formatting |
| client/lessons.go | Removed HTTPActions struct and DelayRequestByMs field; added SleepAfterMs field to CLIStep |
| checks/checks.go | Removed HTTP-specific delay logic and implemented step-level sleep after each step execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
squashd
left a comment
squashd
left a comment
There was a problem hiding this comment.
One change needed (I think). I'd recommend adding these changes to lessons in a course and testing whether it looks right in the CLI and on the frontend.
| if step.SleepAfterMs != nil && *step.SleepAfterMs > 0 { | ||
| ch <- messages.SleepMsg{DurationMs: *step.SleepAfterMs} | ||
| time.Sleep(time.Duration(*step.SleepAfterMs) * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
this needs to be moved up or it'll hit the default error case, no? e.g. if a step is solely -sleepAfterMs
There was a problem hiding this comment.
Good point. Will fix that.
|
I think we need to do a version bump too |
No description provided.