Multiplayer deploy collision warning #4533
Open
+410
−27
Conversation
This file contains 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
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
|
| Latest commit: |
0a29279
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3b26c172.windmill.pages.dev |
| Branch Preview URL: | https://deploy-collision-warning.windmill.pages.dev |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e8fd885 in 41 seconds
More details
- Looked at
483lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:115
- Draft comment:
Consider adding error handling for theFlowService.getFlowByPathcall 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.getScriptByPathcall 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.getAppByPathcall 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:
TheonLatestfunction is used to check if the current version is the latest. Consider renaming it toisLatestVersionfor 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:
ThehandleEditScriptfunction is used to handle script editing. Consider renaming it tohandleScriptEditfor 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:
ThehandleUpdateAppfunction is used to handle app updates. Consider renaming it tohandleAppUpdatefor 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 <guilhem@mbp-de-windmill.home> * 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 <guilhemlemouel@gmail.com> Co-authored-by: Guilhem <guilhem@mbp-de-windmill.home> Co-authored-by: Ruben Fiszel <ruben@windmill.dev> Co-authored-by: Ruben Fiszel <ruben@rubenfiszel.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0fe1f19 in 31 seconds
More details
- Looked at
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:108
- Draft comment:
The variablelast_updated_atis declared twice, once globally and once locally within theonLatestfunction. 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.
👍 Looks good to me! Incremental review on 4a21173 in 42 seconds
More details
- Looked at
126lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:122
- Draft comment:
Consider adding a check to ensureflowHistoryis not empty before accessingflowHistory[0]?.idto 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]?.idalready handles the case whereflowHistorymight be empty, as it will returnundefinedinstead 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 ononLatestbeing set correctly, but the optional chaining should suffice for preventing runtime errors.
The optional chaining is a standard practice to handle potentialundefinedvalues, and the logic foronLatestseems 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 ensureflowHistoryis not empty before accessingflowHistory[0]?.idto 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.getFlowHistoryis not empty before accessing[0]?.idto 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.getFlowHistoryis not empty before accessing[0]?.idto 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.
👍 Looks good to me! Incremental review on 5c46f49 in 1 minute and 21 seconds
More details
- Looked at
308lines of code in8files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. backend/windmill-api/src/scripts.rs:959
- Draft comment:
The SQL query should useLIMIT 1to ensure only the latest version is fetched. This applies to similar queries inflows.rsandapps.rs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Thefetch_onemethod is used, which implies that the query is expected to return a single result. AddingLIMIT 1could be seen as redundant sincefetch_onewill 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_onecould potentially throw an error if more than one result is returned, andLIMIT 1would 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_onesuggests that the developer expects only one result, and the ordering bycreated_at DESCshould ensure that the latest version is returned. AddingLIMIT 1might be unnecessary in this context.
The comment should be deleted because the use offetch_onealready 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 incompareVersionsto prevent unhandled promise rejections. This applies to similar functions inScriptBuilder.svelteandAppEditorHeader.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 modifiedcompareVersionsfunction. 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 incompareVersionsto prevent unhandled promise rejections. This applies to similar functions inFlowBuilder.svelteandAppEditorHeader.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 incompareVersionsto prevent unhandled promise rejections. This applies to similar functions inFlowBuilder.svelteandScriptBuilder.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.
👍 Looks good to me! Incremental review on dc2fdb8 in 46 seconds
More details
- Looked at
160lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/FlowBuilder.svelte:296
- Draft comment:
The functionsyncWithDeployedis 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.svelteto show a warning if another user has deployed a new version.DeployOverrideConfirmationModal.sveltecomponent for handling deployment conflicts.handleSaveFlow(),handleEditScript(), andhandleUpdateApp()modified to check for the latest version and open confirmation modal if needed.get_latest_versionendpoints inopenapi.yamlfor flows, scripts, and apps.get_latest_versionfunctions inflows.rs,scripts.rs, andapps.rsto fetch the latest version details.deployedBy,deployedValue,confirmCallback, andopenvariables to track deployment state and handle modal interactions.This description was created by
for dc2fdb8. It will automatically update as commits are pushed.