Skip to content

fix(composer): 🐛 Persist attachments in thread drafts & remove plan mode toggle#184

Merged
jorben merged 2 commits into
masterfrom
fix/composer-loss-image
May 16, 2026
Merged

fix(composer): 🐛 Persist attachments in thread drafts & remove plan mode toggle#184
jorben merged 2 commits into
masterfrom
fix/composer-loss-image

Conversation

@HayWolf
Copy link
Copy Markdown
Contributor

@HayWolf HayWolf commented May 16, 2026

Summary

  • Fix image/file attachments being lost when switching between threads by persisting attachmentData in composer drafts
  • Remove the plan/default mode toggle from the prompt composer UI (simplification)

Changes

  • composer-store: Add attachmentData field to ComposerDraftData; getDraft() normalizes missing/legacy data with backward compatibility
  • runtime-thread-surface: Pass initialAttachmentData and onAttachmentDataChange to the composer so attachments survive thread switches
  • workbench-prompt-composer: Remove RunModeToggle component and related props (runMode, showRunModeToggle, onRunModeChange, runModeDisabled); hard-code run mode to "default" in buildSubmissionFromPromptInput
  • dashboard-workbench: Remove plan mode toggle props from new-thread composer
  • composer-store.test: Add tests for attachmentData persistence, independent update, backward compat for legacy drafts missing attachmentData

Test Plan

  • Attach an image, switch threads, switch back — image should still be present
  • Attach a file, switch threads, switch back — file should still be present
  • Verify legacy thread drafts (string or object without attachmentData) render without errors
  • Confirm plan/default mode toggle is no longer visible in the composer
  • Run npm run typecheck and npm run test:unit to verify no regressions

🤖 Generated with TiyCode

@github-actions
Copy link
Copy Markdown

AI Code Review Summary

PR: #184 (fix(composer): 🐛 Persist attachments in thread drafts & remove plan mode toggle)
Preferred language: English

Overall Assessment

Detected 1 actionable findings, prioritize CRITICAL/HIGH before merge.

Major Findings by Severity

  • MEDIUM (1)
    • src/modules/workbench-shell/model/composer-store.ts:120 - Missing test coverage for draft backward-compatibility normalization

Actionable Suggestions

  • Run npm run typecheck to confirm no remaining callers pass removed runMode props to WorkbenchPromptComposer.
  • Confirm that plan mode UI is intentionally relocated and not accidentally removed.
  • Add or update unit tests for composer-store.ts getDraft backward compatibility.
  • Add Vitest tests covering getDraft backward compatibility (string drafts and missing fields)
  • Add a setDraft/getDraft roundtrip test that includes populated attachmentData
  • Audit and update any tests mounting WorkbenchPromptComposer with removed runMode props
  • Add a behavioral test verifying handlePromptSubmit passes runMode: 'default' to buildSubmissionFromPromptInput
  • Evaluate adding a max size or count limit for attachmentData to prevent localStorage bloat from large base64 payloads.
  • Confirm that dead code related to removed run-mode props (e.g., selectedRunMode, newThreadRunMode state) is fully cleaned up in the modified files.

Potential Risks

  • TypeScript compilation errors from parent components still passing removed run-mode props.
  • Functional regression if plan mode toggle is removed without an alternative entry point.
  • Referential equality breakage for consumers depending on stable draft object references from getDraft.
  • Untested normalization of legacy draft payloads could lead to undefined attachment arrays in production
  • Removed runMode props may cause TypeScript or test failures in unmodified parent component test files
  • Storing base64 data URLs in composer drafts may approach localStorage limits if users attach multiple or large images.
  • The ReadonlyArray-to-mutable cast for attachmentData assumes the underlying store will not mutate the array; ensure immutability is preserved.
  • Base64 media payloads in Zustand drafts may accumulate memory across many threads or large attachments if users attach multiple high-resolution images.

Test Suggestions

  • Unit test getDraft with missing, string, partial, and full draft shapes.
  • Component test asserting WorkbenchPromptComposer does not render RunModeToggle and submits with default mode.
  • Integration test verifying draft save/restore carries attachmentData end-to-end.
  • Test getDraft returns attachmentData: [] for legacy string drafts (composer-store.ts)
  • Test getDraft normalizes missing referencedFiles and attachmentData via ?? [] (composer-store.ts)
  • Test setDraft/getDraft roundtrip preserves SerializableAttachment[] (composer-store.ts)
  • Update WorkbenchPromptComposer unit tests to omit removed runMode props (workbench-prompt-composer.tsx)
  • Add test asserting submission runMode is always 'default' after prompt submit (workbench-prompt-composer.tsx)
  • Consider adding a test that verifies getDraft returns a stable empty array for missing attachmentData rather than mutating the stored legacy object.

File-Level Coverage Notes

  • src/modules/workbench-shell/model/composer-store.ts: schema-change-with-backward-compat-but-untested
  • src/modules/workbench-shell/ui/workbench-prompt-composer.tsx: behavior-change-with-surface-reduction-needs-test-audit
  • src/modules/workbench-shell/model/composer-store.test.ts: No runtime performance impact. Changes are test-only additions validating backward compatibility and draft field persistence. (Tests confirm the draft model stores attachment payloads as inline base64 data URLs. For large files this can bloat memory, but the test file itself is not a runtime concern.)
  • src/modules/workbench-shell/ui/runtime-thread-surface.tsx: No measurable performance regression. New attachmentData initial value and callback follow the same imperative store-access pattern already used for referencedFiles. (If future profiling reveals re-render churn in the composer, memoizing the draft callbacks and coalescing setDraft updates would help, but the current change maintains existing conventions.)
  • src/modules/workbench-shell/ui/dashboard-workbench.tsx: No performance impact. Only removes unused runMode-related props from the composer.

Inline Downgraded Items (processed but not inline)

  • None

Coverage Status

  • Target files: 5
  • Covered files: 5
  • Uncovered files: 0
  • No-patch/binary covered as file-level: 0
  • Findings with unknown confidence (N/A): 0

Uncovered list:

  • None

No-patch covered list:

  • None

Runtime/Budget

  • Rounds used: 1/4
  • Planned batches: 2
  • Executed batches: 2
  • Sub-agent runs: 5
  • Planner calls: 1
  • Reviewer calls: 6
  • Model calls: 7/64
  • Structured-output summary-only degradation: NO

Copy link
Copy Markdown

@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.

Automated PR review completed.

  • Findings kept: 1
  • Findings with unknown confidence: 0
  • Inline comments attempted: 1
  • Target files: 5
  • Covered files: 5
  • Uncovered files: 0
    See the summary comment for detailed analysis and coverage details.

return {
text: draft.text,
referencedFiles: draft.referencedFiles ?? [],
attachmentData: draft.attachmentData ?? [],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MEDIUM] Missing test coverage for draft backward-compatibility normalization

getDraft now normalizes missing attachmentData and referencedFiles via ?? [], but no test verifies this path for partially persisted legacy drafts.

Suggestion: Add Vitest tests for getDraft covering: (1) legacy string drafts, (2) object drafts missing attachmentData, (3) object drafts missing referencedFiles, and (4) fully populated drafts.

Risk: Legacy partial payloads could cause silent UI failures if normalization regresses.

Confidence: 0.85

[From SubAgent: testing]

@jorben jorben merged commit 68dd8ff into master May 16, 2026
4 checks passed
@jorben jorben deleted the fix/composer-loss-image branch May 16, 2026 02:34
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.

2 participants