feat(desktop): named loading stages (sending/thinking/streaming/parsing/rendering)#37
feat(desktop): named loading stages (sending/thinking/streaming/parsing/rendering)#37
Conversation
Replace opaque spinner with stage-labelled feedback bar (sending → thinking → parsing → rendering → done). Implements §9 P3 + Step 4 of UX master plan (docs/research/18-ux-master-plan.md). - store: add GenerationStage type, generationStage and streamingTokenCount state fields; sendPrompt advances through all stages; cancel/error reset to idle/error - LoadingState: overlay stage label + lucide icon + native <progress> bar on top of existing skeleton; accepts optional props for testing - i18n: loading.stage.* keys in en + zh-CN - tests: store.generationStage.test.ts (5 stage transition tests), preview/LoadingState.test.ts (stage list + i18n coverage) Signed-off-by: hqhq1025 <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major]
streamingstage is never entered, so the new stage model is incomplete and the token badge path is effectively dead UI — evidenceapps/desktop/src/renderer/src/store.ts:232,apps/desktop/src/renderer/src/store.ts:393,apps/desktop/src/renderer/src/preview/LoadingState.tsx:95
Suggested fix:async function runGenerate(get: GetState, set: SetState, generationId: string, payload: Parameters<CodesignApi['generate']>[0]) { advanceStageIfCurrent(get, set, generationId, 'thinking'); advanceStageIfCurrent(get, set, generationId, 'streaming'); const result = await window.codesign!.generate(payload); advanceStageIfCurrent(get, set, generationId, 'parsing'); advanceStageIfCurrent(get, set, generationId, 'rendering'); applyGenerateSuccess(set, generationId, result as { artifacts: Array<{ content: string }>; message: string }); }
-
[Minor] Hardcoded English unit string (
"tokens") bypasses i18n in a localized UI — evidenceapps/desktop/src/renderer/src/preview/LoadingState.tsx:97
Suggested fix:<span className="font-mono text-xs text-[var(--color-text-tertiary)]"> {t('loading.tokens', { count: tokenCount })} </span>
-
[Minor] Stage-transition test does not assert
streaming, so the main regression in this PR is not caught by tests — evidenceapps/desktop/src/renderer/src/store.generationStage.test.ts:81
Suggested fix:expect(stagesBefore).toContain('streaming');
Summary
- Review mode: initial. Found 1 major and 2 minor issues in the new loading-stage implementation. Main risk is functional:
streamingis declared and rendered but never reached, so users never see that stage and token-related UI remains unexercised.
Testing
- Not run (automation)
open-codesign Bot
| const userMessage: ChatMessage = { role: 'user', content: prompt }; | ||
| set((s) => ({ | ||
| messages: [...s.messages, userMessage], | ||
| messages: [...s.messages, { role: 'user', content: request.prompt }], |
There was a problem hiding this comment.
streaming is never set in this flow (only thinking -> parsing -> rendering -> done), so the new stage model is incomplete and the streaming-specific UI path is never reached.
Suggested minimal fix:
advanceStageIfCurrent(get, set, generationId, thinking);
advanceStageIfCurrent(get, set, generationId, streaming);
const result = await window.codesign!.generate(payload);| <span className="stage-label">{t(`loading.stage.${activeStage}`)}</span> | ||
| {activeStage === 'streaming' && tokenCount > 0 && ( | ||
| <span className="font-mono text-xs text-[var(--color-text-tertiary)]"> | ||
| {tokenCount} tokens |
There was a problem hiding this comment.
This introduces a hardcoded English string (tokens) in localized UI.
Suggested fix:
{t(loading.tokens, { count: tokenCount })}and add loading.tokens in both locale files.
| // Must pass through sending, thinking (at least), then parsing, rendering, done | ||
| expect(stagesBefore).toContain('sending'); | ||
| expect(stagesBefore).toContain('thinking'); | ||
| expect(stagesBefore).toContain('parsing'); |
There was a problem hiding this comment.
This test covers sending/thinking/parsing/rendering/done but not streaming, so the missing stage transition is not caught.
Suggested fix:
expect(stagesBefore).toContain(streaming);advanceStageIfCurrent now moves to 'streaming' right before the window.codesign.generate() await, so the LoadingState UI shows "receiving response" during the IPC round-trip instead of staying stuck in 'thinking'. The token-badge branch in LoadingState.tsx is no longer dead code. Updated the 5-stage test to also assert the 'streaming' transition. Signed-off-by: hqhq1025 <[email protected]>
There was a problem hiding this comment.
Findings
- [Minor] Hardcoded English unit in a localized loading UI causes mixed-language output (
"tokens") and bypasses locale control, evidenceapps/desktop/src/renderer/src/preview/LoadingState.tsx:97
Suggested fix:// LoadingState.tsx <span className="font-mono text-xs text-[var(--color-text-tertiary)]"> {t('loading.tokens', { count: tokenCount })} </span>
// packages/i18n/src/locales/en.json "loading": { "stage": { ... }, "tokens_one": "{{count}} token", "tokens_other": "{{count}} tokens" }
// packages/i18n/src/locales/zh-CN.json "loading": { "stage": { ... }, "tokens": "{{count}} 个 token" }
Summary
- Review mode: follow-up after new commits. One previously reported localization issue is still present on the latest head; no new blocker/major regressions found in this update.
Testing
- Not run (automation)
open-codesign Bot
| <span className="stage-label">{t(`loading.stage.${activeStage}`)}</span> | ||
| {activeStage === 'streaming' && tokenCount > 0 && ( | ||
| <span className="font-mono text-xs text-[var(--color-text-tertiary)]"> | ||
| {tokenCount} tokens |
There was a problem hiding this comment.
Hardcoded English text (tokens) bypasses i18n in a localized UI.
Suggested minimal fix:
{t('loading.tokens', { count: tokenCount })}and add locale keys in en.json / zh-CN.json.
Replaces hardcoded English 'tokens' with i18n key so Chinese locale no longer shows mixed-language output. Uses i18next plural suffixes (_one/_other) for English; Chinese has a single form. Addresses Codex review on PR #37.
There was a problem hiding this comment.
Findings
- No high-confidence issues found in added/modified lines.
Summary
- Review mode: follow-up after new commits. Previously reported i18n token-label issue is resolved on commit
227376a6774e501e99bb04b7e28a6e15e10f0497; no new blocker/major/minor issues found in the latest diff. - Residual context risk:
docs/VISION.mdanddocs/PRINCIPLES.mdare Not found in repo/docs in this checkout, so policy validation relied onCLAUDE.mdand changed files only.
Testing
- Not run (automation)
open-codesign Bot
1 participant
Summary
docs/research/18-ux-master-plan.mdGenerationStagetype added to store (idle | sending | thinking | streaming | parsing | rendering | done | error);streamingTokenCountfield added for future streaming supportsendPromptadvances through stages: sending → thinking (before IPC) → parsing → rendering → done (or error on failure); cancel resets to idleLoadingStatecomponent now readsgenerationStagefrom store and renders a stage label + lucide icon + native<progress>bar on top of the existing skeletonloading.stage.*added in bothenandzh-CNCompatibility / upgrade / bloat / elegance
GenerationStageis an exported type — consumers can extend display logic without touching the storesendPromptto keep cognitive complexity within the 15-limit ruleTest plan
store.generationStage.test.ts— 5 unit tests: idle start, full stage sequence, error path, streamingTokenCount reset, sending-is-first-stage assertionpreview/LoadingState.test.ts— stage list completeness + i18n key coverage for all 6 stagespnpm -r typecheckcleanpnpm lintclean (no new errors)