feat: dependency job debouncing #6769
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
….com:windmill-labs/windmill into fix-relative-imports-cache-invalidation-apps Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
…g-reflected-in-dependency_map
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
…g-reflected-in-dependency_map
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
…-in-dependency_map' of github.com:windmill-labs/windmill into pyra/win-1566-fix-flowapps-renaming-not-being-reflected-in-dependency_map
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
|
/updatesqlx |
|
Starting sqlx update... |
|
/eeref |
|
Starting ee ref update... |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 390dfe6 in 4 minutes and 10 seconds. Click for details.
- Reviewed
3213lines of code in13files - Skipped
0files when reviewing. - Skipped posting
24draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-worker/src/worker_lockfiles.rs:1500
- Draft comment:
Removing/recreating the entire job directory inside the loop (around line 1501) may unintentionally delete files needed for later modules. Consider moving this directory reset outside the loop or ensure it’s safe to do so. - 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. backend/windmill-worker/src/worker_lockfiles.rs:106
- Draft comment:
Using unwrap() on to_str() (line 111) can panic if path normalization fails. Consider handling the error gracefully. - 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. backend/windmill-worker/src/worker_lockfiles.rs:1218
- Draft comment:
The 'lock_modules' function is very complex and deeply nested. Consider refactoring or modularizing its recursive logic for improved 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.
4. backend/windmill-worker/src/worker_lockfiles.rs:1909
- Draft comment:
The conditional logic in 'skip_creating_new_lock' (around line 1909) for Bun/Bunnative scripts is unclear. Please add comments to clarify the intended behavior. - 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.
5. backend/windmill-worker/src/worker_lockfiles.rs:187
- Draft comment:
Defaulting to a hash value of 0 (e.g. using ScriptHash(0)) when 'job.runnable_id' is missing might hide errors. Consider handling missing IDs explicitly. - 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.
6. backend/tests/common/mod.rs:689
- Draft comment:
Typographical error: Instead of "will result into conflicts!!!", consider changing it to "will result in conflicts!!!" for correct wording. - 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% While technically correct that "result in" is more proper than "result into", this is a minor grammatical issue in a comment. The meaning is still perfectly clear. Comments are meant to convey information, and this one succeeds at that despite the minor grammatical imperfection. PR comments should focus on substantive code issues rather than nitpicking grammar in comments. The grammar correction is technically accurate. Poor grammar in comments could be seen as reducing code quality and professionalism. While technically correct, this level of grammatical nitpicking in comments does not provide enough value to justify a PR comment. The meaning is clear and this is not confusing to readers. Delete this comment as it is too minor of an issue to warrant a PR comment. The slight grammatical imperfection does not impact understanding.
7. backend/tests/fixtures/djob_debouncing.sql:80
- Draft comment:
Typo found in the summary: "leaf RIght" should be "leaf Right" for module c in the flow insertion. - 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% This is a minor capitalization issue in test fixture data. The typo doesn't affect functionality since it's just a display label in test data. The comment is technically correct but extremely low value. We generally want to avoid nitpicking comments about capitalization, especially in test data. The inconsistent capitalization could make the test data harder to read and maintain. It may indicate lack of attention to detail. While consistency is good, this is test fixture data where perfect capitalization is not critical. The benefit of fixing this is minimal compared to the overhead of addressing the comment. Delete this comment as it's an extremely minor capitalization issue in test data that doesn't impact functionality.
8. backend/tests/fixtures/djob_debouncing.sql:134
- Draft comment:
Typo found in the summary: "leaf RIght" should be "leaf Right" for module c in the flow_version insertion. - 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% This is a test fixture file, so the exact text of summaries is not critical to functionality. The typo is very minor and doesn't affect how the code works. The comment is about something that was changed (new file), but it's an extremely low-value change that doesn't impact anything meaningful. The inconsistent capitalization could theoretically make the test fixtures less readable or maintainable. It might be worth keeping consistent naming conventions even in test data. While consistency is good, this is far too minor of an issue to warrant a PR comment. Test fixture readability is not significantly impacted by this tiny capitalization inconsistency. Delete this comment as it points out an extremely minor cosmetic issue in test data that doesn't impact functionality or meaningful readability.
9. backend/tests/relative_imports.rs:166
- Draft comment:
Typo: "Immidiately" should be corrected to "Immediately". - Reason this comment was not posted:
Comment was on unchanged code.
10. backend/tests/relative_imports.rs:554
- Draft comment:
Typo: In the comment "# Double referenced even", "even" seems incorrect. Possibly it should be "event" or "example". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The word "even" in "Double referenced even" does seem awkward, but looking at the context, it's not clear that "event" would be better. The section is describing a topology where nodes are referenced multiple times, not necessarily an event. The comment may be pointing out a real issue but suggesting the wrong fix. The entire phrase is unclear regardless of using "even" or "event". Am I sure that "event" would be better than "even"? Neither word seems to clearly fit the context of describing a topology with multiple references. You're right - while "even" seems wrong, "event" may not be the right fix. The issue is with the clarity of the entire phrase, not just this one word. The comment identifies a real issue but suggests a fix that may not be correct. The entire phrase needs clarification, not just swapping one word.
11. backend/tests/relative_imports.rs:576
- Draft comment:
Typo: "What test gurantees:" should be "What test guarantees:". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The rules say not to make purely informative comments. While this is a typo fix, it's in documentation which is important for readability and professionalism. However, the rules are quite clear about not making comments unless there is "clearly a code change required". A typo in a comment is not a code change. The typo does affect documentation quality. Documentation is important for maintainability. Maybe this should be an exception to the rule? No, the rules are very clear - comments must require code changes. Documentation fixes, while valuable, should be handled differently, perhaps through direct commits or documentation-specific reviews. Delete the comment. While fixing typos is good, the rules explicitly state to only comment when code changes are required.
12. backend/tests/relative_imports.rs:595
- Draft comment:
Typo: In the comment "Also makes sures that dependency job does not create new flow version", "sures" should be replaced with "ensures" (or rephrased as "makes sure"). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is pointing out a minor typo in a code comment. While technically correct, this kind of comment violates several rules: 1) It's not about a code change - the comment was already there and is just being moved, 2) It's a very minor/unimportant issue, 3) It doesn't require any actual code changes, just documentation tweaks. The rules say to assume the PR author made changes for good reasons and to only comment on actual code issues. Could fixing typos in comments help improve code quality and readability for future developers? Should we maintain high standards even for comments? While clear documentation is important, the rules explicitly state not to make purely informative comments or comments about obvious/unimportant issues. A minor typo in a comment doesn't impact functionality and isn't worth the PR author's time to fix. This comment should be deleted as it violates multiple review rules - it's about unchanged code, it's unimportant, and it doesn't require actual code changes.
13. backend/tests/relative_imports.rs:689
- Draft comment:
Typo: "atleast" should be two words: "at least". - 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% The comment is about a spelling mistake in a code comment, not about actual code logic. The rules state we should not make purely informative comments. While technically correct, fixing this typo won't impact the code's functionality. The comment is also quite minor and obvious. The spelling correction would improve code quality and readability slightly. Comments are part of the codebase and maintaining good spelling helps with professionalism. While good spelling is nice, this is too minor of an issue to warrant a PR comment. The rules clearly state not to make purely informative comments or comments about obvious/unimportant things. Delete this comment as it is purely informative and too minor to be worth a PR comment.
14. backend/tests/relative_imports.rs:717
- Draft comment:
Typo: "doublicats" should be "duplicates". - 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% While the comment is technically correct in identifying a typo, it's just a typo in a comment, not in actual code. The meaning is still clear from context. Comments are documentation and should be clear, but this is a very minor issue that doesn't impact code functionality or significantly impact readability. The typo could theoretically make the code slightly harder to understand for non-native English speakers. Also, maintaining high quality even in comments shows attention to detail. While quality is important, this is an extremely minor issue in a test file. The meaning is clear from context and fixing it provides minimal value. We should focus reviewer attention on more impactful issues. Delete the comment. The typo is too minor to warrant a PR comment, especially since it's in a test file's comments and the meaning is clear from context.
15. backend/tests/relative_imports.rs:836
- Draft comment:
Typo: In the doc comment, "should be ran in debug mode" should be "should be run in debug mode". - 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% While the comment is technically correct (the past participle "run" should be used instead of "ran"), this is a very minor grammatical issue in a doc comment. Doc comments are important, but this change is so minor that it doesn't meaningfully impact code quality or documentation clarity. The meaning is perfectly clear either way. The grammar error could be seen as unprofessional in documentation. Doc comments are part of the public API and should be held to high standards. While documentation quality is important, this is an internal test file, not public API documentation. The tiny grammar improvement doesn't justify the overhead of a code review comment. Delete the comment. The grammar fix is too minor to warrant a review comment, especially in test code.
16. backend/tests/relative_imports.rs:839
- Draft comment:
Typo: "correcly" should be "correctly". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is about a typo in a comment, not about actual code. The rules state to only comment if there is clearly a code change required. While typos in comments can be distracting, fixing them is not essential for the code to work correctly. Additionally, this is a test file, where comment clarity is less critical than in production code. The typo could make the code slightly less professional looking and harder to read. Documentation quality does matter to some degree. While documentation quality matters, the rules explicitly state not to make purely informative comments and to only comment when code changes are required. This is a minor typo in a test file comment that doesn't impact functionality. Delete the comment. It's a minor documentation issue that doesn't require code changes, and the rules explicitly say not to make purely informative comments.
17. backend/tests/worker.rs:2934
- Draft comment:
There's a phrasing issue in this TODO comment. The segment "and that essentials" is unclear and appears to be a typographical error. Perhaps it should read something like "...and that the essential features work..." to clarify the intent. - 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% While the comment identifies a real grammatical issue, TODO comments are informal developer notes and don't require perfect grammar. The meaning is still understandable despite the minor error. The comment doesn't suggest any actual code changes that would affect functionality. The comment is technically correct about the grammar, but is it worth the overhead of updating a TODO comment that will likely be removed once the feature is implemented? Even though TODOs are temporary, clear communication is still valuable. However, the current phrasing, while not perfect, is understandable enough for its purpose. The comment points out a real but minor grammatical issue in a TODO comment. The benefit of fixing this is too small to justify the overhead of a code change.
18. backend/windmill-queue/src/jobs.rs:2282
- Draft comment:
Typographical suggestion: The comment reads "// Whether or not try to pull from suspended jobs first". Consider revising it to "// Whether or not to try to pull from suspended jobs first" for clarity. - 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% This is a purely cosmetic change to a comment that doesn't affect functionality. While technically correct, it's a very minor issue that doesn't impact code understanding or maintainability in any meaningful way. The original comment is still perfectly understandable. The typo fix does make the comment slightly more grammatically correct. Comments are part of code documentation and should ideally be well-written. While good documentation is important, this change is too minor to justify a PR comment. The meaning is clear either way, and nitpicking such small issues creates noise in code reviews. Delete the comment as it's too minor of an issue to warrant attention in code review.
19. backend/windmill-queue/src/jobs.rs:2279
- Draft comment:
Grammatical suggestion: The comment "/// Pull the job from queue" might be clearer as "/// Pull a job from the queue". - 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% This is a purely grammatical suggestion that doesn't affect functionality. While technically correct English, the original docstring is clear enough. The comment doesn't point out any real issues or suggest important improvements. The comment is technically correct English. The suggested change would make the docstring slightly more precise. While technically correct, this kind of minor grammatical suggestion creates noise in code reviews without providing meaningful value. The original docstring is clear enough for developers to understand. Delete this comment. Minor grammatical suggestions like this don't provide enough value to justify the review noise.
20. backend/windmill-queue/src/jobs.rs:2443
- Draft comment:
Typo in variable name: "has_concurent_limit" seems to be misspelled. It should likely be "has_concurrent_limit". - Reason this comment was not posted:
Comment was on unchanged code.
21. backend/windmill-queue/src/jobs.rs:4411
- Draft comment:
Typographical error: In the comment "This covers and edge case with race condition", "and" should be "an". - 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% While the comment is technically correct and points out a real typo, fixing typos in comments is very low value. Comments are for human readability and this typo doesn't impact understanding. The rules state not to make purely informative comments or comments that are obvious/unimportant. The typo does slightly impact readability and professionalism of the codebase. Fixing it would make the code marginally better. The marginal benefit is extremely small and not worth the overhead of a PR comment. The rules explicitly say not to make unimportant comments. Delete this comment as it provides very little value and violates the rule about not making unimportant comments.
22. backend/windmill-queue/src/jobs.rs:4417
- Draft comment:
Typographical error: In the comment, "bc of that job is being marked as running..." "bc" should be replaced with "because" for clarity. - 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% While the suggestion would improve readability slightly, this is a very minor stylistic change to a comment. The comment is still perfectly understandable with "bc". The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category. The comment does point out a real readability improvement. "bc" is informal shorthand that could be confusing to some readers. While true, the improvement is extremely minor and the meaning is still clear. The rules explicitly say not to make unimportant comments or comments that don't require code changes. This comment should be deleted as it suggests a very minor stylistic change to documentation that doesn't impact functionality or significantly improve understanding.
23. backend/windmill-worker/src/worker_lockfiles.rs:640
- Draft comment:
Typo: The word "commited" is misspelled in the comment. It should be "committed". - 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% While the comment is technically correct about the spelling error, it's just a typo in a comment. Comments are documentation and while accuracy is good, this is an extremely minor issue that doesn't affect code functionality. The rules state not to make comments that are obvious or unimportant. The spelling error could theoretically cause confusion for non-native English speakers. Documentation quality does matter. While documentation quality matters, this is an extremely minor spelling error that most developers would understand correctly in context. The cost of the review comment outweighs the minor benefit. Delete this comment. Spelling corrections in comments are too minor to warrant a review comment unless they significantly impact understanding.
24. backend/windmill-worker/src/worker_lockfiles.rs:675
- Draft comment:
Typographical suggestion: Consider changing "May be" to "maybe" in the TODO comment for clarity. - 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% According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. A typo in a TODO comment is very minor and does not affect functionality. The TODO comment is for developers, not users, so perfect grammar is not critical. Could fixing this typo help with code readability and professionalism? Some might argue that maintaining high standards even in comments is important. While clean code is important, this is an extremely minor issue in a TODO comment that will likely be removed later anyway. The meaning is clear either way. This kind of nitpick creates noise in code reviews. Delete the comment. It's too minor of an issue and doesn't affect functionality or important documentation.
Workflow ID: wflow_xVckozdHU9039djK
You can customize 
|
Successfully updated ee-repo-ref.txt |
|
Successfully ran sqlx update |
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>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ca43cdf in 3 minutes and 45 seconds. Click for details.
- Reviewed
1257lines of code in44files - Skipped
0files when reviewing. - Skipped posting
38draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/.sqlx/query-04ce5c530c80ae6f911dfe0dc9ed7d1a2e10342bbbc7f8486df0b73f5657a493.json:3
- Draft comment:
Verify that the COUNT(*) returning an Int8 is handled correctly in the application. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json:3
- Draft comment:
Ensure that the non-null assertions (e.g. 'id!') match application expectations and DB schema. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. backend/.sqlx/query-08e5832c4002a0970d4105bb80371dcc58cb037a59afe08cd1372b51791b4165.json:3
- Draft comment:
Simple COUNT query; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. backend/.sqlx/query-19addf05e39d6068e172f52daa65b8a5e96a4653ee6023b7d0980b0df387b474.json:3
- Draft comment:
Query is parameterized properly; ensure the key type ($1::text) aligns with expected input. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. backend/.sqlx/query-1a54356c1e1353950bf6ab1d25ab21270131e9e93ca10d195664e7e5a774fe9e.json:3
- Draft comment:
Check that the explicit cast (::text::json) for parameter $9 is intentional and its format validated. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. backend/.sqlx/query-1c047fef05e8cfc07aef7ea9a5454334eb7375b483e8ffd80364c60c3aad04b4.json:3
- Draft comment:
The app_id is hardcoded to '2'; verify this is intended for your multi-tenant design. - 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% This is a generated SQLx metadata file, not hand-written code. The actual SQL query with the hardcoded value would be somewhere else in the codebase. Without seeing the actual code that uses this query, we can't judge if the hardcoded value is intentional or problematic. The comment is asking for verification rather than pointing out a clear issue. The hardcoded ID could potentially be a real issue in a multi-tenant system. Maybe we should keep the comment to be safe? No - the comment violates multiple rules: it's asking for verification ("verify this is intended"), and we can't see enough context to know if this is actually an issue. We should assume the author knows what they're doing unless we have strong evidence otherwise. Delete the comment. It's asking for verification rather than pointing out a clear issue, and we don't have enough context to judge if the hardcoded value is actually problematic.
7. backend/.sqlx/query-3af3c1080cde18cfbeb08e290f63b465b4bbf83bc86c0efacd6133bb432ee13d.json:3
- Draft comment:
Consider aliasing the computed interval (scheduled_for - created_at) for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
8. backend/.sqlx/query-421c7e0388326889b33fae4c8fa2fd7cb4a7be6dc50c9f7507eaa15f138484dc.json:3
- Draft comment:
Query is straightforward; ensure the fixed path 'f/dre/flow' is correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. backend/.sqlx/query-44d3e7dce67967471fa638f2beedf8567579218d5c4332b01e169732c836ebdc.json:3
- Draft comment:
Ensure that filtering for non-archived scripts matches business logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. backend/.sqlx/query-47a55edc0f5c54ac9f5e48665c8dafc044d1861048857ec2fc70f929ab358373.json:3
- Draft comment:
The query returns non-null job queue IDs; this appears correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. backend/.sqlx/query-53b236c65e8790b7474e919daa0583902596c4faed6d83e668be85aba8eb644a.json:3
- Draft comment:
COUNT query on debounce_key is simple; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. backend/.sqlx/query-5848d416e2d96eb20c7417dec608f8ca36db091c67cf9ec5e7cec40486532ab9.json:3
- Draft comment:
Repeated COUNT on v2_job_queue; ensure this duplicate is intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
13. backend/.sqlx/query-59a6afc7c2f4d2c00b9389f3d2b6b675074e92a174ba775802a9bcaddd30cd8a.json:3
- Draft comment:
The DELETE is parameterized properly; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
14. backend/.sqlx/query-5bedbfe98b3cacd1c6f4b2344a9a626245abe58c306392a76710252d16d0bd44.json:3
- Draft comment:
Ensure that the returned Int8Array for 'versions' is correctly parsed by the consumer. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
15. backend/.sqlx/query-6641d63a691d712bc24be9c972646fed29a76e579e72d6c8539cd0de73b424db.json:3
- Draft comment:
The COUNT on debounce_stale_data is straightforward; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
16. backend/.sqlx/query-720d7f0d258d52a6b89e2f4b32ea0fd360da8a9f06e0ab59d9e724851ced4247.json:3
- Draft comment:
This query duplicates the COUNT on debounce_stale_data; consider consolidating if not needed separately. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
17. backend/.sqlx/query-73fcf81d272c1613e094d60c0a088f9d694bc37caacef7269e3738de8b5f6013.json:3
- Draft comment:
Query for retrieving a key by job_id is clear; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
18. backend/.sqlx/query-77f33de1d95e38a44968ea7f026476226c66fe42c9f992bd4c6c232a68ade2bd.json:3
- Draft comment:
Query to fetch archived script hash looks appropriate; ensure 'archived = true' meets requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
19. backend/.sqlx/query-7ec724b84479c2f737637e91b8cbed6cae29f361167deee879b8b683ad1bf684.json:3
- Draft comment:
In the INSERT ... ON CONFLICT query, consider using COALESCE for 'to_relock' to avoid null concatenation. - 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% The nullable array is empty which suggests these columns are NOT NULL. The array_cat function in PostgreSQL actually handles NULL arrays gracefully - if one array is NULL, it returns the other array. If both are NULL, it returns NULL. Given the schema shows these aren't nullable, and array_cat handles NULLs well anyway, this suggestion seems unnecessary. I could be misinterpreting the nullable field in the schema. There might be edge cases where NULL values could appear that aren't captured in the schema. Even if there were NULL values possible, array_cat handles them appropriately without needing COALESCE. The suggestion adds complexity without clear benefit. The comment should be deleted as it suggests unnecessary complexity when array_cat already handles NULL cases appropriately, and the schema suggests NULLs aren't possible anyway.
20. backend/.sqlx/query-7ed404f3a8b23f98fb7c15a26b14f9e3e416e1baf20bdb25c9a8ac9efc288931.json:3
- Draft comment:
Simple INSERT into debounce_key; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
21. backend/.sqlx/query-91709563c6f6a60b01b767250a6611f98757e7f821169cf8acc715fff0cac093.json:3
- Draft comment:
Ensure the INSERT into app_version correctly replicates all desired columns from the source. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
22. backend/.sqlx/query-91a13927f7e0577e52755fd8463a4fc58818c109deda6678c79474bf45428966.json:3
- Draft comment:
Query for checking 'running' status looks straightforward; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
23. backend/.sqlx/query-9b37ec5aa9b979393c6e5c8d98f467dcabc2c3367fb33592e5608bd985c9e436.json:3
- Draft comment:
Join between debounce_key and debounce_stale_data is clear; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
24. backend/.sqlx/query-9d1bd189940d345f27e4850651c423b891b57e9a65a02b52966377c1960ab89e.json:3
- Draft comment:
Ensure the array indexing ([1]) for 'versions' is correct based on Postgres indexing (1-based). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
25. backend/.sqlx/query-a14270c6a2af936539c7d7e4a950b45df00a1ae022b3c29730af2347bd76deef.json:3
- Draft comment:
Query selecting IDs from v2_job_completed is straightforward. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
26. backend/.sqlx/query-a206326b6c13c88b773adcd1f7b3e0822cb91bde2324327f1828236112d278b1.json:3
- Draft comment:
This interval computation is similar to earlier; ensure consistency across queries. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
27. backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json:3
- Draft comment:
Special column aliases ('email!' and 'username?') are unusual; ensure downstream code can handle these names. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
28. backend/.sqlx/query-bfc96c870fe0c50ebb9e9bc5ccff231912db86c05c881aff1ffb5460f9711390.json:3
- Draft comment:
Query for 'parent_hashes' is clear; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
29. backend/.sqlx/query-c42326e2121b79d1381a3c91ef90f6929f5dcbf29f3938e40c7f93d98aa7f49c.json:3
- Draft comment:
Selecting runnable_path from v2_job is straightforward; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
30. backend/.sqlx/query-c6ef0acdf20bd71dd26de981fb49f178ba8a1b8c1e01e0fec1dfd6a54ea7a894.json:3
- Draft comment:
In the DELETE query, verify whether '{}'::jsonb is the intended empty JSON array (consider '[]' if appropriate). - Reason this comment was not posted:
Comment was on unchanged code.
31. backend/.sqlx/query-cbc8e3e22862a76e1a8386cca247694378b2be15f3a40e7e6690f31157bdb5af.json:3
- Draft comment:
Simple SELECT from debounce_key; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
32. backend/.sqlx/query-cfe06702916362aaf5122bb95593eff389e0d44b7a58b69fd5c79629599902fc.json:3
- Draft comment:
DELETE query with RETURNING clause looks proper; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
33. backend/.sqlx/query-d6a060b255f02a2a776a6a3ac4024d3b06170d3a02a06a4c209e49e5ef175916.json:3
- Draft comment:
Query selecting 'lock' from script is clear; ensure that nullable handling for 'lock' is intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
34. backend/.sqlx/query-db7a756d541dbedbdeb23ab8f914d0ab538043fff345a0438a40708c4066771b.json:3
- Draft comment:
Selecting the second element (versions[2]) – verify that array index is correct per Postgres' 1-based indexing. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
35. backend/.sqlx/query-dbd7730a34ce6c57c574ec541466cdf13bc0768f8b437515c284f839b87b7a67.json:3
- Draft comment:
UPDATE on debounce_key is straightforward; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
36. backend/.sqlx/query-e49b72cf5a05b47f76ebcdc5306928e1be4696120c5d78c1649d5e430a5dae4c.json:3
- Draft comment:
JSON extraction query looks good; verify that the JSON path is correct for the module locks. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
37. backend/.sqlx/query-eaeeb4708a6d9bf6be3265dcaaaf97f2301599a39668fcd20e1642fd828e3eec.json:3
- Draft comment:
Comparison of completed_at and started_at is clear; ensure date/timestamp values are non-null. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
38. backend/.sqlx/query-19addf05e39d6068e172f52daa65b8a5e96a4653ee6023b7d0980b0df387b474.json:13
- Draft comment:
Typo: The key 'Left' is capitalized whereas other keys in this file are lower-case. Consider renaming it to 'left' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This appears to be an auto-generated file by the sqlx library. The 'Left' capitalization is likely intentional and part of the library's schema. Making such a change could break the functionality. We shouldn't suggest changes to auto-generated files or library-specific formats. Could this actually be a user-defined key that should follow project conventions? Could inconsistent capitalization cause issues in some environments? Given this is in the .sqlx directory and follows a specific format with a hash, this is clearly an auto-generated file by the sqlx library. We should not suggest modifications to library-generated files. Delete this comment as it suggests modifying an auto-generated file's structure, which could break functionality.
Workflow ID: wflow_eEaERd6nwVYueXsr
You can customize
Signed-off-by: pyranota <[email protected]>
…into djobs-debouncing
Signed-off-by: pyranota <[email protected]>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4fd3179 in 56 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/ee-repo-ref.txt:1
- Draft comment:
Verify updated commit hash is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_sUV9s8ZlRrMs5myA
You can customize
|
/ai implement test_2 for script to also hit race condition edge case (find comment for it in pull func). add comments to created tests. remove test_2 for script and flow. please let me know if you think this PR needs more tests for other (edge) cases. |
|
Claude finished @pyranota's task —— View job ✅ Test improvements completed!Todo List:
What was done:✅ Implemented test_2 for scripts that specifically tests the race condition edge case where a job is marked as running but the debounce_key hasn't been cleaned up yet The race condition test:The new test simulates the edge case described in
📝 Recommendations for additional edge case tests:Based on my analysis of the debouncing implementation, here are the edge cases that would benefit from additional testing: 1. Concurrent debounce key creation raceTest when two threads try to create the same debounce key simultaneously. The system should handle this with ON CONFLICT DO NOTHING, but it's worth validating. 2. Stale data accumulation limitsTest what happens when a very large number of nodes/components need to be relocked (e.g., 1000+ items). This could stress the array operations in the database. 3. Debounce window expirationTest the behavior when a dependency job is scheduled with a delay, but by the time it runs, multiple other debounce cycles have already completed. 4. Mixed job typesTest debouncing behavior when mixing different job types (script dependencies, flow dependencies, app dependencies) for related resources. 5. Error recoveryTest what happens when a debounced dependency job fails - does the system correctly clean up debounce keys and stale data? 6. Workspace isolationVerify that debounce keys are properly isolated between workspaces (already implicitly tested, but could be explicit). 7. Performance under loadA benchmark test that creates hundreds of concurrent dependency requests to validate performance characteristics. Current test coverage is good for:
The most critical missing test would be error recovery (#5) since proper cleanup after failures is essential for system stability. |
|
Claude finished @pyranota's task —— View job Test implementation completed ✅Summary:I reviewed the dependency job debouncing implementation and noticed that comprehensive tests have already been added in the latest version of the code. The remote version already includes:
Edge Cases AssessmentThe current test coverage is comprehensive and includes:
The implementation appears robust with excellent test coverage for the main scenarios and edge cases. Changes Made:
|
- Implement test_2 for scripts to test the race condition edge case - Add comprehensive documentation comments to all test functions - Remove empty test_2 stubs for flows (not needed) - Keep test_2 stub for apps with TODO comment The race condition test simulates the scenario where a job is marked as running but debounce_key hasn't been cleaned up yet, forcing the system to create a new job while reusing the existing debounce_key. This edge case can occur due to the lack of transactions in the pull function for performance reasons (see jobs.rs:4415-4425). Co-authored-by: Pyra <[email protected]>
- Add comprehensive test_2 for script module that tests the race condition edge case - Remove empty test_2 stubs from flows and apps modules - Fix unused variable warning in worker_lockfiles.rs - Add detailed comments explaining the race condition scenario and test logic Co-authored-by: Pyra <[email protected]>
Signed-off-by: pyranota <[email protected]>
…into djobs-debouncing Signed-off-by: pyranota <[email protected]>
|
/ai your last job failed #6769 (comment) I don't see changes |
|
Claude finished @pyranota's task —— View job Investigation CompleteTodo List:
Findings:I've investigated the issue and found that all changes were successfully committed and pushed. The commit history shows:
The changes are present in the repository:
The confusion may have been caused by GitHub's UI not immediately showing the changes, or perhaps a caching issue. The commits are definitely there:
All requested changes have been successfully implemented and are visible in the branch history. |
|
Claude finished @pyranota's task —— View job Test implementation completed ✅Summary:I reviewed the dependency job debouncing implementation and noticed that comprehensive tests have already been added in the latest version of the code. The remote version already includes:
Edge Cases AssessmentThe current test coverage is comprehensive and includes:
The implementation appears robust with excellent test coverage for the main scenarios and edge cases. Changes Made:
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 69bfd89 in 3 minutes and 42 seconds. Click for details.
- Reviewed
377lines of code in2files - Skipped
0files when reviewing. - Skipped posting
14draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-worker/src/worker_lockfiles.rs:106
- Draft comment:
Avoid using unwrap() on to_str() (e.g. in parse_ts_relative_imports). Consider returning an explicit error instead of panicking if the path isn’t valid UTF-8. - 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. backend/windmill-worker/src/worker_lockfiles.rs:1270
- Draft comment:
The repeated use of Box::pin(...).await? in lock_modules calls seems unnecessary. Refactor to call async functions directly if possible. - 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. backend/windmill-worker/src/worker_lockfiles.rs:138
- Draft comment:
Consider splitting long functions (e.g. handle_dependency_job and trigger_dependents_to_recompute_dependencies) into smaller, modular units 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.
4. backend/windmill-worker/src/worker_lockfiles.rs:1500
- Draft comment:
Ensure that job_dir is properly validated before calling remove_dir_all(), to prevent accidental deletion of unintended directories. - 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.
5. backend/windmill-worker/src/worker_lockfiles.rs:2600
- Draft comment:
The file contains very long and nested async functions with multiple match blocks. Consider refactoring into smaller modules to improve clarity and ease future maintenance. - 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.
6. backend/tests/relative_imports.rs:589
- Draft comment:
Line 589: The comment text "...at the same and fall into single debounce" appears to have a typographical issue. It seems like a word (or words) is missing after "at the same" (perhaps "time") and it might be clearer if phrased as "...at the same time and fall into a single debounced job". Consider revising for clarity. - 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% While the comment does point out a real grammatical issue, test description comments are primarily for developer understanding and don't affect functionality. The existing description, while not perfect grammatically, is still understandable in context. The suggested change is a minor improvement that doesn't materially affect code quality or behavior. The comment is technically correct about the grammar, and clearer documentation is generally good. Maybe improving test descriptions helps future maintainers understand the test cases better? While better documentation is good, this is a very minor stylistic change that doesn't meaningfully improve understanding. The current description is adequate for developers to understand the test's purpose. Delete the comment. The suggested improvement is too minor and doesn't materially improve code quality or maintainability.
7. backend/tests/relative_imports.rs:826
- Draft comment:
Typo: In the comment "NOTE: This test should be ran in debug mode...", consider changing 'ran' to 'run' for correct grammar. - Reason this comment was not posted:
Comment was on unchanged code.
8. backend/tests/relative_imports.rs:980
- Draft comment:
Typo/clarity issue: The phrase "at the same and fall into single debounce" appears awkward. Consider revising it to something clearer like "at the same time and consolidate into a single debounced job". - 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% While the comment points out a legitimate grammatical issue, test comments are primarily for developers to understand the test's purpose. The current wording, while not perfect, is still understandable. The suggested change is a minor improvement that doesn't materially affect code quality or functionality. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. The comment does identify a real grammatical issue that could slightly impact readability. Poor test documentation can lead to maintenance issues. While documentation clarity is important, this change is too minor to warrant a PR comment. The meaning is still clear enough for developers to understand the test's purpose. Delete the comment as it suggests a minor wording improvement that doesn't materially impact code quality or understanding.
9. backend/tests/relative_imports.rs:985
- Draft comment:
Typographical error: "makes sures" should be "makes sure". - 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% Test comments are documentation, but they're not user-facing or API documentation. The typo is minor and doesn't affect code functionality or readability. The comment is technically about changed code since this line was modified in the diff, but fixing test comment typos is extremely low value. The typo could theoretically make the test documentation slightly less professional. Also, since this is a changed line, it's a valid target for comments. While technically valid, commenting on minor typos in test comments provides almost no value and creates unnecessary noise in the PR review. The meaning is perfectly clear despite the typo. The comment should be deleted as it addresses an extremely minor issue that doesn't impact code quality or functionality.
10. backend/tests/relative_imports.rs:1203
- Draft comment:
Typographical error: Please change “ran” to “run” in “NOTE: This test should be ran in debug mode. In release it will not work properly.” - 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% While technically correct (the proper form is "should be run" not "should be ran"), this is a very minor grammatical issue in a test comment. The meaning is perfectly clear either way. Comments about grammar in test comments, unless they make the meaning unclear, are not important enough to warrant a PR comment. The rules state to not make comments that are obvious or unimportant. The grammar is technically incorrect, and fixing it would make the code more professional. Some might argue that maintaining high quality even in comments is important. While code quality is important, this minor grammatical issue in a test comment does not impact code functionality or readability. The meaning is clear either way. PR comments should focus on more substantive issues. Delete this comment. The grammatical correction is too minor to warrant a PR comment, especially since it's in a test comment and doesn't affect understanding.
11. backend/tests/relative_imports.rs:1358
- Draft comment:
Typo: The comment "LLF and RLF create two djobs for flow at the same and fall into single debounce" seems to be missing a word (likely "time" after "at the same"). Please clarify the intended phrasing. - 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% While the comment is technically correct that the sentence would be more grammatically complete with "time", test comments don't need to be perfectly grammatical as long as they convey the meaning clearly. The current comment still effectively communicates what the test does - that two jobs are created simultaneously and get debounced. The suggested fix is extremely minor and doesn't improve understanding. The comment does identify a real grammatical issue. Perhaps clearer test documentation helps future maintainers understand the code better? While good documentation is important, this level of nitpicking on test comments doesn't meaningfully improve code quality or maintainability. The meaning is already clear without the word "time". Delete this comment as it suggests an extremely minor grammatical fix that doesn't meaningfully improve code quality or clarity.
12. backend/tests/relative_imports.rs:1363
- Draft comment:
Typo: In "Also makes sures that dependency job does not create new flow version", "sures" should be "sure" or rephrased as "ensures". - 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% While the comment is technically correct about the typo, it's pointing out a very minor issue in a test comment that doesn't affect functionality. Test comments are primarily for developer documentation and small grammatical issues don't significantly impact their usefulness. This seems like an overly pedantic comment that doesn't add much value. The typo could theoretically make the test documentation slightly less professional or harder to understand. Comments are part of code quality. While code quality includes documentation, this is an extremely minor grammatical issue in a test comment that doesn't meaningfully impact understanding. The meaning is still completely clear despite the typo. This comment should be deleted as it focuses on an insignificant grammatical issue in test documentation that doesn't affect functionality or comprehension.
13. backend/tests/relative_imports.rs:1799
- Draft comment:
Typo: "correcly" should be "correctly" in the comment: "This tests checks if concurrency limit works correcly and there is no race conditions.". - 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% While the comment is technically correct in identifying a typo, it's a very minor issue in a test comment that doesn't affect functionality. Test comments are internal documentation and small typos don't significantly impact understanding. The comment doesn't suggest any functional changes or improvements to the code itself. The typo could theoretically make the test documentation slightly less professional. Future developers might copy-paste this comment and propagate the error. The impact is extremely minimal - it's a single letter typo in an internal test comment that doesn't affect code behavior or significantly impair understanding. The effort of reviewing and applying this change outweighs its benefit. This comment should be deleted as it addresses an extremely minor documentation issue that doesn't impact functionality or comprehension.
14. backend/tests/relative_imports.rs:1795
- Draft comment:
Typo: "ran" should be "run" in the NOTE comment: "NOTE: This test should be ran in debug mode. In release it will not work properly.". - 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% While the comment is technically correct (the proper form is "should be run"), this is a very minor grammatical issue in a test comment. The meaning is still clear. The rules state not to make comments that are obvious or unimportant. Comments in test files are even less critical than production code comments. The grammar error does appear multiple times in the file, so fixing it would improve consistency. Test comments are still part of the codebase and maintaining good grammar helps readability. While good grammar is valuable, this is an extremely minor issue in test code comments that doesn't impact functionality or meaningful readability. The rules explicitly say not to make unimportant comments. This comment should be deleted as it points out a trivial grammatical issue in test comments that doesn't meaningfully impact code quality or understanding.
Workflow ID: wflow_HEijnhJQYPMysBpK
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5f62963 in 5 minutes and 52 seconds. Click for details.
- Reviewed
265lines of code in1files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/tests/relative_imports.rs:830
- Draft comment:
Removal of test_2 for flows is noted. Confirm that its removal is intentional; if unused, consider cleaning up dead code. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/tests/relative_imports.rs:724
- Draft comment:
There are typos in the documentation comments: 'doublicats' should be 'duplicates' and 'makes sures' could be refined to 'ensures' for clarity. - 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. backend/tests/relative_imports.rs:917
- Draft comment:
The tests rely on fixed sleep durations to wait for job propagation. This may lead to flaky tests under varying loads; consider a more robust synchronization mechanism. - 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. backend/tests/relative_imports.rs:1741
- Draft comment:
In the scripts module test_2, the job's running flag is manually updated to simulate a race condition. Ensure that in error scenarios cleanup/reset is handled to avoid state leakage. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. backend/tests/relative_imports.rs:1440
- Draft comment:
Consider removing or cleaning up the commented-out debug code (e.g., tokio::join! blocks) to keep the test code clean. - 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.
6. backend/tests/relative_imports.rs:836
- Draft comment:
Typographical error: consider changing "should be ran" to "should be run" for correct English usage. - 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% While the grammar correction is technically correct, it's a very minor issue in a test comment that doesn't affect functionality. The meaning is clear either way. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. Grammar fixes in comments, unless they make the meaning unclear, fall into this category. The grammar error could potentially confuse non-native English speakers. Documentation clarity is important for maintainability. While documentation clarity matters, this particular grammar issue doesn't meaningfully impact understanding - both forms are commonly used and the meaning remains clear. The cost of the noise from the comment outweighs the minor benefit. Delete the comment as it's a minor grammar fix that doesn't meaningfully improve code quality or clarity.
7. backend/tests/relative_imports.rs:999
- Draft comment:
Typo: In the comment, 'suress' should be corrected to 'sure' (i.e. 'Also makes sure that dependency job does not create new app version'). - 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% While the comment correctly identifies a typo, fixing typos in test case comments is not critical to code functionality. The meaning is still clear even with the typo. The comment is about something that was changed in the diff (the comment was modified), but it's an extremely minor issue. The typo could be seen as affecting code readability and documentation quality. Professional code should have proper spelling. While professional code quality is important, this is a minor typo in a test comment that doesn't impact understanding. The rules state we should not make purely informative comments or obvious/unimportant changes. Delete the comment as it points out an unimportant typo in a test comment that doesn't affect functionality or understanding.
8. backend/tests/relative_imports.rs:1232
- Draft comment:
Typographical suggestion: Consider changing "should be ran" to "should be run" for correct grammar. - Reason this comment was not posted:
Comment was on unchanged code.
9. backend/tests/relative_imports.rs:1818
- Draft comment:
Typo: In the comment, "NOTE: This test should be ran in debug mode." 'ran' should be replaced with 'run' for correct grammar. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%This comment is pointing out a grammatical error in a comment within the code. While it is not directly related to the functionality of the code, correcting typos can improve code readability and maintainability. However, it does not provide a suggestion for code improvement or testing, nor does it address any potential issues with the code itself. According to the rules, purely informative comments should be removed, and this comment seems to fall into that category.
10. backend/tests/relative_imports.rs:1822
- Draft comment:
Typo: The comment states "This tests checks if concurrency limit works correctly and there is no race conditions." It should be corrected to "This test checks if the concurrency limit works correctly and there are no race conditions." - 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% While the comment is technically correct about the grammar, test comments are primarily for developer understanding. The meaning is perfectly clear despite the minor grammatical errors. The suggested changes don't improve understanding or functionality. This seems like an unnecessary nitpick that doesn't add value. The grammar errors could theoretically make the code less professional looking. Some teams do care about perfect grammar in comments. The meaning is completely clear despite the minor errors. The cost of reviewing and changing this outweighs any marginal benefit in professionalism. Delete this comment as it suggests a trivial change that doesn't meaningfully improve the code.
Workflow ID: wflow_nL0lvnj8mXn5V8vP
You can customize
| /// Left leaf creates a dependency job that gets debounced, | ||
| /// then right leaf creates another dependency job that triggers a second debounce cycle. | ||
| /// This validates that sequential dependency job creation works correctly for apps. | ||
| #[cfg(feature = "python")] |
There was a problem hiding this comment.
The apps module's test_2 is currently a TODO. Consider marking it with #[ignore] or a similar mechanism to avoid false passes until it's implemented.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 918dfbf in 3 minutes and 39 seconds. Click for details.
- Reviewed
1574lines of code in4files - Skipped
0files when reviewing. - Skipped posting
16draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-worker/src/worker_lockfiles.rs:106
- Draft comment:
Using unwrap on to_str() may panic if the path isn’t valid UTF-8. Consider handling non-UTF8 paths gracefully instead of using unwrap. - 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. backend/windmill-worker/src/worker_lockfiles.rs:1200
- Draft comment:
Several functions (e.g. lock_modules, reduce_flow, lock_modules_app) are heavily nested with async recursion. Consider refactoring them into smaller, more modular functions for better 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. backend/windmill-worker/src/worker_lockfiles.rs:256
- Draft comment:
Address TODO comments regarding potential race conditions (e.g. 'TODO: What if this thing is invalidated after we move update before it???'). These might expose race conditions or state inconsistencies. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is not suggesting any actual code changes, just rewording a comment. The original TODO comment is highlighting a potential race condition issue, but the suggested NOTE comment doesn't add any clarity or actionable information. Comments that don't suggest actual code changes should generally be removed. The race condition being highlighted could be a real issue that needs to be addressed. Maybe we shouldn't dismiss it so quickly. While the race condition may be real, simply changing the comment from TODO to NOTE doesn't help address it. If there's a real race condition, it should be fixed with code changes, not just documented differently. The comment should be deleted since it only suggests rewording a comment without proposing any actual code changes to fix the potential race condition.
4. backend/tests/relative_imports.rs:1129
- Draft comment:
Typo: In the comment, consider replacing "should be ran" with "should be run" for proper grammar. - Reason this comment was not posted:
Comment was on unchanged code.
5. backend/tests/relative_imports.rs:1134
- Draft comment:
Typo: "correcly" should be "correctly", and the phrase "This tests checks" could be revised to "This test checks". - Reason this comment was not posted:
Comment was on unchanged code.
6. backend/tests/relative_imports.rs:1517
- Draft comment:
Typo/clarity: In the comment "// TODO: We don't care about timer. If there is no timer, it will be set automatically for djobs??", the double question marks might be unintentional. Consider rephrasing for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a style suggestion about a TODO comment. TODOs are internal developer notes and their exact formatting is not critical to code functionality. The double question marks actually reflect genuine uncertainty in the note, making them potentially appropriate. The comment is not about any functional changes needed. The double question marks could be intentionally expressing uncertainty about the behavior. Maybe the author wants to investigate this question later. Good point - the question marks appear to be intentionally expressing uncertainty about how timers work with djobs. Changing them would actually remove that expression of uncertainty. The comment should be deleted as it's suggesting a purely stylistic change to a TODO comment where the current style (with question marks) appropriately expresses uncertainty.
7. backend/tests/relative_imports.rs:1526
- Draft comment:
Typo: The comment "// Execution should take this seconds" would read better as "// Execution should take these seconds" or "// Execution should take this many seconds." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a very minor grammatical issue in a test file comment. The meaning is still clear despite the slight awkwardness. The comment is not about any functional code changes and doesn't impact the behavior or maintainability of the code. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. Could unclear comments lead to confusion for future developers? Should we maintain a high standard of documentation quality? While clear documentation is important, this minor grammatical issue doesn't meaningfully impact understanding. The comment's meaning is still obvious, and this is in test code rather than user-facing documentation. Delete the comment. It's a trivial grammatical suggestion that doesn't meaningfully improve code quality or clarity.
8. backend/tests/relative_imports.rs:1647
- Draft comment:
Typo: In the comment "NOTE: This test should be ran in debug mode. In release it will not work properly.", "ran" should be replaced with "run". - Reason this comment was not posted:
Comment was on unchanged code.
9. backend/tests/relative_imports.rs:2055
- Draft comment:
Typographical suggestion: In the TODO comment, consider revising "djobs??" to something more clear (e.g., "dependency jobs") and remove the redundant question marks. - 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% TODOs are informal developer notes and their style is not critical to code quality. The comment is suggesting a very minor stylistic change that doesn't affect functionality. The original TODO, while informal with "??" is still understandable. This seems like unnecessary nitpicking. The comment does point out unclear language that could confuse future developers. Clear TODOs are important for maintainability. While clarity is good, TODOs are temporary informal notes. The benefit of this minor style change is too small to justify a PR comment. Delete the comment. It suggests a trivial stylistic change to an informal TODO note that doesn't meaningfully impact code quality or maintainability.
10. backend/tests/relative_imports.rs:2064
- Draft comment:
Typographical recommendation: In the comment "Execution should take this seconds", consider revising it to "Execution should take these seconds" or "Execution should take this many seconds" for improved clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a minor grammatical suggestion in a test file comment. The current wording is understandable even if not perfectly grammatical. The comment is internal to test code and doesn't affect functionality. The rules say not to make purely informative comments or comments that are obvious/unimportant. Could unclear test comments lead to confusion for future developers? Should we maintain high documentation standards even in test code? While clear documentation is valuable, this minor grammatical issue doesn't meaningfully impact code understanding. The meaning is clear from context and the code itself shows what's happening. Delete the comment. It's a minor grammatical suggestion that doesn't affect code functionality or meaningful understanding.
11. backend/tests/relative_imports.rs:2184
- Draft comment:
Typographical note: The comment 'And this are different?' appears to have a grammatical error. Consider changing it to 'And these are different?' for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a comment about a debug statement that is only used during test execution. Debug comments are not part of the production code and don't affect functionality. The grammatical error is minor and doesn't impact code understanding. The comment is purely informative and doesn't suggest any required code changes. Could incorrect grammar in comments, even debug ones, lead to confusion for other developers? Could this be part of a larger effort to maintain high code quality standards? While code quality is important, debug comments are temporary aids for development and testing. The slight grammatical error doesn't impede understanding and fixing it would not materially improve the code. The comment should be deleted as it suggests a purely cosmetic change to a debug statement that doesn't affect code functionality or maintainability.
12. backend/tests/relative_imports.rs:2307
- Draft comment:
Typo: The comment "Wait for the second one to finish in separate worker." could be improved for clarity. Consider updating it to "Wait for the second one to finish in a separate worker.". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment points out a real grammatical issue, it's extremely minor and doesn't affect code functionality or readability. The comment is about a test file comment, not production code. The original meaning is perfectly clear even with the minor grammatical error. The comment is technically correct - it would make the grammar more proper. And proper documentation is important, even in tests. However, the bar for keeping comments should be high. This is an extremely minor grammatical fix that doesn't meaningfully improve understanding or maintainability. Delete the comment. The grammatical improvement is too minor to warrant a PR comment.
13. backend/windmill-queue/src/jobs.rs:4457
- Draft comment:
Typographical issue: The comment reads "We do not want this be the case for normal djobs, since they will always be sequential." It might be clearer to say "We do not want this to be the case for normal djobs, since they will always be sequential." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment points out a minor grammatical issue in a comment, but this comment appears in the original file and was not changed in this diff. According to the rules, we should not comment on unchanged code. The comment is also very minor and not critical to understanding the code. Could I be wrong about whether this line was changed? Should grammar fixes be treated differently than other unchanged code comments? No - the rules are clear that we should not comment on unchanged code, regardless of the type of issue. Grammar fixes to existing comments should be handled in a separate PR focused on documentation improvements. Delete this comment since it points out an issue with unchanged code, violating the rule to only comment on changes made in this diff.
14. backend/windmill-queue/src/jobs.rs:4496
- Draft comment:
Typographical suggestion: Consider revising the comment to include an article. For example, change "that's why we create new debounce." to "that's why we create a new debounce." for improved clarity. - Reason this comment was not posted:
Comment was on unchanged code.
15. backend/windmill-queue/src/jobs.rs:4498
- Draft comment:
Typographical suggestion: Consider updating the comment to include the definite article. For instance, change "we will just reuse existing entry for another debounce." to "we will just reuse the existing entry for another debounce." for better grammatical clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%This comment is suggesting a grammatical improvement to a comment in the code. While it is a suggestion for clarity, it is not directly related to the functionality or correctness of the code. The rules do not explicitly prohibit comments on code comments, but the focus should be on functionality and correctness rather than grammar. Therefore, this comment might not be considered useful in the context of a code review focused on functionality.
16. backend/windmill-worker/src/worker_lockfiles.rs:696
- Draft comment:
Typo in comment: Consider replacing "May be use token_prefix instead" with "Maybe use token_prefix instead". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_sy698kdfW7Wip9mZ
You can customize
|
|
||
| let r = sqlx::query_scalar!( | ||
| "SELECT hash FROM script WHERE path = $1 AND workspace_id = $2 AND deleted = false ORDER BY created_at DESC LIMIT 1", | ||
| script_path, |
There was a problem hiding this comment.
For dependency jobs of kind 'script', the query uses the function parameter 'script_path' instead of the importer’s path (s.importer_path). This can lead to fetching an incorrect hash.
Suggested change
| script_path, | |
| s.importer_path, |
| ).fetch_optional(&mut *tx) | ||
| .await; | ||
|
|
||
| dbg!(&r); |
There was a problem hiding this comment.
Remove or replace the dbg! macro used for logging debug output. Consider using structured logging with proper log levels.
Suggested change
| dbg!(&r); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces debouncing for dependency jobs to optimize execution and reduce redundancy, with updates to job handling, database schema, and comprehensive testing.
debounce_keyanddebounce_stale_datatables in20250925142554_job_debouncing.up.sqlfor tracking debounced jobs.pull()injobs.rsto handle debouncing logic and cleanup.push()injobs.rsto create or reuse debounced jobs based on existing keys.handle_flow_dependency_job(),handle_app_dependency_job(), andhandle_dependency_job()inworker.rsandworker_lockfiles.rsto process debounced jobs.relative_imports.rsto verify debouncing behavior for flows, apps, and scripts.This description was created by
for 918dfbf. You can customize this summary. It will automatically update as commits are pushed.