Skip to content
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

feat: track workspace runnables used in flows #5369

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Feb 24, 2025

Important

Add feature to track workspace runnables in flows by creating flow_workspace_runnables table and updating related API and code logic.

  • Database:
    • Create flow_workspace_runnables table in 20250224131521_flow_workspace_runnables.up.sql with columns for flow_path, runnable_path, script_hash, runnable_is_flow, and workspace_id.
    • Add unique indexes on flow_workspace_runnables for entries with and without script_hash.
    • Add queries in .sqlx files to insert, update, and delete entries in flow_workspace_runnables.
  • API:
    • Add endpoint /w/{workspace}/flows/list_paths_from_workspace_runnable/{runnable_kind}/{path} in openapi.yaml to list flow paths from a workspace runnable.
    • Implement list_paths_from_workspace_runnable in flows.rs to query flow_workspace_runnables.
  • Code Changes:
    • Update update_flow in flows.rs to delete entries from flow_workspace_runnables when a flow is updated.
    • Modify update_username_in_workpsace in users.rs to update flow_workspace_runnables when a username changes.
    • Add logic in worker_lockfiles.rs to insert entries into flow_workspace_runnables when locking modules.

This description was created by Ellipsis for e87d27b. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Feb 24, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: e87d27b
Status: ✅  Deploy successful!
Preview URL: https://4c40ff3c.windmill.pages.dev
Branch Preview URL: https://hugo-win-996-track-workspace.windmill.pages.dev

View logs

@HugoCasa HugoCasa marked this pull request as ready for review February 24, 2025 21:37
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to d4a3829 in 4 minutes and 53 seconds

More details
  • Looked at 457 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 19 drafted comments based on config settings.
1. backend/windmill-api/src/workspaces_extra.rs:29
  • Draft comment:
    The change_workspace_id function is very long and complex. Consider splitting it into smaller helper functions to improve readability and ease of 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.
2. backend/windmill-api/src/workspaces_extra.rs:52
  • Draft comment:
    Verify that duplicating the workspace record via INSERT INTO workspace SELECT ... preserves all necessary fields and foreign-key constraints. A comment explaining the rationale would help.
  • 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-api/src/workspaces_extra.rs:140
  • Draft comment:
    Many similar UPDATE queries are executed sequentially to change the workspace_id across tables. Consider refactoring these into a loop or helper that iterates over a list of table names to DRY up the code and reduce maintenance burden.
  • 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-api/src/workspaces_extra.rs:208
  • Draft comment:
    Before deleting the old workspace (line 208 onward) ensure that cascading effects on child records are as expected. Adding inline comments to justify the ordering and safety of deletions would be beneficial.
  • 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-api/src/workspaces_extra.rs:29
  • Draft comment:
    The change_workspace_id function issues many similar update/insert queries to migrate workspace data. Consider refactoring these repetitive queries into helper functions or using database cascading/stored procedures to improve 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.
6. backend/windmill-api/src/workspaces_extra.rs:172
  • Draft comment:
    The duplication of workspace flows via an INSERT ... SELECT is complex. Please add a comment explaining why flows are duplicated instead of simply updating the workspace_id.
  • 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.
7. backend/windmill-api/src/workspaces_extra.rs:52
  • Draft comment:
    Multiple sequential update statements (e.g. updating account, app, audit, capture, capture_config, http_trigger, etc.) can be consolidated into batch operations or a stored procedure to improve performance and reduce transaction overhead.
  • 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.
8. backend/windmill-api/src/workspaces_extra.rs:170
  • Draft comment:
    The ordering of update and delete queries is critical for maintaining referential integrity. Consider adding inline comments to explain the required sequence to avoid future mistakes.
  • 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.
9. backend/windmill-api/src/workspaces_extra.rs:408
  • Draft comment:
    The delete_workspace function contains many DELETE statements. Evaluate if cascading deletes or modular functions could simplify this logic, and add inline documentation describing the deletion order to ensure foreign key constraints are satisfied.
  • 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.
10. backend/windmill-api/src/workspaces_extra.rs:250
  • Draft comment:
    Consider the performance impact of numerous individual update/delete queries on large workspaces. Bulk operations or optimized indexing strategies might be required.
  • 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.
11. backend/windmill-api/src/workspaces_extra.rs:390
  • Draft comment:
    Adding more inline documentation within these complex functions would greatly help future maintainability. Explain the rationale behind each major block (e.g., duplicating flows, handling group_ data, etc.).
  • 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.
12. backend/.sqlx/query-2bb2cf6accb18d3e37a63388cca52a6591e7593b1a7c3d7a6848587679a48187.json:7
  • Draft comment:
    Typographical Note: The key 'Left' at line 7 uses a capitalized form, whereas the other keys are lowercase. Please confirm if this is intentional or if it should be changed 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%
  1. This is an auto-generated file by sqlx, not hand-written code. 2. The 'Left' capitalization is likely intentional and part of sqlx's schema format. 3. Changing auto-generated code is generally not recommended. 4. The comment is asking for confirmation rather than pointing out a clear issue.
    I might be wrong about this being auto-generated, or there might be a way to configure the generation format.
    Even if configurable, this is a minor stylistic issue in what appears to be an auto-generated file, and the comment is asking for confirmation rather than stating a clear problem.
    Delete this comment as it's asking for confirmation about a likely-intentional capitalization in what appears to be an auto-generated file.
13. backend/.sqlx/query-9aeee333b1dbe58ba819ba3b2713242b54d77b46b5f785ae66b8104f89f43219.json:3
  • Draft comment:
    There appears to be a typographical error in the JOIN condition of the SQL query. The current condition:

    ON fwr.flow_path = f.path AND fwr.workspace_id = f.workspace_id

likely intends to compare 'fwr.workspace_id' with 'f.workspace_id' instead of comparing it to itself. Please update this to ensure the join is correct.

  • 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 shows a misunderstanding of SQL JOIN syntax. When joining tables, comparing columns with the same name from different tables is completely valid and common. The alias 'fwr' refers to flow_workspace_runnables table and 'f' refers to flow table, so we're correctly joining on matching workspace_ids between these tables.
    Could there be some domain-specific reason why these workspace_ids shouldn't be compared? Could there be a different intended join condition?
    No - this is a standard SQL join pattern, and the comparison between workspace_ids from different tables is clearly intentional as part of ensuring data consistency across tables.
    The comment should be deleted as it incorrectly identifies a valid JOIN condition as an error.
14. backend/.sqlx/query-acbea8740b28c26942c50edcf5618cd141e68cf83a7dcae7d3c1b8a7ba94425b.json:7
  • Draft comment:
    The key 'Left' in the 'parameters' object may be a typographical error. Consider using a consistent naming convention (e.g., 'left' or another appropriate key) if this is unintentional.
  • 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%
  1. This is an auto-generated file by sqlx, not hand-written code. 2. "Left" is likely a valid enum variant in the sqlx Rust code that generates these files. 3. The naming convention is determined by the sqlx library, not something the developer can or should change. 4. Making changes to auto-generated files is generally not recommended.
    Maybe "Left" actually is a mistake and the developer has control over this naming through some configuration.
    Given this is an auto-generated file from a well-established Rust SQL toolkit, it's extremely unlikely that "Left" is a mistake - it's more likely a deliberate part of sqlx's type system.
    The comment should be deleted as it suggests changing an auto-generated file's structure, which would likely break functionality and be overwritten anyway.
15. backend/migrations/20250224131521_flow_workspace_runnables.up.sql:7
  • Draft comment:
    Line 7 uses a tab for indentation instead of using spaces like the other lines. This is a minor stylistic inconsistency that should be fixed to maintain consistent formatting throughout the file.
  • 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 inconsistency, SQL formatting is generally less strict than programming languages. The indentation difference doesn't affect the SQL execution. This seems like an extremely minor stylistic issue that doesn't impact functionality or readability significantly.
    The inconsistent formatting could make the file harder to maintain if there's a strict style guide in place. Some teams do care deeply about consistent formatting.
    However, this is a one-off migration file that won't be frequently maintained. The benefit of fixing this minor formatting issue doesn't justify the overhead of a code review comment.
    This comment should be removed as it addresses an extremely minor formatting issue in a SQL migration file that doesn't impact functionality or meaningful readability.
16. backend/windmill-api/src/flows.rs:1192
  • Draft comment:
    Typographical Error: In the SQL query inside the get_flow_by_path_w_draft function, 'flow,description' appears to be a typo. It should likely be 'flow.description' to correctly reference the description column of the 'flow' table.
  • 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.
17. backend/windmill-api/src/users.rs:2294
  • Draft comment:
    Typo: The function name 'update_username_in_workpsace' appears to have a misspelling. It should likely be 'update_username_in_workspace' to consistently reflect what the function does.
  • 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.
18. backend/windmill-api/src/users.rs:1160
  • Draft comment:
    Typo: The error message contains the word 'Usermame' which should be corrected to 'Username'. This typo appears in at least two places (e.g., in join_workspace and rename_user functions).
  • 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.
19. backend/windmill-api/src/utils.rs:110
  • Draft comment:
    There's a small typographical issue in the comment where it states 'get users that have a no instance username and either 1 or 0 workspace usernames'. Consider rephrasing it for clarity, such as 'get users without an instance username and with at most one workspace username'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_U2BsXuErgunIhive


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 3a861cd in 1 minute and 35 seconds

More details
  • Looked at 244 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_lockfiles.rs:188
  • Draft comment:
    Using unwrap() on to_str() in try_normalize may panic for non‑UTF8 paths. Consider using to_string_lossy() or proper error handling.
  • 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:820
  • Draft comment:
    Repeatedly calling e.get_value()? in lock_modules can be optimized by caching the result to avoid duplicate parsing.
  • 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:1412
  • Draft comment:
    Avoid unwrapping inlineScript with unwrap(); handle the possibility of a missing key to prevent potential panics.
  • 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:1241
  • Draft comment:
    In reduce_flow, error handling for parsing module values could be more robust rather than replacing with Identity and using unreachable!().
  • 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:97
  • Draft comment:
    SQL queries using 'ON CONFLICT DO NOTHING' effectively mask database conflicts. Ensure conflicts aren’t hiding important issues.
  • 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/windmill-worker/src/worker_lockfiles.rs:190
  • Draft comment:
    Using unwrap on to_str() may panic if the path isn’t valid UTF-8. Consider handling the conversion 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.
7. backend/windmill-worker/src/worker_lockfiles.rs:98
  • Draft comment:
    The SQL query in add_relative_imports_to_dependency_map uses a non-sequential parameter order ($1, $2, $4, $3, $5). Please add a comment clarifying this mapping.
  • 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.
8. backend/windmill-worker/src/worker_lockfiles.rs:1385
  • Draft comment:
    The logic in skip_creating_new_lock is non-intuitive, particularly for Bun vs Bunnative. Please clarify the intended behavior (e.g. when to skip creating a new lock) with inline comments.
  • 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.
9. backend/windmill-worker/src/worker_lockfiles.rs:844
  • Draft comment:
    The repeated use of Box::pin() for recursive async calls in lock_modules makes the code verbose. Consider refactoring this recursion to simplify the async flow 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.
10. backend/windmill-worker/src/worker_lockfiles.rs:1
  • Draft comment:
    This file is very large and complex. Consider breaking it into smaller modules to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/windmill-api/src/flows.rs:1090
  • Draft comment:
    Typographical error: In the SQL query in get_flow_by_path_w_draft, the column is written as 'flow,description'. It should likely be 'flow.description'. Please correct this typo.
  • 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.
12. backend/windmill-worker/src/worker_lockfiles.rs:808
  • Draft comment:
    Typo: In the comment on line 808, "unmmodified" should be corrected to "unmodified".
  • 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.
13. backend/windmill-worker/src/worker_lockfiles.rs:1688
  • Draft comment:
    Typo: In the comment starting around line 1688, "precendence" should be corrected to "precedence".
  • 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.
14. backend/windmill-worker/src/worker_lockfiles.rs:1610
  • Draft comment:
    Typo: In the comment regarding cancellation (around line 1610), "accidentially" should be corrected to "accidentally".
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_pN9kXV9NATizZ5yq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on bb8f709 in 2 minutes and 24 seconds

More details
  • Looked at 147 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. backend/parsers/windmill-parser-py/src/lib.rs:96
  • Draft comment:
    Changing from a tuple (Typ, bool) to only Typ removes the prior mechanism to mark a parameter as having a default derived from a binary op (Optional) expression. Confirm that always using default.is_some() for has_default is correct.
  • 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 removed code specifically handled Optional[T] by returning (parse_expr(&x.left).0, true) for expressions like T | None. However, looking at the test cases, there are no tests that rely on this behavior. The tests show that default values are properly handled through default.is_some(). The Optional[T] syntax seems to have been an alternative way to mark optionals that wasn't actually being used.
    I could be wrong about the Optional[T] syntax not being used - there could be real-world code using it that isn't covered by the test suite.
    While possible, the lack of test coverage and the fact that default.is_some() is a more direct way to determine optionality suggests this was likely dead code. The PR author probably verified this before making the change.
    The comment raises a valid point about a behavior change, but there's no strong evidence that the removed Optional[T] parsing was actually being used. The new approach using default.is_some() appears to be cleaner and sufficient.
2. backend/parsers/windmill-parser-py/src/lib.rs:158
  • Draft comment:
    The handling for binary operators and Optionals (detecting 'None' on the right) is removed—verify this change is intentional and doesn’t break Optional parameter type inference.
  • 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%
    Looking at the changes:
  1. The code for handling Optional types was deliberately removed
  2. The tests that were removed show that Optional type handling was intentionally removed
  3. The comment asks to "verify" the change is intentional, which violates our rules about not asking for verification
  4. The comment doesn't point out a specific issue, just asks for confirmation
    The code changes seem deliberate and the tests were updated accordingly. The comment might be raising a valid concern about Optional type handling being removed.
    While Optional type handling was removed, this appears to be an intentional change with corresponding test updates. The comment violates our rules by asking for verification rather than pointing out a specific issue.
    Delete this comment because it asks for verification rather than pointing out a specific issue, and the changes appear to be intentional based on the test updates.
3. frontend/package.json:145
  • Draft comment:
    The version for windmill-parser-wasm-py was downgraded from ^1.467.1 to ^1.429.0. Please confirm that this downgrade is intended.
  • 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 explicitly state not to comment on dependency changes or library versions. The comment is asking for confirmation of intention, which is also explicitly forbidden. The fact that other related packages use the same version suggests this was likely intentional version alignment.
    Could this version downgrade potentially introduce breaking changes that would affect functionality? Should we be more concerned about version downgrades vs upgrades?
    While version downgrades can be risky, the rules clearly state not to comment on dependency changes. If there are breaking changes, they would be caught by the build and tests, which we're told to trust.
    This comment should be deleted as it violates multiple rules - don't comment on dependency changes and don't ask authors to confirm their intentions.
4. backend/parsers/windmill-parser-py/src/lib.rs:96
  • Draft comment:
    Refactored parameter mapping: removed tuple destructuring from annotation parsing. Confirm that dropping the extra bool (used for Optional defaults) is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. backend/parsers/windmill-parser-py/src/lib.rs:138
  • Draft comment:
    Updated Arg construction: 'has_default' now relies solely on the existence of a default value. Double-check that parameters annotated as Optional no longer implicitly mark has_default true.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to double-check something, which violates the rules. It is not making a specific code suggestion or asking for a specific test to be written. Therefore, it should be removed.
6. backend/parsers/windmill-parser-py/src/lib.rs:161
  • Draft comment:
    parse_expr has been refactored to return only a Typ. The removed branches for BinOp and Optional handling simplify the code but may affect detection of implicit defaults in Optional annotations. Verify that this behavior change is acceptable.
  • 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%
    Looking at the code changes:
  1. The function signature changed from returning (Typ, bool) to just Typ
  2. The bool was used to track if a type was optional/had default
  3. The has_default field is now only set based on default.is_some()
  4. Looking at the tests, this appears to be the intended behavior - there are no failing tests
  5. The Optional type handling was likely redundant since has_default already tracks optionality
    I could be missing some edge cases where the Optional type annotation had different semantics than just having a default value. The tests may not cover all cases.
    The code appears to be deliberately simplified to unify the handling of optionality through the default value mechanism. The removed code paths were likely redundant rather than essential functionality.
    This comment should be deleted. The changes appear to be an intentional simplification that maintains the required behavior, as evidenced by the passing tests.
7. frontend/package.json:145
  • Draft comment:
    Version change: 'windmill-parser-wasm-py' downgraded from ^1.467.1 to ^1.429.0. Ensure that this aligns with expected behavior and compatibility with the updated parser logic.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_aBwjVokonjWMFc76


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@HugoCasa HugoCasa marked this pull request as draft February 25, 2025 10:32
@HugoCasa HugoCasa marked this pull request as ready for review February 25, 2025 11:16
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to e87d27b in 3 minutes and 5 seconds

More details
  • Looked at 427 lines of code in 16 files
  • Skipped 0 files when reviewing.
  • Skipped posting 25 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_lockfiles.rs:191
  • Draft comment:
    Avoid using unwrap() when converting a normalized PathBuf to a string. If to_str() fails, it will panic. Consider handling this possibility 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.
2. backend/windmill-worker/src/worker_lockfiles.rs:1390
  • Draft comment:
    Review the logic in skip_creating_new_lock. Its return value dictates whether a new lock should be generated. Adding inline comments or renaming the function might improve 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/windmill-worker/src/worker_lockfiles.rs:1024
  • Draft comment:
    Extensive use of Box::pin in recursive async functions (e.g. in lock_modules) makes the code complex. Consider refactoring or adding more documentation to clarify the control flow.
  • 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:1239
  • Draft comment:
    Consider adding additional documentation to the reduce_flow function. Its recursive processing of modules is complex; inline comments may help future maintainers.
  • 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:1440
  • Draft comment:
    When handling dependency jobs or triggering dependent recomputation, verify that the already_visited list is correctly maintained to prevent cycles. Consider adding comments to explain the safety guarantees.
  • 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/windmill-api/src/workspaces_extra.rs:29
  • Draft comment:
    This very long function performs many similar UPDATE/INSERT queries consecutively. Consider refactoring parts into smaller helper functions or batching updates for improved readability and performance.
  • 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.
7. backend/windmill-api/src/workspaces_extra.rs:41
  • Draft comment:
    The function checks for super-admin vs admin before changing the workspace id. Ensure that the workspace id conflict check via check_w_id_conflict is robust and that conflicts are handled 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.
8. backend/windmill-worker/src/worker_lockfiles.rs:817
  • Draft comment:
    The 'lock_modules' function is very complex with deeply nested match arms handling many flow module variants. Consider refactoring into smaller helper functions to improve maintainability and testability of the dependency-locking logic.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-worker/src/worker_lockfiles.rs:1019
  • Draft comment:
    The call to 'skip_creating_new_lock' determines if a new lock should be generated based on language and content. Verify that its logic regarding TypeScriptAnnotations for Bun vs Bunnative covers all cases and document the intended behavior clearly.
  • 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.
10. backend/windmill-worker/src/worker_lockfiles.rs:1027
  • Draft comment:
    In the branch for FlowModuleValue::Script, the code defaults a missing hash to 0 using 'unwrap_or'. Ensure that a default hash of 0 won't cause collisions or unexpected behavior in the dependency map.
  • 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.
11. backend/windmill-worker/src/worker_lockfiles.rs:1111
  • Draft comment:
    The 'lock_modules' function executes individual SQL queries inside a loop, which may become a performance bottleneck for many modules. Consider batching similar queries to improve performance.
  • 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.
12. backend/windmill-api/src/utils.rs:67
  • Draft comment:
    The helper 'generate_instance_wide_unique_username' uses a lazy_static regex. Consider using newer crates like once_cell for better ergonomics, and ensure that deriving a username from the email prefix is sufficiently unique.
  • 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.
13. backend/windmill-worker/src/worker_lockfiles.rs:1395
  • Draft comment:
    The decision logic in 'skip_creating_new_lock' for Bun/Bunnative appears convoluted. Adding inline comments or refactoring into a clearly named helper function could improve 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.
14. backend/windmill-worker/src/worker_lockfiles.rs:1396
  • Draft comment:
    The file contains several large functions with nested async recursions (e.g., lock_modules, reduce_flow, handle_dependency_job). Breaking these into smaller, documented functions would ease future maintenance and reduce cognitive load.
  • 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.
15. backend/windmill-worker/src/worker_lockfiles.rs:1690
  • Draft comment:
    In 'capture_dependency_job', multiple branches for different ScriptLang variants are handled. Ensure that each branch returns a proper lock file string and that the default cases are acceptable. Adding unit tests for these branches might also be beneficial.
  • 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.
16. backend/.sqlx/query-2bb2cf6accb18d3e37a63388cca52a6591e7593b1a7c3d7a6848587679a48187.json:7
  • Draft comment:
    The key 'Left' (line 7) is capitalized, which is inconsistent with the rest of the JSON keys that use lowercase (e.g., 'db_name', 'query', etc.). If this capitalization is unintentional, please consider changing 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 = 10% vs. threshold = 50%
    Since this is in a .sqlx directory and contains a hash, it's almost certainly generated by SQLx (a Rust SQL toolkit). The capitalization of 'Left' is likely intentional and part of SQLx's schema. We shouldn't try to modify generated files. Even if we did change it, it would likely just get overwritten next time the file is generated.
    Maybe this isn't actually a generated file? Maybe the capitalization really is a mistake that should be fixed?
    The presence of a hash and the .sqlx directory strongly indicates this is generated. Even if it weren't, the comment violates our rule about only keeping comments that show STRONG EVIDENCE of being correct.
    Delete this comment. We shouldn't modify generated files, and there's no strong evidence that the capitalization is actually wrong.
17. backend/.sqlx/query-c35f44f91b08fa57e29a2b4a685706f62e700695810f23108e975dfcd1fee7a3.json:7
  • Draft comment:
    Consider reviewing the key 'Left' in the 'parameters' object. It is capitalized while other keys in similar contexts are typically lowercase, which might be unintentional or inconsistent with your naming conventions.
  • 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%
    Since this is an auto-generated file by sqlx, the capitalization is likely intentional and part of the library's schema. The file is not meant to be edited manually. The comment is suggesting changing something that's generated by a tool, which would likely be overwritten anyway.
    Maybe there's a configuration in sqlx that could change this capitalization if it really is inconsistent with project standards?
    Even if there is such a configuration, this comment is still not useful because it's addressing auto-generated code, and we should trust the tool's defaults unless there's a compelling reason to change them.
    Delete this comment as it's suggesting changes to auto-generated code and doesn't indicate any actual problem that needs fixing.
18. backend/migrations/20250224131521_flow_workspace_runnables.down.sql:1
  • Draft comment:
    It appears that there is no newline at the end of the file. While not functionally problematic, adding a trailing newline can help maintain consistency with other files and avoid potential issues with some tools.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is purely informative and does not provide a specific code suggestion or ask for a test. It mentions a best practice but does not indicate a potential issue or ask for confirmation of intent. Therefore, it violates the rule against purely informative comments.
19. backend/windmill-api/openapi.yaml:9975
  • Draft comment:
    Typographical error: The operationId for the GET /configs/get/{name} endpoint is written as "get config". Please change it to "getConfig" for consistency.
  • 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.
20. backend/windmill-api/openapi.yaml:4579
  • Draft comment:
    Typographical error: The operationId is "geDefaultTags". Please correct this to "getDefaultTags" for consistency.
  • 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.
21. backend/windmill-api/openapi.yaml:6965
  • Draft comment:
    Typographical error: The operationId "getJob logs" should be modified to "getJobLogs" (remove the space) to match standard naming conventions.
  • 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.
22. backend/windmill-api/openapi.yaml:9588
  • Draft comment:
    Typographical error: The operationId "removeUserToGroup" in the POST /w/{workspace}/groups/removeuser/{name} endpoint likely should be "removeUserFromGroup". Please update it 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.
23. backend/windmill-api/src/flows.rs:1090
  • Draft comment:
    Typo in SQL query: It looks like 'flow,description' is written instead of 'flow.description' in the SELECT clause of the get_flow_by_path_w_draft function. Please fix it.
  • 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.
24. backend/windmill-api/src/users.rs:2294
  • Draft comment:
    Typographical error: The function name 'update_username_in_workpsace' appears to have a typo. It should be spelled 'update_username_in_workspace'.
  • 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.
25. backend/windmill-api/src/users.rs:2242
  • Draft comment:
    Typographical error: The error message currently reads "Usermame can only contain alphanumeric characters...". Consider correcting 'Usermame' to 'Username'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_EX3vEoDdFZiomRcG


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant