Conversation
f4c29a3 to
f532943
Compare
There was a problem hiding this comment.
Findings
-
[Blocker]
DesignMessageschema rejects persistedsystemmessages — this creates an internal contract break: DB and IPC both allowsystem, but shared schema narrows touser|assistant, which can cause invalid typed data and downstream regressions. Evidencepackages/shared/src/snapshot.ts:31,apps/desktop/src/main/snapshots-db.ts:65,apps/desktop/src/main/snapshots-db.ts:377,apps/desktop/src/main/snapshots-ipc.ts:329
Suggested fix:export const DesignMessageV1 = z.object({ schemaVersion: z.literal(1).default(1), designId: z.string().min(1), role: z.enum(['user', 'assistant', 'system']), content: z.string(), ordinal: z.number().int().nonnegative(), createdAt: z.string(), });
-
[Major] Silent persistence failure in
persistDesignStateviolates the project hard constraint (no silent fallbacks) and risks unnoticed data loss in design/message history. Evidenceapps/desktop/src/renderer/src/store.ts:228
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.saveFailed'), description: msg, }); set({ lastError: msg, errorMessage: msg }); }
-
[Major]
loadDesignsswallows IPC/DB errors and marksdesignsLoaded=truewithout surfacing failure, which is another silent fallback and can hide broken project loading state. Evidenceapps/desktop/src/renderer/src/store.ts:737
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ designsLoaded: true, errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('projects.notifications.loadFailed'), description: msg, }); }
Summary
- Review mode: initial
- 3 issues found (1 blocker, 2 major), all introduced in modified lines.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md(review usedCLAUDE.md+ diffed files).
Testing
- Not run (automation)
open-codesign Bot
| export const DesignMessageV1 = z.object({ | ||
| schemaVersion: z.literal(1).default(1), | ||
| designId: z.string().min(1), | ||
| role: z.enum(['user', 'assistant']), |
There was a problem hiding this comment.
role is narrower than the producer/DB contract in this PR (system is accepted and persisted). This introduces a schema/type mismatch. Please include system in the enum to keep shared types aligned with IPC + DB.
| await window.codesign.snapshots.setThumbnail(designId, thumbText); | ||
| } | ||
| await get().loadDesigns(); | ||
| } catch (err) { |
There was a problem hiding this comment.
This catch currently logs only, which is a silent fallback for persistence failures. Project constraints require surfacing every error in UI (or throwing with context). Please surface this via toast + lastError/errorMessage.
| try { | ||
| const designs = await window.codesign.snapshots.listDesigns(); | ||
| set({ designs, designsLoaded: true }); | ||
| } catch (err) { |
There was a problem hiding this comment.
loadDesigns failure is swallowed and state is marked loaded, leaving users without error visibility. Please surface this error (toast and/or state error) instead of console-only logging.
… PR #52) DesignMessageV1 zod schema previously narrowed role to user|assistant, but DB CHECK constraint and IPC validator both accept 'system'. This created an internal contract break: persisted system messages would fail schema validation downstream. Extend DesignMessageV1.role to include 'system' to match ChatMessage and the DB/IPC contract. Additive change — existing rows remain valid (no schemaVersion bump needed). - Add vitest coverage for persist+load of role='system' - Apply biome auto-format fixes pulled in by --fix
f532943 to
d532398
Compare
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback on persistence failure can hide data-loss conditions —
persistDesignStateonly logs to console and does not surface any UI/throw context, which violates the no-silent-fallback constraint and leaves users unaware that messages/thumbnail were not saved. Evidenceapps/desktop/src/renderer/src/store.ts:228
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.saveFailed'), description: msg, }); set({ errorMessage: msg, lastError: msg }); throw new Error(`persistDesignState failed: ${msg}`); }
-
[Major]
loadDesignsfailure is swallowed while marking data as loaded — settingdesignsLoaded=truein the catch branch without surfacing an error can render an empty/stale project list with no user-visible failure signal. Evidenceapps/desktop/src/renderer/src/store.ts:737
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ designsLoaded: true, errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('projects.notifications.loadFailed'), description: msg, }); throw new Error(`loadDesigns failed: ${msg}`); }
-
[Major] Hardcoded UI color token bypass (
text-white) violates the token-only UI constraint — this introduces a raw color instead of a design token-backed value. Evidenceapps/desktop/src/renderer/src/components/DeleteDesignDialog.tsx:47
Suggested fix:className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-text-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"
Summary
- Review mode: follow-up after new commits
- 3 issues found (Major), all on added/modified lines in this PR diff.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md(repo indicatesdocs/is gitignored in public clone).
Testing
- Not run (automation)
open-codesign Bot
| await window.codesign.snapshots.setThumbnail(designId, thumbText); | ||
| } | ||
| await get().loadDesigns(); | ||
| } catch (err) { |
There was a problem hiding this comment.
This catch introduces a silent fallback (console-only). Please surface this in UI and keep context by throwing.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
get().pushToast({
variant: 'error',
title: tr('projects.notifications.saveFailed'),
description: msg,
});
set({ errorMessage: msg, lastError: msg });
throw new Error(`persistDesignState failed: ${msg}`);
}| try { | ||
| const designs = await window.codesign.snapshots.listDesigns(); | ||
| set({ designs, designsLoaded: true }); | ||
| } catch (err) { |
There was a problem hiding this comment.
loadDesigns currently swallows IPC/DB errors and marks data as loaded; this hides failure state from users. Please surface + throw with context.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
set({ designsLoaded: true, errorMessage: msg, lastError: msg });
get().pushToast({
variant: 'error',
title: tr('projects.notifications.loadFailed'),
description: msg,
});
throw new Error(`loadDesigns failed: ${msg}`);
}| <button | ||
| type="button" | ||
| onClick={() => void softDeleteDesign(target.id)} | ||
| className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-white text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity" |
There was a problem hiding this comment.
This adds a hardcoded color (text-white) instead of a token-backed value. Please use a semantic UI token class.
className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-text-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"… PR #52) DesignMessageV1 zod schema previously narrowed role to user|assistant, but DB CHECK constraint and IPC validator both accept 'system'. This created an internal contract break: persisted system messages would fail schema validation downstream. Extend DesignMessageV1.role to include 'system' to match ChatMessage and the DB/IPC contract. Additive change — existing rows remain valid (no schemaVersion bump needed). - Add vitest coverage for persist+load of role='system' - Apply biome auto-format fixes pulled in by --fix Signed-off-by: hqhq1025 <1506751656@qq.com>
d532398 to
8223f2f
Compare
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback hides persistence failures and violates the "no silent fallbacks" constraint —
persistDesignStatecatches DB/IPC errors and only logs to console, so users can continue without any signal that messages/thumbnail were not saved. Evidenceapps/desktop/src/renderer/src/store.ts:228.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.saveFailed'), description: msg, }); set({ errorMessage: msg, lastError: msg }); throw new Error(`persistDesignState failed: ${msg}`); }
-
[Major] Silent fallback on designs load can leave stale/empty UI state —
loadDesignssetsdesignsLoaded=trueeven when the fetch fails and surfaces no user-visible error, which also violates the no-silent-fallback rule. Evidenceapps/desktop/src/renderer/src/store.ts:737.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ designsLoaded: true, errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('projects.notifications.loadFailed'), description: msg, }); throw new Error(`loadDesigns failed: ${msg}`); }
-
[Major] Hardcoded color token bypass in UI —
text-whiteintroduces a raw color instead of a token-backed semantic value, conflicting with the UI token constraint. Evidenceapps/desktop/src/renderer/src/components/DeleteDesignDialog.tsx:47.
Suggested fix:className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"
Summary
- Review mode: follow-up after new commits
- 3 Major issues found on added/modified lines.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| await window.codesign.snapshots.setThumbnail(designId, thumbText); | ||
| } | ||
| await get().loadDesigns(); | ||
| } catch (err) { |
There was a problem hiding this comment.
This catch is a silent fallback (console only). Per project constraints, surface to UI and throw with context so save failures are explicit.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
get().pushToast({
variant: 'error',
title: tr('projects.notifications.saveFailed'),
description: msg,
});
set({ errorMessage: msg, lastError: msg });
throw new Error(`persistDesignState failed: ${msg}`);
}| try { | ||
| const designs = await window.codesign.snapshots.listDesigns(); | ||
| set({ designs, designsLoaded: true }); | ||
| } catch (err) { |
There was a problem hiding this comment.
loadDesigns currently swallows failure and still marks as loaded. Please surface a user-visible error and throw so callers can react.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
set({ designsLoaded: true, errorMessage: msg, lastError: msg });
get().pushToast({
variant: 'error',
title: tr('projects.notifications.loadFailed'),
description: msg,
});
throw new Error(`loadDesigns failed: ${msg}`);
}| <button | ||
| type="button" | ||
| onClick={() => void softDeleteDesign(target.id)} | ||
| className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-white text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity" |
There was a problem hiding this comment.
This adds a hardcoded color (text-white) instead of a token-backed semantic color. Please switch to the design token class.
className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"… PR #52) DesignMessageV1 zod schema previously narrowed role to user|assistant, but DB CHECK constraint and IPC validator both accept 'system'. This created an internal contract break: persisted system messages would fail schema validation downstream. Extend DesignMessageV1.role to include 'system' to match ChatMessage and the DB/IPC contract. Additive change — existing rows remain valid (no schemaVersion bump needed). - Add vitest coverage for persist+load of role='system' - Apply biome auto-format fixes pulled in by --fix Signed-off-by: hqhq1025 <1506751656@qq.com>
8223f2f to
ca77b2b
Compare
There was a problem hiding this comment.
Findings
-
[Major] New
snapshots:v1:*project-management payloads are not schema-versioned end-to-end, breaking the project’s upgradeability contract and making protocol evolution brittle. Evidenceapps/desktop/src/preload/index.ts:200,apps/desktop/src/main/snapshots-ipc.ts:267.
Suggested fix:// preload ipcRenderer.invoke('snapshots:v1:rename-design', { schemaVersion: 1, id, name }); // main const r = raw as Record<string, unknown>; requireSchemaV1(r, 'snapshots:v1:rename-design');
-
[Major]
persistDesignStatestill swallows persistence failures with console-only logging, which is a silent fallback and can hide message/thumbnail data loss from users. Evidenceapps/desktop/src/renderer/src/store.ts:301.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.saveFailed'), description: msg, }); set({ errorMessage: msg, lastError: msg }); throw new Error(`persistDesignState failed: ${msg}`); }
-
[Major]
loadDesignsstill marksdesignsLoaded=trueafter failure without surfacing an actionable UI error, leaving the app in a misleading loaded state. Evidenceapps/desktop/src/renderer/src/store.ts:888.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ designsLoaded: true, errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('projects.notifications.loadFailed'), description: msg, }); throw new Error(`loadDesigns failed: ${msg}`); }
-
[Minor] Hardcoded
text-whitebypasses tokenized UI color constraints. Evidenceapps/desktop/src/renderer/src/components/DeleteDesignDialog.tsx:47.
Suggested fix:className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"
Summary
- Review mode: follow-up after new commits
- 4 issues found (3 Major, 1 Minor) on added/modified lines.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| return runDb('get-design', () => getDesign(db, id)); | ||
| }); | ||
|
|
||
| ipcMain.handle('snapshots:v1:rename-design', (_e: unknown, raw: unknown): Design => { |
There was a problem hiding this comment.
This new snapshots:v1:* handler path skips schemaVersion validation, while existing v1 handlers enforce it. Please gate this payload with requireSchemaV1(...) (and mirror that in other new handlers) to preserve protocol upgradeability and avoid mixed v1 contract behavior.
const r = raw as Record<string, unknown>;
requireSchemaV1(r, 'snapshots:v1:rename-design');| // Persistence failures should not break the chat experience — log to console | ||
| // and leave the in-memory state untouched. The next successful persist will | ||
| // overwrite anyway because we replace the whole list. | ||
| console.error('persistDesignState failed', err); |
There was a problem hiding this comment.
This still silently falls back to console logging on persistence failure. Please surface a user-visible error and throw with context so failed message/thumbnail writes are explicit.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
get().pushToast({
variant: 'error',
title: tr('projects.notifications.saveFailed'),
description: msg,
});
set({ errorMessage: msg, lastError: msg });
throw new Error(`persistDesignState failed: ${msg}`);
}| const designs = await window.codesign.snapshots.listDesigns(); | ||
| set({ designs, designsLoaded: true }); | ||
| } catch (err) { | ||
| console.error('loadDesigns failed', err); |
There was a problem hiding this comment.
loadDesigns still swallows failure and marks loaded, which can leave stale/empty UI with no actionable signal. Please surface an error toast/state and throw with context.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
set({ designsLoaded: true, errorMessage: msg, lastError: msg });
get().pushToast({
variant: 'error',
title: tr('projects.notifications.loadFailed'),
description: msg,
});
throw new Error(`loadDesigns failed: ${msg}`);
}| <button | ||
| type="button" | ||
| onClick={() => void softDeleteDesign(target.id)} | ||
| className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-white text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity" |
There was a problem hiding this comment.
text-white is a hardcoded color class and bypasses tokenized semantic color usage. Please switch to the token-backed foreground color.
className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"…Major)
Extends schemaVersion: 1 enforcement to all snapshots:v1:* IPC channels
that previously omitted it: get-design, rename-design, set-thumbnail,
soft-delete-design, duplicate-design, list-messages, replace-messages.
Both ends updated:
- preload sends { schemaVersion: 1, ... } for every payload
- main handlers call requireSchemaV1() and reject IPC_BAD_INPUT otherwise
(added directly in rename/set-thumbnail/duplicate/replace-messages,
centralized in parseIdPayload + parseDesignIdPayload helpers covering
get-design, soft-delete-design, list-messages)
Test coverage extended: schemaVersion gating loop now exercises all 13
channels; existing happy-path tests wrapped with v1() helper. 81 tests pass.
There was a problem hiding this comment.
Findings
-
[Major]
persistDesignStateswallows persistence failures with console-only logging, which violates the no-silent-fallback constraint and can hide message/thumbnail data loss. Evidenceapps/desktop/src/renderer/src/store.ts:301
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.saveFailed'), description: msg, }); set({ errorMessage: msg, lastError: msg }); throw new Error(`persistDesignState failed: ${msg}`); }
-
[Major]
maybeAutoRenamesuppresses rename failures withconsole.erroronly, so rename persistence errors are invisible to users. Evidenceapps/desktop/src/renderer/src/store.ts:318
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); get().pushToast({ variant: 'error', title: tr('projects.notifications.renameFailed'), description: msg, }); set({ errorMessage: msg, lastError: msg }); throw new Error(`maybeAutoRename failed: ${msg}`); }
-
[Major]
loadDesignsmarksdesignsLoaded=trueafter failure without surfacing actionable UI error state, creating a misleading loaded state. Evidenceapps/desktop/src/renderer/src/store.ts:888
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ designsLoaded: true, errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('projects.notifications.loadFailed'), description: msg, }); throw new Error(`loadDesigns failed: ${msg}`); }
-
[Minor] Hardcoded
text-whitebypasses tokenized semantic color usage. Evidenceapps/desktop/src/renderer/src/components/DeleteDesignDialog.tsx:47
Suggested fix:className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"
Summary
- Review mode: follow-up after new commits
- 4 issues found on modified lines (3 Major, 1 Minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| // Persistence failures should not break the chat experience — log to console | ||
| // and leave the in-memory state untouched. The next successful persist will | ||
| // overwrite anyway because we replace the whole list. | ||
| console.error('persistDesignState failed', err); |
There was a problem hiding this comment.
persistDesignState currently logs and continues on persistence failure, which makes data-write failure invisible. Please surface UI error state and throw with context.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
get().pushToast({
variant: 'error',
title: tr('projects.notifications.saveFailed'),
description: msg,
});
set({ errorMessage: msg, lastError: msg });
throw new Error(`persistDesignState failed: ${msg}`);
}| await window.codesign.snapshots.renameDesign(designId, newName); | ||
| await get().loadDesigns(); | ||
| } catch (err) { | ||
| console.error('maybeAutoRename failed', err); |
There was a problem hiding this comment.
This catch suppresses rename failures with console logging only. Please surface the failure to the user and propagate context.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
get().pushToast({
variant: 'error',
title: tr('projects.notifications.renameFailed'),
description: msg,
});
set({ errorMessage: msg, lastError: msg });
throw new Error(`maybeAutoRename failed: ${msg}`);
}| const designs = await window.codesign.snapshots.listDesigns(); | ||
| set({ designs, designsLoaded: true }); | ||
| } catch (err) { | ||
| console.error('loadDesigns failed', err); |
There was a problem hiding this comment.
loadDesigns marks loaded after failure but does not surface actionable UI error state. Please set error state + toast and throw with context.
} catch (err) {
const msg = err instanceof Error ? err.message : tr('errors.unknown');
set({ designsLoaded: true, errorMessage: msg, lastError: msg });
get().pushToast({
variant: 'error',
title: tr('projects.notifications.loadFailed'),
description: msg,
});
throw new Error(`loadDesigns failed: ${msg}`);
}| <button | ||
| type="button" | ||
| onClick={() => void softDeleteDesign(target.id)} | ||
| className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-white text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity" |
There was a problem hiding this comment.
text-white is a hardcoded color utility and bypasses tokenized semantic color usage. Please switch to the token-backed foreground color.
className="h-9 px-3 rounded-[var(--radius-md)] bg-[var(--color-error)] text-[var(--color-on-accent)] text-[var(--text-sm)] font-medium hover:opacity-90 transition-opacity"There was a problem hiding this comment.
Findings
-
[Major] Inline-comment edits are not persisted per design, which can lose user changes after switching designs.
switchDesignrestores state from SQLite (apps/desktop/src/renderer/src/store.ts:968), but persistence is only wired in the generate-success path (apps/desktop/src/renderer/src/store.ts:373). The inline-comment success path updates in-memory state without persisting it.
Suggested fix:// after applyInlineComment success updates state const designId = get().currentDesignId; if (designId) { void persistDesignState(get, designId, get().messages, get().previewHtml); }
-
[Minor] A hardcoded overlay color was introduced instead of using token-based UI values, violating the UI token constraint. Evidence
apps/desktop/src/renderer/src/components/DesignsView.tsx:158.
Suggested fix:className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-[color-mix(in_srgb,var(--color-background)_10%,transparent)]"
Summary
- Review mode: follow-up after new commits
- 2 issues found on modified lines (1 Major, 1 Minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested: add a store test that verifies
applyInlineCommentpersists messages/preview and survivesswitchDesignreload.
open-codesign Bot
| activeGenerationId: null, | ||
| generationStage: 'done' as GenerationStage, | ||
| })); | ||
| const designId = get().currentDesignId; |
There was a problem hiding this comment.
[Major] Persistence is currently only triggered on the generate-success path. Since design switching restores from SQLite, inline-comment edits can be lost unless that success path also persists.
Suggested fix:
const designId = get().currentDesignId;
if (designId) {
void persistDesignState(get, designId, get().messages, get().previewHtml);
}| aria-label={`${t('projects.view.open')} — ${design.name}`} | ||
| > | ||
| {design.thumbnailText ? ( | ||
| <span className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-gradient-to-t from-black/10 via-transparent to-transparent"> |
There was a problem hiding this comment.
[Minor] from-black/10 is a hardcoded color utility and bypasses tokenized UI values.
Suggested fix:
className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-[color-mix(in_srgb,var(--color-background)_10%,transparent)]"… PR #52) DesignMessageV1 zod schema previously narrowed role to user|assistant, but DB CHECK constraint and IPC validator both accept 'system'. This created an internal contract break: persisted system messages would fail schema validation downstream. Extend DesignMessageV1.role to include 'system' to match ChatMessage and the DB/IPC contract. Additive change — existing rows remain valid (no schemaVersion bump needed). - Add vitest coverage for persist+load of role='system' - Apply biome auto-format fixes pulled in by --fix Signed-off-by: hqhq1025 <1506751656@qq.com>
…Major)
Extends schemaVersion: 1 enforcement to all snapshots:v1:* IPC channels
that previously omitted it: get-design, rename-design, set-thumbnail,
soft-delete-design, duplicate-design, list-messages, replace-messages.
Both ends updated:
- preload sends { schemaVersion: 1, ... } for every payload
- main handlers call requireSchemaV1() and reject IPC_BAD_INPUT otherwise
(added directly in rename/set-thumbnail/duplicate/replace-messages,
centralized in parseIdPayload + parseDesignIdPayload helpers covering
get-design, soft-delete-design, list-messages)
Test coverage extended: schemaVersion gating loop now exercises all 13
channels; existing happy-path tests wrapped with v1() helper. 81 tests pass.
9b01f64 to
433453d
Compare
There was a problem hiding this comment.
Findings
-
[Major] Deleting the current design is allowed while generation is active, which can write generation output into the wrong design after the store auto-switches.
softDeleteDesignhas noisGeneratingguard (apps/desktop/src/renderer/src/store.ts:1050), whileapplyGenerateSuccesspersists to whatevercurrentDesignIdis at completion time (apps/desktop/src/renderer/src/store.ts:377).
Suggested fix:async softDeleteDesign(id: string) { if (!window.codesign) return; if (get().isGenerating) { get().pushToast({ variant: 'info', title: tr('projects.notifications.switchBlockedGenerating'), }); return; } // existing delete flow... }
-
[Minor] Hardcoded color utility
from-black/10bypasses tokenized UI values, violating the UI token constraint. Evidenceapps/desktop/src/renderer/src/components/DesignsView.tsx:158.
Suggested fix:className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-[color-mix(in_srgb,var(--color-background)_10%,transparent)]"
Summary
- Review mode: follow-up after new commits
- 2 issues found on modified lines (1 Major, 1 Minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested: add a store test that deletes the active design during an in-flight generation and asserts no cross-design message/preview persistence.
open-codesign Bot
| } | ||
| }, | ||
|
|
||
| async softDeleteDesign(id: string) { |
There was a problem hiding this comment.
[Major] softDeleteDesign can run while a generation is active. If the current design is deleted, the store switches/creates another design, and when the in-flight generation resolves, applyGenerateSuccess persists into the new currentDesignId, causing cross-design contamination.
Suggested fix:
if (get().isGenerating) {
get().pushToast({
variant: info,
title: tr(projects.notifications.switchBlockedGenerating),
});
return;
}| aria-label={`${t('projects.view.open')} — ${design.name}`} | ||
| > | ||
| {design.thumbnailText ? ( | ||
| <span className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-gradient-to-t from-black/10 via-transparent to-transparent"> |
There was a problem hiding this comment.
[Minor] from-black/10 is a hardcoded color utility and bypasses tokenized UI values.
Suggested fix:
className="absolute inset-0 flex items-end p-3 text-[var(--text-xs)] text-[var(--color-text-primary)] bg-[color-mix(in_srgb,var(--color-background)_10%,transparent)]"… duplicate / delete Adds first-class project (design) management: users can create new designs, browse a grid of past work, switch / rename / duplicate / delete, and have each project carry its own message history + preview snapshot. Schema - Additive migration on `designs`: thumbnail_text + deleted_at columns. - New `design_messages` table for per-design chat history with cascade. - New DB functions: getDesign, renameDesign, setDesignThumbnail, softDeleteDesign, duplicateDesign (with snapshot parent rewiring), listMessages, replaceMessages. IPC (snapshots:v1:* namespace) - get-design, rename-design, set-thumbnail, soft-delete-design, duplicate-design, list-messages, replace-messages. Renderer - TopBar breadcrumb dropdown (DesignSwitcher) — recent 5 + new + rename + view-all entries. - DesignsView overlay grid with per-card hover actions, search, and hash-derived gradient placeholders (real iframe thumbnails deferred to a follow-up PR). - RenameDesignDialog + DeleteDesignDialog (focus-trapped overlays). - Auto-name from the first user prompt (Claude Design / Lovable pattern). - cmd+N shortcut for new design; command palette gains "Browse all designs" and "Rename current design" entries. - Store: currentDesignId + per-design message swap on switchDesign; generation success persists messages + thumbnail to SQLite. i18n - Adds projects.* namespace (en + zh-CN) — switcher, view, rename, delete, time, notifications, command palette additions. Tests - snapshots-db: rename / soft-delete / duplicate (with parent rewiring) / set-thumbnail / messages round-trip / cascade / migration idempotency. - snapshots-ipc: rename / set-thumbnail / soft-delete / duplicate / list+replace messages / get-design. - store: switchDesign isolation / createNewDesign reset / generation-block-on-switch toast. Built on top of #29 (snapshots SQLite + IPC). Targets main as base; merging order should be #29 → this PR. Background research: docs/research/26-project-management-ux.md (internal). Refs user feedback that there was no obvious way to start a new project or see history. Signed-off-by: hqhq1025 <1506751656@qq.com>
Signed-off-by: hqhq1025 <1506751656@qq.com>
… PR #52) DesignMessageV1 zod schema previously narrowed role to user|assistant, but DB CHECK constraint and IPC validator both accept 'system'. This created an internal contract break: persisted system messages would fail schema validation downstream. Extend DesignMessageV1.role to include 'system' to match ChatMessage and the DB/IPC contract. Additive change — existing rows remain valid (no schemaVersion bump needed). - Add vitest coverage for persist+load of role='system' - Apply biome auto-format fixes pulled in by --fix Signed-off-by: hqhq1025 <1506751656@qq.com>
After rebase onto main (#29), align preload comma syntax, expand SNAPSHOTS_CHANNELS_V1 to cover the project-management handlers, and update the soft-delete-design test to use the object payload now required by snapshots:v1:list-designs. Signed-off-by: hqhq1025 <1506751656@qq.com>
…Major)
Extends schemaVersion: 1 enforcement to all snapshots:v1:* IPC channels
that previously omitted it: get-design, rename-design, set-thumbnail,
soft-delete-design, duplicate-design, list-messages, replace-messages.
Both ends updated:
- preload sends { schemaVersion: 1, ... } for every payload
- main handlers call requireSchemaV1() and reject IPC_BAD_INPUT otherwise
(added directly in rename/set-thumbnail/duplicate/replace-messages,
centralized in parseIdPayload + parseDesignIdPayload helpers covering
get-design, soft-delete-design, list-messages)
Test coverage extended: schemaVersion gating loop now exercises all 13
channels; existing happy-path tests wrapped with v1() helper. 81 tests pass.
…dex follow-up)
…e overlay (PR #52 codex) - softDeleteDesign now bails with an info toast when isGenerating is true, preventing applyGenerateSuccess from writing into a stale design. - Replace hardcoded `from-black/10` overlay in DesignsView with the text-primary token via color-mix so the gradient respects light/dark themes and routes through packages/ui tokens. - Add deleteBlockedGenerating i18n key (en + zh-CN) and a vitest case asserting softDeleteDesign noops + surfaces the toast while generating.
433453d to
7d23b69
Compare
There was a problem hiding this comment.
Findings
-
[Major] Non-tokenized UI values are introduced in new renderer components, which violates the hard UI-token constraint and can drift from theme/system tokens. Evidence
apps/desktop/src/renderer/src/components/DesignSwitcher.tsx:54(also:64,:68,:90,:93, etc.).
Suggested fix:className="inline-flex items-center gap-[var(--space-1_5)] max-w-[var(--size-chip-max)] rounded-[var(--radius-md)] px-[var(--space-2)] py-[var(--space-1)] text-[var(--text-sm)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] transition-colors"
-
[Major]
createNewDesignsilently no-ops while generation is active (if (get().isGenerating) return null;), so user-triggered actions from hotkey/command palette appear broken with no surfaced error/toast. Evidenceapps/desktop/src/renderer/src/store.ts:935.
Suggested fix:if (get().isGenerating) { get().pushToast({ variant: 'info', title: tr('projects.notifications.createBlockedGenerating'), }); return null; }
Summary
- Review mode: follow-up after new commits
- 2 issues found on modified lines (2 Major).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
- Suggested: add a store test that asserts
createNewDesign()during active generation emits an info toast and does not mutate current design state.
open-codesign Bot
| aria-haspopup="menu" | ||
| aria-expanded={open} | ||
| aria-label={t('projects.switcher.menuLabel')} | ||
| className="inline-flex items-center gap-1.5 max-w-[260px] rounded-[var(--radius-md)] px-2 py-1 text-[var(--text-sm)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] transition-colors" |
There was a problem hiding this comment.
[Major] This new class string introduces hardcoded layout values (max-w-[260px], numeric spacing, and related raw px usage nearby) instead of packages/ui tokens, which violates the hard UI-token constraint.
Suggested fix:
className="inline-flex items-center gap-[var(--space-1_5)] max-w-[var(--size-chip-max)] rounded-[var(--radius-md)] px-[var(--space-2)] py-[var(--space-1)] text-[var(--text-sm)] text-[var(--color-text-primary)] hover:bg-[var(--color-surface-hover)] transition-colors"|
|
||
| async createNewDesign() { | ||
| if (!window.codesign) return null; | ||
| if (get().isGenerating) return null; |
There was a problem hiding this comment.
[Major] User-invoked createNewDesign silently returns during active generation, so shortcuts/palette actions look broken with no feedback. This conflicts with the no-silent-fallback behavior expected in this project.
Suggested fix:
if (get().isGenerating) {
get().pushToast({
variant: 'info',
title: tr('projects.notifications.createBlockedGenerating'),
});
return null;
}
Summary
Adds first-class project management. Users can create new designs, browse history, switch / rename / duplicate / soft-delete past work. Each project carries its own message history and preview snapshot.
Built on top of #29 (snapshots SQLite + IPC). Targets
mainas base — merge order should be #29 → this PR.Addresses user feedback: there was no obvious way to start a new project or see history.
What ships
Cmd+Nfor new design; command palette gains "Browse all designs" + "Rename current design"Schema (additive, non-breaking)
designs.thumbnail_textanddesigns.deleted_atcolumns added via additive migration (PRAGMA-detected, idempotent).design_messagestable withON DELETE CASCADEand a composite primary key on(design_id, ordinal).IPC (
snapshots:v1:*namespace)get-design,rename-design,set-thumbnail,soft-delete-design,duplicate-design(with snapshot parent rewiring),list-messages,replace-messages.Why no real iframe thumbnails yet
A sandboxed iframe-thumbnail pipeline is the eventual goal but is expensive to build correctly (per-card srcdoc + import map + GC). For v1 we ship hash-derived gradient cards + first-prompt snippet, with the
thumbnail_textcolumn already in place to keep the upgrade path clean. Seedocs/research/26-project-management-ux.mdfor the rationale and competitor scan.PRINCIPLES §5b
schemaVersion: 1; thumbnails column reserved for the upgrade.Test plan
vitest: 26 new tests across snapshots-db (rename / soft-delete / duplicate / set-thumbnail / messages / migration idempotency / cascade) and snapshots-ipc (rename / set-thumbnail / soft-delete / duplicate / list+replace messages / get-design)vitest: store tests forswitchDesignisolation,createNewDesignreset, generation-block-on-switch toastpnpm typecheckcleanpnpm lintclean (8 pre-existing warnings, 0 errors)Cmd+N→ new design → first prompt → design auto-names from prompt