fix(desktop): preview perf + token counter wiring#47
Conversation
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback on usage storage errors violates the "no silent fallbacks" constraint — localStorage parse/write failures are swallowed and replaced with zeroed state, which hides operational errors and can silently reset/lose displayed weekly spend (
apps/desktop/src/renderer/src/store.ts:173,apps/desktop/src/renderer/src/store.ts:182)
Suggested fix:function persistWeekUsage(usage: WeekUsage): string | null { if (typeof window === 'undefined') return null; try { window.localStorage.setItem(WEEK_USAGE_STORAGE_KEY, JSON.stringify(usage)); return null; } catch (err) { return err instanceof Error ? err.message : 'Failed to persist weekly usage'; } } // call site const persistError = persistWeekUsage(nextWeek); if (persistError) { get().pushToast({ variant: 'error', title: tr('notifications.usagePersistFailed'), description: persistError, }); }
-
[Minor] New tooltip text is hardcoded in English, bypassing i18n and regressing zh-CN UI consistency (
apps/desktop/src/renderer/src/components/TopBar.tsx:43)
Suggested fix:tooltipParts.push( t('topbar.lastUsageTooltip', { input: lastUsage.inputTokens.toLocaleString(), output: lastUsage.outputTokens.toLocaleString(), cost: formatCostUsd(lastUsage.costUsd), }), );
Summary
- Review mode: initial
- 2 issues found in diff-added lines: one hard-constraint violation (silent fallback) and one localization regression.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| outputTokens: parsed.outputTokens, | ||
| costUsd: parsed.costUsd, | ||
| }; | ||
| } catch { |
There was a problem hiding this comment.
readStoredWeekUsage swallows parse/storage errors and silently resets to an empty bucket. This conflicts with the no-silent-fallback constraint and can hide data-loss conditions. Please surface an error (toast/throw with context) instead of silently returning fresh.
| const tooltipParts: string[] = [t('topbar.spendTooltip')]; | ||
| if (lastUsage) { | ||
| tooltipParts.push( | ||
| `Last: ${lastUsage.inputTokens.toLocaleString()} in / ${lastUsage.outputTokens.toLocaleString()} out · $${formatCostUsd(lastUsage.costUsd)}`, |
There was a problem hiding this comment.
This newly added tooltip segment is hardcoded English (Last: ... in / ... out) and bypasses i18n. Please move this to locale keys and interpolate values so zh-CN stays fully localized.
Replaces the silent catch on weekUsage persistence with an error path: persistWeekUsage now returns the error message, callers log via console.warn and push an error toast (i18n key errors.storageFailed). In-memory weekUsage stays intact so the session keeps tracking even when localStorage is full or disabled. Addresses Codex Major review on PR #47.
6eabec8 to
58da137
Compare
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback still present on weekly usage read failure —
readStoredWeekUsagecatches localStorage/JSON errors and returns a zeroed bucket, which can hide data-loss/corruption conditions and violates the “no silent fallbacks” constraint. Evidence:apps/desktop/src/renderer/src/store.ts:173.
Suggested fix:} catch (err) { const msg = err instanceof Error ? err.message : 'Failed to read weekly usage'; console.warn('[open-codesign] failed to read weekly usage:', msg); return { ...fresh, readError: msg }; } // then surface once during init/sendPrompt: if (state.weekUsageReadError) { get().pushToast({ variant: 'error', title: tr('errors.storageFailed'), description: state.weekUsageReadError }); }
-
[Minor] Tooltip append is hardcoded English and bypasses i18n — added
Last: ... in / ... outtext is not localized, causing mixed-language UI in zh-CN. Evidence:apps/desktop/src/renderer/src/components/TopBar.tsx:43.
Suggested fix:tooltipParts.push( t('topbar.lastUsageTooltip', { input: lastUsage.inputTokens.toLocaleString(), output: lastUsage.outputTokens.toLocaleString(), cost: formatCostUsd(lastUsage.costUsd), }), );
Summary
- Review mode: follow-up after new commits
- 2 issues found in diff-added lines: one hard-constraint violation (silent fallback on storage read) and one localization regression.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| outputTokens: parsed.outputTokens, | ||
| costUsd: parsed.costUsd, | ||
| }; | ||
| } catch { |
There was a problem hiding this comment.
readStoredWeekUsage still swallows read/parse errors and silently resets the usage bucket. That remains a no-silent-fallback violation.
Suggested fix:
} catch (err) {
const msg = err instanceof Error ? err.message : "Failed to read weekly usage";
console.warn("[open-codesign] failed to read weekly usage:", msg);
get().pushToast({ variant: "error", title: tr("errors.storageFailed"), description: msg });
return fresh;
}| const tooltipParts: string[] = [t('topbar.spendTooltip')]; | ||
| if (lastUsage) { | ||
| tooltipParts.push( | ||
| `Last: ${lastUsage.inputTokens.toLocaleString()} in / ${lastUsage.outputTokens.toLocaleString()} out · $${formatCostUsd(lastUsage.costUsd)}`, |
There was a problem hiding this comment.
This tooltip segment is hardcoded English and bypasses i18n (Last: ... in / ... out).
Suggested fix:
tooltipParts.push(
t(topbar.lastUsageTooltip, {
input: lastUsage.inputTokens.toLocaleString(),
output: lastUsage.outputTokens.toLocaleString(),
cost: formatCostUsd(lastUsage.costUsd),
}),
);Replaces the silent catch on weekUsage persistence with an error path: persistWeekUsage now returns the error message, callers log via console.warn and push an error toast (i18n key errors.storageFailed). In-memory weekUsage stays intact so the session keeps tracking even when localStorage is full or disabled. Addresses Codex Major review on PR #47. Signed-off-by: hqhq1025 <1506751656@qq.com>
58da137 to
7fe58d4
Compare
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback on weekly usage read masks storage/parse errors and violates the no-silent-fallback constraint, so data-loss/corruption conditions are hidden from users. Evidence
apps/desktop/src/renderer/src/store.ts:173
Suggested fix:} catch (err) { const message = err instanceof Error ? err.message : 'Unknown error'; throw new Error(`Failed to load weekly usage from localStorage: ${message}`); }
-
[Minor] New tooltip segment is hardcoded English (
Last: ... in / ... out) and bypasses i18n, creating mixed-language UI in zh-CN. Evidenceapps/desktop/src/renderer/src/components/TopBar.tsx:48
Suggested fix:tooltipParts.push( t('topbar.lastUsageTooltip', { input: lastUsage.inputTokens.toLocaleString(), output: lastUsage.outputTokens.toLocaleString(), cost: formatCostUsd(lastUsage.costUsd), }), );
Summary
- Review mode: follow-up after new commits
- 2 issues found in latest diff-added/modified lines.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| outputTokens: parsed.outputTokens, | ||
| costUsd: parsed.costUsd, | ||
| }; | ||
| } catch { |
There was a problem hiding this comment.
[Major] Silent fallback still present: this catch returns a zeroed bucket and hides storage/parse failures. That violates the no-silent-fallback rule (errors must surface in UI or throw with context).
Suggested fix:
} catch (err) {
const message = err instanceof Error ? err.message : 'Unknown error';
throw new Error(`Failed to load weekly usage from localStorage: ${message}`);
}| const tooltipParts: string[] = [t('topbar.spendTooltip')]; | ||
| if (lastUsage) { | ||
| tooltipParts.push( | ||
| `Last: ${lastUsage.inputTokens.toLocaleString()} in / ${lastUsage.outputTokens.toLocaleString()} out · $${formatCostUsd(lastUsage.costUsd)}`, |
There was a problem hiding this comment.
[Minor] This tooltip text is hardcoded English and bypasses i18n (Last: ... in / ... out), causing mixed-language UI in non-English locales.
Suggested fix:
tooltipParts.push(
t('topbar.lastUsageTooltip', {
input: lastUsage.inputTokens.toLocaleString(),
output: lastUsage.outputTokens.toLocaleString(),
cost: formatCostUsd(lastUsage.costUsd),
}),
);Replaces the silent catch on weekUsage persistence with an error path: persistWeekUsage now returns the error message, callers log via console.warn and push an error toast (i18n key errors.storageFailed). In-memory weekUsage stays intact so the session keeps tracking even when localStorage is full or disabled. Addresses Codex Major review on PR #47. Signed-off-by: hqhq1025 <1506751656@qq.com>
7fe58d4 to
2d4092f
Compare
There was a problem hiding this comment.
Findings
-
[Major] Silent fallback on weekly usage read still masks storage/parse errors and violates the no-silent-fallback constraint; corruption/availability issues are hidden instead of surfaced. Evidence
apps/desktop/src/renderer/src/store.ts:244
Suggested fix:function readStoredWeekUsage(now: Date): WeekUsage { const fresh: WeekUsage = { isoWeek: isoWeekKey(now), inputTokens: 0, outputTokens: 0, costUsd: 0, }; if (typeof window === 'undefined') return fresh; const raw = window.localStorage.getItem(WEEK_USAGE_STORAGE_KEY); if (!raw) return fresh; let parsed: Partial<WeekUsage>; try { parsed = JSON.parse(raw) as Partial<WeekUsage>; } catch (err) { const msg = err instanceof Error ? err.message : String(err); throw new Error(`Failed to parse weekly usage from localStorage: ${msg}`); } if ( typeof parsed.isoWeek !== 'string' || typeof parsed.inputTokens !== 'number' || typeof parsed.outputTokens !== 'number' || typeof parsed.costUsd !== 'number' ) { throw new Error('Invalid weekly usage payload in localStorage'); } if (parsed.isoWeek !== fresh.isoWeek) return fresh; return parsed as WeekUsage; }
-
[Minor] Last-usage tooltip text is still hardcoded English and bypasses i18n, causing mixed-language UI in zh-CN. Evidence
apps/desktop/src/renderer/src/components/TopBar.tsx:48
Suggested fix:if (lastUsage) { tooltipParts.push( t('topbar.lastUsageTooltip', { input: lastUsage.inputTokens.toLocaleString(), output: lastUsage.outputTokens.toLocaleString(), cost: formatCostUsd(lastUsage.costUsd), }), ); }
Summary
- Review mode: follow-up after new commits
- 2 issues found in latest diff-added/modified lines.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| costUsd: parsed.costUsd, | ||
| }; | ||
| } catch (err) { | ||
| console.warn('[open-codesign] failed to read weekly usage from storage:', err); |
There was a problem hiding this comment.
[Major] This catch still silently falls back to a zeroed week bucket. That hides storage/parse failures and violates the no-silent-fallback rule.
Suggested fix:
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
throw new Error(`Failed to parse weekly usage from localStorage: ${msg}`);
}| const tooltipParts: string[] = [t('topbar.spendTooltip')]; | ||
| if (lastUsage) { | ||
| tooltipParts.push( | ||
| `Last: ${lastUsage.inputTokens.toLocaleString()} in / ${lastUsage.outputTokens.toLocaleString()} out · $${formatCostUsd(lastUsage.costUsd)}`, |
There was a problem hiding this comment.
[Minor] Tooltip segment is hardcoded English (Last: ... in / ... out) and bypasses i18n, so zh-CN UI becomes mixed language.
Suggested fix:
tooltipParts.push(
t('topbar.lastUsageTooltip', {
input: lastUsage.inputTokens.toLocaleString(),
output: lastUsage.outputTokens.toLocaleString(),
cost: formatCostUsd(lastUsage.costUsd),
}),
);…ar tooltip (PR #47 codex follow-up)
There was a problem hiding this comment.
Findings
- [Major] Non-finite usage values can corrupt weekly totals and persist silently — new usage parsing accepts any
number(includingNaN/Infinity), and storage validation also accepts non-finite numbers, so a bad provider/IPC payload can poison counters across sessions. Evidenceapps/desktop/src/renderer/src/store.ts:379,apps/desktop/src/renderer/src/store.ts:293,apps/desktop/src/renderer/src/store.ts:230
Suggested fix:function toFiniteNonNegative(value: unknown): number { return typeof value === 'number' && Number.isFinite(value) && value > 0 ? value : 0; } const usage: UsageSnapshot = { inputTokens: toFiniteNonNegative(result.inputTokens), outputTokens: toFiniteNonNegative(result.outputTokens), costUsd: toFiniteNonNegative(result.costUsd), }; if ( typeof parsed.isoWeek !== 'string' || !Number.isFinite(parsed.inputTokens) || !Number.isFinite(parsed.outputTokens) || !Number.isFinite(parsed.costUsd) ) { warnReason = 'weekly usage entry has unexpected shape'; }
Summary
- Review mode: follow-up after new commits
- 1 issue found in latest modified lines.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation). Suggest adding a Vitest case where usage fields are
NaN/Infinityand asserting store values remain finite.
open-codesign Bot
| generationStage: 'done' as GenerationStage, | ||
| })); | ||
| const usage: UsageSnapshot = { | ||
| inputTokens: typeof result.inputTokens === 'number' ? result.inputTokens : 0, |
There was a problem hiding this comment.
[Major] typeof ... === "number" accepts NaN/Infinity. That can persist corrupted weekly usage. Please normalize to finite non-negative values (e.g., Number.isFinite) before accumulating/persisting, and validate persisted payloads with Number.isFinite too.
…ruption A bad provider/IPC payload (NaN, Infinity, or negative) would silently poison weekUsage totals across sessions, since the previous guard only checked typeof === 'number'. Tighten validation with Number.isFinite + non-negative at both the parse-side (IPC payload → state) and the storage-load side (rehydrate from localStorage). On rejection: drop the bad value, console.warn, and surface a toast via the existing weekly-usage error UI surface (errors.weekUsageInvalid). Addresses PR #47 codex Major.
|
Addressed Codex [Major] on Fix in 9a4f69d:
|
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: follow-up after new commits
- No issues found in added/modified lines for the latest head commit.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.- Residual risk: no Playwright coverage in this PR for iframe refresh behavior and week-usage persistence/reset UX.
Testing
- Not run (automation)
open-codesign Bot
The preview iframe used `key={previewHtml.length}` which forced React to
fully unmount and re-mount the iframe whenever the HTML grew or shrank by a
single character. During streaming (and even on incremental edits via
applyComment) this discarded scroll position, re-parsed the entire document,
and re-ran the overlay script on every chunk — the dominant cause of the
"preview is laggy" feedback.
- Drop the length-based `key`; prop changes already trigger srcdoc swap.
- useMemo `buildSrcdoc(previewHtml)` so the regex-heavy strip+inject only
runs when the source HTML actually changes (toast/theme renders no longer
re-execute it).
Signed-off-by: hqhq1025 <1506751656@qq.com>
…response The TopBar BYOK badge has been hard-coded to "$0.00 this week" since the shell landed — the generate IPC already returned `inputTokens`, `outputTokens`, and `costUsd` (via @open-codesign/core), but the renderer store discarded everything except `artifacts` and `message`, and the badge read no usage state at all. Result: real spend was never visible. This wires the response end-to-end: - Type the preload `generate` / `applyComment` results as `GenerateResponse` (artifacts + message + usage triple) so the store consumes a real shape. - Add `lastUsage` and `weekUsage` to the store. `applyGenerateSuccess` and the `applyInlineComment` happy path now record per-call usage and accumulate it into the active ISO-week bucket. - Persist `weekUsage` in localStorage under `open-codesign:week-usage`. The bucket auto-resets when the ISO week rolls over (no migration needed). Matches the existing localStorage pattern for theme; SQLite-backed history can absorb this later without schema churn. - TopBar reads `weekUsage.costUsd` and `lastUsage` from the store. New `formatCostUsd` helper renders sub-cent values at 4 decimals so users see a non-zero number after the first request, then collapses to 2 decimals once spend crosses a cent. - Tooltip now surfaces last-call token counts; i18n strings updated to drop the "coming soon" disclaimer (en + zh-CN). - Tests: store reducer accepts usage payload (with and without fields), accumulator handles week rollover and clamps negative deltas, and formatter covers zero / sub-cent / multi-cent paths. Signed-off-by: hqhq1025 <1506751656@qq.com>
Replaces the silent catch on weekUsage persistence with an error path: persistWeekUsage now returns the error message, callers log via console.warn and push an error toast (i18n key errors.storageFailed). In-memory weekUsage stays intact so the session keeps tracking even when localStorage is full or disabled. Addresses Codex Major review on PR #47. Signed-off-by: hqhq1025 <1506751656@qq.com>
…back (PR #47 codex Major)
…ar tooltip (PR #47 codex follow-up)
…ruption A bad provider/IPC payload (NaN, Infinity, or negative) would silently poison weekUsage totals across sessions, since the previous guard only checked typeof === 'number'. Tighten validation with Number.isFinite + non-negative at both the parse-side (IPC payload → state) and the storage-load side (rehydrate from localStorage). On rejection: drop the bad value, console.warn, and surface a toast via the existing weekly-usage error UI surface (errors.weekUsageInvalid). Addresses PR #47 codex Major.
9a4f69d to
d828162
Compare
|
Rebased onto latest origin/main (HEAD d828162). Conflicts in PreviewPane.tsx (memoized srcDoc merged with new viewport/zoom/PhoneFrame logic) and store.ts (token usage tracking + NaN/Infinity guards merged with main's persistDesignState/loadDesigns wiring). pnpm typecheck, lint, and full test suite (578 tests) all green. Ready for re-review. |
There was a problem hiding this comment.
Findings
- [Blocker]
applyGenerateSuccessis called with wrong argument order inrunGenerate, so successful generations can fail to finalize state updates (messages/preview/isGenerating/tokens) and may leave the UI stuck. Evidenceapps/desktop/src/renderer/src/store.ts:667.
Suggested fix:applyGenerateSuccess( set, get, generationId, payload.prompt, result as { artifacts: Array<{ type?: string; content: string }>; message: string; inputTokens?: number; outputTokens?: number; costUsd?: number; }, );
Summary
- Review mode: follow-up after new commits
- 1 blocker found in modified lines that affects generation completion flow in the renderer store.
docs/VISION.md: Not found in repo/docs.docs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| @@ -494,11 +665,18 @@ async function runGenerate( | |||
| advanceStageIfCurrent(get, set, generationId, 'parsing'); | |||
| advanceStageIfCurrent(get, set, generationId, 'rendering'); | |||
| applyGenerateSuccess( | |||
There was a problem hiding this comment.
[Blocker] The callsite argument order is incorrect here (get, set, get, ...) for applyGenerateSuccess(set, get, generationId, prompt, result). This breaks success-path state finalization.
Suggested fix:
applyGenerateSuccess(
set,
get,
generationId,
payload.prompt,
result as {
artifacts: Array<{ type?: string; content: string }>;
message: string;
inputTokens?: number;
outputTokens?: number;
costUsd?: number;
},
);Leftover from #47 rebase merge: the call site passed `get, set, get, ...` where the function takes `set, get, ...`. Resulted in a typecheck error (TS2554: Expected 5 arguments, but got 6) that broke pnpm typecheck on main.
Summary
Two user-reported bugs from the v0.x feedback round:
A. Preview iframe was laggy.
PreviewPanesetkey={previewHtml.length}on the iframe, which forced React to fully unmount and re-mount it whenever the HTML grew or shrank by even one character. During streaming and on everyapplyCommentrevision the entire document was re-parsed from scratch, scroll position was discarded, and the overlay script re-attached on each chunk.buildSrcdoc()(regex-heavy) also ran on every unrelated render (toast / theme / errors).B. TopBar always showed
$0.00 this week. The badge was hard-coded. The generate IPC already returnedinputTokens/outputTokens/costUsdfrom@open-codesign/core, but the renderer store cast the result to{ artifacts, message }and threw the rest away. There was no end-to-end wire.Changes
PreviewPane: drop the length-basedkey, memoizebuildSrcdoc(previewHtml).generate/applyCommentreturns asGenerateResponse.lastUsageandweekUsage; bothapplyGenerateSuccessandapplyInlineCommentnow record per-call usage and accumulate it into the active ISO-week bucket.weekUsageto localStorage underopen-codesign:week-usage. Auto-resets when the ISO week rolls over (no migration). Matches existing theme persistence pattern; SQLite-backed history can absorb this later without schema churn.weekUsage.costUsdand showslastUsagein the tooltip. NewformatCostUsdshows 4 decimals for sub-cent values so the user sees a non-zero number after their first request.Root causes
key={previewHtml.length}is the smoking gun — it forced full iframe remounts on every char change in the HTML.applyGenerateSuccessonly consumedartifacts+message, andByokBadgenever read store state.Tests
store.test.ts: usage payload accepted (full + missing-fields fallback), accumulator handles week rollover and clamps negative deltas. 5 new test cases.TopBar.test.ts: new —formatCostUsdcovers zero / sub-cent / multi-cent paths.pnpm testgreen;pnpm typecheckclean;pnpm lintclean (0 errors, 7 pre-existing complexity warnings unchanged).PRINCIPLES §5b
WeekUsage.isoWeekcarries the bucket key; future migration to SQLite is a one-shot read of localStorage.weekUsagein store), no plumbing through preload beyond a typed return.Known follow-ups
contentDocument.body.innerHTMLpatch that preserves scroll. Out of scope here — needs the sandbox-runtime research thread to land first (docs/RESEARCH_QUEUE.md).weekUsageinto ausage_bucketstable keyed by ISO week;accumulateWeekUsageis already pure and reusable.pi-aiproviders that don't return usage will show$0.00; a chars/4 fallback estimator can be added behind a feature flag if users complain.cc: do not merge until Codex bot review reports No findings.