feat(desktop): workspace for each design projects#173
feat(desktop): workspace for each design projects#173hqhq1025 merged 21 commits intoOpenCoworkAI:mainfrom
Conversation
There was a problem hiding this comment.
Findings
-
[Blocker] Silent workspace write-through failure violates no-silent-fallback constraint — write failures are logged and then suppressed, so users can continue with DB state diverging from workspace files without an explicit failure in UI/runtime, evidence
apps/desktop/src/main/index.ts:230
Suggested fix:try { mkdirSync(path_module.dirname(destinationPath), { recursive: true }); writeFileSync(destinationPath, content, 'utf8'); } catch (err) { const message = err instanceof Error ? err.message : String(err); logger.error('runtime.fs.writeThrough.fail', { designId, filePath, workspacePath: design.workspacePath, message, }); throw new Error(`Workspace write-through failed for ${filePath}: ${message}`); }
-
[Major] Workspace IPC collapses actionable bind/migration errors into a generic message — migration collisions and missing tracked files are converted to
Workspace update failed, removing context users need to resolve the issue, evidenceapps/desktop/src/main/snapshots-ipc.ts:410
Suggested fix:} catch (err) { if (err instanceof CodesignError) throw err; if (err instanceof Error) { if (err.message.includes('already bound')) { throw new CodesignError(err.message, 'IPC_CONFLICT', { cause: err }); } if ( err.message.includes('Workspace migration collision') || err.message.includes('Tracked workspace file missing') ) { throw new CodesignError(err.message, 'IPC_BAD_INPUT', { cause: err }); } throw new CodesignError(`Workspace update failed: ${err.message}`, 'IPC_DB_ERROR', { cause: err, }); } throw new CodesignError('Workspace update failed', 'IPC_DB_ERROR', { cause: err }); }
-
[Minor] Rebind prompt can trigger for semantically identical paths (separator/trailing-slash differences) — raw string comparison can show a rebind confirmation for the same folder, evidence
apps/desktop/src/renderer/src/components/FilesTabView.tsx:53
Suggested fix:const normalizeUiPath = (p: string) => p.replaceAll('\\', '/').replace(/\/+$/, ''); if ( currentDesign.workspacePath && normalizeUiPath(currentDesign.workspacePath) !== normalizeUiPath(path) ) { requestWorkspaceRebind(currentDesign, path); }
Summary
- Review mode: initial
- 3 issues found (1 Blocker, 1 Major, 1 Minor) in workspace write-through/error handling path.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Coverage gap: no assertion that workspace write-through failures are surfaced (throw/toast) instead of only logged.
open-codesign Bot
| const design = getDesign(db, designId); | ||
| if (design === null || design.workspacePath === null) return; | ||
| const destinationPath = path_module.join(design.workspacePath, filePath); | ||
| try { |
There was a problem hiding this comment.
Silent fallback: write-through errors are logged but swallowed here, which violates the project constraint of surfacing all errors. Consider throwing with file/workspace context so the failure reaches UI/runtime state handling.
| if (err instanceof Error && err.message.includes('already bound')) { | ||
| throw new CodesignError(err.message, 'IPC_CONFLICT'); | ||
| } | ||
| throw new CodesignError('Workspace update failed', 'IPC_DB_ERROR', { cause: err }); |
There was a problem hiding this comment.
This catch maps most bind/migration failures to a generic Workspace update failed, which strips actionable details (for example migration collisions). Preserve and classify known error messages so users get a resolvable error.
| setPicking(true); | ||
| const path = await window.codesign.snapshots.pickWorkspaceFolder(); | ||
| if (path && currentDesign && currentDesignId) { | ||
| if (currentDesign.workspacePath && currentDesign.workspacePath !== path) { |
There was a problem hiding this comment.
Raw path string comparison can treat the same folder as different (for example trailing slash or separator differences), causing an unnecessary rebind confirmation. Normalize both paths before comparing.
- Surface write-through failure in persistMutation instead of silently swallowing the error, so callers can propagate it to the UI - Classify workspace migration collision and missing tracked-file errors as IPC_BAD_INPUT in snapshots-ipc for actionable client-side messages - Normalize workspace paths before comparison in FilesTabView to avoid false rebind prompts caused by trailing slashes or path-separator diffs Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
There was a problem hiding this comment.
Findings
-
[Blocker] Workspace write-through can target a different on-disk path than the DB path on POSIX —
upsertDesignFilenormalizes separators (e.g.nested\\index.html->nested/index.html), but disk write still uses the rawfilePath, so DB and workspace can diverge and later migration/checks act on the wrong file, evidenceapps/desktop/src/main/index.ts:229.
Suggested fix:function persistMutation(filePath: string, content: string): void { if (designId === null || db === null) return; const persisted = upsertDesignFile(db, designId, filePath, content); const design = getDesign(db, designId); if (design === null || design.workspacePath === null) return; const destinationPath = path_module.join(design.workspacePath, persisted.path); mkdirSync(path_module.dirname(destinationPath), { recursive: true }); writeFileSync(destinationPath, content, 'utf8'); }
-
[Major] Workspace action failures are not surfaced to users in Files panel (console-only) — catches only call
console.error, which violates the “no silent fallbacks” constraint because users receive no UI error/toast for failed pick/open/clear operations, evidenceapps/desktop/src/renderer/src/components/FilesPanel.tsx:69.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : t('errors.unknown'); useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: msg, }); }
Summary
- Review mode: follow-up after new commits
- 2 issues found (1 Blocker, 1 Major), both introduced in workspace sync/error-handling paths.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Missing coverage: no renderer test asserting workspace operation failures surface a toast/error state (instead of console-only handling).
open-codesign Bot
| upsertDesignFile(db, designId, filePath, content); | ||
| const design = getDesign(db, designId); | ||
| if (design === null || design.workspacePath === null) return; | ||
| const destinationPath = path_module.join(design.workspacePath, filePath); |
There was a problem hiding this comment.
[Blocker] is written to disk without normalization, while DB uses normalized path via . On POSIX this can create a different file than the tracked DB path (e.g. backslashes), causing divergence. Suggested fix:
| useCodesignStore.setState({ designs: updated }); | ||
| } | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
[Major] This failure path is console-only and does not surface an error to users. That creates a silent fallback for workspace operations. Suggested fix:
There was a problem hiding this comment.
Findings
-
[Blocker] Workspace write-through uses a non-normalized path, which can diverge from DB-tracked file paths on POSIX (e.g.
nested\index.htmlpersisted asnested/index.htmlin DB but written as a different on-disk name). This can break migration/collision checks and produce data inconsistency, evidenceapps/desktop/src/main/index.ts:258.
Suggested fix:const persisted = upsertDesignFile(db, designId, filePath, content); const destinationPath = path_module.join(design.workspacePath, persisted.path); mkdirSync(path_module.dirname(destinationPath), { recursive: true }); writeFileSync(destinationPath, content, 'utf8');
-
[Major] Workspace operation failures in Files panel are console-only, so users get no UI-visible error on failed pick/open/clear actions (violates “no silent fallbacks”), evidence
apps/desktop/src/renderer/src/components/FilesPanel.tsx:70.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : t('errors.unknown'); useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: msg, }); }
-
[Major] Same-workspace detection in Files panel compares raw strings, so equivalent paths with separator/trailing-slash differences can incorrectly trigger rebind flow, evidence
apps/desktop/src/renderer/src/components/FilesPanel.tsx:61.
Suggested fix:const normalizePath = (p: string) => p.replaceAll('\\', '/').replace(//+$/, ''); if (currentDesign.workspacePath && normalizePath(currentDesign.workspacePath) !== normalizePath(path)) { requestWorkspaceRebind(currentDesign, path); }
Summary
- Review mode: follow-up after new commits
- 3 issues found (1 Blocker, 2 Major).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Missing coverage: renderer tests that assert workspace operation errors surface user-visible toasts in
FilesPanelpaths.
open-codesign Bot
| return { path }; | ||
| }, | ||
| strReplace(path: string, oldStr: string, newStr: string) { | ||
| const current = fsMap.get(path); |
There was a problem hiding this comment.
upsertDesignFile normalizes path separators before DB persistence, but disk write still uses raw filePath. On POSIX this can create a different filename than DB-tracked path (e.g. backslash in name). Please write through using the normalized persisted path.
const persisted = upsertDesignFile(db, designId, filePath, content);
const destinationPath = path_module.join(design.workspacePath, persisted.path);| } | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to pick workspace:', err); |
There was a problem hiding this comment.
This failure path is console-only, so users get no visible error when workspace operations fail. Please surface via toast (same for the other workspace handlers in this component) to avoid silent fallback behavior.
const msg = err instanceof Error ? err.message : t('errors.unknown');
useCodesignStore.getState().pushToast({
variant: 'error',
title: t('canvas.workspace.updateFailed'),
description: msg,
});| setWorkspaceLoading(true); | ||
| const path = await window.codesign.snapshots.pickWorkspaceFolder(); | ||
| if (path && currentDesign && currentDesignId) { | ||
| if (currentDesign.workspacePath && currentDesign.workspacePath !== path) { |
There was a problem hiding this comment.
Raw string compare can treat equivalent paths as different (separator/trailing slash differences), causing unnecessary rebind prompts. Normalize both sides before comparing.
const normalizePath = (p: string) => p.replaceAll('\\', '/').replace(//+$/, '');
if (currentDesign.workspacePath && normalizePath(currentDesign.workspacePath) !== normalizePath(path)) {
requestWorkspaceRebind(currentDesign, path);
}- Use normalized persisted path from upsertDesignFile for workspace write-through so DB path and on-disk filename always match - Surface workspace errors as error toasts in FilesPanel instead of silent console.error (pick, open, clear handlers) - Normalize workspace path before comparison in FilesPanel to avoid false rebind prompts on trailing-slash or separator differences Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
There was a problem hiding this comment.
Findings
-
[Major] Workspace availability check errors are silently swallowed, so users get no feedback when the check fails and the UI quietly degrades to an unknown state. This violates the “no silent fallbacks” constraint, evidence
apps/desktop/src/renderer/src/components/FilesPanel.tsx:45,apps/desktop/src/renderer/src/components/FilesTabView.tsx:38.
Suggested fix:.catch((err) => { setFolderExists(null); useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: err instanceof Error ? err.message : t('errors.unknown'), }); });
-
[Major]
handleOpenWorkspacein the files tab firesopenWorkspaceFolderwithout awaiting/catching, so failures are not surfaced in UI and can turn into unhandled rejections. This is another silent-failure path, evidenceapps/desktop/src/renderer/src/components/FilesTabView.tsx:78.
Suggested fix:async function handleOpenWorkspace() { if (!currentDesignId || !window.codesign?.snapshots.openWorkspaceFolder) return; try { await window.codesign.snapshots.openWorkspaceFolder(currentDesignId); } catch (err) { useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: err instanceof Error ? err.message : t('errors.unknown'), }); } }
-
[Minor] New UI code introduces hardcoded pixel values and a hardcoded white background in changed lines (
text-[10px],text-[11px],h-[16px],bg-white), which conflicts with the token-only UI constraint, evidenceapps/desktop/src/renderer/src/components/FilesPanel.tsx:153,apps/desktop/src/renderer/src/components/FilesPanel.tsx:228,apps/desktop/src/renderer/src/components/FilesTabView.tsx:83,apps/desktop/src/renderer/src/components/FilesTabView.tsx:276.
Suggested fix:// example replacements using existing tokenized classes className="text-[var(--text-xs)] ..." className="... bg-[var(--color-background-secondary)] ..."
Summary
- Review mode: follow-up after new commits
- 3 issues found (2 Major, 1 Minor).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Missing coverage: renderer tests for workspace-check/open failure paths in
FilesPanel/FilesTabViewto assert user-visible error toasts.
open-codesign Bot
| window.codesign?.snapshots | ||
| .checkWorkspaceFolder?.(currentDesignId) | ||
| .then((r) => setFolderExists(r.exists)) | ||
| .catch(() => setFolderExists(null)); |
There was a problem hiding this comment.
This .catch(() => setFolderExists(null)) swallows the failure with no user-visible feedback. Please surface a toast (or throw) so workspace-check errors are not silent.
| .pushToast({ variant: 'info', title: t('canvas.workspace.busyGenerating') }); | ||
| return; | ||
| } | ||
| void window.codesign.snapshots.openWorkspaceFolder(currentDesignId); |
There was a problem hiding this comment.
void window.codesign.snapshots.openWorkspaceFolder(...) drops rejections; open failures won’t surface in UI. Please await inside try/catch and show an error toast.
|
|
||
| return ( | ||
| <div className="flex items-center gap-[var(--space-2)] px-[var(--space-4)] py-[var(--space-2)] border-b border-[var(--color-border-muted)] min-w-0"> | ||
| <span className="text-[10px] uppercase tracking-[var(--tracking-label)] text-[var(--color-text-muted)] font-medium shrink-0"> |
There was a problem hiding this comment.
This introduces hardcoded pixel typography (text-[10px]) in a new UI path. Please use shared typography tokens (e.g., var(--text-*)) to keep token-only styling.
There was a problem hiding this comment.
There appears to be a bug with the shared typography tokens currently in use, so we won't consider it for now; we'll submit a single pull request for it later.
There was a problem hiding this comment.
Findings
- [Major] Partial-success create is reported as failure when workspace binding fails —
createNewDesignnow creates and selects a design first, then attemptsupdateWorkspace. IfupdateWorkspacefails (for example,IPC_CONFLICTbecause folder is already bound), the broadcatchreturnsnulland showscreateFailed, but the design was already created and switched to. This produces user-visible inconsistency and unexpected extra designs, evidenceapps/desktop/src/renderer/src/store.ts:1947,apps/desktop/src/renderer/src/store.ts:1970,apps/desktop/src/renderer/src/store.ts:1974.
Suggested fix:const design = await window.codesign.snapshots.createDesign(name); // ...existing state setup... if (workspacePath) { try { await window.codesign.snapshots.updateWorkspace(design.id, workspacePath, false); await get().loadDesigns(); } catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('canvas.workspace.updateFailed'), description: msg, }); } } return design;
Summary
- Review mode: follow-up after new commits
- 1 issue found (Major).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Missing coverage: add a store test for
createNewDesign(workspacePath)wherecreateDesignsucceeds andupdateWorkspacefails; assert the design still exists/is selected and the error toast iscanvas.workspace.updateFailed.
open-codesign Bot
There was a problem hiding this comment.
Findings
-
[Major] Main-process sync disk writes on every text-editor mutation can stall generation UI — workspace write-through now performs
mkdirSync+writeFileSyncinside the generation event path, so large/slow disk operations block Electron main and can delay IPC/event delivery, evidenceapps/desktop/src/main/index.ts:264,apps/desktop/src/main/index.ts:265.
Suggested fix:import { mkdir, writeFile } from 'node:fs/promises'; async function persistMutation(filePath: string, content: string): Promise<void> { // ...existing guards... const destinationPath = path_module.join(design.workspacePath, persisted.path); try { await mkdir(path_module.dirname(destinationPath), { recursive: true }); await writeFile(destinationPath, content, 'utf8'); } catch (err) { const message = err instanceof Error ? err.message : String(err); logger.error('runtime.fs.writeThrough.fail', { designId, filePath, workspacePath: design.workspacePath, message }); throw new Error(`Workspace write-through failed for ${filePath}: ${message}`); } }
-
[Major] Workspace pick path still has an unhandled rejection branch (no UI error surfaced) — when binding a workspace for a design without an existing workspace,
updateWorkspace/listDesignsfailures inhandlePickWorkspaceare not caught, so the user gets no toast and the failure is effectively silent in UI, evidenceapps/desktop/src/renderer/src/components/FilesTabView.tsx:67,apps/desktop/src/renderer/src/components/FilesTabView.tsx:68.
Suggested fix:} else if (!currentDesign.workspacePath) { try { await window.codesign.snapshots.updateWorkspace(currentDesignId, path, false); const updated = await window.codesign.snapshots.listDesigns(); useCodesignStore.setState({ designs: updated }); } catch (err) { useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: err instanceof Error ? err.message : t('errors.unknown'), }); } }
Summary
- Review mode: follow-up after new commits
- 2 issues found (Major, Major).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Suggested coverage: add a renderer test for
FilesTabViewwheresnapshots.updateWorkspacerejects after folder selection and assertcanvas.workspace.updateFailedtoast is shown.
open-codesign Bot
| if (current.indexOf(oldStr, idx + oldStr.length) !== -1) { | ||
| throw new Error(`old_str is ambiguous in ${path}; provide more context`); | ||
| } | ||
| const next = current.slice(0, idx) + newStr + current.slice(idx + oldStr.length); |
There was a problem hiding this comment.
Main-process writeFileSync in the runtime text-editor mutation path can block Electron event processing under slow disks/large writes. Please switch this write-through to async fs APIs and await it so errors still surface without blocking the main thread.
| ) { | ||
| requestWorkspaceRebind(currentDesign, path); | ||
| } else if (!currentDesign.workspacePath) { | ||
| await window.codesign.snapshots.updateWorkspace(currentDesignId, path, false); |
There was a problem hiding this comment.
This branch still lacks a local try/catch. If updateWorkspace (or subsequent listDesigns) fails, users don’t get the workspace error toast and only see an unhandled rejection. Please mirror the explicit toast-based error handling used elsewhere in this flow.
|
Hi @MoveCloudROY — thanks for the patient iteration on this PR. The review findings have all been addressed and the code quality is solid. Main has moved on since your last push (#193 image generation, #194 provider reasoning fix, #196 coding-plan allowlist warning, plus README updates), so this branch now shows as Suggested flow: git fetch origin main
git checkout feat/user-choose-workspace
git rebase origin/main
# resolve any conflicts (likely just in apps/desktop/src/main/index.ts)
pnpm install
pnpm lint && pnpm test
git push --force-with-leaseOne update on shipping plans: we're targeting v0.2.0 for this feature rather than the upcoming v0.1.4 patch. The workspace-binding work you've done here is the natural foundation for the v0.2 agentic design loop (project model, path sandbox, agent read/write on workspace), so it makes more sense to ship it together as a cohesive story than to land it mid-v0.1 and iterate the shape. We'll still happily merge the rebased PR once it's clean so the code lives on main — just held from If you hit any trouble with the conflicts, drop a comment here and we can pair on it. |
…panel workspace section
- Created FilesPanel.test.tsx with comprehensive workspace state and IPC handler tests - Tests cover: state retrieval, action handlers, action flows, error handling, state consistency - All 784 tests pass (19 new tests added) - Verifies workspace binding, clearing, changing, and cancellation flows - Tests mock window.codesign API and store state management
- Add requestWorkspaceRebind, cancelWorkspaceRebind, confirmWorkspaceRebind actions to store - Implement generation-blocking guard: prevent workspace changes while current design is generating - Add RebindWorkspaceDialog component with three outcomes (cancel, switch-only, switch+copy) - Add workspaceRebindPending state to track pending rebind operations - Add i18n strings for workspace rebind UI (en + zh-CN) - Add comprehensive tests for rebind flow and generation-blocking guards - Fix FilesPanel.test.tsx Design type contract (remove prompt field, add schemaVersion) - All 784+ tests passing, typecheck clean
…der state - Wrap dialog.showOpenDialog in try/catch → CodesignError in workspace:pick handler - Wrap checkWorkspaceFolderExists in try/catch → CodesignError in workspace:check handler - Add logger.warn to initSnapshotsDb close-failure catch (was empty) - FilesPanel workspace section always visible; shows unavailable warning when folder missing - Add checkWorkspaceFolder preload method and snapshots:v1:workspace:check IPC handler - Add canvas.workspace.unavailable i18n key (en + zh-CN)
- Add NewDesignDialog component that prompts users to optionally select a workspace folder when creating a new design - Add newDesignDialogOpen state + openNewDesignDialog/closeNewDesignDialog actions to store; update createNewDesign to accept optional workspacePath - Remove the erroneous auto-pickWorkspaceFolder call from createNewDesign - Wire all three new-design entry points (RecentTab, DesignSwitcher, DesignsView) to openNewDesignDialog instead of calling createNewDesign directly - Mount NewDesignDialog in App.tsx alongside RebindWorkspaceDialog - Refactor FilesTabView WorkspaceSection to single-row layout; remove Clear button; fix openWorkspaceFolder fire-and-forget (no longer locks buttons) - Add canvas.newDesignDialog i18n keys in en.json and zh-CN.json
- Surface write-through failure in persistMutation instead of silently swallowing the error, so callers can propagate it to the UI - Classify workspace migration collision and missing tracked-file errors as IPC_BAD_INPUT in snapshots-ipc for actionable client-side messages - Normalize workspace paths before comparison in FilesTabView to avoid false rebind prompts caused by trailing slashes or path-separator diffs Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
…ature Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
- Use normalized persisted path from upsertDesignFile for workspace write-through so DB path and on-disk filename always match - Surface workspace errors as error toasts in FilesPanel instead of silent console.error (pick, open, clear handlers) - Normalize workspace path before comparison in FilesPanel to avoid false rebind prompts on trailing-slash or separator differences Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
… hardcoded px values with tokens - Add toast in .catch() for checkWorkspaceFolder in FilesPanel and FilesTabView so availability check failures are visible to users - Convert handleOpenWorkspace in FilesTabView to async with try/catch toast instead of fire-and-forget void - Replace hardcoded text-[10px]/text-[11px]/h-[16px]/bg-white with token equivalents (text-[var(--text-xs)], h-[var(--space-4)], bg-[var(--color-background-secondary)]) in changed files - Add t to useEffect dependency arrays (lint: useExhaustiveDependencies) Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
…ure in createNewDesign If updateWorkspace fails after createDesign succeeds, the design is already created and selected. Wrapping both in a single catch caused the broad handler to report createFailed and return null, leaving the UI with an orphaned design and no indication the design itself was created. Split workspace binding into its own try/catch: updateWorkspace errors now show canvas.workspace.updateFailed toast while the created design is still returned to the caller. Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
…rived values Replace the blanket --text-xs (12px) substitution with semantically correct tokens that preserve the original visual sizes: - text-[10px] → text-[var(--text-2xs)] (10px, defined in ui tokens) - text-[11px] → text-[var(--font-size-body-xs)] (11px, body minimum) h-[var(--space-4)] (16px) is kept as-is since it is already a token. This satisfies the no-hardcoded-px constraint without changing appearance. Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
…ate tests and core interface - Make persistMutation async (mkdir/writeFile from node:fs/promises) - Make fs.create/strReplace/insert async to propagate await chain - Wrap updateWorkspace+listDesigns in try/catch with pushToast in FilesTabView - Update all 6 workspace tests to async/await - Widen TextEditorFsCallbacks interface to accept Promise returns - Add await to fs.create/strReplace/insert call sites in makeTextEditorTool Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
…h rule Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
54581b8 to
8fc8540
Compare
There was a problem hiding this comment.
Findings
-
[Major] Workspace conflict detection is still case-sensitive on Windows, so the same folder can be rebound as
C:/Workandc:/Workand bypass the per-design uniqueness check. That lets two designs write into one workspace and directly risks file overwrite/data mixing, evidenceapps/desktop/src/main/design-workspace.ts:24,apps/desktop/src/main/design-workspace.ts:45.
Suggested fix:function workspacePathKey(p: string): string { const normalized = stripTrailingSlash(path.resolve(p).replaceAll('\\', '/')); return process.platform === 'win32' ? normalized.toLowerCase() : normalized; } export function normalizeWorkspacePath(p: string): string { return workspacePathKey(p); }
-
[Major] Write-through still advances SQLite before the workspace file write completes. If
writeFile()fails, the DB row and live preview keep the new content while the bound folder stays stale, so the feature no longer has a single consistent source of truth after an error, evidenceapps/desktop/src/main/index.ts:316,apps/desktop/src/main/index.ts:321.
Suggested fix:async function persistMutation(filePath: string, content: string): Promise<void> { if (designId === null || db === null) return; const normalizedPath = normalizeDesignFilePath(filePath); const design = getDesign(db, designId); if (design?.workspacePath) { const destinationPath = path_module.join(design.workspacePath, normalizedPath); await mkdir(path_module.dirname(destinationPath), { recursive: true }); await writeFile(destinationPath, content, 'utf8'); } upsertDesignFile(db, designId, normalizedPath, content); }
Summary
- Review mode: follow-up after new commits
- 2 issues found (Major, Major).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Suggested tests: add a Windows-path normalization case proving
C:/Workconflicts withc:/Work, and a write-through failure case asserting the DB row is not advanced when workspace sync rejects.
open-codesign Bot
| return value.replace(/\/+$/, ''); | ||
| } | ||
|
|
||
| export function normalizeWorkspacePath(p: string): string { |
There was a problem hiding this comment.
normalizeWorkspacePath() normalizes slashes and trailing separators, but it still leaves Windows path casing distinct. path.win32.resolve('C:/Work') and path.win32.resolve('c:/Work') stay different strings, so two designs can bind the same folder and then overwrite each other’s mirrored files. Please compare/store a case-folded key on Windows, e.g.
function workspacePathKey(p: string): string {
const normalized = stripTrailingSlash(path.resolve(p).replaceAll('\\', '/'));
return process.platform === 'win32' ? normalized.toLowerCase() : normalized;
}|
|
||
| async function persistMutation(filePath: string, content: string): Promise<void> { | ||
| if (designId === null || db === null) return; | ||
| const persisted = upsertDesignFile(db, designId, filePath, content); |
There was a problem hiding this comment.
upsertDesignFile() runs before the workspace writeFile(). If the disk write fails, the DB and preview advance but the bound folder stays stale, so the write-through contract is broken after the error. Please avoid committing the DB row until the workspace write succeeds (or explicitly roll it back), e.g.
const normalizedPath = normalizeDesignFilePath(filePath);
const design = getDesign(db, designId);
if (design?.workspacePath) {
const destinationPath = path_module.join(design.workspacePath, normalizedPath);
await mkdir(path_module.dirname(destinationPath), { recursive: true });
await writeFile(destinationPath, content, 'utf8');
}
upsertDesignFile(db, designId, normalizedPath, content);Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
There was a problem hiding this comment.
Findings
-
[Major] Failed workspace writes still push unsaved HTML into the live preview —
fsMapandfs_updatedare updated beforepersistMutation()runs, so ifwriteFile()rejects the renderer keeps showing content that was never written to disk or SQLite. That leaves the user looking at a ghost state after an error, evidenceapps/desktop/src/main/index.ts:346,apps/desktop/src/main/index.ts:361,apps/desktop/src/main/index.ts:373.
Suggested fix:async create(path: string, content: string) { await persistMutation(path, content); fsMap.set(path, content); emitFsUpdated(path, content); emitIndexIfAssetChanged(path); return { path }; }
Apply the same ordering or explicit rollback to
strReplace()andinsert(). -
[Minor] Same-folder picks on Windows still trigger the rebind dialog in the renderer — the UI-side comparison only normalizes separators and trailing slashes, so
C:/Workandc:/Workare treated as different folders even though the main process now treats them as the same path. That produces a false rebind prompt in both picker entry points, evidenceapps/desktop/src/renderer/src/components/FilesPanel.tsx:68,apps/desktop/src/renderer/src/components/FilesTabView.tsx:60.
Suggested fix:const workspaceKey = (p: string) => { const normalized = p.replaceAll('\\', '/').replace(/\/+$/, ''); return navigator.userAgent.includes('Windows') ? normalized.toLowerCase() : normalized; }; if ( currentDesign.workspacePath && workspaceKey(currentDesign.workspacePath) !== workspaceKey(path) ) { requestWorkspaceRebind(currentDesign, path); }
Or better, expose the main-process comparison helper through preload so the two codepaths cannot drift again.
Summary
- Review mode: follow-up after new commits
- 2 issues found (Major, Minor).
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.- DCO: several earlier commits in this PR history still lack
Signed-off-bylines, so the repo's DCO requirement is not satisfied yet.
Testing
- Not run (automation)
- Suggested tests: add a write-through failure case that asserts no
fs_updated/preview advance on failure, and Windows casing tests for bothFilesPanelandFilesTabView.
open-codesign Bot
| }, | ||
| async create(path: string, content: string) { | ||
| fsMap.set(path, content); | ||
| emitFsUpdated(path, content); |
There was a problem hiding this comment.
persistMutation() now protects SQLite vs. disk, but create() / strReplace() / insert() still mutate fsMap and emit fs_updated before that await. If the workspace write rejects, the run errors out while the renderer keeps showing unsaved HTML. Please either persist first or roll the in-memory change back on failure, e.g.
await persistMutation(path, content);
fsMap.set(path, content);
emitFsUpdated(path, content);
emitIndexIfAssetChanged(path);| setWorkspaceLoading(true); | ||
| const path = await window.codesign.snapshots.pickWorkspaceFolder(); | ||
| if (path && currentDesign && currentDesignId) { | ||
| const normalizePath = (p: string) => p.replaceAll('\\', '/').replace(/\/+$/, ''); |
There was a problem hiding this comment.
This comparison still treats C:/Work and c:/Work as different folders on Windows, so the UI can open a false rebind dialog even though bindWorkspace() now no-ops that case. Please share the same comparison key here (and in FilesTabView) so renderer and main stay in sync, e.g.
const workspaceKey = (p: string) => {
const normalized = p.replaceAll('\\', '/').replace(/\/+$/, '');
return navigator.userAgent.includes('Windows') ? normalized.toLowerCase() : normalized;
};Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: roy1994 <dev.mancitrus@outlook.com>
There was a problem hiding this comment.
Findings
-
[Major] FilesTabView still drops folder-picker failures without surfacing them —
handlePickWorkspace()only usestry/finally, so a rejectedpickWorkspaceFolder()exits the click handler with no toast even though this PR hardened the same failure mode elsewhere. That violates the repo’s “no silent fallbacks” rule, evidenceapps/desktop/src/renderer/src/components/FilesTabView.tsx:57,apps/desktop/src/renderer/src/components/FilesTabView.tsx:59,apps/desktop/src/renderer/src/components/FilesTabView.tsx:81.
Suggested fix:try { setPicking(true); const path = await window.codesign.snapshots.pickWorkspaceFolder(); // existing logic } catch (err) { useCodesignStore.getState().pushToast({ variant: 'error', title: t('canvas.workspace.updateFailed'), description: err instanceof Error ? err.message : t('errors.unknown'), }); } finally { setPicking(false); }
-
[Major] The follow-up commits reintroduce hardcoded pixel values in the files UI — changed lines now use
text-[10px],text-[11px],h-[16px], andpx-[5px]instead of shared tokens, which conflicts with the repo’s “every UI value via packages/ui tokens” constraint, evidenceapps/desktop/src/renderer/src/components/FilesPanel.tsx:161,apps/desktop/src/renderer/src/components/FilesPanel.tsx:236,apps/desktop/src/renderer/src/components/FilesTabView.tsx:107,apps/desktop/src/renderer/src/components/FilesTabView.tsx:222.
Suggested fix:<h2 className="text-[var(--font-size-body-xs)] uppercase tracking-[var(--tracking-label)] text-[var(--color-text-muted)] font-medium m-0"> {t('canvas.files.sectionTitle')} </h2> <span className="inline-flex items-center justify-center h-[var(--space-4)] px-[var(--space-1_5)] rounded-[var(--radius-sm)] bg-[var(--color-background-secondary)] text-[var(--text-2xs)] text-[var(--color-text-muted)]"> {files.length} </span>
Apply the same token substitution to the other
10px/11pxliterals in both files.
Summary
- Review mode: follow-up after new commits
- 2 Major issues found.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.CLAUDE.mdsaysdocs/is gitignored in public checkouts.- DCO is still not satisfied: earlier commits
f84629035479,ab3fdb9d8c1c,379fa68707f2,e6802f64e01e,4e7d16bd3025, and4a54725452a6are missingSigned-off-bylines.
Testing
- Not run (automation)
- Suggested tests: add a
FilesTabViewinteraction test that makespickWorkspaceFolder()reject and asserts the error toast.
open-codesign Bot
| } | ||
| try { | ||
| setPicking(true); | ||
| const path = await window.codesign.snapshots.pickWorkspaceFolder(); |
There was a problem hiding this comment.
pickWorkspaceFolder() can reject here, but this handler only has try/finally, so the error never reaches a toast. That leaves one remaining silent workspace failure path in this PR. Please add the same error surface used in FilesPanel, e.g. wrap the existing body in catch (err) { pushToast(...) }.
| {t('canvas.files.sectionTitle')} | ||
| </h2> | ||
| <span | ||
| className="inline-flex items-center justify-center min-w-[18px] h-[16px] px-[5px] rounded-[var(--radius-sm)] bg-[var(--color-background-secondary)] text-[10px] text-[var(--color-text-muted)]" |
There was a problem hiding this comment.
This reintroduces hardcoded pixel values into the changed files (text-[10px], text-[11px], h-[16px], px-[5px] here and the matching FilesTabView lines), which conflicts with the repo’s UI-token constraint. Please switch these to tokenized values such as text-[var(--text-2xs)], text-[var(--font-size-body-xs)], h-[var(--space-4)], and px-[var(--space-1_5)].
Summary
NewDesignDialogmodal that appears whenever the user clicks "New Design" (Recent tab, Design Switcher, Designs view)Type of change
Linked issue
close #133
Checklist
docs/VISION.md,docs/PRINCIPLES.md, andCLAUDE.mdbefore startinggit commit -s)pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visibleChanges
store.ts— newnewDesignDialogOpenstate + open/close actions;createNewDesignnow accepts optionalworkspacePathNewDesignDialog.tsx— new modal component (created)App.tsx— mounts<NewDesignDialog />RecentTab.tsx,DesignSwitcher.tsx,DesignsView.tsx— callopenNewDesignDialog()instead ofcreateNewDesigndirectlyFilesTabView.tsx— WorkspaceSection single-row layout, fire-and-forget bug fixeden.json/zh-CN.json— newcanvas.newDesignDialog.*i18n keyssnapshots-db.ts— pre-existinggetLogger()missing-scope fixFilesPanel.test.tsx— pre-existing lint/type errors fixedScreenshots / recordings (UI changes)