feat: add desktop workspaces support - #1190
Draft
luluiz wants to merge 13 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces multi-workspace support across the desktop app: a filesystem-backed ChangesWorkspace Management Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Root
participant DesktopWorkspacesManager
participant desktopBridge
participant WorkspacesProvider
participant App
Root->>DesktopWorkspacesManager: getCurrent()
DesktopWorkspacesManager->>desktopBridge: getCurrentWorkspace()
desktopBridge-->>DesktopWorkspacesManager: DesktopWorkspace or undefined
DesktopWorkspacesManager-->>Root: Workspace or undefined
Root->>WorkspacesProvider: mount(manager, currentWorkspaceId, onSwitchWorkspace)
WorkspacesProvider->>App: render App(workspaceId=...)
sequenceDiagram
participant WorkspaceSwitcher
participant WorkspacesProvider
participant IWorkspacesManager
participant desktopBridge
WorkspaceSwitcher->>WorkspacesProvider: createWorkspace(name, "local")
WorkspacesProvider->>IWorkspacesManager: create(name, namespace)
IWorkspacesManager->>desktopBridge: createWorkspace(name, namespace)
desktopBridge-->>IWorkspacesManager: DesktopWorkspace
IWorkspacesManager-->>WorkspacesProvider: Workspace
WorkspacesProvider->>WorkspacesProvider: refresh() via manager.list
WorkspaceSwitcher->>WorkspacesProvider: switchWorkspace(newId)
WorkspacesProvider->>IWorkspacesManager: setCurrent(newId)
WorkspacesProvider-->>WorkspaceSwitcher: onSwitchWorkspace()
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.tsx (1)
291-322: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a desktop
org/filesystemtest, and flatten the loader selectionpackages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.tsx:295-296
ExtensionCatalogProvider.test.tsxdoesn’t cover the new desktoporg→filesystemuninstall branch; add a case so this path doesn’t regress.- The nested ternary used to compute
loaderTypeis still hard to scan; split it into a smallif/else.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.tsx` around lines 291 - 322, The uninstall flow in ExtensionCatalogProvider needs two updates: the new desktop org namespace path should be covered by a test, and the loader-type selection should be easier to read. Add a unit test in ExtensionCatalogProvider.test.tsx that exercises uninstallExtension for a desktop org extension using the filesystem loader, and verify the correct uninstall call is made. In ExtensionCatalogProvider.tsx, simplify the loaderType computation inside uninstallExtension by replacing the nested ternary with a small if/else using isDesktopApp(), while keeping the same local/org behavior.Source: Linters/SAST tools
🧹 Nitpick comments (13)
packages/suite-base/src/components/AppBar/WorkspaceSwitcher.style.ts (1)
7-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded rgba colors bypass the theme system.
borderColor/hoverborderColor/backgroundColoruse rawrgba(255, 255, 255, ...)literals instead of theme tokens (e.g., via MUI'salpha()ontheme.palette.common.whiteortheme.palette.action.hover). As per path instructions, style files using MUI/emotion should be checked for "no hardcoded colors that bypass the theme system."♻️ Example using theme-derived alpha values
+import { alpha } from "`@mui/material`"; import { makeStyles } from "tss-react/mui"; export const useStyles = makeStyles()((theme) => ({ button: { font: "inherit", height: theme.spacing(4), fontSize: theme.typography.body2.fontSize, color: theme.palette.common.white, - borderColor: "rgba(255, 255, 255, 0.24)", + borderColor: alpha(theme.palette.common.white, 0.24), "&:hover": { - borderColor: "rgba(255, 255, 255, 0.4)", - backgroundColor: "rgba(255, 255, 255, 0.08)", + borderColor: alpha(theme.palette.common.white, 0.4), + backgroundColor: alpha(theme.palette.common.white, 0.08), }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/components/AppBar/WorkspaceSwitcher.style.ts` around lines 7 - 18, The WorkspaceSwitcher.style button styles are using hardcoded rgba literals for border and hover colors instead of theme-derived values. Update the style object in WorkspaceSwitcher.style.ts to use MUI theme tokens or alpha-based colors from theme.palette.common.white or theme.palette.action so the button styling stays consistent with the theme system. Refer to the button style block in the WorkspaceSwitcher style definition when making the replacement.Source: Path instructions
packages/suite-base/src/IdbLayoutStorage.ts (2)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
#dbasreadonly.It's only assigned in the constructor and never reassigned afterward. Static analysis flags this.
🔧 Proposed fix
- `#db`; + readonly `#db`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/IdbLayoutStorage.ts` at line 48, Mark the private field `#db` as readonly in IdbLayoutStorage since it is only initialized in the constructor and never reassigned. Update the class field declaration for `#db` so static analysis recognizes the intended immutability, keeping the constructor assignment unchanged.Source: Linters/SAST tools
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAsync DB open in constructor (Sonar flag) — pre-existing pattern, low materiality.
The prior code did the same async
IDB.openDB(...)work as a field initializer; moving it into an explicit constructor doesn't change runtime behavior, it just makes the pattern visible to the linter. A "fix" would require an async factory (static async create(...)) instead ofnew IdbLayoutStorage(...), which is a breaking API change for every caller (includingApp.tsx). Given the deferred-promise pattern is consumed correctly by every method (await this.#db), I'd treat this as low priority.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/IdbLayoutStorage.ts` around lines 54 - 63, The async DB open in IdbLayoutStorage’s constructor is a pre-existing deferred-promise pattern and does not need a behavioral change. Keep the current IDB.openDB setup in the constructor and, if this Sonar warning must be addressed, add a targeted suppression or explanatory annotation around the constructor rather than changing IdbLayoutStorage to an async factory, since that would break callers like App.tsx.Source: Linters/SAST tools
e2e/page-objects/WorkspaceSwitcher.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent file naming vs. other page objects.
Existing page objects use kebab-case filenames (
app-menu,data-source-dialog,extension-manager,layout-manager,player-controls), while this one usesWorkspaceSwitcher.ts. Consider renaming toworkspace-switcher.tsfor consistency (and updating the barrel import accordingly).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/page-objects/WorkspaceSwitcher.ts` at line 10, The WorkspaceSwitcher page object file name is inconsistent with the other page objects’ kebab-case naming. Rename the WorkspaceSwitcher module to use a kebab-case filename like the other page objects, and update any barrel export or import references that currently point to WorkspaceSwitcher so they resolve to the new path.packages/suite-base/src/components/AppBar/WorkspaceSwitcher.test.tsx (1)
171-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo coverage for the org-namespace create flow.
handleCreateaccepts aNamespaceparameter ("local" | "org"), but tests only exercisecreate-personal-workspace. Consider adding a case for the org creation path if a corresponding menu item/test id exists, to actually validate the namespace argument is passed correctly rather than just the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/components/AppBar/WorkspaceSwitcher.test.tsx` around lines 171 - 192, The WorkspaceSwitcher test only covers the local create flow, leaving the Namespace argument path in handleCreate unverified. Add a second test in WorkspaceSwitcher.test.tsx that triggers the org create menu item (using the matching test id if present) and assert createWorkspace is called with the prompt value and "org", so both namespace branches of handleCreate are covered.packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts (2)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
node:protocol for builtin imports.Static analysis flags
crypto,fs,fs/promises, andpathimports; prefer thenode:prefixed specifiers, per theunicorn/prefer-node-protocolconvention.🔧 Proposed fix
-import { randomUUID } from "crypto"; -import { existsSync } from "fs"; -import { mkdir, readdir, readFile, rm, writeFile } from "fs/promises"; -import { join as pathJoin } from "path"; +import { randomUUID } from "node:crypto"; +import { existsSync } from "node:fs"; +import { mkdir, readdir, readFile, rm, writeFile } from "node:fs/promises"; +import { join as pathJoin } from "node:path";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts` around lines 8 - 11, The builtin imports in WorkspacesManager should use the node: protocol to satisfy unicorn/prefer-node-protocol. Update the existing import statements for randomUUID, existsSync, mkdir/readdir/readFile/rm/writeFile, and pathJoin to use node:crypto, node:fs, node:fs/promises, and node:path respectively. Keep the rest of the WorkspacesManager module unchanged and make sure all referenced symbols still resolve correctly after the import specifier update.Source: Linters/SAST tools
56-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnsynchronized read-modify-write on
state.json.
#readState/#writeStateand their callers indelete()(Lines 156-159) andsetCurrent()(Lines 183-184) perform a read-then-write with no locking. Concurrent IPC calls (e.g. adeleteracing asetCurrent) can interleave and clobber each other's update tocurrentWorkspaceId. Given this only affects a small persisted selection (not workspace content), impact is limited but still worth serializing writes (e.g., an in-memory promise chain per instance) for correctness.Also applies to: 153-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts` around lines 56 - 70, The `WorkspacesManager` state updates are doing unsynchronized read-modify-write cycles in `#readState`, `#writeState`, and the callers in `delete()` and `setCurrent()`, so concurrent IPC requests can overwrite `currentWorkspaceId`. Serialize access to `state.json` within `WorkspacesManager` by introducing an in-memory per-instance write queue/promise chain and using it around both read-update-write paths so `delete()` and `setCurrent()` cannot interleave.packages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.ts (2)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
#bridgeasreadonly.It's only assigned in the constructor.
🎨 Proposed fix
- `#bridge`: Desktop; + readonly `#bridge`: Desktop;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.ts` at line 22, The DesktopWorkspacesManager `#bridge` field is only initialized in the constructor and never reassigned, so mark the private field as readonly. Update the class definition for DesktopWorkspacesManager to make `#bridge` immutable while keeping its constructor assignment intact.Source: Linters/SAST tools
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
#bridgeasreadonly. It’s only assigned in the constructor, so it can be immutable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.ts` around lines 8 - 9, Mark the `#bridge` field in DesktopWorkspacesManager as readonly since it is only assigned in the constructor. Update the class definition so `#bridge` is immutable after initialization, keeping the constructor assignment unchanged and using the existing `#bridge` symbol as the reference point.packages/suite-base/src/index.ts (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse multi-line import per formatting rule.
SonarCloud flags this as fitting on a single line within the project's print-width. As per coding guidelines, "Use 100-character print width" for
**/*.{ts,tsx,js,jsx,mjs,cjs}.🎨 Proposed formatting fix
-export type { - IWorkspacesManager, - Workspace, -} from "./services/workspaces/IWorkspacesManager"; +export type { IWorkspacesManager, Workspace } from "./services/workspaces/IWorkspacesManager";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-base/src/index.ts` around lines 42 - 45, The type export in the index entrypoint is unnecessarily split across multiple lines and should be collapsed to a single line to match the project’s 100-character print-width rule. Update the export in the module entrypoint so the `export type` statement for `IWorkspacesManager` and `Workspace` stays within the formatter’s preferred single-line form, preserving the same re-exports while matching the existing style.Sources: Coding guidelines, Linters/SAST tools
packages/suite-desktop/src/common/types.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
Namespacefrom the package rootUse
@lichtblick/suite-basehere instead of@lichtblick/suite-base/src/types/Namespace;Namespaceis already re-exported from the public entry point, and keeping imports on the package surface avoids boundary drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/common/types.ts` around lines 8 - 9, The Namespace import is reaching into an internal path instead of the public package surface. Update the import in types.ts to use the package root re-export from `@lichtblick/suite-base`, keeping the reference to Namespace but avoiding the deep src/types/Namespace path.packages/suite-desktop/src/main/index.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
node:pathfor the built-in import.Static analysis flags this; use the
node:protocol prefix for built-in modules.♻️ Suggested fix
-import { join as pathJoin } from "path"; +import { join as pathJoin } from "node:path";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/main/index.ts` at line 9, The built-in path import in index.ts should use the node: protocol instead of the bare module specifier. Update the existing pathJoin import to reference the built-in module via node:path, keeping the same alias and usage unchanged so the rest of the file continues to work.Source: Linters/SAST tools
packages/suite-desktop/src/main/workspaces/WorkspacesManager.test.ts (1)
1-366: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid, well-structured test suite (GWT, meaningful assertions).
Once workspace
idvalidation is added at the IPC/manager boundary (see companion comment onmain/index.ts), consider adding cases here assertingrename/delete/setCurrentreject ids containing path-traversal sequences (e.g."../evil").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.test.ts` around lines 1 - 366, The WorkspacesManager test suite should cover the new workspace id validation at the boundary by adding negative cases for path-traversal ids. Extend the existing rename, delete, and setCurrent specs in WorkspacesManager.test to assert that ids like "../evil" are rejected with the expected error, so the behavior is verified alongside the current not-found cases. Use the existing WorkspacesManager methods and the current error assertions as the place to add these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/suite-base/src/App.tsx`:
- Around line 124-127: The layout storage in App.tsx is recreated when
workspaceId changes, but the previous IdbLayoutStorage instance is never
disposed, leaving old IndexedDB connections open. Update the App component’s
layoutStorage handling to add a close/dispose path on IdbLayoutStorage and
invoke it in cleanup whenever the memoized storage changes or the component
unmounts, so stale workspace databases can be released and deleted cleanly.
In `@packages/suite-base/src/components/AppBar/WorkspaceSwitcher.tsx`:
- Around line 40-103: Add proper async error handling in WorkspaceSwitcher’s
handleCreate, handleRename, handleDelete, and handleSwitch callbacks: each
workspacesContext call should be wrapped in try/catch instead of being
fire-and-forget with void so rejections are not swallowed. Keep the existing
handleClose flow, but on failure surface a meaningful user-visible error (or
toast/dialog) and optionally log the underlying exception. Focus the fix around
the handleCreate/handleRename/handleDelete/handleSwitch functions and the
workspacesContext.createWorkspace, renameWorkspace, deleteWorkspace, and
switchWorkspace calls.
In `@packages/suite-desktop/src/main/index.ts`:
- Around line 230-252: Validate the renderer-supplied workspace id before any
filesystem operation in the WorkspacesManager flow, especially through the
workspaces:rename and workspaces:delete IPC handlers. Add a root-bound path
check in the manager methods that resolve ids into paths so values like
../../Documents are rejected before pathJoin or rm are reached. Keep the fix
localized to the workspace id handling paths used by ipcMain.handle and
WorkspacesManager.rename/delete, and return an error for ids outside the
workspace root.
In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts`:
- Around line 39-45: The workspace path helpers and IPC-backed methods are using
renderer-supplied `id` values directly, which allows path traversal outside
`#workspacesRoot`. Add validation/sanitization in `WorkspacesManager` before
`#configPath`, `rename`, `delete`, `setCurrent`, and `getConfig` use the id, and
reject any id that is not a safe workspace identifier. Keep `create()` using
`randomUUID()` as-is, and ensure the `ipcMain.handle`-driven flows only operate
on validated ids so filesystem reads, writes, and deletes stay confined to the
workspace root.
In `@packages/suite-desktop/src/renderer/Root.tsx`:
- Around line 106-113: `handleSwitchWorkspace` currently awaits
`workspacesManager.getCurrent()` without any error handling, so a rejected IPC
call can become an unhandled promise rejection and leave `currentWorkspace`
stale. Update the async IIFE inside `handleSwitchWorkspace` in `Root` to wrap
the `getCurrent()` call in try/catch, follow the same error-handling pattern
used in the mount effect above, and log or surface a meaningful message while
only calling `setCurrentWorkspace` when the fetch succeeds and the component is
still mounted.
---
Outside diff comments:
In
`@packages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.tsx`:
- Around line 291-322: The uninstall flow in ExtensionCatalogProvider needs two
updates: the new desktop org namespace path should be covered by a test, and the
loader-type selection should be easier to read. Add a unit test in
ExtensionCatalogProvider.test.tsx that exercises uninstallExtension for a
desktop org extension using the filesystem loader, and verify the correct
uninstall call is made. In ExtensionCatalogProvider.tsx, simplify the loaderType
computation inside uninstallExtension by replacing the nested ternary with a
small if/else using isDesktopApp(), while keeping the same local/org behavior.
---
Nitpick comments:
In `@e2e/page-objects/WorkspaceSwitcher.ts`:
- Line 10: The WorkspaceSwitcher page object file name is inconsistent with the
other page objects’ kebab-case naming. Rename the WorkspaceSwitcher module to
use a kebab-case filename like the other page objects, and update any barrel
export or import references that currently point to WorkspaceSwitcher so they
resolve to the new path.
In `@packages/suite-base/src/components/AppBar/WorkspaceSwitcher.style.ts`:
- Around line 7-18: The WorkspaceSwitcher.style button styles are using
hardcoded rgba literals for border and hover colors instead of theme-derived
values. Update the style object in WorkspaceSwitcher.style.ts to use MUI theme
tokens or alpha-based colors from theme.palette.common.white or
theme.palette.action so the button styling stays consistent with the theme
system. Refer to the button style block in the WorkspaceSwitcher style
definition when making the replacement.
In `@packages/suite-base/src/components/AppBar/WorkspaceSwitcher.test.tsx`:
- Around line 171-192: The WorkspaceSwitcher test only covers the local create
flow, leaving the Namespace argument path in handleCreate unverified. Add a
second test in WorkspaceSwitcher.test.tsx that triggers the org create menu item
(using the matching test id if present) and assert createWorkspace is called
with the prompt value and "org", so both namespace branches of handleCreate are
covered.
In `@packages/suite-base/src/IdbLayoutStorage.ts`:
- Line 48: Mark the private field `#db` as readonly in IdbLayoutStorage since it
is only initialized in the constructor and never reassigned. Update the class
field declaration for `#db` so static analysis recognizes the intended
immutability, keeping the constructor assignment unchanged.
- Around line 54-63: The async DB open in IdbLayoutStorage’s constructor is a
pre-existing deferred-promise pattern and does not need a behavioral change.
Keep the current IDB.openDB setup in the constructor and, if this Sonar warning
must be addressed, add a targeted suppression or explanatory annotation around
the constructor rather than changing IdbLayoutStorage to an async factory, since
that would break callers like App.tsx.
In `@packages/suite-base/src/index.ts`:
- Around line 42-45: The type export in the index entrypoint is unnecessarily
split across multiple lines and should be collapsed to a single line to match
the project’s 100-character print-width rule. Update the export in the module
entrypoint so the `export type` statement for `IWorkspacesManager` and
`Workspace` stays within the formatter’s preferred single-line form, preserving
the same re-exports while matching the existing style.
In `@packages/suite-desktop/src/common/types.ts`:
- Around line 8-9: The Namespace import is reaching into an internal path
instead of the public package surface. Update the import in types.ts to use the
package root re-export from `@lichtblick/suite-base`, keeping the reference to
Namespace but avoiding the deep src/types/Namespace path.
In `@packages/suite-desktop/src/main/index.ts`:
- Line 9: The built-in path import in index.ts should use the node: protocol
instead of the bare module specifier. Update the existing pathJoin import to
reference the built-in module via node:path, keeping the same alias and usage
unchanged so the rest of the file continues to work.
In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.test.ts`:
- Around line 1-366: The WorkspacesManager test suite should cover the new
workspace id validation at the boundary by adding negative cases for
path-traversal ids. Extend the existing rename, delete, and setCurrent specs in
WorkspacesManager.test to assert that ids like "../evil" are rejected with the
expected error, so the behavior is verified alongside the current not-found
cases. Use the existing WorkspacesManager methods and the current error
assertions as the place to add these checks.
In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts`:
- Around line 8-11: The builtin imports in WorkspacesManager should use the
node: protocol to satisfy unicorn/prefer-node-protocol. Update the existing
import statements for randomUUID, existsSync,
mkdir/readdir/readFile/rm/writeFile, and pathJoin to use node:crypto, node:fs,
node:fs/promises, and node:path respectively. Keep the rest of the
WorkspacesManager module unchanged and make sure all referenced symbols still
resolve correctly after the import specifier update.
- Around line 56-70: The `WorkspacesManager` state updates are doing
unsynchronized read-modify-write cycles in `#readState`, `#writeState`, and the
callers in `delete()` and `setCurrent()`, so concurrent IPC requests can
overwrite `currentWorkspaceId`. Serialize access to `state.json` within
`WorkspacesManager` by introducing an in-memory per-instance write queue/promise
chain and using it around both read-update-write paths so `delete()` and
`setCurrent()` cannot interleave.
In `@packages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.ts`:
- Line 22: The DesktopWorkspacesManager `#bridge` field is only initialized in the
constructor and never reassigned, so mark the private field as readonly. Update
the class definition for DesktopWorkspacesManager to make `#bridge` immutable
while keeping its constructor assignment intact.
- Around line 8-9: Mark the `#bridge` field in DesktopWorkspacesManager as
readonly since it is only assigned in the constructor. Update the class
definition so `#bridge` is immutable after initialization, keeping the constructor
assignment unchanged and using the existing `#bridge` symbol as the reference
point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd98e464-5d9c-4176-866f-7ccac63c8b5c
📒 Files selected for processing (36)
e2e/fixtures/electron.tse2e/page-objects/WorkspaceSwitcher.tse2e/page-objects/index.tse2e/tests/desktop/workspaces/create-switch-workspace.desktop.spec.tse2e/tests/desktop/workspaces/persist-current-workspace.desktop.spec.tspackages/suite-base/src/App.tsxpackages/suite-base/src/IdbLayoutStorage.test.tspackages/suite-base/src/IdbLayoutStorage.tspackages/suite-base/src/components/AppBar/WorkspaceSwitcher.stories.tsxpackages/suite-base/src/components/AppBar/WorkspaceSwitcher.style.tspackages/suite-base/src/components/AppBar/WorkspaceSwitcher.test.tsxpackages/suite-base/src/components/AppBar/WorkspaceSwitcher.tsxpackages/suite-base/src/components/AppBar/index.tsxpackages/suite-base/src/context/WorkspacesContext.tspackages/suite-base/src/i18n/en/index.tspackages/suite-base/src/i18n/en/workspaces.tspackages/suite-base/src/index.tspackages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.test.tsxpackages/suite-base/src/providers/ExtensionCatalogProvider/ExtensionCatalogProvider.tsxpackages/suite-base/src/providers/WorkspacesProvider.test.tsxpackages/suite-base/src/providers/WorkspacesProvider.tsxpackages/suite-base/src/services/workspaces/IWorkspacesManager.tspackages/suite-base/src/testing/builders/WorkspaceBuilder.tspackages/suite-base/src/types.tspackages/suite-base/src/types/Namespace.tspackages/suite-desktop/src/common/types.tspackages/suite-desktop/src/common/workspaces.tspackages/suite-desktop/src/main/index.tspackages/suite-desktop/src/main/workspaces/WorkspacesManager.test.tspackages/suite-desktop/src/main/workspaces/WorkspacesManager.tspackages/suite-desktop/src/preload/index.tspackages/suite-desktop/src/renderer/Root.tsxpackages/suite-desktop/src/renderer/services/DesktopExtensionLoader.test.tspackages/suite-desktop/src/renderer/services/DesktopExtensionLoader.tspackages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.test.tspackages/suite-desktop/src/renderer/services/DesktopWorkspacesManager.ts
Comment on lines
+230
to
+252
| // Workspace management. Each workspace groups the extensions and layouts loaded by the app; when | ||
| // no workspace is selected the app falls back to the legacy global folders. | ||
| const workspacesManager = new WorkspacesManager( | ||
| pathJoin(app.getPath("home"), SUITE_ROOT_FOLDER, WORKSPACES_FOLDER), | ||
| ); | ||
| ipcMain.handle("workspaces:list", async () => await workspacesManager.list()); | ||
| ipcMain.handle( | ||
| "workspaces:create", | ||
| async (_ev, name: string, namespace: WorkspaceNamespace) => | ||
| await workspacesManager.create(name, namespace), | ||
| ); | ||
| ipcMain.handle( | ||
| "workspaces:rename", | ||
| async (_ev, id: string, name: string) => await workspacesManager.rename(id, name), | ||
| ); | ||
| ipcMain.handle("workspaces:delete", async (_ev, id: string) => { | ||
| await workspacesManager.delete(id); | ||
| }); | ||
| ipcMain.handle("workspaces:getCurrent", async () => await workspacesManager.getCurrent()); | ||
| ipcMain.handle("workspaces:setCurrent", async (_ev, id: string | undefined) => { | ||
| await workspacesManager.setCurrent(id); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm WorkspacesManager performs no id sanitization before building paths.
rg -n -A3 '`#configPath`|`#toWorkspace`|async delete\(' packages/suite-desktop/src/main/workspaces/WorkspacesManager.tsRepository: lichtblick-suite/lichtblick
Length of output: 1572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workspace manager and workspace types for any existing validation.
sed -n '1,260p' packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts
printf '\n---\n'
rg -n "type WorkspaceNamespace|interface WorkspaceNamespace|export .*WorkspaceNamespace|WorkspaceNamespace" packages/suite-desktop/src/main -g '!**/*.map'
printf '\n---\n'
rg -n "workspaces:(create|rename|delete|setCurrent)|desktopBridge\.(createWorkspace|renameWorkspace|deleteWorkspace|setCurrentWorkspace)" packages/suite-desktop/src -g '!**/*.map'Repository: lichtblick-suite/lichtblick
Length of output: 7607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where workspace IDs are created and whether they are constrained to UUIDs.
rg -n "crypto\.randomUUID|randomUUID\(|id:" packages/suite-desktop/src packages -g '!**/*.map'Repository: lichtblick-suite/lichtblick
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts
printf '\n---\n'
rg -n "type WorkspaceNamespace|interface WorkspaceNamespace|export .*WorkspaceNamespace|WorkspaceNamespace" packages/suite-desktop/src/main -g '!**/*.map'
printf '\n---\n'
rg -n "workspaces:(create|rename|delete|setCurrent)|desktopBridge\.(createWorkspace|renameWorkspace|deleteWorkspace|setCurrentWorkspace)" packages/suite-desktop/src -g '!**/*.map'
printf '\n---\n'
rg -n "crypto\.randomUUID|randomUUID\(" packages/suite-desktop/src packages -g '!**/*.map'Repository: lichtblick-suite/lichtblick
Length of output: 7806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the actual runtime-relevant definition of WorkspaceNamespace and any existing validation helpers.
sed -n '1,220p' packages/suite-desktop/src/common/types.ts
printf '\n---\n'
sed -n '1,220p' packages/suite-desktop/src/main/workspaces/WorkspacesManager.test.tsRepository: lichtblick-suite/lichtblick
Length of output: 14882
Validate workspace ids before filesystem access workspaces:rename and workspaces:delete forward renderer-controlled id into pathJoin(...)/rm(...) with no sanitization. A crafted value like ../../Documents can escape workspacesRoot; reject paths outside the workspace root before calling the manager.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/suite-desktop/src/main/index.ts` around lines 230 - 252, Validate
the renderer-supplied workspace id before any filesystem operation in the
WorkspacesManager flow, especially through the workspaces:rename and
workspaces:delete IPC handlers. Add a root-bound path check in the manager
methods that resolve ids into paths so values like ../../Documents are rejected
before pathJoin or rm are reached. Keep the fix localized to the workspace id
handling paths used by ipcMain.handle and WorkspacesManager.rename/delete, and
return an error for ids outside the workspace root.
Source: Linters/SAST tools
Comment on lines
+39
to
+45
| #configPath(id: string): string { | ||
| return pathJoin(this.#workspacesRoot, id, WORKSPACE_CONFIG_FILE); | ||
| } | ||
|
|
||
| #statePath(): string { | ||
| return pathJoin(this.#workspacesRoot, WORKSPACES_STATE_FILE); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Path traversal via unauthenticated id — arbitrary directory read/write/delete.
#configPath, rename, delete, setCurrent, and getConfig all join the caller-supplied id directly into filesystem paths with no sanitization (Lines 39-41, 130-185). Per the provided context, main/index.ts wires ipcMain.handle("workspaces:delete"/"rename"/"setCurrent", ...) handlers that delegate directly to these methods, so the renderer fully controls id. A crafted id such as "../../Documents" passed to delete() (Line 154) would resolve outside #workspacesRoot and trigger rm(..., { recursive: true, force: true }) on an arbitrary directory — an unauthenticated, destructive path-traversal vulnerability. The same traversal applies to rename's write (Line 146) and getConfig's read (Line 74).
create() is unaffected since its id is generated internally via randomUUID() (Line 109).
🔒 Proposed fix: validate id before use
`#configPath`(id: string): string {
+ this.#assertSafeId(id);
return pathJoin(this.#workspacesRoot, id, WORKSPACE_CONFIG_FILE);
}
+
+ `#assertSafeId`(id: string): void {
+ if (
+ id.length === 0 ||
+ id === "." ||
+ id === ".." ||
+ id.includes("/") ||
+ id.includes("\\")
+ ) {
+ throw new Error(`Invalid workspace id: ${id}`);
+ }
+ } public async delete(id: string): Promise<void> {
+ this.#assertSafeId(id);
await rm(pathJoin(this.#workspacesRoot, id), { recursive: true, force: true }); public async setCurrent(id: string | undefined): Promise<void> {
if (id != undefined) {
+ this.#assertSafeId(id);
const config = await this.getConfig(id);Also applies to: 130-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/suite-desktop/src/main/workspaces/WorkspacesManager.ts` around lines
39 - 45, The workspace path helpers and IPC-backed methods are using
renderer-supplied `id` values directly, which allows path traversal outside
`#workspacesRoot`. Add validation/sanitization in `WorkspacesManager` before
`#configPath`, `rename`, `delete`, `setCurrent`, and `getConfig` use the id, and
reject any id that is not a safe workspace identifier. Keep `create()` using
`randomUUID()` as-is, and ensure the `ipcMain.handle`-driven flows only operate
on validated ids so filesystem reads, writes, and deletes stay confined to the
workspace root.
Add workspace support to the desktop (Electron) build, grouping layouts
and extensions per workspace. Workspaces are stored under
~/.lichtblick-suite/workspaces/<id>/{extensions,layouts} with a
workspace.json manifest and a root state.json tracking the current
workspace. Falls back to the legacy ~/.lichtblick-suite/{extensions,layouts}
folders when no workspace is active (fully backward compatible).
Includes the main-process WorkspacesManager plus IPC, preload dynamic
directory resolution, a shared WorkspacesContext/Provider, namespace-aware
extension loading, per-workspace layout storage isolation, and an AppBar
WorkspaceSwitcher for create/rename/delete/switch. Reuses the existing
Namespace type (local/org), extracted into a lightweight module.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Declare the Playwright electronArgs, preInstalledExtensions and preSeededWorkspaces fixtures as proper options so multi-element test.use overrides are passed through instead of being misread as a [value, options] tuple, which crashed the persisted-workspace spec with "object is not iterable". Add DataSourceDialog.closeIfVisible and dismiss the start dialog that reopens after the workspace keyed remount, and apply the Prettier formatting fixes flagged by CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Recreating the storage on workspaceId change left the prior IndexedDB connection open, leaking connections and blocking cleanup of stale per-workspace databases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Playwright's isFixtureTuple treats an option value that is an array whose second element is an object as a [value, options] tuple, collapsing a bare PreSeededWorkspace[] to its first entry and crashing the fixture with "object is not iterable". Wrap the list in an object so it is passed through intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Validate workspace ids against path traversal and serialize state writes in WorkspacesManager - Add async error handling with user notifications in WorkspaceSwitcher and Root.tsx - Mark IdbLayoutStorage #db and DesktopWorkspacesManager #bridge readonly - Use node: protocol imports in desktop main/WorkspacesManager - Replace hardcoded rgba with theme alpha() colors in WorkspaceSwitcher styles - Simplify ExtensionCatalogProvider loader-type selection and add desktop-org uninstall test - Rename e2e workspace switcher page object to kebab-case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
luluiz
force-pushed
the
feature/desktop-workspaces
branch
from
July 16, 2026 15:07
52b3aa6 to
a38a026
Compare
Add coverage for get/put/delete, list namespace isolation and migration error handling, close, database-name scoping, importLayouts empty-source no-op, and migrateUnnamespacedLayouts localStorage migration. Also apply Biome formatting to the persist-current-workspace e2e spec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the standalone layoutDatabaseName export so IdbLayoutStorage.ts only exposes the class. The constructor now takes an optional workspaceId and computes the scoped database name internally, keeping the naming responsibility inside the class. Update App.tsx and tests accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…eSwitcher story The @storybook/react specifier is not a declared dependency of @lichtblick/suite-base, failing the dependency lint. Use the declared @storybook/react-webpack5 package like the other stories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bezerra Luiz, (Luiz.Bezerra@ctw.bmwgroup.com) <luiz.bezerra@ctw.bmwgroup.com>
…ot test Add missing @testing-library/react dev dependency, move RootProps into a shared Root.types.ts, and replace the hand-written mock types in Root.test.tsx with a MockedMethods helper derived from the real IAppConfiguration, Desktop, IWorkspacesManager, and NativeWindow types. Also trim redundant comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The build:packages job type-checks Root.test.tsx through renderer/tsconfig.json which sets rootDir to the package root. Importing IWorkspacesManager via the "@lichtblick/suite-base/services/..." specifier hit the tsconfig paths mapping, resolving suite-base sources as project inputs and cascading the entire package into the rootDir-constrained program (TS6059, 313 errors). Use the "@lichtblick/suite-base/src/..." form so resolution falls back to node_modules (external), matching the existing DesktopWorkspacesManager test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Adds workspace support to the desktop (Electron) build. Workspaces group layouts and extensions and previously existed only on the web build (via a closed-source wrapper). This introduces a generic, open-source implementation that stores workspaces on disk and lets users switch between them from the AppBar.
Motivation
On web, a closed-source wrapper manages workspaces through an LB API, associating extensions and layouts with each workspace. The desktop build had no equivalent. Desktop loads layouts and extensions from the user's
~/.lichtblick-suiteroot on disk, so this feature extends that model with per-workspace isolation.On-disk layout
Workspaces live under
~/.lichtblick-suite/workspaces/<id>/:workspace.json—{ id, name, namespace, createdAt, updatedAt }extensions/— per-workspace extensionslayouts/— per-workspace layoutsA root
state.jsontracks the current workspace. When no workspace is active, the app falls back to the legacy~/.lichtblick-suite/{extensions,layouts}folders (backward compatible).What's included
WorkspacesManagerfilesystem CRUD (create/list/rename/delete/get/set current) + IPC handlers.WorkspacesContext+WorkspacesProvider(hidden on web, where no provider is present). Reuses the existingNamespace = "local" | "org"type ("local" = personal, "org" = organization), extracted into a lightweighttypes/Namespace.tsand re-exported fromtypes.ts. Namespace-aware extension loading and per-workspaceIdbLayoutStorageisolation.WorkspaceSwitcherdropdown in the AppBar (desktop only) to switch, create, rename, and delete workspaces.Switching behavior
Selecting a workspace persists it via the main process (single source of truth) and triggers a keyed remount of the app, so extension loaders and layout storage rebind to the new workspace directories — no full page reload.
Testing
tsc(suite-base, suite-desktop, quicklook), ESLint, and the production desktop webpack build all pass.Notes
localcreation is exposed).🤖 This PR was prepared with assistance from GitHub Copilot CLI.
Summary by CodeRabbit
New Features
Bug Fixes