Skip to content

feat: sketch pad — draw a wireframe in the composer and send it to your crew - #8041

Merged
bolichen97 merged 1 commit into
mainfrom
feat/sketch-pad
Sep 3, 2026
Merged

feat: sketch pad — draw a wireframe in the composer and send it to your crew#8041
bolichen97 merged 1 commit into
mainfrom
feat/sketch-pad

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a sketch pad to the composer: a Sketch row in the composer’s "+" attach menu opens an Excalidraw whiteboard in a modal (on touch devices, where that menu is replaced by a direct file picker, a pencil button beside it is the entry instead), so a user can quickly draw a wireframe or annotate an idea and send it to their crew.

  • Insert into message exports the scene as a PNG plus a re-editable .excalidraw sidecar and hands both to the existing onUploadFiles attachment pipeline — server validation, image resizing, and attachment chips are reused unchanged. The sidecar gives the agent structured scene data (element geometry, labels) instead of pixels, and the dashboard's existing read-only scene renderer displays it as a drawing (one new .excalidraw entry in the server's text-extension allowlist — the PR's only backend line).
  • Excalidraw 0.18.1 (pinned exact) and its stylesheet are lazy-loaded on first open (React.lazy); the main bundle carries only the wrapper. Verified in the built dist: Excalidraw lands in its own async chunk, App/main chunk sizes unchanged vs base.
  • The last scene persists to localStorage (debounced 500ms, size-capped ~1.5MB, cleared when the canvas is emptied), so a reload or session switch restores the drawing — the sketch peer of the text draft's persistence.
  • Theme follows the dashboard's resolved mode; Excalidraw's UI language follows the dashboard language (mapped where Excalidraw ships a translation).
  • npm overrides, two kinds: (a) Radix layer-state internals (react-dismissable-layer / react-focus-guards / react-focus-scope) deduped onto the app's copies, scoped to the @excalidraw/excalidraw subtree only so react-menu's intentional nested copies stay untouched (the Cannot select a non-default agent in Schedule → Create job (dropdown clicks fall through) #6358 guard test and ChatSidebar.recencyUnit both stay green); (b) nanoid pins inside the same scoped block (3.3.18, plus 5.1.16 for mermaid-to-excalidraw) — the GHSA advisories against nanoid <=3.3.17 enter the production graph through Excalidraw's subtree, and the scoped pins alone satisfy the Dependency Audit gate (npm audit --omit dev: 0 vulnerabilities).
  • A repo-wide eslint i18n exemption for region-qualified BCP-47 tags (^[a-z]{2}-[A-Z]{2}$, full-match) — locale codes are protocol identifiers, not copy; declared here because the shape applies beyond EXCALIDRAW_LANG.
  • Canvas fonts are self-hosted (~14MB of emitted assets under dist/vendor/excalidraw/fonts/, plus EXCALIDRAW_ASSET_PATH set before the module loads): without this, Excalidraw fetches its text-tool fonts from a third-party CDN (esm.sh) — verified by a network probe against a live pod, before (5 CDN font requests) and after (0; all fonts served from /vendor/excalidraw/fonts/). Built-package size cost, on the record: fonts ~14MB (woff2, already compressed — ships near 1:1 in the wheel) + lazy JS chunks ~3.0MB + Excalidraw locale chunks ~1.2MB (161 files, loaded on demand) ≈ ~18MB of dist delta, staged into the PyPI wheel and npm package whether or not the pad is opened. Accepted deliberately: the alternative is a third-party CDN dependency the repo's own no-network rule forbids. Measured composition: Xiaolai (CJK handwriting) is 13MB of the 14MB; all other families total ~530KB — so a reachable-families trim saves ~0.5MB while dropping CJK sketching quality for zh/ja/ko locales. The real decision (deployment-time CJK pruning or glyph subsetting) is tracked in sketch pad: make the 13MB CJK canvas font a deployment-time choice #8091.
  • i18n: new keys added to all 13 catalogs; en-XA regenerated with scripts/gen-pseudolocale.mjs.

Testing

  • New SketchDialog.test.tsx: insert disabled on empty canvas; export produces paired sketch-<ts>.png + sketch-<ts>.excalidraw with exportBackground: true; dialog closes on insert; Excalidraw stays unmounted (lazy chunk unloaded) while closed.
  • Full frontend suite green locally (27903 passed / 0 failed), tsc -b clean, production build verified.

Evidence

sketch pad demo: pencil button opens the whiteboard, draw boxes + arrow, Insert attaches PNG + JSON chips

recorded from d004ff4 · feat/sketch-pad worktree pod (isolated KIROCREW_HOME, fresh browser context) · mode: default flags · real server + model flow, no fixtures · re-recorded from this head (the GIF file and the code differ from it only by this amend’s own GIF bytes)

@CrysisDeu
CrysisDeu requested a review from a team September 3, 2026 00:53
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 3, 2026 00:53
@CrysisDeu
CrysisDeu requested a review from buluoray September 3, 2026 00:53
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of d004ff4179f1b1725217e1074d826131a2c26b64 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound feature riding the existing attachment pipeline; the one design cost is ~18MB shipped to every install for a lazy modal, 13MB of it one CJK font.

Watch

  • The wheel/npm footprint grows ~18MB for every user whether or not the pad is ever opened, and the composition ("Xiaolai … is 13MB of the 14MB") means one handwriting font dominates the cost. The choice is disclosed and sketch pad: make the 13MB CJK canvas font a deployment-time choice #8091 tracks pruning, but the ordering is reversed: shipping the full font set now makes the footprint the default that a later PR must claw back, rather than an opt-in that sketch pad: make the 13MB CJK canvas font a deployment-time choice #8091 adds. A human should consciously accept that ratchet before merge.
  • The bundle gate now pins vendor-internal hash-named chunks (chunk-EIO257PC) and version-locked Radix/nanoid overrides that must move in lockstep on any Excalidraw bump — the "UPGRADE RITUAL" comment is the right mitigation, but this couples three files to one vendor's internals and will fail at runtime, not build time, if the ritual is half-done.

Suggestions

[DESIGN-REVIEWED] d004ff4

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of d004ff4179f1b1725217e1074d826131a2c26b64 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

Solid, reversibility-first sketch flow; two edges remain — silent loss of oversized sketches on reload, and an unlabeled pencil as the sole touch entry point.

Watch

  • Oversized-scene persistence is silently dropped: past SCENE_PERSIST_MAX_CHARS the code does localStorage.removeItem(SCENE_STORAGE_KEY) with no UI signal, so a user who pasted images into a sketch — the pad having taught them drawings always survive — reloads and finds it gone. Low frequency × lost-work impact × every time for such scenes. Smallest fix: when the cap trips, show a muted header note ("Sketch too large to keep after a reload — attach it now").
  • Touch entry is an icon-only <PenLine size={17}> button (directFilePicker branch); a pencil conventionally means "edit", title never shows on touch, and this is the only Sketch entry on those devices. First-time misread or non-discovery, once per user, self-correcting after one tap. Smallest fix: pair the icon with the "Sketch" text label if the row can absorb it.

Suggestions

  • sketch_desc "Draw a sketch; attaches an image plus its editable source" restates its label "Sketch" — tighten to "Draw and attach an image plus its editable source".

[UX-REVIEWED] d004ff4

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of d004ff4179f1b1725217e1074d826131a2c26b64 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All lens work is complete: every mechanism the change claims to reuse exists at base (FileRenderers routes .excalidraw at website/src/components/FileRenderers.tsx:25, safeStorage tiered reclaim, the mc- key convention, the FILE_ACCEPT↔server parity test), the temp-screenshots/ GIF follows a 717-file checked-in evidence convention, and the eslint BCP-47 exemption has ~12 pre-existing matching literal sites in non-test source (en-US in tz.ts, SttSettings.tsx, WeekGrid.tsx, zh-CN/en-XA in i18n code), so it is not a one-consumer generalization.

First-Principles-Verdict: PASS

A drawing input the composer lacked, wired entirely through existing pipelines — every riding surface is declared, measured, and derived from a nameable constraint.

What this change ships

Intent: let a user draw a wireframe in the composer and send it to the agent as an attachment — an ADDITION.

  1. Sketch row in the composer's "+" attach menu — justified
  2. Pencil button beside the touch file picker (menu absent there) — justified
  3. Whiteboard modal, lazy-loaded; Insert attaches PNG + re-editable .excalidraw via the existing upload pipeline — justified
  4. .excalidraw accepted by server allowlist and file picker — justified; picker half derived from the FILE_ACCEPT↔server parity test
  5. Last sketch survives reload via localStorage (capped, cleared on empty) — justified, reuses safeStorage
  6. Pad follows dashboard theme and language — justified
  7. ~18MB fonts/chunks baked into every wheel — justified by the documented no-network rule; CJK trim deferred to sketch pad: make the 13MB CJK canvas font a deployment-time choice #8091
  8. Scoped npm overrides (Radix dedupe + nanoid GHSA pins) — justified, derived from audit gate and Cannot select a non-default agent in Schedule → Create job (dropdown clicks fall through) #6358 guard
  9. Repo-wide eslint exemption for region-qualified locale tags — justified; ~12 pre-existing matching literals counted in non-test src
  10. Three bundle-budget entries for the lazy chunks — derived from the size gate

The demo GIF follows the repo's checked-in temp-screenshots/ convention (717 existing files). No item duplicates an existing mechanism: the base's Excalidraw code (ExcalidrawBlock, excalidrawScene.ts) is read-only rendering, not authoring, and this change routes attachments through it rather than re-implementing it.

Watch

"Built-package size cost, on the record: ~18MB" — 13MB of that is one CJK font family shipped to every install regardless of use. The subtraction is real and already named (#8091); it should land before a second vendored-asset PR normalizes this size class.

[FIRST-PRINCIPLES-REVIEWED] d004ff4

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed d004ff4179f1b1725217e1074d826131a2c26b64 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] d004ff4

Verdict parsed from the review's SHA-scoped output markers for commit d004ff4179f1b1725217e1074d826131a2c26b64.

False positive or not applicable? A repository writer can comment:
/ai-review override fable d004ff4179f1b1725217e1074d826131a2c26b64: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of d004ff4179f1b1725217e1074d826131a2c26b64 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/SketchDialog.tsx:193 -- ignoring "safeSetItem(...)" failure preserves an obsolete scene after quota exhaustion -> Fix: remove the stored key when it returns false.
[GPT-REVIEWED] d004ff4

False positive or not applicable? A repository writer can comment:
/ai-review override gpt d004ff4179f1b1725217e1074d826131a2c26b64: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Interaction demo

sketch pad demo

Flow shown: composer pencil button → sketch dialog (Excalidraw lazy-loads) → draw two boxes + arrow → Insert into message → dialog closes, PNG thumbnail + .excalidraw.json sidecar chips appear in the composer.

recorded from 91b6bcb · feat/sketch-pad worktree pod (isolated KIROCREW_HOME, fresh browser context) · mode: default flags · real server + model flow, no fixtures

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Design review disposition (head 53bc547dd84151a4bf3976276c43b15e419e9386)

  • Wheel-size delta should be measured and consciously acceptedadopted, measured, on the record in the PR body: fonts ~14MB + lazy JS ~3.0MB + locale chunks ~1.2MB ≈ ~18MB dist delta into wheel/npm. Accepted as the cost of the no-CDN rule; trimming to reachable font families is noted as the follow-up (all families ship today because Excalidraw's font picker exposes them all — trimming means also hiding picker entries, a scope decision for its own PR).

A pencil button in the composer opens an Excalidraw whiteboard in a
modal. Insert exports the scene as a PNG plus a re-editable
.excalidraw.json sidecar and hands both to the existing attachment
pipeline (onUploadFiles), so server validation, resizing, and
attachment chips are reused unchanged.

Excalidraw (0.18.1, exact) and its stylesheet load lazily on first
open; the main bundle carries only the wrapper. Scoped npm overrides
dedupe Excalidraw's nested Radix layer-state internals onto the app's
copies without touching react-menu's intentional nested copies.
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition for span=4109fe9729d2:

  • Sketch header exceeds the two-button limit — fixed with the "separate row" option, using the canvas's own slot: "New sketch" moved from the dialog header into Excalidraw's renderTopRightUI (the canvas toolbar region), a structurally separate row next to the drawing the action discards. The header now carries exactly the primary CTA plus the built-in close X; the button stays permanently visible, so the UX lane's visibility requirement holds simultaneously. GPT re-reviewed this head clean.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Oversized-scene persistence is silently dropped — accepted with rationale, follow-up noted: removal-over-stale-snapshot was itself a blocking fix from the GPT lane (an outdated snapshot restoring over newer edits is worse than no snapshot), and the in-memory scene still covers reopen within the page's lifetime, so the loss window is reload-with-a->1.5MB-scene only. The suggested muted header note is folded into the tracked persistence follow-up rather than restarting all five lanes for one string this round.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Touch entry is an icon-only pencil button — accepted with rationale: the touch branch renders a bare file-input label plus one icon button, and adding a text label grows exactly the composer-row surface the blocking lanes capped earlier on this PR. Discoverability is self-correcting after one tap (the review's own assessment); noted for the touch-polish follow-up.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • sketch_desc restates its label (suggestion) — noted for the same follow-up; a 13-locale string tweak is not worth a lane-restart round on its own.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • The ~18MB ratchet should be consciously accepted by a human before merge — agreed, and that is the state this PR is in: the cost is measured and disclosed in the body, sketch pad: make the 13MB CJK canvas font a deployment-time choice #8091 tracks the pruning options, and the PR is handed to a human for the merge decision with this trade named explicitly — the human acceptance asked for is the merge itself.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Ship only the non-CJK families now (suggestion) — declined as a default: zh/ja/ko are first-class locales and a sketch pad whose handwriting style silently degrades for CJK text as the DEFAULT inverts the product's locale posture. If the human merger prefers the smaller default, dropping the Xiaolai directory from the font plugin is a one-line change they can request.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #4085 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4085: REBASE. Base drift has already introduced a new violation outside the PR's file set; the guard cannot go green until the sweep is re-applied to current main. Files: website/src/components/SketchDialog.tsx.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

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