Deleting current workspace switches to another instead of closing [ backend implementation ] #1623
Conversation
…che to avoid blank window bug )
WalkthroughThe pull request introduces comprehensive changes across multiple files to enhance workspace and tab management functionality. Key modifications include updates to the The new return value, These changes enhance the functionality related to workspace management and tab handling, ensuring better state management. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Jalileh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/service/workspaceservice/workspaceservice.go (1)
100-103: No workspace deletion performed but no error: Clarify logic.When
deleted == falsehere, the function returns no error and no updates. Yet, the user may expect more insight into why the workspace was not deleted (e.g., named, pinned tabs). Consider adding logs or comments for clarity.pkg/wcore/workspace.go (1)
166-166: Potentially unify error handling for window closure.Here, if
CloseWindowfails, you return a wrapped error. In other cases (like lines 162–163), you skip the closure and return early. Consider consolidating this logic to make the control flow more transparent and reduce branching.emain/emain-window.ts (2)
306-308: Handle potential undefinedcurWorkspace.The comment indicates a potential bug if
curWorkspaceis undefined after a workspace deletion. Consider robust error-handling or early-return logic in casecurWorkspaceis null to avoid runtime errors.if (!workspaceList?.find((wse) => wse.workspaceid === workspaceId)?.windowid) { const curWorkspace = await WorkspaceService.GetWorkspace(this.workspaceId); - if (curWorkspace && isNonEmptyUnsavedWorkspace(curWorkspace)) { + if (!curWorkspace) { + console.warn(`Workspace ${this.workspaceId} not found or was already deleted.`); + return; + } + + if (isNonEmptyUnsavedWorkspace(curWorkspace)) { console.log( `existing unsaved workspace ${this.workspaceId} has content, opening workspace ${workspaceId} in new window` ); await createWindowForWorkspace(workspaceId); return; } }
704-713: Check fornullor emptymoveToNewWorkspaceand handle errors.When deleting a workspace,
DeleteWorkspacereturnsmoveToNewWorkspace. Verify that a valid workspace ID is returned before callingswitchWorkspace, or handle the null/empty string scenario. Consider wrapping the call in a try/catch block to ensure errors in the backend do not leave the UI in an inconsistent state.const moveToNewWorkspace = await WorkspaceService.DeleteWorkspace(workspaceId); console.log("delete-workspace done", workspaceId, ww?.waveWindowId); if (ww?.workspaceId == workspaceId) { if (workspaceList?.length > 1) { - await ww.switchWorkspace(moveToNewWorkspace); + if (moveToNewWorkspace) { + await ww.switchWorkspace(moveToNewWorkspace); + } else { + console.warn(`No valid workspace to move to after deletion of ${workspaceId}. Closing window instead.`); + ww.destroy(); + } } else { console.log("delete-workspace closing window", workspaceId, ww?.waveWindowId); ww.destroy(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/launch.json(1 hunks)emain/emain-window.ts(4 hunks)frontend/app/store/services.ts(1 hunks)pkg/service/workspaceservice/workspaceservice.go(2 hunks)pkg/wcore/window.go(2 hunks)pkg/wcore/workspace.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/launch.json
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/wcore/workspace.go
137-137: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (14)
pkg/wcore/window.go (2)
55-55: Consider handling or documenting the ignored return value.
Here, the second return parameter from DeleteWorkspace (i.e., claimableWorkspace) is ignored. If you intend to switch a user to this "claimable" workspace rather than deleting it outright, consider returning or logging this value to provide better clarity in the workflow. Otherwise, document why it’s safe to ignore.
134-134: Re-examine the ignored return value for future workspace logic.
As above, the second return parameter (claimableWorkspace) is also ignored here. Double-check whether ignoring it is consistent with the intended behavior. If you plan to reuse or transition to any unclaimed workspace, you may need to capture or handle that value rather than discarding it.
frontend/app/store/services.ts (1)
192-192: Confirm the front-end usage of the new return type.
The DeleteWorkspace method now returns a Promise<string>. Ensure that all invocations of this method correctly handle or ignore the returned string and that no callers still expect a void return type.
pkg/service/workspaceservice/workspaceservice.go (4)
57-58: Stylistic update with trailing comma is acceptable.
Adding the trailing comma to the event publishing struct is consistent with Go's recommended formatting style. No further changes are required here.
91-91: Signature change to include a string return is consistent with the PR objective.
The updated signature returning (waveobj.UpdatesRtnType, string, error) aligns well with the new requirement to track unclaimed workspaces. Ensure all callers are updated to handle the additional string.
95-98: Interpretation of an unclaimed workspace short-circuit.
Returning immediately when claimableWorkspace != "" (line 96) means your updates or errors are skipped past. Confirm that this early return is the correct behavior and that ignoring waveobj.UpdatesRtnType is desired in this scenario.
110-110: Asynchronously sending update events is a good approach.
The use of a goroutine for sending update events prevents blocking. Ensure all potential race conditions and concurrency issues are handled, particularly if the rest of the code modifies internal state after spawning the goroutine.
pkg/wcore/workspace.go (5)
69-70: Minor consistency fix with trailing comma.
Including the trailing comma when creating the WaveEvent struct is stylistically aligned. This is a minor improvement and is good to keep consistent across the codebase.
118-118: Signature now returns a “claimableWorkspace” string.
This change reflects the new feature requirements. Verify that all existing calls to DeleteWorkspace are updated accordingly to capture or consciously ignore this new string value.
122-126: Graceful handling of named workspaces.
The early return skips deletion when a workspace is named and has tabs. Verify that this logic aligns with the updated product requirements, especially if the user truly wants the workspace removed under certain conditions (e.g., forced deletion).
148-164: Check logic when a workspace is unclaimed.
Upon discovering UnclaimedWorkspace != "", you return immediately at line 162, skipping a window closure. Confirm that the calling code can handle this scenario where the old workspace is deleted and the user is expected to switch to the unclaimed workspace.
169-169: Ensuring final return is correct.
When no unclaimed workspace remains and everything else succeeds, returning (true, "", nil) is appropriate. Validate that callers correctly interpret an empty claimable workspace string as “no alternative workspace available.”
emain/emain-window.ts (2)
21-22: Consider removing unused imports or verify usage.
Importing { time } from "console" and { timestamp } from "rxjs" can introduce confusion if they are not actually used in the code. Please ensure they are necessary, or remove them if not used to keep the code clean.
422-422: Add unit tests to verify queued tab creations.
The queueCreateTab function adds a new tab creation request to _queueActionInternal. Ensuring consistent handling of pinned vs non-pinned tabs and concurrency would be beneficial. Please confirm that you have sufficient test coverage for these scenarios.
| return false, "", fmt.Errorf("error retrieving workspaceList: %w", err) | ||
| } | ||
| UnclaimedWorkspace := "" | ||
| for _, ws := range workspaces { |
There was a problem hiding this comment.
I liked your old logic which would attempt to find the workspace at index closest to the one that was deleted, could you incorporate that here?
I think you can do this in one pass. Create three variables, curWsIndex, freeWsIndexBeforeCurrent and freeWsIndexAfterCurrent. Then, you iterate through the workspace list, setting freeWsIndexBeforeCurrent to the index in the workspace list where you find a free workspace (not claimed by a window). Once you find the current index, you set curWsIndex and from there you continue iterating until you find another free workspace, where you set freeWsIndexAfterCurrent. Then, you just take whichever one of the two indices is closer to curWsIndex (or whichever isn't null).
There was a problem hiding this comment.
sure i can implement that if we can ignore this return error, because im relying on getting any claimable workspace to not upset this code because deleteworkspace also runs in switchworkspace for some reason i dont know
There was a problem hiding this comment.
Ah it runs in switch workspace to clean up unsaved workspaces. For saved ones, delete without a force flag is a no-op
There was a problem hiding this comment.
how do I actually retrieve the error string in the error data structure? if I can get the string to reliably know if 'not found' happened I wont have to limit the claimworkspace implementation, I only saw msg inside of it but couldn't access it, sorry I'm new to go.
There was a problem hiding this comment.
I think you can return empty string if nothing is deleted. This should never happen if you call deleteworkspace from the workspace service since that passes the force flag as true to the wcore implementation
There was a problem hiding this comment.
OK ill implement the old logic tomorrow and notify you, ty!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wcore/workspace.go (1)
148-158: Enhance workspace selection logic.The current implementation selects the first unclaimed workspace. Consider implementing the suggested approach of selecting the closest workspace to provide a more intuitive user experience.
Consider this approach:
- UnclaimedWorkspace := "" - for _, ws := range workspaces { - if ws.WindowId == "" && workspaceId != ws.WorkspaceId { - UnclaimedWorkspace = ws.WorkspaceId - break - } - } + var ( + curWsIndex int + freeWsIndexBeforeCurrent = -1 + freeWsIndexAfterCurrent = -1 + ) + + // Find the current workspace index and free workspaces + for i, ws := range workspaces { + if ws.WorkspaceId == workspaceId { + curWsIndex = i + continue + } + if ws.WindowId == "" { + if i < curWsIndex && (freeWsIndexBeforeCurrent == -1 || i > freeWsIndexBeforeCurrent) { + freeWsIndexBeforeCurrent = i + } else if i > curWsIndex && (freeWsIndexAfterCurrent == -1 || i < freeWsIndexAfterCurrent) { + freeWsIndexAfterCurrent = i + } + } + } + + // Select the closest free workspace + var UnclaimedWorkspace string + if freeWsIndexBeforeCurrent != -1 && freeWsIndexAfterCurrent != -1 { + if curWsIndex-freeWsIndexBeforeCurrent <= freeWsIndexAfterCurrent-curWsIndex { + UnclaimedWorkspace = workspaces[freeWsIndexBeforeCurrent].WorkspaceId + } else { + UnclaimedWorkspace = workspaces[freeWsIndexAfterCurrent].WorkspaceId + } + } else if freeWsIndexBeforeCurrent != -1 { + UnclaimedWorkspace = workspaces[freeWsIndexBeforeCurrent].WorkspaceId + } else if freeWsIndexAfterCurrent != -1 { + UnclaimedWorkspace = workspaces[freeWsIndexAfterCurrent].WorkspaceId + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emain/emain-window.ts(4 hunks)pkg/wcore/window.go(2 hunks)pkg/wcore/workspace.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emain/emain-window.ts
- pkg/wcore/window.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/wcore/workspace.go
137-137: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (4)
pkg/wcore/workspace.go (4)
69-70: LGTM: Clean struct initialization.
The addition of a trailing comma in the event struct initialization improves maintainability for future field additions.
118-126: LGTM: Consistent error handling with new return value.
The function signature change and early returns properly handle the new unclaimed workspace ID return value while maintaining robust error handling.
134-134: LGTM: Proper error propagation.
The error handling correctly includes the empty string return value while maintaining clear error context.
137-140:
Fix ineffectual error assignment.
The error from DBFindWindowForWorkspaceId is never checked before being overwritten.
Apply this fix:
- windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
- err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)
+ windowId, findErr := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
+ if findErr != nil {
+ return false, "", fmt.Errorf("error finding window: %w", findErr)
+ }
+ err := wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
137-137: ineffectual assignment to err
(ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/wcore/workspace.go (1)
158-175: Improve variable naming for better readability.The current implementation correctly finds the closest unclaimed workspace as requested, but the naming could be more descriptive.
Consider these naming improvements:
- UnclaimedWorkspace, findAfter := "", false + closestUnclaimedWorkspace, searchAfterCurrent := "", false for i, ws := range workspaces { if workspaceId == ws.WorkspaceId { for N := i; N >= 0; N-- { if workspaces[N].WindowId == "" { - UnclaimedWorkspace = workspaces[N].WorkspaceId + closestUnclaimedWorkspace = workspaces[N].WorkspaceId break } } - if UnclaimedWorkspace != "" { + if closestUnclaimedWorkspace != "" { break } - findAfter = true - } else if findAfter && workspaces[i].WindowId == "" { - UnclaimedWorkspace = workspaces[i].WorkspaceId + searchAfterCurrent = true + } else if searchAfterCurrent && workspaces[i].WindowId == "" { + closestUnclaimedWorkspace = workspaces[i].WorkspaceId break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/wcore/window.go(2 hunks)pkg/wcore/workspace.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/wcore/window.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/wcore/workspace.go
146-146: SA4006: this value of err is never used
(staticcheck)
🔇 Additional comments (3)
pkg/wcore/workspace.go (3)
69-70: LGTM!
The addition of a trailing comma in the event struct initialization follows Go conventions and improves Git diff readability.
118-118: LGTM! Function signature change aligns with PR objectives.
The updated signature (bool, string, error) enables returning an unclaimed workspace ID, supporting the new workspace switching behavior.
146-149:
Fix ineffectual assignment to err.
The result of DBFindWindowForWorkspaceId is never checked before being overwritten.
Apply this fix:
- windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
- err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)
+ windowId, findErr := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
+ if findErr != nil {
+ return false, "", fmt.Errorf("error finding window: %w", findErr)
+ }
+ err := wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
146-146: SA4006: this value of err is never used
(staticcheck)
pkg/wcore/workspace.go
Outdated
| func DeleteWorkspace(ctx context.Context, workspaceId string, force bool) (bool, string, error) { | ||
| log.Printf("DeleteWorkspace %s\n", workspaceId) | ||
| workspace, err := wstore.DBMustGet[*waveobj.Workspace](ctx, workspaceId) | ||
| if err != nil && err.Error() == "not found" { | ||
| return true, "", fmt.Errorf("workspace probably already deleted %w", err) | ||
| } | ||
| // @jalileh list needs to be saved early on i assume | ||
| workspaces, err := ListWorkspaces(ctx) | ||
| if err != nil { | ||
| return false, "", fmt.Errorf("error retrieving workspaceList: %w", err) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return false, fmt.Errorf("error getting workspace: %w", err) | ||
| return false, "", fmt.Errorf("error getting workspace: %w", err) | ||
| } | ||
| if workspace.Name != "" && workspace.Icon != "" && !force && (len(workspace.TabIds) > 0 || len(workspace.PinnedTabIds) > 0) { | ||
| log.Printf("Ignoring DeleteWorkspace for workspace %s as it is named\n", workspaceId) | ||
| return false, nil | ||
| return false, "", nil | ||
| } | ||
|
|
||
| // delete all pinned and unpinned tabs | ||
| for _, tabId := range append(workspace.TabIds, workspace.PinnedTabIds...) { | ||
| log.Printf("deleting tab %s\n", tabId) | ||
| _, err := DeleteTab(ctx, workspaceId, tabId, false) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error closing tab: %w", err) | ||
| return false, "", fmt.Errorf("error closing tab: %w", err) | ||
| } | ||
| } | ||
| windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId) | ||
| err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error deleting workspace: %w", err) | ||
| return false, "", fmt.Errorf("error deleting workspace: %w", err) | ||
| } | ||
| log.Printf("deleted workspace %s\n", workspaceId) | ||
| wps.Broker.Publish(wps.WaveEvent{ | ||
| Event: wps.Event_WorkspaceUpdate}) | ||
| Event: wps.Event_WorkspaceUpdate, | ||
| }) | ||
|
|
||
| if windowId != "" { | ||
| err = CloseWindow(ctx, windowId, false) | ||
|
|
||
| UnclaimedWorkspace, findAfter := "", false | ||
| for i, ws := range workspaces { | ||
| if workspaceId == ws.WorkspaceId { | ||
| for N := i; N >= 0; N-- { | ||
| if workspaces[N].WindowId == "" { | ||
| UnclaimedWorkspace = workspaces[N].WorkspaceId | ||
| break | ||
| } | ||
| } | ||
| if UnclaimedWorkspace != "" { | ||
| break | ||
| } | ||
| findAfter = true | ||
| } else if findAfter && workspaces[i].WindowId == "" { | ||
| UnclaimedWorkspace = workspaces[i].WorkspaceId | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if UnclaimedWorkspace != "" { | ||
| return true, UnclaimedWorkspace, nil | ||
| } else { | ||
| err = CloseWindow(ctx, windowId, false) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return false, fmt.Errorf("error closing window: %w", err) | ||
| return false, "", fmt.Errorf("error closing window: %w", err) | ||
| } | ||
| } | ||
| return true, nil | ||
| return true, "", nil |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for DeleteWorkspace scenarios
The codebase lacks dedicated test coverage for the DeleteWorkspace functionality. The implementation in pkg/wcore/workspace.go handles complex scenarios including:
- Workspace deletion with state management
- Window and tab cleanup
- Unclaimed workspace handling
- Force deletion behavior
Critical areas needing test coverage:
- Deleting workspaces with/without unclaimed workspaces before/after
- Force deletion vs. protected named workspaces
- Window reassignment logic
- Tab cleanup during workspace deletion
- Error handling for various failure scenarios
Note: I've found that while there are some tests for delete operations in other parts of the codebase (like syncmap_test.go and blockstore_test.go), there's no specific test coverage for the DeleteWorkspace functionality. This is concerning given the complexity of the implementation and the various edge cases it handles.
🔗 Analysis chain
Verify the workspace switching behavior with different scenarios.
Please ensure testing covers these scenarios:
- Deleting a workspace with unclaimed workspaces before it
- Deleting a workspace with unclaimed workspaces after it
- Deleting a workspace with no unclaimed workspaces
- Deleting the last workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for DeleteWorkspace scenarios
# Check for test files covering DeleteWorkspace
echo "Checking for test coverage..."
rg -l "TestDeleteWorkspace" --type go
# Look for existing test scenarios
echo "Checking existing test scenarios..."
rg "func.*Test.*DeleteWorkspace|func.*Test.*Workspace.*Delete" --type go -A 10
Length of output: 268
Script:
#!/bin/bash
# Let's try a broader search for workspace-related tests and deletion functionality
# Search for workspace test files
echo "Searching for workspace test files..."
fd "workspace.*test.*\.go$"
# Search for any test functions related to workspace operations
echo "Searching for workspace test functions..."
rg "func Test.*Workspace" --type go -A 5
# Search for any delete-related test functions
echo "Searching for delete-related test functions..."
rg "func Test.*Delete" --type go -A 5
# Look for actual implementation to understand test coverage needs
echo "Searching for DeleteWorkspace implementation references..."
rg "DeleteWorkspace" --type go -A 2 -B 2
Length of output: 4444
🧰 Tools
🪛 golangci-lint (1.62.2)
146-146: SA4006: this value of err is never used
(staticcheck)
There was a problem hiding this comment.
Deleting workspaces with/without unclaimed workspaces before/after TESTED ✔
Force deletion vs. protected named workspaces ???
Window reassignment logic TESTED WORKS WONDERFULLY ✔
Tab cleanup during workspace deletion TESTED ✔
Error handling for various failure scenarios TESTED/DEBUGGED x4^1000 ✔
There was a problem hiding this comment.
@Jalileh, thanks for clarifying. It looks like everything is tested except for the force deletion of a protected (named) workspace scenario. Below is an example Go test that could help ensure coverage for force deletion vs. protected named workspaces. This test checks whether a named workspace is not deleted without force, but is deleted with force:
package wcore_test
import (
"context"
"testing"
"github.com/stretchr/testify/require"
"github.com/wavetermdev/waveterm/pkg/wcore"
"github.com/wavetermdev/waveterm/pkg/waveobj"
"github.com/wavetermdev/waveterm/pkg/wstore"
)
// TestDeleteWorkspaceForce checks that named, non-empty workspaces
// are only deleted when force is true.
func TestDeleteWorkspaceForce(t *testing.T) {
ctx := context.Background()
// Step 1: Create a workspace
ws, err := wcore.CreateWorkspace(ctx, "MyProtectedWorkspace", "triangle", "#FF9500", false, false)
require.NoError(t, err, "error creating workspace")
// Ensure it remains if not forced
deleted, unclaimed, err := wcore.DeleteWorkspace(ctx, ws.OID, false)
require.NoError(t, err, "unexpected error calling DeleteWorkspace")
require.False(t, deleted, "expected not to delete a named, non-empty workspace without force")
require.Empty(t, unclaimed, "no unclaimed workspace ID should be returned if not deleted")
// Step 2: Now delete with force
deleted, unclaimed, err = wcore.DeleteWorkspace(ctx, ws.OID, true)
require.NoError(t, err, "unexpected error calling DeleteWorkspace with force=true")
require.True(t, deleted, "expected workspace to be deleted when forced")
require.Empty(t, unclaimed, "no unclaimed workspace ID should be returned since we forced deletion")
// Verify the workspace is gone
storedWS, getErr := wstore.DBGet[*waveobj.Workspace](ctx, ws.OID)
require.Nil(t, storedWS, "workspace should no longer exist after deletion")
require.NoError(t, getErr, "should not produce an error if workspace is truly gone")
}This example:
- Creates a named workspace.
- Attempts to delete it without force, verifying it fails.
- Attempts again with
force=true, ensuring that it is deleted.
Let me know if you have any further questions!
✏️ Learnings added
Learnt from: Jalileh
PR: wavetermdev/waveterm#1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.392Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I dont think I want to talk to you anymore
There was a problem hiding this comment.
@Jalileh, I understand your decision. If you do have any requests in the future, I’ll be here to help. Take care.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/wcore/workspace.go (3)
118-118: Update function documentation to reflect new return values.The function comment should document the new string return value that represents the unclaimed workspace ID.
-// Returns true if workspace was deleted, false if it was not deleted. +// Returns: +// - bool: true if workspace was deleted, false if it was not deleted +// - string: ID of an unclaimed workspace if available, empty string otherwise +// - error: any error that occurred during the operation
121-128: Improve error handling and remove TODO comment.
- The error handling could be more idiomatic
- The TODO comment about list saving should be removed
- if err != nil && wstore.ErrNotFound == err { - return true, "", fmt.Errorf("workspace already deleted %w", err) - } - // @jalileh list needs to be saved early on i assume - workspaces, err := ListWorkspaces(ctx) - if err != nil { - return false, "", fmt.Errorf("error retrieving workspaceList: %w", err) - } + if err != nil { + if wstore.ErrNotFound == err { + return true, "", nil // Already deleted is not an error condition + } + return false, "", fmt.Errorf("error getting workspace: %w", err) + } + + workspaces, err := ListWorkspaces(ctx) + if err != nil { + return false, "", fmt.Errorf("error retrieving workspace list: %w", err) + }
158-175: Improve readability of workspace switching logic.While the logic is correct, the code could be more readable with better variable naming and documentation.
- UnclaimedWorkspace, findAfter := "", false + // First try to find an unclaimed workspace before the current one, + // then look after if none is found + candidateWorkspaceId, searchAfterCurrent := "", false for i, ws := range workspaces { if workspaceId == ws.WorkspaceId { for N := i; N >= 0; N-- { if workspaces[N].WindowId == "" { - UnclaimedWorkspace = workspaces[N].WorkspaceId + candidateWorkspaceId = workspaces[N].WorkspaceId break } } - if UnclaimedWorkspace != "" { + if candidateWorkspaceId != "" { break } - findAfter = true - } else if findAfter && workspaces[i].WindowId == "" { - UnclaimedWorkspace = workspaces[i].WorkspaceId + searchAfterCurrent = true + } else if searchAfterCurrent && workspaces[i].WindowId == "" { + candidateWorkspaceId = workspaces[i].WorkspaceId break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wcore/workspace.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/wcore/workspace.go
146-146: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (3)
pkg/wcore/workspace.go (3)
69-70: LGTM! Event publishing changes look good.
The addition of trailing commas in event struct initialization improves consistency.
Also applies to: 153-154
146-149:
Fix ineffectual error assignment.
The error from DBFindWindowForWorkspaceId is never checked before being overwritten.
windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
+ if err != nil {
+ return false, "", fmt.Errorf("error finding window: %w", err)
+ }
err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)
if err != nil {
return false, "", fmt.Errorf("error deleting workspace: %w", err)
}Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
146-146: ineffectual assignment to err
(ineffassign)
157-187: Verify workspace switching behavior.
Let's verify that the implementation correctly handles all scenarios for workspace switching.
✅ Verification successful
Workspace switching implementation is correctly implemented
The implementation correctly handles workspace switching with proper fallback behavior:
-
When deleting a workspace, it first searches for an unclaimed workspace to switch to:
- Looks backward from the current workspace position for any unclaimed workspace
- If none found before, looks forward from the current position
- Only closes the window if no unclaimed workspace is available
-
The implementation includes proper error handling:
- Workspace existence verification
- Window closure error handling
- Proper cleanup of tabs and resources
-
The switching logic is well-integrated with the window management system through
CloseWindowand workspace store operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workspace switching implementation
# Check for proper error handling in workspace switching
echo "Checking error handling in workspace switching..."
rg "DeleteWorkspace.*return.*false.*string.*error" --type go
# Verify window closure only happens when no unclaimed workspaces exist
echo "Checking window closure conditions..."
rg "CloseWindow.*ctx.*windowId.*false" -B 5 --type go
# Look for test coverage of the new functionality
echo "Checking test coverage..."
rg "TestDeleteWorkspace|TestWorkspaceSwitching" --type go
Length of output: 1502
Script:
#!/bin/bash
# Let's verify the workspace switching logic more comprehensively
# Check the complete DeleteWorkspace function implementation
echo "Checking DeleteWorkspace implementation..."
ast-grep --pattern 'func DeleteWorkspace(ctx context.Context, $$$) ($_, $_, error) {
$$$
}'
# Look for workspace switching related functions
echo "Checking workspace-related functions..."
rg "func.*Workspace" --type go -A 1
# Check for workspace store operations
echo "Checking workspace store operations..."
rg "DBFindWorkspaceById|DBListWorkspaces" --type go -A 2
Length of output: 13225
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wcore/workspace.go (1)
158-173: Consider optimizing the workspace switching logic.The current implementation uses a two-pass approach to find unclaimed workspaces. This could be optimized to a single pass by tracking both before and after unclaimed workspaces simultaneously.
- UnclaimedWorkspace, findAfter := "", false - for _, ws := range workspaces { - if ws.WorkspaceId == workspaceId { - if UnclaimedWorkspace != "" { - break - } - findAfter = true - continue - } - if findAfter && ws.WindowId == "" { - UnclaimedWorkspace = ws.WorkspaceId - break - } else if ws.WindowId == "" { - UnclaimedWorkspace = ws.WorkspaceId - } - } + var beforeCurrent, afterCurrent string + var foundCurrent bool + for _, ws := range workspaces { + if ws.WindowId == "" { + if !foundCurrent { + beforeCurrent = ws.WorkspaceId + } else { + afterCurrent = ws.WorkspaceId + break + } + } + if ws.WorkspaceId == workspaceId { + foundCurrent = true + } + } + UnclaimedWorkspace := afterCurrent + if UnclaimedWorkspace == "" { + UnclaimedWorkspace = beforeCurrent + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wcore/workspace.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/wcore/workspace.go (1)
Learnt from: Jalileh
PR: wavetermdev/waveterm#1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
🪛 golangci-lint (1.62.2)
pkg/wcore/workspace.go
146-146: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (4)
pkg/wcore/workspace.go (4)
69-70: LGTM: Event publishing format is consistent.
The trailing comma in the event struct initialization improves maintainability.
118-118: LGTM: Improved error handling for deleted workspaces.
The function now properly handles the case where the workspace is already deleted, returning appropriate values.
Also applies to: 121-123
146-149:
Fix ineffectual error assignment.
The error from DBFindWindowForWorkspaceId is assigned but never checked before being overwritten.
- windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
- err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)
+ windowId, _ := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
+ err := wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
146-146: ineffectual assignment to err
(ineffassign)
133-135: 💡 Codebase verification
Missing test coverage for force deletion scenarios in named workspaces
Based on the code inspection, while the implementation correctly handles force deletion logic for named workspaces, there appears to be no dedicated test coverage for these critical scenarios. The implementation shows:
- Named workspaces with content are protected from deletion unless force=true
- Empty named workspaces can be deleted regardless of force flag
- Force flag bypasses the protection for named workspaces
Key locations requiring test coverage:
pkg/wcore/workspace.go: Implementation ofDeleteWorkspacefunction- Missing test file that should cover:
- Deletion attempt on named workspace with content (should fail)
- Force deletion of named workspace with content (should succeed)
- Deletion of empty named workspace (should succeed)
This aligns with the previously learned concern about ensuring test coverage for "force deletion vs. protected named workspaces."
🔗 Analysis chain
Verify force deletion behavior for named workspaces.
The implementation correctly handles force deletion of named workspaces, but ensure test coverage exists for these scenarios:
- Named workspace with content (should not delete without force)
- Named workspace with force flag (should delete)
- Empty named workspace (should delete regardless of force flag)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for force deletion scenarios
# Look for test cases covering force deletion
rg -A 10 "TestDelete.*Workspace.*force" --type go
# Check implementation references
rg -A 5 "DeleteWorkspace.*force.*true" --type go
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for workspace deletion tests and force flag usage
# Search for workspace deletion tests
rg -l "func.*Test.*Workspace.*Delete" --type go
# Search for force flag usage in workspace tests
rg "force.*=.*true" --type go ./pkg/wcore/
# Look for workspace deletion test cases
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
DeleteWorkspace($$$)
$$$
}'
Length of output: 182
Script:
#!/bin/bash
# Let's try a different approach to find test files and workspace deletion logic
# Find test files in pkg/wcore
fd ".*_test.go" pkg/wcore/
# Look for workspace-related test files
rg -l "workspace" pkg/wcore/ --type go | grep "_test"
# Check the actual implementation for force deletion logic
rg -A 5 -B 5 "DeleteWorkspace" pkg/wcore/workspace.go
Length of output: 1181
⛔ Skipped due to learnings
Learnt from: Jalileh
PR: wavetermdev/waveterm#1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
|
Just tested, it works great! I've submitted a small formatting fix for the TypeScript files, I'm guessing you don't have Prettier set up for your Neovim. I'm going to sign off on this, feel free to merge it when you're ready 😃 Thank you so much for your contribution!! |
I did not mean to close the previous pr, anyway i tried to implement what you suggested, the backend now does most of it
and DeleteWorkspace will return an unclaimed id and avoid closing the window.