Skip to content

feat(desktop): rewrite chain (chat + comment + status + topbar) end-to-end#46

Closed
hqhq1025 wants to merge 13 commits intomainfrom
wt/chain-rewrite
Closed

feat(desktop): rewrite chain (chat + comment + status + topbar) end-to-end#46
hqhq1025 wants to merge 13 commits intomainfrom
wt/chain-rewrite

Conversation

@hqhq1025
Copy link
Copy Markdown
Collaborator

@hqhq1025 hqhq1025 commented Apr 18, 2026

Why

Day-zero users hit four chain-level problems on first use (full audit in docs/research/23-chain-audit.md):

  1. Empty ```html ``` fence rendered as a placeholder assistant bubble right under the user message — the artifact had been extracted but the empty fence was left behind in collected.text.
  2. Sidebar mixes tooling (attach files / link design system / reference URL field) and chat history in one column. The tools block ate ~30% of vertical space.
  3. "Click any element to comment" was rendered as a floating overlay inside the iframe, so it covered the design's nav on every preview.
  4. No visible iterate / regenerate / branch entry point. No model / token info anywhere.

Two screenshots from a follow-up review surfaced three more issues addressed in round 2:

  1. Edit p: 重写 user message + 已应用。 assistant bubble polluted the chat for every inline comment.
  2. The inline comment composer floated bottom-right with a raw XPath header and a 50%-opacity 'Applying...' button that looked broken.
  3. The TopBar still surfaced 'Generating your design...' as the breadcrumb, taking over the design's identity even though the PreviewToolbar already owns that progress signal.

What changed

┌────────────────────────────────────────────────────────────────────┐
│ TopBar  Logo "Open CoDesign" preview / Untitled  · provider · model│
│                                            [Regen][Reuse][Branch]   │
├──────────────┬─────────────────────────────────────────────────────┤
│ ToolsPanel   │ PreviewToolbar                                      │
│ [📎][🗂][🔗]  │ [stage pill]  click-to-comment hint  …  [Export ▾]  │
│              ├─────────────────────────────────────────────────────┤
│ ChatHistory  │           iframe preview                            │
│  user        │   ┌──────────────────────────────────────────────┐  │
│  assistant   │   │ clicked element                              │  │
│   • text     │   ├──────────────────────────────────────────────┤  │
│   • [💬 chip]│   │ InlineCommentPopover (anchored, auto-flip)   │  │
│   • [redo]   │   │  Editing heading · "Pricing"                  │  │
│     [copy]   │   │  [textarea]                                   │  │
│     [snap]   │   │            [Cancel]   [⟳ Applying…]          │  │
├──────────────┤   └──────────────────────────────────────────────┘  │
│ Composer     │                                                     │
└──────────────┴─────────────────────────────────────────────────────┘

Commits in this PR

SHA Subject
cb6d7b4 fix(artifacts): drop empty code fences after artifact extraction
8aaf310 fix(desktop): suppress system status text as assistant message
92d37d5 feat(desktop): element-label util + friendly comment target
278e5ee refactor(desktop): split Sidebar into ToolsPanel + ChatHistory + Composer
28e21b6 feat(desktop): generation-stage pill, preview-hint relocation, TopBar iterate row
e4ba2ff feat(desktop): anchored InlineCommentPopover with positioning + highlight
ec1eccb fix(ui): InlineCommentPopover a11y polish — semantic section + ref-based focus
8ecf51f refactor(desktop): TopBar layout — brand cluster + design name breadcrumb

Comment & TopBar fixes (added round 2)

Driven by two new screenshots from the user review:

  • A. Empty-fence assistant bubble + 'Applied.' system textcb6d7b4 strips orphan ```html\n``` pairs in runModel; 8aaf310 skips pushing the assistant bubble in applyInlineComment when the model returned no prose. Success now shows a transient toast (notifications.inlineCommentApplied).
  • B. Edit p: 重写 synthesized user message → store applyInlineComment no longer pushes user/assistant messages. Instead appliedComments records (per-message anchor + getElementLabel + comment) render as small chips inside the assistant bubble (AssistantMessage). The chat stays clean.
  • C. Bottom-right floater with XPath → new InlineCommentPopover anchors to the clicked rect via computeAnchoredPosition (auto-flip above when below would overflow, clamp to safe left margin). Header now reads Editing heading · "…" instead of /main[1]/section[3]/.../p[1].
  • D. Element-label utilapps/desktop/src/renderer/src/lib/element-label.ts maps tag → role (heading / paragraph / button / image / icon / list / section / …) plus a 30-char text preview. Six unit tests cover roles, fallback, whitespace, truncation, missing-text, and combined display.
  • E. Loading button + scroll/highlight → spinner + 'Applying…' + min-width 112px so it never reflows; disabled colour is the muted token (not opacity-40); cancel is now a clear ghost button. On success the previous rect is highlighted with a 1.2s pulsing accent ring drawn over the iframe.
  • F. TopBar redesignWordmark size="lg" (20px mark to feel close to the requested 24px in a 44px header); breadcrumb shows Untitled / 未命名设计 instead of the generation status, so the design's identity stays visible while the PreviewToolbar pill carries progress.

i18n

New keys in en.json + zh-CN.json:

  • comment.{chip, target, applyButton, applyButtonBusy, cancel}
  • notifications.inlineCommentApplied
  • topbar.{brandBadge, designUntitled}

Test plan

  • pnpm --filter @open-codesign/artifacts teststripEmptyCodeFences cases (empty / real content / no language tag / mixed orphans)
  • pnpm --filter @open-codesign/desktop test — 140/140 (added applyInlineComment no-bubble case, getElementLabel coverage, computeAnchoredPosition flip/clamp/centre cases)
  • pnpm typecheck — clean across workspace
  • pnpm lint — 0 errors (8 pre-existing cognitive-complexity warnings, untouched)
  • Manual: click element → popover anchors below (or above near bottom) → submit → spinner → close → 1.2s ring on the original rect → assistant bubble shows [💬 heading · "Pricing" — make it bolder] chip; no Edit p: user bubble appears.

PRINCIPLES checklist

  • Compatibility — no IPC schema changes; appliedComments is renderer-only state; existing chat history shape unchanged.
  • UpgradeabilitygetElementLabel and computeAnchoredPosition are exported pure functions for future reuse (mobile preview, snapshot diff).
  • No bloat — no new deps. @floating-ui/react was considered (mentioned in scope) but rejected: a 30-line computeAnchoredPosition covers the auto-flip + clamp need and saves the install-size budget.
  • Elegance — comments anchor to the assistant bubble that owns them instead of fabricating user turns; the canvas update IS the confirmation, surfaced by a 1.2s ring + a quiet toast.

Deferred

  • Real @floating-ui/react integration. The simple anchor handles the current single-popover-on-iframe use case; introduce floating-ui when (a) the popover needs portaling outside the iframe-clip container, or (b) middleware like arrow / shift add real value.
  • Sending a HIGHLIGHT message into the iframe to flash the actual element. The current implementation draws the ring over the previous rect, which is correct for the same selection in the same render but won't survive an artifact swap. A parent→iframe overlay extension is a separate change.
  • Per-design names (the breadcrumb shows 'Untitled' / '未命名设计' until naming lands).

@hqhq1025 hqhq1025 changed the title feat(desktop): rewrite input→processing→reply chain (assistant bubble redesign + tools/chat split + status pill + iterate) feat(desktop): rewrite chain (chat + comment + status + topbar) end-to-end Apr 18, 2026
hqhq1025 added a commit that referenced this pull request Apr 18, 2026
Token discipline pass on the three onboarding screens — all raw px sizes
swapped for the equivalent token utility, label/heading recipes aligned
across the flow, motion-safe variants applied to hover translates, and
chip control sized to --size-control-sm for visual rhythm with the input.

- index.tsx Stepper: drop raw text-[11px], use --text-2xs +
  --tracking-label; add screen-reader step count, motion-safe transition.
- Welcome: title bumped to --text-xl with --tracking-heading;
  subtitle/captions on token recipe; PathButton hover translate gated by
  motion-safe + active state for press feedback.
- ChooseModel: heading/description on shared recipe; ModelPicker chips
  bumped to h-[var(--size-control-sm)] (32 px) so the chip row matches
  the 40 px input rhythm; chips carry aria-pressed; cost/baseUrl notes
  on --text-xs / --leading-ui.

No new strings, no new tokens, no layout/visual restructuring — the
purpose is to align this surface with the same recipes Settings #45 and
the chain-rewrite #46 already converged on, so dark-mode + zh-CN behave
predictably.

PRINCIPLES:
- Compatibility: render-only changes, no schema/IPC touch.
- Upgradeability: tokens centralize for future theme work.
- No bloat: -8 LOC net.
- Elegance: removes raw-px inconsistencies the audit doc flagged.

Signed-off-by: hqhq1025 <1506751656@qq.com>
hqhq1025 added a commit that referenced this pull request Apr 18, 2026
Token discipline pass on the three onboarding screens — all raw px sizes
swapped for the equivalent token utility, label/heading recipes aligned
across the flow, motion-safe variants applied to hover translates, and
chip control sized to --size-control-sm for visual rhythm with the input.

- index.tsx Stepper: drop raw text-[11px], use --text-2xs +
  --tracking-label; add screen-reader step count, motion-safe transition.
- Welcome: title bumped to --text-xl with --tracking-heading;
  subtitle/captions on token recipe; PathButton hover translate gated by
  motion-safe + active state for press feedback.
- ChooseModel: heading/description on shared recipe; ModelPicker chips
  bumped to h-[var(--size-control-sm)] (32 px) so the chip row matches
  the 40 px input rhythm; chips carry aria-pressed; cost/baseUrl notes
  on --text-xs / --leading-ui.

No new strings, no new tokens, no layout/visual restructuring — the
purpose is to align this surface with the same recipes Settings #45 and
the chain-rewrite #46 already converged on, so dark-mode + zh-CN behave
predictably.

PRINCIPLES:
- Compatibility: render-only changes, no schema/IPC touch.
- Upgradeability: tokens centralize for future theme work.
- No bloat: -8 LOC net.
- Elegance: removes raw-px inconsistencies the audit doc flagged.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] Removed translation keys break existing UI copy at runtime — the diff deletes key groups still referenced across the app (packages/i18n/src/locales/en.json:1, packages/i18n/src/locales/zh-CN.json:1; consumers include apps/desktop/src/renderer/src/components/ThemeToggle.tsx:12, apps/desktop/src/renderer/src/components/Settings.tsx:376, apps/desktop/src/renderer/src/views/hub/ExamplesTab.tsx:52, apps/desktop/src/renderer/src/components/Toast.tsx:76). This will surface raw i18n keys in shipped UI.
    Suggested fix:

    {
      "common": {
        "dismissNotification": "Dismiss notification"
      },
      "theme": {
        "toggleAria": "Toggle theme",
        "switchToLight": "Switch to light",
        "switchToDark": "Switch to dark"
      },
      "disabledReason": {
        "typePromptToSend": "..."
      },
      "examples": {
        "title": "..."
      }
    }
  • [Major] Hardcoded hex colors violate token-only UI constraint — stage pill colors are literal hex values instead of packages/ui tokens (apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:22). This conflicts with the project’s hard constraint and will fragment theming.
    Suggested fix:

    const STAGE_TONE: Record<GenerationStage, string> = {
      parsing:
        'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
      rendering:
        'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
      done:
        'bg-[var(--color-success-soft)] text-[var(--color-success)] border-[var(--color-success-muted)]',
      error:
        'bg-[var(--color-danger-soft)] text-[var(--color-danger)] border-[var(--color-danger-muted)]',
    };
  • [Major] Inline comment popover can render off-screen on the right edge — horizontal positioning only clamps left and never caps right (apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:153). Users clicking elements near the iframe’s right edge can lose access to the composer controls.
    Suggested fix:

    export function computeAnchoredPosition(rect: ElementSelectionRect, cardHeight: number, containerHeight: number | null): AnchoredPosition {
      const elementBottom = rect.top + rect.height;
      const flipBelowToAbove =
        containerHeight !== null && elementBottom + cardHeight + POPOVER_GAP > containerHeight;
      const top = flipBelowToAbove
        ? Math.max(POPOVER_GAP, rect.top - cardHeight - POPOVER_GAP)
        : elementBottom + POPOVER_GAP;
    
      const desiredLeft = rect.left + rect.width / 2 - POPOVER_WIDTH / 2;
      const maxLeft = Math.max(POPOVER_GAP, rect.left + rect.width - POPOVER_GAP - POPOVER_WIDTH);
      const left = Math.min(maxLeft, Math.max(POPOVER_GAP, desiredLeft));
      return { top, left };
    }

Summary

  • Review mode: initial
  • 3 issues found: 1 blocker (i18n key removals causing runtime key leakage), 2 majors (token-constraint violation, right-edge popover overflow).
  • Residual risk: no added coverage for i18n key integrity across locales.

Testing

  • Not run (automation)

open-codesign Bot

"error": "Failed"
}
},
"assistantMessage": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en.json now removes several pre-existing key groups (common.dismissNotification, theme.*, disabledReason.*, examples.*) that are still referenced in renderer code. This introduces visible raw i18n keys at runtime. Please restore the removed keys (and mirror in zh-CN.json) until all call sites are migrated.

{
  "common": { "dismissNotification": "Dismiss notification" },
  "theme": {
    "toggleAria": "Toggle theme",
    "switchToLight": "Switch to light",
    "switchToDark": "Switch to dark"
  },
  "disabledReason": { "typePromptToSend": "..." },
  "examples": { "title": "..." }
}

'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
streaming:
'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
parsing: 'bg-[#FFF4E0] text-[#9A6A1B] border-[#F1CE8B]',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces hardcoded hex values (#FFF4E0, #9A6A1B, etc.) in renderer UI. Project constraints require token-driven values from packages/ui. Please switch these to semantic CSS tokens.

parsing:
  'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
rendering:
  'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
done:
  'bg-[var(--color-success-soft)] text-[var(--color-success)] border-[var(--color-success-muted)]',
error:
  'bg-[var(--color-danger-soft)] text-[var(--color-danger)] border-[var(--color-danger-muted)]',

const top = flipBelowToAbove
? Math.max(POPOVER_GAP, rect.top - cardHeight - POPOVER_GAP)
: elementBottom + POPOVER_GAP;
const desiredLeft = rect.left + rect.width / 2 - POPOVER_WIDTH / 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeAnchoredPosition clamps only the left boundary; it never clamps the right edge. For elements near the right side, popover content can render partially outside the preview container.

const desiredLeft = rect.left + rect.width / 2 - POPOVER_WIDTH / 2;
const maxLeft = Math.max(POPOVER_GAP, rect.left + rect.width - POPOVER_GAP - POPOVER_WIDTH);
const left = Math.min(maxLeft, Math.max(POPOVER_GAP, desiredLeft));

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Stage pill still uses hardcoded hex colors, violating token-only UI constraint — PreviewToolbar introduces literal #... values for parsing/rendering/done/error states (apps/desktop/src/renderer/src/components/PreviewToolbar.tsx:22). This breaks the project-wide token contract and can drift from theme tokens.
    Suggested fix:

    const STAGE_TONE: Record<GenerationStage, string> = {
      idle: 'bg-[var(--color-surface)] text-[var(--color-text-muted)] border-[var(--color-border-muted)]',
      sending: 'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
      thinking: 'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
      streaming: 'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
      parsing: 'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
      rendering: 'bg-[var(--color-warning-soft)] text-[var(--color-warning)] border-[var(--color-warning-muted)]',
      done: 'bg-[var(--color-success-soft)] text-[var(--color-success)] border-[var(--color-success-muted)]',
      error: 'bg-[var(--color-error-soft)] text-[var(--color-error)] border-[var(--color-error-muted)]',
    };
  • [Major] Inline comment popover can still overflow the right edge — horizontal positioning clamps only the left side and never caps left against container width (apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:153). This reproduces the same right-edge accessibility regression in follow-up commits.
    Suggested fix:

    export function computeAnchoredPosition(
      rect: ElementSelectionRect,
      cardHeight: number,
      containerHeight: number | null,
      containerWidth: number | null,
    ): AnchoredPosition {
      const elementBottom = rect.top + rect.height;
      const flipBelowToAbove =
        containerHeight !== null && elementBottom + cardHeight + POPOVER_GAP > containerHeight;
      const top = flipBelowToAbove
        ? Math.max(POPOVER_GAP, rect.top - cardHeight - POPOVER_GAP)
        : elementBottom + POPOVER_GAP;
    
      const desiredLeft = rect.left + rect.width / 2 - POPOVER_WIDTH / 2;
      const maxLeft = containerWidth === null
        ? Number.POSITIVE_INFINITY
        : Math.max(POPOVER_GAP, containerWidth - POPOVER_WIDTH - POPOVER_GAP);
      const left = Math.min(maxLeft, Math.max(POPOVER_GAP, desiredLeft));
      return { top, left };
    }
  • [Major] Applied-comment chips can attach to the wrong assistant bubble after regenerate — chips are keyed by assistantMessageIndex, but regenerateLast() mutates messages by popping assistant/user items without remapping or clearing appliedComments (apps/desktop/src/renderer/src/store.ts:542). This can make historical edit chips appear under unrelated future responses.
    Suggested fix:

    async regenerateLast() {
      if (get().isGenerating) return;
      const lastPromptInput = get().lastPromptInput;
      if (!lastPromptInput) return;
    
      const messages = [...get().messages];
      while (messages.length > 0 && messages.at(-1)?.role === 'assistant') messages.pop();
      if (messages.at(-1)?.role === 'user' && messages.at(-1)?.content === lastPromptInput.prompt) {
        messages.pop();
      }
    
      // Drop chips anchored to removed assistant messages to prevent index drift.
      const maxAssistantIndex = messages.length - 1;
      set({
        messages,
        appliedComments: get().appliedComments.filter((c) => c.assistantMessageIndex <= maxAssistantIndex),
        errorMessage: null,
      });
    
      await get().sendPrompt(lastPromptInput);
    }

Summary

  • Review mode: follow-up after new commits
  • 3 issues found (all Major).
  • Context docs check: docs/VISION.md and docs/PRINCIPLES.md are Not found in repo/docs in this checkout, so review used CLAUDE.md + current diff/files only.

Testing

  • Not run (automation)
  • Suggested additions: Vitest case for right-edge clamp in InlineCommentPopover.test.ts, and store test asserting appliedComments remains consistent after regenerateLast().

open-codesign Bot

'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
streaming:
'bg-[var(--color-accent-soft)] text-[var(--color-accent)] border-[var(--color-accent-muted)] animate-pulse',
parsing: 'bg-[#FFF4E0] text-[#9A6A1B] border-[#F1CE8B]',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded #... colors here violate the token-only UI constraint. Please switch these tones to semantic packages/ui CSS variables (warning/success/error families) so theme and design-system consistency is preserved.

const top = flipBelowToAbove
? Math.max(POPOVER_GAP, rect.top - cardHeight - POPOVER_GAP)
: elementBottom + POPOVER_GAP;
const desiredLeft = rect.left + rect.width / 2 - POPOVER_WIDTH / 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeAnchoredPosition still clamps only the left edge. For right-side selections, left can push the popover out of bounds. Please clamp against container width (max left = containerWidth - popoverWidth - gap) and add a right-edge test case.

const lastPromptInput = get().lastPromptInput;
if (!lastPromptInput) return;

const messages = [...get().messages];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regenerateLast() rewrites messages but does not update appliedComments (which are keyed by assistantMessageIndex). That index can drift and reattach chips to unrelated bubbles after regeneration. Please remap/remove stale chip entries when pruning message history.

hqhq1025 added a commit that referenced this pull request Apr 19, 2026
…ht edge

Addresses codex review on PR #46:
- Replace hardcoded hex colors in PreviewToolbar stage pill with new
  --color-stage-* tokens (sending/thinking/streaming/parsing/rendering/
  done/error). Tokens reference semantic colors so light/dark flip
  automatically.
- computeAnchoredPosition now accepts containerWidth and clamps the
  popover left so it never overflows the right edge of its container.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Applied-comment chips can attach to the wrong assistant bubble after regenerate — regenerateLast() removes trailing messages but never remaps or prunes appliedComments indices, so chips can drift to unrelated future responses, evidence apps/desktop/src/renderer/src/store.ts:542 and render path apps/desktop/src/renderer/src/components/Sidebar/AssistantMessage.tsx:28.
    Suggested fix:

    async regenerateLast() {
      if (get().isGenerating) return;
      const lastPromptInput = get().lastPromptInput;
      if (!lastPromptInput) return;
    
      const messages = [...get().messages];
      while (messages.length > 0 && messages.at(-1)?.role === 'assistant') messages.pop();
      if (messages.at(-1)?.role === 'user' && messages.at(-1)?.content === lastPromptInput.prompt) {
        messages.pop();
      }
    
      const survivingAssistantIndexes = new Set<number>();
      for (let i = 0; i < messages.length; i++) if (messages[i]?.role === 'assistant') survivingAssistantIndexes.add(i);
    
      set({
        messages,
        appliedComments: get().appliedComments.filter((c) => c.assistantMessageIndex === -1 || survivingAssistantIndexes.has(c.assistantMessageIndex)),
        errorMessage: null,
      });
    
      await get().sendPrompt(lastPromptInput);
    }
  • [Major] Silent fallback on clipboard copy hides runtime errors, violating the no-silent-fallback constraint — copy failures are swallowed with an empty catch and no surfaced error, evidence apps/desktop/src/renderer/src/components/Sidebar/AssistantMessage.tsx:36.
    Suggested fix:

    const pushToast = useCodesignStore((s) => s.pushToast);
    
    async function copyArtifact(): Promise<void> {
      if (!previewHtml) return;
      try {
        await navigator.clipboard.writeText(previewHtml);
        setCopied(true);
        window.setTimeout(() => setCopied(false), 1500);
      } catch (err) {
        const msg = err instanceof Error ? err.message : t('errors.unknown');
        pushToast({
          variant: 'error',
          title: t('notifications.copyFailed'),
          description: msg,
        });
      }
    }
  • [Major] New renderer UI introduces hardcoded values instead of UI tokens, violating the token-only constraint — examples include fixed text-[12px], text-[13px], w-[360px], and text-white, evidence apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:70, apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:76, apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:101, apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:116.
    Suggested fix:

    <section className="absolute z-10 w-[var(--size-stage-min)] max-w-[calc(100%-3rem)] ...">
    ...
    <div className="... text-[var(--text-xs)] ...">
    ...
    <textarea className="... text-[var(--text-sm)] leading-[var(--leading-body)] ..." />
    ...
    <button className="... text-[var(--text-xs)] text-[var(--color-on-accent)] ..." />

Summary

  • Review mode: follow-up after new commits
  • 3 issues found (all Major), all attributable to added/modified lines in this diff.
  • Context note: docs/VISION.md and docs/PRINCIPLES.md are Not found in repo/docs in this checkout.

Testing

  • Not run (automation)
  • Suggested tests: add a store test that validates appliedComments stays correctly anchored after regenerateLast(), and a component test for clipboard failure surfacing an error toast.

open-codesign Bot

Comment thread apps/desktop/src/renderer/src/store.ts Outdated
if (messages.at(-1)?.role === 'user' && messages.at(-1)?.content === lastPromptInput.prompt) {
messages.pop();
}
set({ messages, errorMessage: null });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regenerateLast() prunes trailing chat messages but leaves appliedComments untouched. Because chips are keyed by assistantMessageIndex, index drift can reattach chips to unrelated assistant bubbles after regenerate. Please prune/remap appliedComments to surviving assistant indices before sendPrompt() runs.

await navigator.clipboard.writeText(previewHtml);
setCopied(true);
window.setTimeout(() => setCopied(false), 1500);
} catch {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch silently swallows clipboard failures. Project constraints require no silent fallbacks; please surface an error toast (or throw with context) so copy failures are visible to the user.

type="button"
disabled={!draft.trim() || busy}
onClick={handleSubmit}
className="inline-flex items-center justify-center gap-1.5 min-w-[112px] rounded-[var(--radius-md)] bg-[var(--color-accent)] px-3 py-2 text-[12px] font-medium text-white shadow-[var(--shadow-soft)] transition-colors hover:bg-[var(--color-accent-hover)] disabled:bg-[var(--color-text-muted)] disabled:text-white disabled:pointer-events-none"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token-only UI constraint: this introduces hardcoded values (text-[12px] and text-white) in new renderer UI. Please replace with tokenized values (for example text-[var(--text-xs)] and text-[var(--color-on-accent)]), and apply the same tokenization pattern to nearby hardcoded sizes in this component.

hqhq1025 added a commit that referenced this pull request Apr 19, 2026
…ht edge

Addresses codex review on PR #46:
- Replace hardcoded hex colors in PreviewToolbar stage pill with new
  --color-stage-* tokens (sending/thinking/streaming/parsing/rendering/
  done/error). Tokens reference semantic colors so light/dark flip
  automatically.
- computeAnchoredPosition now accepts containerWidth and clamps the
  popover left so it never overflows the right edge of its container.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Silent fallback on clipboard copy hides runtime errors, violating the no-silent-fallback constraint. Evidence apps/desktop/src/renderer/src/components/Sidebar/AssistantMessage.tsx:37.
    Suggested fix:

    const pushToast = useCodesignStore((s) => s.pushToast);
    
    async function copyArtifact(): Promise<void> {
      if (!previewHtml) return;
      try {
        await navigator.clipboard.writeText(previewHtml);
        setCopied(true);
        window.setTimeout(() => setCopied(false), 1500);
      } catch (err) {
        const msg = err instanceof Error ? err.message : t('errors.unknown');
        pushToast({
          variant: 'error',
          title: t('notifications.copyFailed'),
          description: msg,
        });
      }
    }
  • [Major] New renderer UI introduces non-tokenized/hardcoded values (w-[360px], text-[12px], text-[13px], text-white), violating the token-only UI constraint for newly added UI. Evidence apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:70.
    Suggested fix:

    <section className="absolute z-10 w-[var(--size-inline-comment-popover)] max-w-[calc(100%-3rem)] ...">
    ...
    <div className="... text-[var(--text-xs)] ...">
    ...
    <textarea className="... text-[var(--text-sm)] leading-[var(--leading-body)] ..." />
    ...
    <button className="... text-[var(--text-xs)] text-[var(--color-on-accent)] ..." />

Summary

  • Review mode: follow-up after new commits
  • 2 issues found (both Major), each on added/modified lines in this diff.
  • Context note: docs/VISION.md and docs/PRINCIPLES.md are Not found in repo/docs in this checkout.

Testing

  • Not run (automation)
  • Suggested tests: add a UI test for clipboard write rejection surfacing an error toast; add a static/token-lint assertion (or snapshot) to prevent new hardcoded typography/color values in renderer components.

open-codesign Bot

await navigator.clipboard.writeText(previewHtml);
setCopied(true);
window.setTimeout(() => setCopied(false), 1500);
} catch {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch silently swallows clipboard failures. The project constraint says no silent fallbacks; copy failures need to surface in UI (toast) or throw with context.

Suggested fix:

} catch (err) {
  const msg = err instanceof Error ? err.message : t('errors.unknown');
  pushToast({
    variant: 'error',
    title: t('notifications.copyFailed'),
    description: msg,
  });
}

return (
<section
ref={cardRef}
className="absolute z-10 w-[360px] max-w-[calc(100%-3rem)] overflow-hidden rounded-[var(--radius-2xl)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)]"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token-only UI constraint: this new component still hardcodes sizing/typography/color (w-[360px], text-[12px], text-[13px], text-white in nearby lines). Please switch to semantic tokens (and add missing token(s) in packages/ui/tokens.css if needed).

Suggested fix:

<section className="... w-[var(--size-inline-comment-popover)] ...">
  ...
  <div className="... text-[var(--text-xs)] ...">
  ...
  <textarea className="... text-[var(--text-sm)] leading-[var(--leading-body)] ..." />
  ...
  <button className="... text-[var(--text-xs)] text-[var(--color-on-accent)] ..." />
</section>

hqhq1025 added 12 commits April 19, 2026 09:57
When the model wraps an `<artifact>` tag in a ```html ... ``` fence, the
streaming parser pulls the artifact out cleanly but leaves the empty fence
pair behind in the surrounding text. The empty pair surfaced in chat as a
meaningless assistant bubble (` ```html\n``` `) right under the user message.

- Add `stripEmptyCodeFences` to @open-codesign/artifacts and apply it in
  `runModel` before returning the message.
- Skip pushing an assistant bubble in the store when the message text is
  empty AND an artifact was produced — the canvas IS the reply.
- Cover the new helper in parser.test.ts.

Signed-off-by: hqhq1025 <1506751656@qq.com>
When applyComment returns no prose, the canvas update IS the confirmation —
do not push an 'Applied.' / '已应用。' assistant bubble. Switch the success
signal to a transient toast instead, mirroring the empty-fence fix on the
generate path.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Inline-comment chips and popovers must not surface raw XPath like
`/main[1]/section[3]/.../p[1]` — that intimidates non-technical users
and conveys no semantic intent. The new `getElementLabel(selection)`
util maps tag to a role label (heading / paragraph / button / image /
icon / list / section / ...) and builds a 30-char text preview.

Signed-off-by: hqhq1025 <1506751656@qq.com>
…oser

Day-zero users hit a sidebar that mixed always-visible tooling (attach
files, link design system repo, reference URL input + caption) with the
chat history and composer in a single column. The tools block ate ~30%
of vertical space and pushed the conversation off-screen.

This commit splits the sidebar into three focused components and reflows
the layout:

- `ToolsPanel`: a thin chip row with three icon buttons (Attach files /
  Link design-system repo / Reference URL toggle). The URL input and
  attached-file pills only render when relevant — chat gets the rest of
  the height.
- `ChatHistory`: scroll-to-bottom on every new message and on the
  generation toggle, plus delegates assistant rendering to a new
  `AssistantMessage` bubble with `Regenerate / Copy HTML / Save snapshot`
  inline actions.
- `Composer`: sticky at the bottom; preserves the existing send / stop
  button behaviour and exports `getTextareaLineHeight` for the existing
  Sidebar.test.

Adds the corresponding store actions: `regenerateLast`, `reuseLastPrompt`
(used by the upcoming TopBar iterate button) and a `saveSnapshot` stub
that surfaces a toast until PR #29's snapshot SQLite layer ships.

i18n: en + zh-CN keys for `assistantMessage.*`, `notifications.snapshot*`,
`notifications.branch*`.

Also picks up two adjacent commits from the parallel sidebar-bubble
work — element-label format + the `applyInlineComment` "no system text"
fix in store.ts — that landed on the same branch.

Signed-off-by: hqhq1025 <1506751656@qq.com>
… iterate row

Three closely-related preview/topbar changes that together address the
"my generation has no visible state and I can't iterate" gap.

PreviewToolbar
- New stage pill on the left, driven by store.generationStage. Tone
  cycles gray → pulsing blue (sending/thinking/streaming) → amber
  (parsing/rendering) → green (done) → red (error). aria-live="polite"
  so screen readers announce transitions.
- "Click any element to comment" moves out of the iframe overlay (it
  was covering the design's nav on every preview) into a quiet inline
  label next to the pill. Visible only when there is a preview.

TopBar
- Adds provider · model · token-placeholder strip beside the breadcrumb
  so users always know which model fired.
- When messages.length > 0, surfaces three iterate buttons:
  Regenerate (uses store.regenerateLast), Reuse prompt (puts the last
  prompt back in the composer via the new onReuseLastPrompt callback),
  and Branch (toast stub until snapshots ship in v0.2).

PreviewPane
- Drops the in-iframe floating hint that overlapped content.

i18n: en + zh-CN keys for `preview.stage.*`, `preview.clickToCommentShort`,
`topbar.iterate.*`, `topbar.session.*`, `notifications.branchQueued*`.

Signed-off-by: hqhq1025 <1506751656@qq.com>
…ight

Replaces the bottom-right floating InlineCommentComposer with a popover
anchored to the clicked element's bounding rect. `computeAnchoredPosition`
auto-flips above when below would overflow the canvas and clamps to a safe
left margin so the card never escapes the iframe container.

UX changes against the screenshots:
- No more raw XPath (`/main[1]/section[3]/.../p[1]`) — the header now
  shows the friendly element label from `getElementLabel` (heading /
  paragraph / button / icon …).
- Loading state is a real spinner + 'Applying…' label and a darker
  disabled colour (not the broken-looking 50% opacity).
- Cancel is a clearly secondary ghost button; Apply is the accent
  primary at min-width 112px so spinner + text don't reflow.
- On apply success, the store sets `highlightSelector`; PreviewPane
  draws a 1.2s pulsing accent ring over the original rect and the
  popover closes.

Drops InlineCommentComposer.tsx (legacy bottom-right card).

Signed-off-by: hqhq1025 <1506751656@qq.com>
…sed focus

- Replace role="dialog" wrapper with a real <section> element so the
  biome a11y/useSemanticElements rule (and screen readers) treat it as
  a labelled landmark.
- Drop the autoFocus attribute (biome a11y/noAutofocus) and focus the
  textarea via a useLayoutEffect ref call instead, preserving the
  keyboard-first feel without warning the user out of focus on slow
  hardware.

Signed-off-by: hqhq1025 <1506751656@qq.com>
…rumb

The previous header read 'open/codesign · pre-alpha · Generating your
design…' during a run, which buried the design's identity behind the
generation status. The status pill in PreviewToolbar already covers
'Generating' / 'Ready' / 'Failed' with proper aria-live, so the TopBar
title can stay as the design name.

- Rename the brand pill from 'pre-alpha' to a calmer 'preview' / '预览'.
- Bump the Wordmark mark to a new 'lg' size (20px) so the brand cluster
  matches the 24px logo target while staying in a 44px header.
- Breadcrumb now renders the design name (currently 'Untitled' /
  '未命名设计' until per-design names ship); errorMessage still wins to
  surface a hard-failure state.

i18n: topbar.brandBadge, topbar.designUntitled in en + zh-CN.
Signed-off-by: hqhq1025 <1506751656@qq.com>
Signed-off-by: hqhq1025 <1506751656@qq.com>
Restores common.dismissNotification, theme.*, disabledReason.*, and
examples.* groups that were removed during the chain-rewrite. These
keys are still consumed by Toast, ThemeToggle, Settings, onboarding
flows (PasteKey/ChooseModel/Welcome), and the hub Examples views.
Without them, those UI surfaces would render raw i18n keys at runtime.

Signed-off-by: hqhq1025 <1506751656@qq.com>
…ht edge

Addresses codex review on PR #46:
- Replace hardcoded hex colors in PreviewToolbar stage pill with new
  --color-stage-* tokens (sending/thinking/streaming/parsing/rendering/
  done/error). Tokens reference semantic colors so light/dark flip
  automatically.
- computeAnchoredPosition now accepts containerWidth and clamps the
  popover left so it never overflows the right edge of its container.

Signed-off-by: hqhq1025 <1506751656@qq.com>
…e id

Previously, AppliedComment.assistantMessageIndex referenced an array
position into messages[]. After regenerateLast() popped trailing
assistant bubbles (or whenever two assistants shared identical content),
chips could mis-attach to an unrelated future bubble.

Each assistant bubble now carries a stable RendererChatMessage.id
generated when it is pushed to the store. AppliedComment.targetMessageId
references that id directly, so chips render on exactly the bubble they
were applied to and silently disappear if their target is regenerated.
The id is renderer-only; it is stripped before history goes over IPC to
the model layer.

Adds a vitest covering two assistant messages with identical content:
the chip anchors to the latest message id and never matches the earlier
one by position or by content.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Inline comment flow treats a no-artifact response as success, leaving a chip that implies the edit landed while the canvas remains unchanged. This is a silent failure mode and can mislead users about data state, evidence apps/desktop/src/renderer/src/store.ts:705.
    Suggested fix:

    const firstArtifact = result.artifacts[0];
    if (!firstArtifact) {
      throw new Error('Inline comment returned no artifact');
    }
    set((s) => ({
      messages: shouldPushBubble
        ? [...s.messages, { role: 'assistant', content: trimmedMessage, id: newId() }]
        : s.messages,
      previewHtml: firstArtifact.content,
      isGenerating: false,
      selectedElement: null,
      highlightSelector: selection.selector,
    }));
  • [Major] New UI still introduces hardcoded non-token values (w-[360px], text-[12px], text-[13px], text-white) in renderer code, violating the token-only UI constraint for newly added UI, evidence apps/desktop/src/renderer/src/components/InlineCommentPopover.tsx:70.
    Suggested fix:

    <section className="... w-[var(--size-inline-comment-popover)] ...">
    <div className="... text-[var(--text-xs)] ...">
    <textarea className="... text-[var(--text-sm)] leading-[var(--leading-body)] ..." />
    <button className="... text-[var(--text-xs)] text-[var(--color-on-accent)] ..." />
  • [Minor] Chat bubbles use index-based keys, which can reuse component-local UI state across regenerate/trim operations and show stale affordance state on a different message, evidence apps/desktop/src/renderer/src/components/Sidebar/ChatHistory.tsx:27.
    Suggested fix:

    messages.map((m, i) => (
      <Bubble key={m.id ?? `${m.role}-${i}-${m.content.slice(0, 24)}`} index={i} message={m} />
    ))

Summary

  • Review mode: follow-up after new commits
  • 3 issues found on added/modified lines (2 Major, 1 Minor).
  • Not found in repo/docs in this checkout: docs/VISION.md, docs/PRINCIPLES.md, docs/ARCHITECTURE.md, docs/RESEARCH_QUEUE.md.

Testing

  • Not run (automation)
  • Suggested tests: add a store test for applyInlineComment() when result.artifacts is empty (should surface error + remove pending chip), and a React test asserting chat bubble state does not leak across regenerate when message IDs change.

open-codesign Bot

@@ -610,19 +703,37 @@ export const useCodesignStore = create<CodesignState>((set, get) => ({
attachments,
});
const firstArtifact = result.artifacts[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result.artifacts[0] can be undefined, but this path still exits the loading state without surfacing a failure and keeps the pending chip. Please treat this as an error (or explicitly surface it) so users don't see a phantom applied edit.

Suggested fix:

const firstArtifact = result.artifacts[0];
if (!firstArtifact) {
  throw new Error('Inline comment returned no artifact');
}

return (
<section
ref={cardRef}
className="absolute z-10 w-[360px] max-w-[calc(100%-3rem)] overflow-hidden rounded-[var(--radius-2xl)] border border-[var(--color-border)] bg-[var(--color-surface)] shadow-[var(--shadow-elevated)]"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token-only UI constraint: this new component still hardcodes sizing/typography/color (w-[360px], text-[12px], text-[13px], text-white). Please switch to semantic tokens and add missing tokens in packages/ui/src/tokens.css if needed.

Suggested fix:

<section className="... w-[var(--size-inline-comment-popover)] ...">

{t('sidebar.startHint')}
</p>
) : (
messages.map((m, i) => <Bubble key={`${i}-${m.role}`} index={i} message={m} />)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using index in the key can reuse AssistantMessage local state (copied/snapped) across regenerate/trim flows. Prefer a stable message id when available.

Suggested fix:

<Bubble key={m.id ?? `${m.role}-${i}-${m.content.slice(0, 24)}`} ... />

@hqhq1025
Copy link
Copy Markdown
Collaborator Author

Closing — 1321-line rewrite of chat/comment/status/topbar that's been DIRTY + carrying a Major finding for many cycles. Conflicts cascade with every UI change. The work is valuable but should be split into smaller follow-ups: (1) inline comment chip queue (per docs/research/30 §13 — Claude Design pattern), (2) status pill polish, (3) topbar refactor. Each small PR will land cleanly. Reopen as smaller scoped PRs in v0.2.

@hqhq1025 hqhq1025 closed this Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant