feat(desktop): redesign sidebar input + add cancellation IPC#15
feat(desktop): redesign sidebar input + add cancellation IPC#15
Conversation
There was a problem hiding this comment.
Findings
-
[Major] 取消后旧请求仍会回写并清空当前 generation 状态:
cancelGeneration()先把isGenerating/activeGenerationId置空,但旧的generate()Promise 在稍后 resolve/reject 时,applyGenerateSuccess()和applyGenerateError()仍会无条件写回状态。用户在取消 A 后立刻发起 B 时,A 的完成回调会把 B 的进行中状态清掉,严重时还会用旧结果覆盖当前预览。证据:apps/desktop/src/renderer/src/store.ts:112、apps/desktop/src/renderer/src/store.ts:126、apps/desktop/src/renderer/src/store.ts:208
Suggested fix:const generationId = newId(); const finishIfCurrent = (update: (s: CodesignState) => Partial<CodesignState>) => set((s) => (s.activeGenerationId === generationId ? update(s) : {})); try { const result = await window.codesign.generate({ prompt, history: get().messages, model, generationId }); finishIfCurrent((s) => ({ messages: [...s.messages, { role: 'assistant', content: message || 'Done.' }], previewHtml: firstArtifact?.content ?? s.previewHtml, isGenerating: false, activeGenerationId: null, })); } catch (err) { finishIfCurrent((s) => ({ messages: isCancelled ? s.messages : [...s.messages, { role: 'assistant', content: `Error: ${msg}` }], isGenerating: false, activeGenerationId: null, errorMessage: isCancelled ? null : msg, lastError: isCancelled ? s.lastError : msg, })); } cancelGeneration() { const id = get().activeGenerationId; if (!id || !window.codesign) return; void window.codesign.cancelGeneration(id); }
-
[Major]
codesign:cancel-generation对非法 IPC 输入会取消所有 in-flight 请求,而不是报错:typeof raw !== 'string'时直接进入else分支并遍历inFlight全量abort()。这把一个输入校验问题升级成跨请求副作用,也违反了“errors are user-visible or thrown”的约束。证据:apps/desktop/src/main/index.ts:129
Suggested fix:ipcMain.handle('codesign:cancel-generation', (_e, raw: unknown) => { if (typeof raw !== 'string') { throw new CodesignError('cancel-generation expects a generationId string', 'IPC_BAD_INPUT'); } const controller = inFlight.get(raw); if (!controller) return; controller.abort(); inFlight.delete(raw); logIpc.info('generate.cancelled', { id: raw }); });
Summary
Review mode: initial
发现 2 个 Major,均由这次 cancellation 改动直接引入:一个会让被取消的旧请求污染下一次生成状态,另一个会让非法 IPC 输入取消所有进行中的请求。未看到对应的 Vitest 覆盖来锁住这两条路径。
Testing
- Not run (automation; PR diff treated as untrusted input)
- 建议补 2 个 Vitest:
cancel -> re-submit期间旧请求不得清空新请求状态;codesign:cancel-generation非字符串输入必须抛IPC_BAD_INPUT
open-codesign Bot
| const id = get().activeGenerationId; | ||
| if (!id || !window.codesign) return; | ||
| window.codesign.cancelGeneration(id); | ||
| set({ isGenerating: false, activeGenerationId: null }); |
There was a problem hiding this comment.
cancelGeneration() 在 IPC abort 真正落地前就把 isGenerating / activeGenerationId 清空了,但同一个 request 的 applyGenerateSuccess() / applyGenerateError() 之后仍会无条件落状态。这样 A 被取消后立刻发起 B,A 的完成回调仍会把 B 的进行中状态清掉。建议把终态写回和 generationId 绑定,只在 activeGenerationId === generationId 时收尾,并且这里不要提前清空状态。
| }); | ||
|
|
||
| ipcMain.handle('codesign:cancel-generation', (_e, raw: unknown) => { | ||
| const id = typeof raw === 'string' ? raw : undefined; |
There was a problem hiding this comment.
这里把非字符串输入解释成 cancel all 了:typeof raw !== string 会落到 else 分支并 abort() 所有 in-flight 请求。IPC 边界更安全的行为应该是像 codesign:detect-provider 一样直接抛 IPC_BAD_INPUT;如果确实需要 cancel all,建议单独做一个显式 channel,而不是复用坏输入。
|
目前不建议合并,先处理下面这些问题:
CI 主体已经通过,但这两个行为回归问题没修掉前,不建议合并。 |
There was a problem hiding this comment.
Findings
-
[Major] 新增的 cancellation IPC surface 仍然是未版本化协议,这次 PR 第一次公开
cancelGeneration()/codesign:cancel-generation,但docs/PRINCIPLES.md:75明确要求 public IPC channel 从第一版就版本化。现在把它作为裸字符串 channel 发布,后续一旦需要扩展 reason / actor / bulk-cancel 等字段,就只能继续做 silent shape changes。证据:apps/desktop/src/preload/index.ts:36、apps/desktop/src/main/index.ts:130
Suggested fix:// packages/shared/src/index.ts export const CancelGenerationPayloadV1 = z.object({ schemaVersion: z.literal(1), generationId: z.string(), }); // apps/desktop/src/preload/index.ts cancelGeneration: (generationId: string) => ipcRenderer.invoke('codesign:v1:cancel-generation', { schemaVersion: 1, generationId, }), // apps/desktop/src/main/index.ts ipcMain.handle('codesign:v1:cancel-generation', (_e, raw: unknown) => { const { generationId } = CancelGenerationPayloadV1.parse(raw); cancelGenerationRequest(generationId, inFlight, logIpc); });
-
[Minor] Sidebar 这次新增的输入框/按钮布局继续绕过
packages/uitoken scale,引入了新的 hardcoded UI values:144px、24px、bottom-2/right-2、w-7/h-7、w-3.5/h-3.5。仓库已经有 spacing tokens 和IconButton可复用;把这些字面量继续写进 app code 会让 UI debt 扩散到更多组件。证据:apps/desktop/src/renderer/src/components/Sidebar.tsx:11、apps/desktop/src/renderer/src/components/Sidebar.tsx:80、apps/desktop/src/renderer/src/components/Sidebar.tsx:84、apps/desktop/src/renderer/src/components/Sidebar.tsx:90、packages/ui/src/tokens.css:75、packages/ui/src/components/IconButton.tsx:9
Suggested fix:import { IconButton } from '@open-codesign/ui'; function resizeTextarea(el: HTMLTextAreaElement): void { const rowHeight = parseFloat(getComputedStyle(el).getPropertyValue('--space-6')); const maxHeight = rowHeight * 6; el.style.height = 'auto'; el.style.height = `${Math.min(el.scrollHeight, maxHeight)}px`; } <textarea className="... px-[var(--space-3)] pt-[var(--space-3)] pb-[var(--space-8)] min-h-[var(--space-6)] max-h-[calc(var(--space-6)*6)] ..." /> <IconButton size="sm" label={isGenerating ? 'Stop generation' : 'Send prompt'} className="absolute bottom-[var(--space-2)] right-[var(--space-2)] bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)]" > {isGenerating ? <Square className="w-4 h-4" /> : <ArrowUp className="w-4 h-4" />} </IconButton>
Summary
Review mode: follow-up after new commits
发现 1 个 Major、1 个 Minor。前一轮 bot 指出的 cancellation correctness 问题在这个 head 上已修掉;这次剩下的是两个仍然落在新增代码上的 repo 级约束问题:一个是新 IPC surface 没有从首版开始版本化,另一个是 Sidebar 新增了一批 hardcoded UI values。
Testing
- Not run (automation)
- 建议再补 2 个测试:一个覆盖
cancelGeneration()的 IPC contract(schemaVersion/payload parse),一个做 Sidebar 键盘与 autosize 的 renderer test,锁住 tokenized height/button layout
open-codesign Bot
| } | ||
| }); | ||
|
|
||
| ipcMain.handle('codesign:cancel-generation', (_e, raw: unknown) => { |
There was a problem hiding this comment.
这里第一次把 cancellation 能力公开成 renderer <-> main 的 IPC surface,但 channel 名和 payload 都还是未版本化的裸协议。按 docs/PRINCIPLES.md 的兼容性要求,建议在首发时就切到 codesign:v1:cancel-generation,并把 generationId 放进 shared schema;否则后续一旦要加字段,只能继续做 silent shape changes。
| disabled={isGenerating} | ||
| rows={1} | ||
| className="flex-1 resize-none bg-transparent px-2 py-1 text-[var(--text-sm)] leading-[1.5] text-[var(--color-text-primary)] placeholder:text-[var(--color-text-muted)] focus:outline-none min-h-[24px] max-h-[160px]" | ||
| className="block w-full resize-none bg-transparent px-3 pt-3 pb-10 text-[var(--text-sm)] leading-[1.5] text-[var(--color-text-primary)] placeholder:text-[var(--color-text-muted)] focus:outline-none min-h-[24px] max-h-[144px] overflow-y-auto" |
There was a problem hiding this comment.
这一块新增了多组 layout literals:144px / 24px / bottom-2 right-2 / w-7 h-7 / w-3.5 h-3.5。packages/ui/src/tokens.css 已经有 --space-* scale,仓库里也有 IconButton 可复用,建议把 textarea 高度和按钮布局改成 token/shared component 驱动,别把更多固定值继续写进 app code。
There was a problem hiding this comment.
Findings
-
[Major]
abort/cancel文本匹配会把仍属当前 generation 的真实失败静默吞掉。这里已经有activeGenerationId守卫,用户主动取消后的旧 promise 会被 stale-check 直接忽略;继续按消息文本做 suppression,只会误伤当前请求的真实错误,一旦上游报错文案里带cancel/abort,这里就不会写errorMessage或 toast,违反“errors are user-visible or thrown”。证据apps/desktop/src/renderer/src/store.ts:133、apps/desktop/src/renderer/src/store.ts:224
Suggested fix:function applyGenerateError( get: GetState, set: SetState, generationId: string, err: unknown, ): void { const msg = err instanceof Error ? err.message : 'Unknown error'; if (get().activeGenerationId !== generationId) return; finishIfCurrent(set, generationId, (state) => ({ messages: [...state.messages, { role: 'assistant', content: `Error: ${msg}` }], isGenerating: false, activeGenerationId: null, errorMessage: msg, lastError: msg, })); get().pushToast({ variant: 'error', title: 'Generation failed', description: msg }); }
-
[Minor] Sidebar 的 autosize 上限目前绑定的是 spacing token,不是 textarea 的真实行高。
--space-6是24px,但当前 tokens 里--text-sm=13px且--leading-body=1.6,实际一行约20.8px,所以输入框会接近 7 行才开始滚动;同时 JS/CSS 各维护一套上限,后续 token 调整时也容易继续漂。证据apps/desktop/src/renderer/src/components/Sidebar.tsx:16、apps/desktop/src/renderer/src/components/Sidebar.tsx:87、packages/ui/src/tokens.css:57、packages/ui/src/tokens.css:67、packages/ui/src/tokens.css:81
Suggested fix:const MAX_TEXTAREA_ROWS = 6; function resizeTextarea(el: HTMLTextAreaElement): void { const lineHeight = Number.parseFloat(getComputedStyle(el).lineHeight); const maxHeight = lineHeight * MAX_TEXTAREA_ROWS; el.style.height = 'auto'; el.style.height = `${Math.min(el.scrollHeight, maxHeight)}px`; } <textarea rows={1} className="... overflow-y-auto" />
Summary
Review mode: follow-up after new commits
上一轮指出的 IPC versioning 和 stale completion 问题在这个 head 上已经修掉;当前还剩 1 个会静默吞错的回归点,以及 1 个 Sidebar autosize 上限与 token 实际行高不一致的问题。
Testing
- Not run (automation)
- 建议补一个 store test:当前 generation 抛出包含
cancel/abort的普通错误时,仍然会显示 toast /errorMessage - 建议补一个 Sidebar renderer test:输入超过 6 行时开始滚动,而不是继续增长到接近 7 行
open-codesign Bot
| ): void { | ||
| const msg = err instanceof Error ? err.message : 'Unknown error'; | ||
| const lower = msg.toLowerCase(); | ||
| const isCancelled = lower.includes('abort') || lower.includes('cancel'); |
There was a problem hiding this comment.
这里已经有 generationId 守卫了,用户主动 cancel 的 rejection 会被 stale-check 直接忽略;再按错误文案匹配 cancel / abort,只会把当前请求的真实失败静默吞掉。
| const FALLBACK_ROW_HEIGHT = 24; | ||
|
|
||
| function getTextareaRowHeight(el: HTMLTextAreaElement): number { | ||
| const rowHeight = Number.parseFloat(getComputedStyle(el).getPropertyValue('--space-6')); |
There was a problem hiding this comment.
这里把 textarea 的“行高”绑定到 --space-6,但当前 token 里 --text-sm=13px、--leading-body=1.6,真实一行约 20.8px,所以会接近 7 行才开始滚动。建议直接读 computed lineHeight,并只保留这一套上限。
There was a problem hiding this comment.
Findings
-
[Major]
cancelGeneration()在ipcRenderer.invoke()返回前就把activeGenerationId/isGenerating清空了。只要主进程拒绝这次 cancel,UI 就会显示“已停止”,但原请求其实还在跑,后续结果还会因为 stale guard 被直接丢掉,既违反“errors are user-visible or thrown”,也会造成结果丢失。证据apps/desktop/src/renderer/src/store.ts:217apps/desktop/src/renderer/src/store.ts:220apps/desktop/src/preload/index.ts:37
Suggested fix:cancelGeneration() { const id = get().activeGenerationId; if (!id || !window.codesign) return; void window.codesign .cancelGeneration(id) .then(() => { finishIfCurrent(set, id, () => ({ isGenerating: false, activeGenerationId: null })); }) .catch((err) => { const msg = err instanceof Error ? err.message : 'Unknown error'; set({ errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: 'Cancellation failed', description: msg }); }); }
-
[Major] 这次改动给
GeneratePayload新增了generationId,但codesign:generate仍然是无版本 IPC / 无schemaVersionpayload。按照CLAUDE.md和PRINCIPLES.md §5b,IPC payload 变更时需要带版本并保留旧通道兼容;否则 main / preload / renderer 之后无法区分旧新 shape。证据packages/shared/src/index.ts:83packages/shared/src/index.ts:88apps/desktop/src/preload/index.ts:30apps/desktop/src/main/index.ts:123
Suggested fix:export const GeneratePayloadV1 = z.object({ schemaVersion: z.literal(1), prompt: z.string().min(1).max(32_000), history: z.array(ChatMessage).max(200), model: ModelRef, baseUrl: z.string().url().optional(), generationId: z.string().min(1), }); ipcMain.handle('codesign:v1:generate', (_e, raw) => { const payload = GeneratePayloadV1.parse(raw); return runGenerate({ payload, controller: new AbortController(), logIpc }); });
并把现有
codesign:generate留成至少一个 minor version 的兼容 shim。
Summary
- Review mode: follow-up after new commits
- 上一轮指出的 cancel payload versioning 和 stale-completion 问题已修复;当前 head 还剩 2 个需要在合并前处理的回归/约束问题。
- 额外检查:commit
4720ecbc9dec5a6c06ddb9eb459a0e0afb7ca7cf未包含Signed-off-by;如果 DCO 校验按 commit 执行,这个 PR 还会继续失败。
Testing
- Not run (automation)
- 建议补一个 renderer/store test:
window.codesign.cancelGeneration()reject 时,errorMessage/ toast 会显示,且activeGenerationId不会被提前清空 - 建议补一个 IPC contract test:
generate的 v1 payload 带schemaVersion,并保留旧 channel 的兼容 shim 至少一个 minor version
open-codesign Bot
| cancelGeneration() { | ||
| const id = get().activeGenerationId; | ||
| if (!id || !window.codesign) return; | ||
| window.codesign.cancelGeneration(id); |
There was a problem hiding this comment.
这里直接 fire-and-forget cancelGeneration(id),下一行就把本地状态清空了。只要 IPC reject,UI 会显示“已停止”,但主进程里的请求还在跑,最终结果还会被 stale guard 丢掉。建议把 reject 显式 surface 到 toast / errorMessage,并在主进程确认后再清空 activeGenerationId。
| history: z.array(ChatMessage).max(200), | ||
| model: ModelRef, | ||
| baseUrl: z.string().url().optional(), | ||
| generationId: z.string().optional(), |
There was a problem hiding this comment.
这里已经给 GeneratePayload 加了新字段,contract 实际上变了;但 codesign:generate 仍然没有 schemaVersion / v1 channel。按 repo 的 IPC versioning 约束,这一层需要版本化,并把旧 channel 留成兼容 shim。
1e7bdaf to
162e637
Compare
There was a problem hiding this comment.
Findings
-
[Major] Cancellation state is cleared before IPC cancel is acknowledged, so rejected cancels leave generation running while UI reports stopped (result can be dropped by stale guard). Evidence
apps/desktop/src/renderer/src/store.ts:368apps/desktop/src/renderer/src/store.ts:369
Suggested fix:cancelGeneration() { const id = get().activeGenerationId; if (!id || !window.codesign) return; void window.codesign .cancelGeneration(id) .then(() => { finishIfCurrent(set, id, () => ({ isGenerating: false, activeGenerationId: null })); }) .catch((err) => { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('notifications.generationFailed'), description: msg }); }); }
-
[Major]
GeneratePayloadshape changed (generationId) butcodesign:generateremains unversioned; this breaks the compatibility/versioning constraint for IPC contract evolution. Evidencepackages/shared/src/index.ts:113apps/desktop/src/preload/index.ts:40apps/desktop/src/main/index.ts:132
Suggested fix:export const GeneratePayloadV1 = z.object({ schemaVersion: z.literal(1), prompt: z.string().min(1).max(32_000), history: z.array(ChatMessage).max(200), model: ModelRef, baseUrl: z.string().url().optional(), referenceUrl: z.string().url().optional(), attachments: z.array(LocalInputFile).max(12).default([]), generationId: z.string().min(1), }); ipcMain.handle('codesign:v1:generate', (_e, raw) => { const payload = GeneratePayloadV1.parse(raw); return runGenerate(payload); }); // keep `codesign:generate` as a compatibility shim for one minor release
-
[Major] Sidebar action buttons use
text-whiteliteral color instead of UI tokens, violating the token-only UI rule for app code. Evidenceapps/desktop/src/renderer/src/components/Sidebar.tsx:107apps/desktop/src/renderer/src/components/Sidebar.tsx:117
Suggested fix:className="bg-[var(--color-accent)] text-[var(--color-on-accent)] hover:bg-[var(--color-accent-hover)] hover:text-[var(--color-on-accent)] ..."
(If
--color-on-accentis missing, add it inpackages/uifirst and consume it here.)
Summary
- Review mode: follow-up after new commits
- 3 merge-blocking issues remain in the latest head: cancel race handling, unversioned
generateIPC contract change, and token-constraint violation in Sidebar button colors.
Testing
- Not run (automation)
- Suggested: add a store test where
window.codesign.cancelGeneration()rejects and assertactiveGenerationIdis retained plus error/toast is surfaced.
open-codesign Bot
| cancelGeneration() { | ||
| const id = get().activeGenerationId; | ||
| if (!id || !window.codesign) return; | ||
| window.codesign.cancelGeneration(id); |
There was a problem hiding this comment.
[Major] cancelGeneration clears local state before ipcRenderer.invoke() resolves. If cancel rejects, UI reports stopped while generation is still running and later response is treated as stale.
Suggested fix:
void window.codesign.cancelGeneration(id).then(() => {
finishIfCurrent(set, id, () => ({ isGenerating: false, activeGenerationId: null }));
}).catch((err) => {
const msg = err instanceof Error ? err.message : tr(errors.unknown);
set({ errorMessage: msg, lastError: msg });
get().pushToast({ variant: error, title: tr(notifications.generationFailed), description: msg });
});| baseUrl: z.string().url().optional(), | ||
| referenceUrl: z.string().url().optional(), | ||
| attachments: z.array(LocalInputFile).max(12).default([]), | ||
| generationId: z.string().optional(), |
There was a problem hiding this comment.
[Major] IPC contract changed (generationId added) but codesign:generate is still unversioned. This violates the repo’s compatibility/versioning rule for IPC payload evolution.
Suggested fix:
export const GeneratePayloadV1 = z.object({
schemaVersion: z.literal(1),
...
generationId: z.string().min(1),
});
ipcMain.handle(codesign:v1:generate, (_e, raw) => {
const payload = GeneratePayloadV1.parse(raw);
return runGenerate(payload);
});Keep codesign:generate as a compatibility shim for at least one minor release.
| size="sm" | ||
| label="Stop generation" | ||
| onClick={cancelGeneration} | ||
| className="bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)] hover:text-white hover:scale-[1.04] active:scale-[0.96] transition-[transform,background-color,color] duration-150 ease-[var(--ease-out)]" |
There was a problem hiding this comment.
[Major] text-white hardcodes a color in app UI code. Project rules require visual values to come from packages/ui tokens.
Suggested fix:
className="... text-[var(--color-on-accent)] hover:text-[var(--color-on-accent)] ..."If the token is missing, add it in packages/ui before use.
There was a problem hiding this comment.
Findings
-
[Major] IPC compatibility regression: legacy
codesign:generatehandler was removed while introducingcodesign:v1:generate, which breaks the project’s compatibility rule requiring old+new IPC contracts to coexist for at least one minor release. Evidenceapps/desktop/src/main/index.ts:132apps/desktop/src/preload/index.ts:42
Suggested fix:// shared helper async function runGenerate(payload: { prompt: string; history: ChatMessage[]; model: ModelRef; baseUrl?: string; referenceUrl?: string; attachments: LocalInputFile[]; generationId: string; }) { // existing generate implementation } ipcMain.handle('codesign:v1:generate', (_e, raw) => { const payload = GeneratePayloadV1.parse(raw); return runGenerate(payload); }); // keep for one minor release as compatibility shim ipcMain.handle('codesign:generate', (_e, raw) => { const legacy = GeneratePayload.parse(raw); return runGenerate({ ...legacy, generationId: legacy.generationId ?? `gen-${Date.now()}` }); });
-
[Major] UI token rule violation:
text-whiteis a hardcoded color utility in app UI code; project rules require visual values to come frompackages/uitokens. Evidenceapps/desktop/src/renderer/src/components/Sidebar.tsx:107apps/desktop/src/renderer/src/components/Sidebar.tsx:117
Suggested fix:className="bg-[var(--color-accent)] text-[var(--color-on-accent)] hover:bg-[var(--color-accent-hover)] hover:text-[var(--color-on-accent)] ..."
If
--color-on-accentis missing, add it inpackages/uifirst, then consume it here.
Summary
- Review mode: follow-up after new commits
- 2 major issues remain on the latest head: IPC backward-compatibility break for
generate, and token-rule violation in Sidebar button text color.
Testing
- Not run (automation)
- Suggested: add a main-process compatibility test that verifies both
codesign:generate(legacy) andcodesign:v1:generateare accepted during the deprecation window.
open-codesign Bot
|
|
||
| ipcMain.handle('codesign:generate', async (_e, raw: unknown) => { | ||
| const payload = GeneratePayload.parse(raw); | ||
| ipcMain.handle('codesign:v1:generate', async (_e, raw: unknown) => { |
There was a problem hiding this comment.
[Major] Replacing codesign:generate with only codesign:v1:generate breaks backward compatibility for existing renderer callers, which conflicts with PRINCIPLES §5b. Keep a temporary legacy shim side-by-side with the new versioned handler for at least one minor release.
Suggested fix:
ipcMain.handle("codesign:v1:generate", (_e, raw) => runGenerate(GeneratePayloadV1.parse(raw)));
ipcMain.handle("codesign:generate", (_e, raw) => {
const legacy = GeneratePayload.parse(raw);
return runGenerate({ ...legacy, generationId: legacy.generationId ?? `gen-${Date.now()}` });
});| size="sm" | ||
| label="Stop generation" | ||
| onClick={cancelGeneration} | ||
| className="bg-[var(--color-accent)] text-white hover:bg-[var(--color-accent-hover)] hover:text-white hover:scale-[1.04] active:scale-[0.96] transition-[transform,background-color,color] duration-150 ease-[var(--ease-out)]" |
There was a problem hiding this comment.
[Major] text-white hardcodes a visual value in app code. Per PRINCIPLES §8, consume a token from packages/ui instead.
Suggested fix:
className="... text-[var(--color-on-accent)] hover:text-[var(--color-on-accent)] ..."If the token is missing, add it in packages/ui first.
There was a problem hiding this comment.
Findings
- [Major] Cancellation can fail silently when renderer bridge is unavailable — this violates the “no silent fallbacks” rule and can leave the UI stuck in generating state with no user feedback, evidence
apps/desktop/src/renderer/src/store.ts:367
Suggested fix:cancelGeneration() { const id = get().activeGenerationId; if (!id) return; if (!window.codesign) { const msg = tr('errors.rendererDisconnected'); set({ errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('notifications.cancellationFailed'), description: msg, }); return; } void window.codesign.cancelGeneration(id) .then(() => { finishIfCurrent(set, id, () => ({ isGenerating: false, activeGenerationId: null })); }) .catch((err) => { const msg = err instanceof Error ? err.message : tr('errors.unknown'); set({ errorMessage: msg, lastError: msg }); get().pushToast({ variant: 'error', title: tr('notifications.cancellationFailed'), description: msg, }); }); }
Summary
- Review mode: follow-up after new commits
- 1 major issue found on latest head:
cancelGenerationreturns early without surfacing an error whenwindow.codesignis unavailable.
Testing
- Not run (automation)
- Suggested: add a store test for
cancelGenerationwhenwindow.codesignis undefined, assertingerrorMessage/lastErrorand an error toast are set.
open-codesign Bot
|
|
||
| cancelGeneration() { | ||
| const id = get().activeGenerationId; | ||
| if (!id || !window.codesign) return; |
There was a problem hiding this comment.
[Major] This early return silently drops a cancellation failure when the renderer bridge is unavailable, which violates the no-silent-fallback rule and can leave users stuck without feedback.
Suggested fix:
if (!window.codesign) {
const msg = tr("errors.rendererDisconnected");
set({ errorMessage: msg, lastError: msg });
get().pushToast({
variant: "error",
title: tr("notifications.cancellationFailed"),
description: msg,
});
return;
}There was a problem hiding this comment.
Findings
-
[Major] Sidebar removes local-context entry points (attachments / reference URL / design system), causing feature regression in generation inputs and conflicting with v1 demo scope, evidence
apps/desktop/src/renderer/src/components/Sidebar.tsx:38,apps/desktop/src/renderer/src/components/Sidebar.tsx:63
Suggested fix:const t = useT(); const inputFiles = useCodesignStore((s) => s.inputFiles); const referenceUrl = useCodesignStore((s) => s.referenceUrl); const setReferenceUrl = useCodesignStore((s) => s.setReferenceUrl); const pickInputFiles = useCodesignStore((s) => s.pickInputFiles); const removeInputFile = useCodesignStore((s) => s.removeInputFile); const pickDesignSystemDirectory = useCodesignStore((s) => s.pickDesignSystemDirectory); const clearDesignSystem = useCodesignStore((s) => s.clearDesignSystem); // Keep the redesigned composer, but restore a compact local-context block above messages.
-
[Major] New user-facing strings are hardcoded in English instead of i18n keys, creating locale regression for non-English UI, evidence
apps/desktop/src/renderer/src/components/Sidebar.tsx:67,apps/desktop/src/renderer/src/components/Sidebar.tsx:95,apps/desktop/src/renderer/src/components/Sidebar.tsx:105,apps/desktop/src/renderer/src/components/Sidebar.tsx:115
Suggested fix:const t = useT(); <p>{t('sidebar.startHint')}</p> <textarea placeholder={t('chat.placeholder')} /> <IconButton label={t('common.stopGeneration')} /> <IconButton label={t('common.send')} />
Summary
- Review mode: follow-up after new commits
- 2 major issues found on latest head: local-context UI regression and i18n regression in Sidebar.
Testing
- Not run (automation)
- Suggested: add
Sidebartests asserting local-context controls are rendered and that labels/placeholders resolve through i18n keys.
open-codesign Bot
| )} | ||
| </div> | ||
|
|
||
| <aside className="flex flex-col min-h-0 border-r border-[var(--color-border)] bg-[var(--color-background-secondary)]"> |
There was a problem hiding this comment.
[Major] 这一处开始的新布局仅保留消息列表+输入框,PR diff 中原有 inputFiles/referenceUrl/designSystem 入口被移除且无替代入口,导致本地上下文能力回归。
Suggested fix:
// Keep redesigned composer, but restore compact local-context actions
const pickInputFiles = useCodesignStore((s) => s.pickInputFiles);
const pickDesignSystemDirectory = useCodesignStore((s) => s.pickDesignSystemDirectory);
const referenceUrl = useCodesignStore((s) => s.referenceUrl);
const setReferenceUrl = useCodesignStore((s) => s.setReferenceUrl);| }} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder={t('chat.placeholder')} | ||
| placeholder="Describe what to design... (Enter to send, Shift+Enter for newline)" |
There was a problem hiding this comment.
[Major] 这里引入了硬编码英文文案,绕过 i18n,会导致中文等 locale 退化。
Suggested fix:
const t = useT();
<textarea placeholder={t(chat.placeholder)} />There was a problem hiding this comment.
Findings
- No high-confidence issues found on added/modified lines in the latest head diff.
Summary
- Review mode: follow-up after new commits
- No new defects identified in this update.
- Residual risk: the cancel flow is covered by unit tests, but there is still no renderer-to-main integration/E2E assertion that verifies UI state transitions and error surfacing across the real IPC boundary.
Testing
- Not run (automation)
- Suggested tests:
- Playwright: start generation, click stop, assert spinner/button state resets and no stale assistant error message is appended.
- Integration/Vitest (with IPC mocks):
codesign:v1:cancel-generationrejects -> renderer setserrorMessageand pushesnotifications.cancellationFailedtoast.
open-codesign Bot
- Replace single-line input with autosize textarea (1–6 rows, max-h 144 px) - Move send button into bottom-right corner of textarea container, icon-only 28 px - During generation, send button becomes a stop button (Square icon) wired to cancelGeneration - Remove redundant keyboard-hint bar; fold hint into placeholder text - Enter sends, Shift+Enter inserts newline, ⌘↵ global send preserved - main: extract runGenerate helper; maintain Map<id, AbortController> for in-flight requests; new codesign:cancel-generation IPC handler; logger scope 'ipc' - preload: expose cancelGeneration(id) on window.codesign - shared: GeneratePayload accepts optional generationId - store: add activeGenerationId state + cancelGeneration action; suppress abort errors silently without pushing an error toast Signed-off-by: hqhq1025 <1506751656@qq.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Signed-off-by: hqhq1025 <1506751656@qq.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Signed-off-by: hqhq1025 <1506751656@qq.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com> Signed-off-by: hqhq1025 <1506751656@qq.com>
- applyGenerateSuccess/applyGenerateError already guarded via finishIfCurrent and the early-return id check; confirm with tests - Add noop test: unknown generationId leaves all in-flight intact - Add schema rejection test: empty generationId and missing schemaVersion both throw via CancelGenerationPayloadV1.parse - Fix biome formatting in store.test.ts (pre-existing) Signed-off-by: hqhq1025 <1506751656@qq.com>
…in features Rebase onto origin/main exposed conflicts where PRs #18, #19, #20 had advanced store, preload, shared and main/index.ts beyond the branch point. Resolve by taking origin/main as the base and layering our cancellation additions on top: - shared/index.ts: add LocalInputFile, SelectedElement, ElementSelectionRect, ApplyCommentPayload, StoredDesignSystem exports; add generationId to GeneratePayload; keep CancelGenerationPayloadV1 - preload/index.ts: add locale API, pickInputFiles, pickDesignSystemDirectory, clearDesignSystem, applyComment; add generationId to generate payload; keep cancelGeneration with schemaVersion: 1 - main/index.ts: add applyComment, scanDesignSystem, preparePromptContext, registerLocaleIpc; wire AbortController into generate handler; keep codesign:v1:cancel-generation handler - store.ts: merge all origin/main state (selectedElement, inputFiles, referenceUrl, i18n tr(), applyInlineComment, etc.) with PR's activeGenerationId / cancelGeneration / finishIfCurrent guards - store.test.ts: update sendPrompt calls to object form; add designSystem: null to OnboardingState fixture; relax toast title assertion for i18n Signed-off-by: hqhq1025 <1506751656@qq.com>
Major #1: cancelGeneration now waits for the IPC round-trip before clearing isGenerating/activeGenerationId. If main rejects the cancel (bad id, already aborted) the error becomes visible via toast instead of being silently swallowed. Updated store test to reflect async cancel and drain microtasks between cancel and the follow-up sendPrompt. Major #2: add GeneratePayloadV1 (schemaVersion: 1, generationId required) to @open-codesign/shared and rename the IPC channel to codesign:v1:generate, matching the pattern established by codesign:v1:cancel-generation. Preload builds the v1 envelope with satisfies check; main validates with GeneratePayloadV1.parse(). Add 5 schema tests for missing/wrong schemaVersion and empty generationId. Add notifications.cancellationFailed i18n key. Signed-off-by: hqhq1025 <1506751656@qq.com>
…ebar
- Add codesign:generate legacy handler that accepts old GeneratePayload
(no schemaVersion) and promotes it to V1 by injecting schemaVersion:1
and falling back generationId to gen-${Date.now()} when absent.
Emits a warn log to track removal in the next minor release.
- Add --color-on-accent token to packages/ui tokens.css (light + dark)
so accent-button text never hard-codes a color value.
- Replace text-white in Sidebar.tsx with text-[var(--color-on-accent)]
on both the stop and send IconButton variants.
- Add 3 tests for GeneratePayload legacy schema and the V1 promotion
path that the shim relies on.
Signed-off-by: hqhq1025 <1506751656@qq.com>
Split the combined `!id || !window.codesign` guard in `cancelGeneration` into two separate checks. A missing `activeGenerationId` is a legitimate no-op, but a missing IPC bridge is a real error that must be surfaced via `errorMessage`/`lastError` and a toast. Adds a Vitest test to cover the new error path (with `beforeAll` i18n init so `tr()` keys resolve). Signed-off-by: hqhq1025 <1506751656@qq.com>
…sidebar strings Restores the local context panel (attachments, reference URL, design system) that was accidentally dropped during rebase when the cancel button was added. All hardcoded English strings are now routed through useT(); chat.send and chat.stop keys were added to en.json / zh-CN.json. Signed-off-by: hqhq1025 <1506751656@qq.com>
Biome import sort and formatter fixes introduced during rebase conflict resolution. Signed-off-by: hqhq1025 <1506751656@qq.com>
536e8cd to
bb05f45
Compare
There was a problem hiding this comment.
Findings
- [Major] Hardcoded typography fallback values violate token-only UI constraint —
FALLBACK_FONT_SIZE = 13andFALLBACK_LINE_HEIGHT_MULTIPLIER = 1.6reintroduce literal UI values in renderer code, which conflicts with PRINCIPLES §8/§5b and can drift frompackages/uitypography tokens. Evidenceapps/desktop/src/renderer/src/components/Sidebar.tsx:14andapps/desktop/src/renderer/src/components/Sidebar.tsx:15.
Suggested fix:const fontSize = Number.parseFloat(styles.fontSize); const leading = Number.parseFloat(styles.getPropertyValue('--leading-body')); if (!Number.isFinite(fontSize) || fontSize <= 0 || !Number.isFinite(leading) || leading <= 0) { throw new Error('Textarea sizing tokens are missing or invalid'); } return fontSize * leading;
Summary
- Review mode: follow-up after new commits
- 1 issue found on modified lines in the latest head diff (token-constraint violation in textarea fallback sizing).
Testing
- Not run (automation)
open-codesign Bot
| } | ||
|
|
||
| const MAX_TEXTAREA_ROWS = 6; | ||
| const FALLBACK_FONT_SIZE = 13; |
There was a problem hiding this comment.
[Major] FALLBACK_FONT_SIZE = 13 introduces a hardcoded UI literal in app code. This conflicts with token-only UI constraints (PRINCIPLES §8/§5b). Please derive sizing from tokens/computed token values and fail loudly if typography tokens are missing, instead of embedding numeric UI defaults.
…kens Removes FALLBACK_FONT_SIZE and FALLBACK_LINE_HEIGHT_MULTIPLIER constants from getTextareaLineHeight. When computed lineHeight is not a finite positive number and either fontSize or --leading-body token is absent or invalid, the function now throws instead of silently using magic numbers. The --leading-body token is already defined in packages/ui/src/tokens.css. Updates Sidebar.test.ts: replaces the "falls back" test with two tests — one verifying valid-token multiplication and one verifying the throw path. Signed-off-by: hqhq1025 <1506751656@qq.com>
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: follow-up after new commits
- No new high-confidence issues found on modified lines in the latest head diff.
- Residual risk: runtime behavior was reviewed statically only in this environment.
Testing
- Not run (automation):
pnpmis not available in this runner.
open-codesign Bot
Summary
Map<generationId, AbortController>;preload 暴露cancelGeneration;store 跟踪activeGenerationId,abort 错误静默处理GeneratePayload新增可选generationId字段(向后兼容)Test plan
pnpm --filter @open-codesign/desktop typecheck✅pnpm lint✅🤖 Generated with subagent