-
Notifications
You must be signed in to change notification settings - Fork 468
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
Multiplayer deploy collision warning #4533
Conversation
Show warning with diff viewer if 2+ users try to deploy flow/script/app at the same time In other words, if user A and user B started editing same flow/script/app and user A deployed it, user B will get warning if they try to deploy as well.
Deploying windmill with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to e8fd885 in 41 seconds
More details
- Looked at
483
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:115
- Draft comment:
Consider adding error handling for theFlowService.getFlowByPath
call to handle potential API errors gracefully. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
2. frontend/src/lib/components/ScriptBuilder.svelte:235
- Draft comment:
Consider adding error handling for theScriptService.getScriptByPath
call to handle potential API errors gracefully. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
3. frontend/src/lib/components/apps/editor/AppEditorHeader.svelte:383
- Draft comment:
Consider adding error handling for theAppService.getAppByPath
call to handle potential API errors gracefully. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
4. frontend/src/lib/components/FlowBuilder.svelte:270
- Draft comment:
TheonLatest
function is used to check if the current version is the latest. Consider renaming it toisLatestVersion
for better clarity and consistency with its purpose. - Reason this comment was not posted:
Confidence changes required:30%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
5. frontend/src/lib/components/ScriptBuilder.svelte:233
- Draft comment:
ThehandleEditScript
function is used to handle script editing. Consider renaming it tohandleScriptEdit
for better consistency with naming conventions. - Reason this comment was not posted:
Confidence changes required:30%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
6. frontend/src/lib/components/apps/editor/AppEditorHeader.svelte:379
- Draft comment:
ThehandleUpdateApp
function is used to handle app updates. Consider renaming it tohandleAppUpdate
for better consistency with naming conventions. - Reason this comment was not posted:
Confidence changes required:30%
The PR introduces a new feature to handle multiplayer deploy collisions by showing a warning and a diff viewer. The implementation seems consistent across different components, but there are some areas that could be improved for better code quality and maintainability.
Workflow ID: wflow_bpmdKWlTdMFcLZSC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* feat(frontend): added list of triggers in the flow graph * feat(frontend): added list of triggers in the flow graph * feat(frontend): clean up * feat(frontend): improve UX * feat(frontend): triggers * feat(frontend): triggers * feat(frontend): done * feat(frontend): fix trigger when position when a preprocessor is presetn * Glm/rework flow settings v2 (#4497) * fat(frontend): simplify flow settings menu * improve scroll * changing mute toggle * Add advanced settings badge * Add nord theme colors * Add bage for advanced options * fix minor issue * fix minor issue * Add triggers menu to flow settings * Add quick trigger access * remove triggers in flow settings * fix minor issue * Move triggers settings to flow right panel * polishing * fix unset store * remove save up to for triggers * fix padding * reset default tag color * remove custom select component * revert path change * revert section modif * Revert unused feature --------- Co-authored-by: Guilhem <[email protected]> * Connect top bar cron to schedules settings * Turn copilot into node * fix copilot placement * remove useless import * fix center copilot * fix binding * remove copilot on top of preprocessor * render copilot node on condition * quickfix * remove copilot node * fix minor issues * fix route count update * fix schedule sync * harmonize colors * fix alignment and add edges * recenter node summary * fix schedules sync * Add id title * all * all * all * iteration * all * all * done * fix * more fixes --------- Co-authored-by: Guilhem <[email protected]> Co-authored-by: Guilhem <[email protected]> Co-authored-by: Ruben Fiszel <[email protected]> Co-authored-by: Ruben Fiszel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 0fe1f19 in 31 seconds
More details
- Looked at
44
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:108
- Draft comment:
The variablelast_updated_at
is declared twice, once globally and once locally within theonLatest
function. The global declaration is unnecessary and should be removed to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_G4JgTOGwnLowpISG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 4a21173 in 42 seconds
More details
- Looked at
126
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:122
- Draft comment:
Consider adding a check to ensureflowHistory
is not empty before accessingflowHistory[0]?.id
to avoid potential runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The use of optional chaining (?.
) inflowHistory[0]?.id
already handles the case whereflowHistory
might be empty, as it will returnundefined
instead of causing a runtime error. Therefore, the comment suggesting an additional check is redundant and not necessary.
I might be overlooking a scenario where further logic depends ononLatest
being set correctly, but the optional chaining should suffice for preventing runtime errors.
The optional chaining is a standard practice to handle potentialundefined
values, and the logic foronLatest
seems to be correctly handled with the current implementation.
The comment is unnecessary because the optional chaining already prevents runtime errors. The comment should be deleted.
2. frontend/src/lib/components/FlowBuilder.svelte:127
- Draft comment:
Consider adding a check to ensureflowHistory
is not empty before accessingflowHistory[0]?.id
to avoid potential runtime errors. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte:71
- Draft comment:
Ensure that the result ofFlowService.getFlowHistory
is not empty before accessing[0]?.id
to prevent potential runtime errors. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte:116
- Draft comment:
Ensure that the result ofFlowService.getFlowHistory
is not empty before accessing[0]?.id
to prevent potential runtime errors. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_kdFpYGMQCZXbq7g0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…indmill into deploy-collision-warning
Create several new endpoints, that returns just latest version without rest of the history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 5c46f49 in 1 minute and 21 seconds
More details
- Looked at
308
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. backend/windmill-api/src/scripts.rs:959
- Draft comment:
The SQL query should useLIMIT 1
to ensure only the latest version is fetched. This applies to similar queries inflows.rs
andapps.rs
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Thefetch_one
method is used, which implies that the query is expected to return a single result. AddingLIMIT 1
could be seen as redundant sincefetch_one
will already enforce this behavior. The comment also references other files, which is outside the scope of this review.
I might be overlooking the fact thatfetch_one
could potentially throw an error if more than one result is returned, andLIMIT 1
would prevent this. However, the current implementation seems to rely on the assumption that the query will only return one result due to the ordering.
The use offetch_one
suggests that the developer expects only one result, and the ordering bycreated_at DESC
should ensure that the latest version is returned. AddingLIMIT 1
might be unnecessary in this context.
The comment should be deleted because the use offetch_one
already implies that only one result is expected, and the ordering ensures it is the latest version.
2. frontend/src/lib/components/FlowBuilder.svelte:119
- Draft comment:
Consider adding error handling for the API call incompareVersions
to prevent unhandled promise rejections. This applies to similar functions inScriptBuilder.svelte
andAppEditorHeader.svelte
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant to the changes made in the diff, as it suggests adding error handling to the modifiedcompareVersions
function. The function indeed lacks error handling for the asynchronous call, which could lead to unhandled promise rejections. The comment is actionable and suggests a clear improvement to the code quality.
The comment mentions other files, which is not relevant to this diff. However, the suggestion to add error handling is valid for the changes made in this file.
While the mention of other files is not relevant, the core suggestion to add error handling is valid and should be considered for this file.
Keep the comment as it suggests a valid code improvement by adding error handling to the modified function.
3. frontend/src/lib/components/ScriptBuilder.svelte:254
- Draft comment:
Consider adding error handling for the API call incompareVersions
to prevent unhandled promise rejections. This applies to similar functions inFlowBuilder.svelte
andAppEditorHeader.svelte
. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/lib/components/apps/editor/AppEditorHeader.svelte:618
- Draft comment:
Consider adding error handling for the API call incompareVersions
to prevent unhandled promise rejections. This applies to similar functions inFlowBuilder.svelte
andScriptBuilder.svelte
. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_sgIDvQkeG8t4fu2a
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on dc2fdb8 in 46 seconds
More details
- Looked at
160
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:296
- Draft comment:
The functionsyncWithDeployed
is duplicated across multiple files with the same logic. Consider refactoring it into a shared utility function to reduce redundancy and improve maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a code quality improvement by refactoring a potentially duplicated function. However, the diff only shows the function in this file, and there is no evidence of duplication across multiple files. Without evidence of duplication, the comment is speculative and should be removed.
I might be missing the context of other files where this function is duplicated. However, the diff does not provide this information, and the comment should be based on the changes visible in the diff.
The diff should provide evidence of the issue being commented on. Without evidence in the diff, the comment is speculative and not actionable based on the current review context.
Remove the comment as it is speculative and not supported by evidence in the diff.
Workflow ID: wflow_RHc3vHYUywXtwtO7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 7a22d58 in 40 seconds
More details
- Looked at
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte:113
- Draft comment:
Update the comment to reflect the use ofgetFlowLatestVersion
for retrieving the flow version, as the current comment is outdated. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about updating a code comment to reflect a change in the function used. This is a valid suggestion as it helps maintain accurate documentation within the code. The change in the function call is evident in the diff, providing strong evidence for the comment's correctness.
The comment is about a non-functional aspect of the code (a comment), which might not be critical. However, keeping code comments accurate is generally good practice.
Accurate code comments are important for code maintainability and understanding, so updating them when the code changes is beneficial.
The comment is valid as it suggests updating a code comment to reflect a change in the code, which is a good practice for maintainability.
Workflow ID: wflow_F8pvibFQmeoC5RgL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
4909207
to
59a1e67
Compare
Show warning with diff viewer if 2+ users try to deploy flow/script/app at the same time
In other words, if user A and user B started editing same flow/script/app and user A deployed it, user B will get warning if they try to deploy as well.
Important
Adds multiplayer deploy collision warning with confirmation modal to handle deployment conflicts in
FlowBuilder.svelte
,ScriptBuilder.svelte
, andAppEditorHeader.svelte
.FlowBuilder.svelte
,ScriptBuilder.svelte
, andAppEditorHeader.svelte
.DeployOverrideConfirmationModal.svelte
to show a warning if another user has deployed a new version.DeployOverrideConfirmationModal.svelte
component for handling deployment conflicts.handleSaveFlow()
,handleEditScript()
, andhandleUpdateApp()
modified to check for the latest version and open confirmation modal if needed.get_latest_version
endpoints inopenapi.yaml
for flows, scripts, and apps.get_latest_version
functions inflows.rs
,scripts.rs
, andapps.rs
to fetch the latest version details.deployedBy
,deployedValue
,confirmCallback
, andopen
variables to track deployment state and handle modal interactions.This description was created by for 7a22d58. It will automatically update as commits are pushed.