fix issue when tab does not exist in workspace #2580
Conversation
…ick issue when the workspace data is messed up.
WalkthroughThe changes introduce more defensive error handling across the codebase. In the frontend, initialization failures are now caught and logged via API instead of potentially causing unhandled exceptions. In the backend, multiple operations replace error-on-missing-record calls (DBMustGet) with nullable-result calls (DBGet), allowing graceful handling when records don't exist. Additionally, the go.mod file resolves a pseudo-version dependency to v0.12.3 and adds an indirect dependency. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
frontend/wave.ts(1 hunks)go.mod(2 hunks)pkg/service/workspaceservice/workspaceservice.go(1 hunks)pkg/wcore/block.go(2 hunks)pkg/wcore/workspace.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 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).
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/wave.ts
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
frontend/wave.ts
📚 Learning: 2025-02-20T00:10:31.048Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1998
File: go.mod:37-37
Timestamp: 2025-02-20T00:10:31.048Z
Learning: The golang.org/x/sync package should remain at version v0.11.0 in go.mod, as explicitly confirmed by the maintainer.
Applied to files:
go.mod
🧬 Code graph analysis (4)
pkg/service/workspaceservice/workspaceservice.go (3)
pkg/wstore/wstore_dbops.go (1)
DBGet(133-136)pkg/waveobj/wtype.go (2)
Tab(185-192)Tab(194-196)pkg/blockcontroller/blockcontroller.go (1)
StopBlockController(241-248)
frontend/wave.ts (2)
pkg/wps/wps.go (1)
Client(20-22)pkg/waveobj/wtype.go (8)
Client(127-135)Client(137-139)Tab(185-192)Tab(194-196)Workspace(169-179)Workspace(181-183)LayoutState(224-233)LayoutState(235-237)
pkg/wcore/workspace.go (3)
pkg/wcore/block.go (1)
DeleteBlock(152-191)pkg/wstore/wstore_dbops.go (1)
DBDelete(274-299)pkg/waveobj/wtype.go (3)
OType_LayoutState(29-29)LayoutState(224-233)LayoutState(235-237)
pkg/wcore/block.go (4)
pkg/wstore/wstore_dbops.go (1)
DBGet(133-136)pkg/waveobj/wtype.go (2)
Block(282-290)Block(292-294)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/wstore/wstore_rtinfo.go (1)
DeleteRTInfo(141-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (5)
pkg/wcore/block.go (1)
153-159: LGTM: Graceful handling of missing blocks.The change from
DBMustGettoDBGetwith nil-check aligns perfectly with the PR objective of handling missing entities gracefully. This prevents panics when attempting to delete a block that no longer exists.pkg/wcore/workspace.go (2)
292-300: LGTM: Defensive block deletion handling.The nil-check guards block deletion, preventing panics when a tab ID exists in the workspace but the corresponding tab record is missing from the database. This aligns with the PR's objective of handling missing tabs gracefully during cleanup.
321-323: LGTM: Defensive LayoutState deletion handling.The nil-check prevents attempts to delete the LayoutState when the tab doesn't exist, avoiding potential errors and ensuring the cleanup path completes successfully even with missing data.
pkg/service/workspaceservice/workspaceservice.go (1)
262-269: LGTM: Conditional block controller shutdown.The change to use
DBGetwith conditional controller shutdown prevents errors when a tab doesn't exist while still allowing the rest of the cleanup process (line 270) to proceed. This pattern aligns with the defensive handling introduced throughout the PR.go.mod (1)
83-83: No issues found — goid v0.3.0 is an expected transitive dependency.The goid v0.3.0 dependency in root go.mod is a legitimate indirect dependency originating from the tsunami module, which has goid v0.3.0 as a direct dependency. No security vulnerabilities were found for this package version.
| try { | ||
| const [client, waveWindow, initialTab] = await Promise.all([ | ||
| WOS.loadAndPinWaveObject<Client>(WOS.makeORef("client", initOpts.clientId)), | ||
| WOS.loadAndPinWaveObject<WaveWindow>(WOS.makeORef("window", initOpts.windowId)), | ||
| WOS.loadAndPinWaveObject<Tab>(WOS.makeORef("tab", initOpts.tabId)), | ||
| ]); | ||
| const [ws, layoutState] = await Promise.all([ | ||
| WOS.loadAndPinWaveObject<Workspace>(WOS.makeORef("workspace", waveWindow.workspaceid)), | ||
| WOS.reloadWaveObject<LayoutState>(WOS.makeORef("layout", initialTab.layoutstate)), | ||
| ]); | ||
| loadAllWorkspaceTabs(ws); | ||
| WOS.wpsSubscribeToObject(WOS.makeORef("workspace", waveWindow.workspaceid)); | ||
| document.title = `Wave Terminal - ${initialTab.name}`; // TODO update with tab name change | ||
| } catch (e) { | ||
| console.error("Failed initialization error", e); | ||
| getApi().sendLog("Error in initialization (wave.ts, loading required objects) " + e.message + "\n" + e.stack); | ||
| } |
There was a problem hiding this comment.
Consider adding early return or UI fallback after initialization failure.
The try-catch correctly logs initialization errors, but the function continues to execute (lines 201-220) even when core objects fail to load. If client, waveWindow, or initialTab are undefined due to loading failures, downstream code (React components, global store access, etc.) may encounter runtime errors.
Consider one of these approaches:
- Early return with user notification:
} catch (e) {
console.error("Failed initialization error", e);
getApi().sendLog("Error in initialization (wave.ts, loading required objects) " + e.message + "\n" + e.stack);
+ pushFlashError("Failed to initialize Wave Terminal. Please restart the application.");
+ return;
}- Conditional rendering guard:
} catch (e) {
console.error("Failed initialization error", e);
getApi().sendLog("Error in initialization (wave.ts, loading required objects) " + e.message + "\n" + e.stack);
+ // Set a flag so App can render an error state
+ globalStore.set(atoms.initFailure, true);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/wave.ts around lines 184 to 200, the catch logs initialization
failures but execution continues causing downstream runtime errors; modify the
catch to stop further initialization by performing an early return (or set a
visible UI error/fallback state and skip the rest of the init code), and ensure
any subsequent code (lines 201-220) is guarded by a check that required objects
(client, waveWindow, initialTab) are present before use or by rendering a
fallback error UI so no undefined objects are accessed.
| github.com/ubuntu/gowsl v0.0.0-20240906163211-049fd49bd93b | ||
| github.com/wavetermdev/htmltoken v0.2.0 | ||
| github.com/wavetermdev/waveterm/tsunami v0.0.0-00010101000000-000000000000 | ||
| github.com/wavetermdev/waveterm/tsunami v0.12.3 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify tsunami v0.12.3 availability and security status.
The dependency has been updated from a pseudo-version to v0.12.3. Please verify that this version exists and is free from known vulnerabilities.
Run the following script to check the version and security status:
🏁 Script executed:
#!/bin/bash
# Check if the tsunami module version exists and look for security advisories
# Check if the version tag exists in the local tsunami directory (since it's replaced with ./tsunami)
if [ -d "./tsunami" ]; then
cd ./tsunami
echo "=== Checking git tags in ./tsunami ==="
git tag -l "v0.12.3"
echo -e "\n=== Checking current commit in ./tsunami ==="
git log -1 --oneline
cd ..
fi
# Check for Go security advisories related to the module
echo -e "\n=== Checking for security vulnerabilities ==="
go list -m -json github.com/wavetermdev/waveterm/tsunami | jq -r '.Version'Length of output: 265
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect go.mod for tsunami dependency details and replace directive
echo "=== Checking go.mod for tsunami dependency and replace directive ==="
rg -A5 -B5 'tsunami' go.mod
echo -e "\n=== Checking if tsunami is a replace directive ==="
grep -n 'replace.*tsunami' go.mod || echo "No replace directive found in go.mod"
echo -e "\n=== Full tsunami dependency line ==="
grep 'tsunami' go.mod
Length of output: 1034
🌐 Web query:
tsunami v0.12.3 Go module security vulnerabilities
💡 Result:
I couldn't find any public security advisories or CVEs specifically affecting the Tsunami Go module at version v0.12.3. Sources checked: the Tsunami GitHub repo/docs, the Go vulnerability database, and NVD (no tsunami v0.12.3 advisories found). [1][2][3]
If you want, I can:
- re-check NVD/GitHub security advisories and the Go vuln DB for any updates (I can run a fresh search now), or
- show commands to scan your codebase (govulncheck / go list) to detect any transitive vulnerabilities affecting your module.
Sources:
- Tsunami GitHub repository / docs — google/tsunami-security-scanner. [https://github.com/google/tsunami-security-scanner]. [1]
- Go vulnerability database documentation / database. [https://go.dev/doc/security/vuln/database]. [2]
- NVD (National Vulnerability Database) search (no matching CVE for tsunami v0.12.3). [https://nvd.nist.gov]. [3]
Version v0.12.3 declared in go.mod does not exist as a git tag in the local tsunami directory.
The dependency uses a replace directive (replace github.com/wavetermdev/waveterm/tsunami => ./tsunami), so the actual version being used is the current state of the local ./tsunami directory at commit 952f980. The version declaration in go.mod (v0.12.3) is mismatched and misleading. Either:
- Tag the local tsunami repository with v0.12.3 to match the declaration, or
- Update go.mod to reflect the actual commit hash of the local tsunami dependency.
No known security vulnerabilities were found for this module.
🤖 Prompt for AI Agents
In go.mod around line 36, the declared version
"github.com/wavetermdev/waveterm/tsunami v0.12.3" does not match the local
replace (./tsunami) state (commit 952f980), which is misleading; fix by either
tagging the local tsunami repo with v0.12.3 so the version exists, or update the
go.mod entry to reference the actual commit (use the pseudo-version or full
commit-based version) that corresponds to 952f980 (and run go mod tidy) so the
declared version matches the local replace.
this fixes a rare case where a workspace tries to load a tab that doesn't exist. this allows the UI to load even despite that condition. in addition it fixes the "delete tab" path so, even if the tab doesn't exist, it will get cleaned up correctly out of the workspace.