-
Notifications
You must be signed in to change notification settings - Fork 602
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
fix: get update endpoint #5245
fix: get update endpoint #5245
Conversation
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.
❌ Changes requested. Reviewed everything up to 7af9abf in 1 minute and 55 seconds
More details
- Looked at
106
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. jobs.rs:3543
- Draft comment:
Consider using exponential backoff (or another event-driven mechanism) in the polling loop in run_wait_result_internal to reduce unnecessary waiting and CPU usage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. jobs.rs:5000
- Draft comment:
This file contains very long and complex functions. Consider refactoring by splitting into separate modules to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. jobs.rs:5556
- Draft comment:
The check for non-authenticated users in get_completed_job_result_maybe is deeply nested. Consider refactoring or using early returns to simplify the logic. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. jobs.rs:5680
- Draft comment:
Ensure that JSON result formatting in functions like get_completed_job_result_maybe handles potential parsing errors gracefully, perhaps by adding explicit error logging or recovery. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. jobs.rs:5240
- Draft comment:
Maintain consistency in using early returns to minimize nested blocks, especially in functions with multiple conditional branches like get_completed_job_result and similar endpoints. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. backend/windmill-api/src/jobs.rs:5217
- Draft comment:
Verify the binding order: $1=log_offset, $2=workspace_id, $3=job_id, $4=get_progress, $5=running. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to verify the binding order of variables. This falls under asking the author to double-check things, which is against the rules. Therefore, this comment should be removed.
7. backend/windmill-api/src/jobs.rs:5175
- Draft comment:
Consider adding a function doc comment to clarify how new logs and log_offset are computed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_mKNu2zWHkucRhFFC
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
) -> JsonResult<JobUpdate> { | ||
let record = sqlx::query!( | ||
"SELECT | ||
c.id IS NOT NULL AS completed, | ||
q.id IS NOT NULL AND q.running AS running, | ||
CASE |
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.
The CASE for 'running' is complex. Consider adding a brief inline comment to explain the role of parameter $5 and its logic.
Important
Modify
get_job_update
injobs.rs
to changerunning
status logic and add SQL query metadata file.get_job_update
injobs.rs
to changerunning
status logic using nestedCASE
statements..sqlx/query-b075c77caad37cdd75faab1d934b7a63521c1ad788033c564ffd7a2944454378.json
for SQL query metadata.This description was created by
for 7af9abf. It will automatically update as commits are pushed.