-
Notifications
You must be signed in to change notification settings - Fork 620
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
perf: fetch only new inputs on refresh of inputs history #5315
base: main
Are you sure you want to change the base?
Conversation
Deploying windmill with
|
Latest commit: |
d5a3ead
|
Status: | ✅ Deploy successful! |
Preview URL: | https://ae59f155.windmill.pages.dev |
Branch Preview URL: | https://perf-since-arg-historic-inpu.windmill.pages.dev |
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 50b9b27 in 2 minutes and 27 seconds
More details
- Looked at
207
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. backend/windmill-api/openapi.yaml:10159
- Draft comment:
Consider adding a format (e.g. date-time) to the 'since' parameter schema instead of only type string for clearer API documentation. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. backend/windmill-api/src/inputs.rs:162
- Draft comment:
Review the fallback behavior using DateTime::default() when 'since' is not provided. Ensure it matches the intended query semantics (e.g. fetching all historical inputs). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking the PR author to ensure that the fallback behavior matches the intended query semantics. This violates the rule against asking the author to ensure behavior is intended. However, it does suggest reviewing the fallback behavior, which could be seen as a specific code suggestion. Overall, it leans more towards asking for confirmation rather than providing a specific suggestion or improvement.
3. frontend/src/lib/components/HistoricList.svelte:34
- Draft comment:
Ensure the discovery flag handling aligns with the new 'since' parameter – verify that updating 'lastChecked' only after a successful non-discovery fetch does not introduce race conditions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that a specific behavior does not introduce race conditions. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
4. frontend/src/lib/components/InfiniteList.svelte:36
- Draft comment:
The signature update to include the 'discovery' parameter is consistent across components. Verify that its usage in loadData and corresponding logic in scenarios such as loadMore and refresh is well-tested. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. backend/windmill-api/src/inputs.rs:162
- Draft comment:
Parsing 'since' as DateTime may cause timezone inconsistencies since other timestamps use Utc. Consider using DateTime or ensuring proper conversion. Additionally, using DateTime::default() as a fallback might be error‐prone if not intended. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/lib/components/HistoricList.svelte:45
- Draft comment:
Updating 'lastChecked' immediately (before awaiting the fetch) might lead to race conditions. Consider updating it after a successful fetch to ensure only new inputs are filtered. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/lib/components/HistoricList.svelte:80
- Draft comment:
Consider splitting the comma-separated operations into separate statements for improved readability. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/lib/components/InfiniteList.svelte:36
- Draft comment:
Loader function calls are updated to include the 'discovery' flag. Ensure that this change is documented and consistently used across all components. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that the change is documented and consistently used across all components. This falls under the rule of not asking the author to ensure behavior or documentation, which is not allowed.
Workflow ID: wflow_oqqBmUEzQQRVUFd4
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.
@@ -10159,6 +10159,10 @@ paths: | |||
in: query | |||
schema: | |||
type: boolean | |||
- name: since |
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.
Consider adding a description and format (e.g. ISO8601) for the new 'since' query parameter to improve API documentation clarity.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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 27ca01b in 3 minutes and 48 seconds
More details
- Looked at
207
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. backend/windmill-api/openapi.yaml:10159
- Draft comment:
New 'since' query parameter added. Confirm that its type (string) and description are sufficient for clients. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. backend/windmill-api/src/inputs.rs:162
- Draft comment:
Using DateTime::default() as a fallback may introduce unexpected start date values. Consider a more meaningful default (e.g., epoch start) or explicitly reject missing 'since' parameter if required. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Using DateTime::default() actually returns the Unix epoch, which is a reasonable and common default when filtering by date - it effectively means "show all history". The comment suggests this might be unexpected, but in this context it's actually a sensible choice that will return all historical data. The comment seems to misunderstand that DateTime::default() is unpredictable when it's actually well-defined.
I could be wrong about whether returning all historical data is the intended behavior. Maybe there are performance implications of potentially querying the entire history.
Even if querying all history isn't ideal, using epoch start is still a clear and well-defined behavior. If performance is a concern, that should be addressed separately with proper pagination (which this code already has).
The comment should be deleted because DateTime::default() returning the epoch is actually a reasonable default that provides clear and useful behavior.
3. backend/windmill-api/src/inputs.rs:137
- Draft comment:
Ensure that the SQL query parameter ordering matches the binding order, especially with the new 'since' parameter in position $4. It appears correct, but double-check against runtime expectations. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
4. frontend/src/lib/components/HistoricList.svelte:34
- Draft comment:
The added 'discovery' parameter alters the loader behavior. The buffering mechanism using 'items', 'potentialItems', and 'perPageBind' is somewhat convoluted. Consider adding comments or refactoring for clarity. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. frontend/src/lib/components/InfiniteList.svelte:21
- Draft comment:
Signature of 'loadInputs' updated to include 'discovery' parameter. Verify that all consumers properly ignore or pass the flag, as seen in SavedInputsPicker and CaptureTable. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. frontend/src/lib/components/SavedInputsPicker.svelte:62
- Draft comment:
Load function in SavedInputsPicker ignores the 'discovery' parameter (named as '_discovery'). Ensure this is intentional so that discovery-based refresh logic isn’t applied in saved inputs. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
7. frontend/src/lib/components/triggers/CaptureTable.svelte:70
- Draft comment:
Similarly, the local load function for captures accepts a '_discovery' parameter. Confirm that discovery logic is not needed for CaptureTable or that its behavior matches design. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
8. backend/windmill-api/openapi.yaml:10159
- Draft comment:
Consider specifying a format or pattern (e.g. ISO 8601) for the 'since' query parameter to improve API clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 40% vs. threshold = 50%
This is a reasonable suggestion to improve API documentation by specifying the expected date/time format for the 'since' parameter. Having a clear format specified would help API consumers know exactly what format to use. However, I need to be cautious about keeping comments that aren't strictly necessary.
The comment is a suggestion for improved documentation rather than pointing out an actual bug or issue. The API would still function without this change. Additionally, the format may be intentionally flexible to accept multiple common date formats.
While not critical, having clear API documentation is important for usability. Specifying the expected format would prevent confusion and potential errors from API consumers using incorrect date formats.
While this is a reasonable suggestion for improving API clarity, it's not pointing out a critical issue that must be fixed. The API can function without this change.
9. backend/windmill-api/src/inputs.rs:162
- Draft comment:
Clarify the expected date format for 'since' and consider whether using DateTime::default() (as fallback) is appropriate. An explicit value (e.g., UNIX epoch) might improve clarity. - Reason this comment was not posted:
Marked as duplicate.
10. frontend/src/lib/components/HistoricList.svelte:42
- Draft comment:
Review the logic merging 'items' and 'potentialItems'. The use of 'perPageBind' to conditionally push potential items could risk race conditions or duplicate ordering; ensure this behavior meets the intended refresh semantics. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code has clear comments explaining why this approach was chosen. The perPageBind check is intentionally designed to prevent unwanted list expansion. While race conditions are theoretically possible, the comment is speculative ("could risk") and doesn't point to a specific, demonstrable issue. The author seems to have thought through the refresh semantics.
I might be overlooking real race conditions in the async nature of the code. The comment could be pointing to legitimate concerns about data consistency.
While async code can be tricky, the comment doesn't provide specific evidence of problems, and the code's comments suggest the behavior is intentional and well-thought-out.
Delete the comment as it's speculative ("could risk") and doesn't point to a specific, demonstrable issue. The code appears to have intentional logic with documented reasoning.
11. frontend/src/lib/components/InfiniteList.svelte:35
- Draft comment:
Ensure that all callers of loadInputs pass the correct 'discovery' flag. This change in the loader signature for pagination must be consistently handled to avoid unexpected behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that all callers of a function pass a specific flag correctly. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, but rather gives a general instruction to ensure consistency, which is not allowed.
Workflow ID: wflow_31DEOoDPtcHe1yqU
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.
3a05600
to
3b6585a
Compare
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
470be4b
to
3188bee
Compare
Important
Optimized input history fetching by adding a
since
parameter to only retrieve new inputs, enhancing performance across backend and frontend components.since
query parameter toget_input_history
ininputs.rs
to fetch inputs created after a specific date.get_input_history
to filter results based onv2_job_completed.started_at
.loadInputsPageFn
inHistoricList.svelte
to usesince
parameter for non-discovery fetches.InfiniteList.svelte
to passdiscovery
flag toloadInputs
function.SavedInputsPicker.svelte
andCaptureTable.svelte
to accommodate newloadInputsPageFn
signature.This description was created by
for 27ca01b. It will automatically update as commits are pushed.