feat(folders): restore auto-generated emoji icons for chat folders - #7353
feat(folders): restore auto-generated emoji icons for chat folders#7353hungtnvu wants to merge 1 commit into
Conversation
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
1 similar comment
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
91f3019 to
1a7cb38
Compare
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound engineering on a product decision the repo previously made the other way, and the reversal is still unratified. WatchReverses #1211's shipped UX decision and deletes the test that pinned it ( Suggestions
[DESIGN-REVIEWED] 3caec12 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The non-object-body 400 guard is a point patch riding along: three more Not justified as shipped
What this change shipsIntent: give every chat folder a distinct emoji glyph again so the sidebar is scannable — a declared ADDITION (deliberate reversal of #1211, demand in #6586).
Watch
[FIRST-PRINCIPLES-REVIEWED] 3caec12 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed Adjudication upheld 1 of 1 blocking finding(s); those must be resolved before merging. BLOCKING -- website/AUTOSDE.yaml:506 -- broadens the emoji exception to a reusable renderer Adjudication (Opus 4.8) — is blocking on each finding proportionate?I've confirmed the finding anchors to the Harm rung: LOW (cosmetic — an emoji folder glyph rendered in [ADJUDICATION] 3caec12 total=1 uphold=1 downgrade=0 |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of All evidence gathered. Composing the review. UX-Verdict: CONCERNS Solid restore with mode-aware hints, but every new control is unseen in this lane, and the shipped icon-error path speaks English to 11 locales. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 3caec12 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
f44830f to
884f46a
Compare
|
Dispositions for the GPT 5.6 review of 1a7cb38, addressed in 884f46a:
|
|
Dispositions for the Design Review of 1a7cb38, addressed in 884f46a:
|
|
Dispositions for the First Principles Review of 1a7cb38, addressed in 884f46a:
|
|
Dispositions for the UX Review of 1a7cb38, addressed in 884f46a:
|
bolichen97
left a comment
There was a problem hiding this comment.
Description ↔ CI mismatch: the pseudolocale i18n gate is listed as green, but CI proves this branch adds a render-time defect.
What the description claims. Under the test summary: "Also green locally: … all i18n gates (dead keys, catalog parity, duplicates, pseudolocale, key references, untranslated-strings ratchet)."
What CI shows. E2E (stub ACP backend, offline) fails at step 12, "i18n render-time gate (en-XA + shipped locales)". The gate (npm run i18n:render → scripts/check-i18n-render.mjs) runs diff-scoped against the base and reports:
[i18n-render] FAIL [vs-base] - this branch ADDS render-time i18n defects
settings-chat.layout: 0 -> 1 (+1)
layout/clipped-without-title worst: en-XA @ 2.12x
One token is wider than the box; no wrapping can fix it. Add overflow-wrap:anywhere.
The 0 -> 1 delta attributes the new defect to this branch, and the surface is the one this PR touches: the six keys added in website/src/i18n/locales/en.manual.json (icon, icon_auto_hint, icon_cleared_hint, icon_placeholder, icon_regenerate, icon_regenerate_pending) overflow the icon row in FolderConfigModal once en-XA expands them.
Why this one is blocking. This repo currently carries a main-inherited Frontend Lint & Type Check red that no individual PR should be held to. This is not that: the i18n:render gate compares against base and isolates the regression to keys this PR introduces, so the red is provably caused by this diff — and the description names that exact gate as passing.
Required fix.
- Make
i18n:rendergreen: addoverflow-wrap:anywhereto the overflowing icon control / hint container inFolderConfigModal, or shorten the token that en-XA expands past the box. - Then correct the description — the pseudolocale gate should not be listed among the green ones until it is.
Not part of this finding (recorded so it is not re-litigated): the backend work matches the description closely — _spawn_chat_folder_icon_task with the expected_icon pin and in-lock re-read, the icon / regenerate_icon validation with icon_invalid / regenerate_icon_invalid / icon_conflict, and the learn-cron-dashboard.md update were all verified against main, and test/test_chat_folder_icons.py covers the 14 named cases. The Backend Tests (Windows) (2) red is a PermissionError in test/test_job_routes.py, which this PR does not touch — environmental, not blocking. The AI-review reds are SHA-scoped to the older 1a7cb38b and already dispositioned.
2ad8b1c to
0b606eb
Compare
|
Disposition of GPT 5.6 findings at
|
|
Disposition of the UX concerns at
|
|
Disposition of the First Principles concerns at
|
|
@bolichen97 — maintainer decision needed on this PR (needs-a-decision disposition for the GPT lane's blocking finding on The question: do you accept the Why this needs you and not another code round: the GPT review lane is in a structural deadlock with the feature itself —
No code change short of deleting the feature satisfies the lane, because the feature is emoji rendering and the rule is Two outcomes, your call:
Separately: your standing CHANGES_REQUESTED (env/merge-ref contamination) was addressed at |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Hi @bolichen97 — gentle ping on the maintainer decision requested in #7353 (comment) (posted yesterday). Quick recap of where this PR stands at head
The decision needed is a product ruling, one of:
Happy to make any changes either way. Thanks! |
0b606eb to
dcc624e
Compare
dcc624e to
151fc7f
Compare
|
self-added: yes
|
|
self-added: yes
|
Bring back the MeshClaw behaviour that did not survive the port: creating a chat folder fires a background task that asks the shared cheap-model one-liner helper for a single emoji matching the folder name, and the icon arrives asynchronously over the slots push. The generator, the grapheme-exact validator and the serialization lock were already in chat_folders.py (the artifact library kept using them); this rewires the chat-folder call sites onto today's store semantics: - the write-back goes through mutate_folders and re-finds the folder by id under the store lock, so a folder deleted mid-generation is never resurrected - PATCH accepts icon (set / clear, single-emoji validated) and regenerate_icon (reset to auto), rejecting the two together; icon joins the validate-then-apply changes dict, so app-ownership enforcement applies unchanged - create accepts an optional explicit icon and skips generation for it - the sidebar renders the emoji in place of the default glyph (palette color keeps tinting only the default glyph), and the folder-settings modal gains an icon field with an edit-mode Auto-generate action, kept mutually exclusive with a manual edit in the draft Fixes kirodotdev#6586
151fc7f to
3caec12
Compare
|
self-added: yes @bolichen97 — refreshed decision request, now that everything else on this PR has cleared. One finding remains and it is yours to rule on.
To accept, paste on this PR:
To reject, say so and I'll close this PR out. |
Problem / Motivation
MeshClaw gave every dashboard chat folder an emoji icon automatically: create a folder, and a few seconds later a fitting glyph showed up in the sidebar, generated by a cheap background LLM call. Kiro Crew removed that system deliberately in #1211 to de-clutter the sessions sidebar — the emoji badge and its LLM auto-generation were dropped, the palette color became the folder's identity mark, and a test pinned the absence. The generator engine itself stayed (artifact-library folders kept using it), so the module docstring in
chat_folders.pystill describes "the shared LLM emoji generator the artifact library uses for ITS folder icons" — idling right next to the handlers that used to call it.This PR is a deliberate reversal of #1211's UX decision, not a regression fix. #6586 shows users read the absence as a loss: a distinct glyph per folder is what made the sidebar scannable at a glance. The two identity marks are reconciled rather than re-fought — the emoji replaces only the glyph shape, while #1211's palette color keeps tinting the default glyph, so both systems coexist.
One data-evolution consequence worth stating: #1211 left stale
iconvalues in existingfolders.jsonfiles and simply ignored them. With this PR those pre-#1211 icons render again in upgraded users' sidebars — plausibly welcome (they were user-visible state once), andFolder settings → Iconcan clear or replace any of them.Why it matters
A distinct glyph per folder is what made the sidebar scannable, and it cost the user nothing — naming the folder was the entire interaction and the icon appeared on its own. #6586 shows the demand survived #1211's de-clutter rationale: users who came from MeshClaw filed the absence as a loss. Cost stays bounded: one short completion per folder created, on the governed cheap model, never on the interactive path.
What changed (motivation → approach → change)
The fix is re-wiring, not rebuilding.
generate_emoji_for_name, the_is_single_emojigrapheme-exact validator, and the_folder_icon_lockserialization all survived the port and are actively used by artifact folders. What was missing: the write-back task, the spawn in the create handler, theicon/regenerate_iconfields on the update handler, and the sidebar rendering. A straight revert of the MeshClaw code would not work because the folder store changed underneath it — the port is onto today's semantics:Backend (
src/kiro_crew/dashboard/chat_folders.py):_spawn_chat_folder_icon_taskmirrors the artifact-library precedent (_spawn_artifact_folder_icon_task): fire-and-forget, strong task refs in a module-level set, and the write-back goes throughstate.mutate_folders, re-finding the folder by id under the store lock — so a folder deleted while generation was in flight is never resurrected (MeshClaw's directfolder["icon"] = ...mutation had a small race here; the locked re-find closes it structurally). The write-back is also pinned to a per-folder icon epoch — an in-memory counter bumped under the store lock by every icon set, icon clear, and rename — and only applies while the epoch still equals its value when the task was scheduled. One invariant closes all three stale-write-back races: a value-pin alone passes whenever the icon VALUE is unchanged, so an explicit clear (absent → absent) or a rename (icon untouched) would let a stale emoji land after the user's action. Deliberately per-folder rather than the store-widefolders_generation()counter, which bumps on every folder mutation anywhere and would cancel legitimate icon delivery whenever an unrelated folder changed mid-generation. The slots push after a successful write is what delivers the icon to the UI asynchronously; the create response returns immediately with no icon.icon(validated by_is_single_emoji, 400icon_invalid) and skips generation when one is supplied — a caller that already chose an icon must not have it silently overwritten seconds later.icon(set / clear;null/""drops the key so "absent means the default glyph" stays the one on-disk representation, mirroringcolor) andregenerate_icon: true("reset to auto").regenerate_iconmust be a real boolean — the string"false"is truthy and would have armed regeneration, so a non-bool is 400regenerate_icon_invalid. The two together are rejected with 400icon_conflict, matching the original MeshClaw contract.iconjoins the existing validate-then-applychangesdict, so app-ownership enforcement (owner_app) applies to icon writes with no new code; the regenerate spawn runs only after_applysucceeded, so an app cannot regenerate a foreign folder's icon, and a rename+regenerate in one PATCH regenerates from the new name._FOLDER_ICON_MODEL = "auto"viarun_bg_oneliner— the repo deliberately dropped MeshClaw's hardcoded model id because it 400s on accounts/partitions that do not serve it; the background one-liner path already guarantees the icon never runs on the user's interactive model.Frontend (
website/src/):FolderGlyph.tsx: an emojiiconreplaces the lucide folder shape; the palettecolorkeeps tinting only the default glyph, so the two identity marks never fight.ChatSidebar.tsx/MoveUndoBar.tsx:iconthreaded through all eight glyph call sites — the sidebar rows, drag ghost, menus, and the move-undo toast's destination glyph, so the toast shows the same mark as the sidebar.FolderConfigModal.tsx: an Icon row (live preview, emoji input, edit-mode Auto-generate button). The draft keeps manual-icon and regenerate exclusive in both directions: typing clears a pending regenerate, and Auto-generate discards a pending manual edit so the pair can never be sent together. While regenerate is armed the input renders empty to match the default-glyph preview; the create-mode hint teaches "empty = auto-picked", and edit mode gets its own cleared-state hint ("empty keeps the plain glyph") so the two modes don't contradict each other. The pending hint promises only what is guaranteed — "an icon will be picked from the folder name", not a new one, since regenerating an unchanged name can pick the same emoji.types/index.ts/api/client.ts:icon?: stringonChatFolderand on the create-folder config type (no endpoint change; the POST body already spreads the config).en.manual.json, translated into the 11 maintained locales, pseudolocale regenerated viagen-pseudolocale.mjs.The chat-folder section of
docs/system-specs/modules/learn-cron-dashboard.mdis updated in the same commit — it previously pinned "folders carry noiconfield" and "no LLM generation runs on the chat-folder lifecycle", both now false.Transport is free:
push_slots_update()already carries folders to the client and the WS handler refetches on store change, so the async icon arrival needs no new plumbing.Tests
test/test_chat_folder_icons.py(new, 19 tests):icon_invalid"") leaves the folder without an icon keyiconon create is an opt-out: no icon stored, generator never invokedicon+regenerate_icontogether is 400icon_conflict; a non-booleanregenerate_icon(e.g. the string"false") is 400regenerate_icon_invalidfolder_not_owned, generator never invoked)website/src/test/FolderConfigModal.test.tsx(extended): icon controls render with the default-glyph preview, typed emoji submits and previews, edit mode seeds from the folder, clear reports a touched icon edit, Auto-generate armsregenerateIconwhile discarding a manual edit, typing after Auto-generate disarms it, an armed regenerate renders an empty input matching the default-glyph preview, and the cleared-state hint appears only when the field is emptied in edit mode. One pre-existing test that pinned the icon system's absence ("the emoji/icon system was removed") was replaced by these — this PR deliberately reverses that removal (#1211).mutate_foldersconfirms persistence, so a surviving folder can never read epoch 0 and accept a stale in-flight generation over a manual icon)Also green locally: the full chat-folder backend suites (159 tests),
tsc -b, eslint (0 errors, and the branch now fits the 603-warning ratchet: removed a stalejsx-a11y/label-has-fordisable that a dep refresh made unused; the earlierno-consoleannotation on the pre-existing federated-searchconsole.warnwas removed once upstream lint debt dropped and the branch fit the ratchet without it), and all i18n gates (dead keys, catalog parity, duplicates, pseudolocale, key references, untranslated-strings ratchet).Manual verification
Exercised end-to-end against a local dev gateway built from this branch (
./dev-backend.shwith the fake ACP test backend, which returns a deterministic single emoji per folder name — so generation genuinely runs the full async path: create → background one-liner → emoji validation →mutate_folderswrite-back → slots push). Driven through the real dashboard UI with playwright-cli; icon state confirmed on disk infolders.jsonafter each step.POST /api/chat-foldersreturns 201 with no icon in the response (async contract), and the icon appears in a later GET.One observation, out of scope for this PR: with a dashboard page already open, the sidebar does not refetch folders on any PATCH in my rig — a plain rename (untouched code path) shows the same lag, so it is pre-existing behavior, not introduced here. A fresh page load renders everything correctly.
Screenshots / video
Captured from the local instance above (dark theme, 1440×900):
Create modal with an explicit icon — Icon row with live preview; the hint explains empty = auto-generated:
Sidebar with both icon origins — 🚀 was set explicitly at create; 📚 on "Reading List" was auto-generated asynchronously:
Folder settings with Auto-generate armed — preview falls back to the default glyph and the save hint is shown:
AI review dispositions (fixes, rebuttals, deferrals)
Automated-review findings and how each landed, so a human reviewer can weigh the reasoning directly:
no-emoji-as-iconsAUTOSDE rule” (FolderGlyph.tsx:25) — resolved as a stale exception path. The rule's own exception list already sanctions exactly this use: “ChatSidebar.tsx (FolderGlyph renders the folder's icon as data)” — written whenFolderGlyphlived insideChatSidebar.tsx. The component has since been extracted tosrc/components/FolderGlyph.tsx, so the sanctioned rendering fell outside the named file. This PR updates the exception's file reference to follow the component (website/AUTOSDE.yaml, one line); no new exemption is introduced.release.set()— fixed. All three gated tests intest/test_chat_folder_icons.pynow release and drain in atry/finally, so a failing assertion can no longer strand a task on a closed loop.icon: ""create-time opt-out (zero consumers) — rebutted. The opt-out is the backend contract for the create-modal “no icon” affordance tracked in Chat folder icon UX follow-ups: fold cue on emoji folders, create-time no-icon affordance, localized validation #7992; removing it would orphan that follow-up. It is also what keeps explicit-empty distinct from absent, which the API needs to separate “auto-generate” from “none”.regenerate_icon+ the Auto-generate button — rebutted. Reset-to-auto is a deliberate part of the restored feature (manual icon → back to generated), not speculative surface; the UX lane asks to extend this control (pending/success state), not remove it.mcp_dashboardor by apps get the same auto-icon treatment as UI-created ones: one short LLM call per folder create, skipped whenever an explicit icon (or theicon: ""opt-out) is passed. This is intended — the icon is folder metadata, not a UI-only nicety._spawn_artifact_folder_icon_taskguards only existence, not set/clear/rename mid-generation. Fixing it here would widen a chat-folder PR into the artifact module; tracked in Artifact folder icon write-back has the stale-generation races chat folders fixed in #7353 #7991.FolderGlyphnow floors the emoji font size at 12px (Math.max(12, …)).colortints only the default folder shape, so the two marks never contend (documented at the component). A combined emoji+color treatment is a design question for the follow-up issue, not a regression in this diff.UX review's three remaining advisory items (collapse-state cue on emoji folders, a create-time “no icon” affordance for the
icon: ""opt-out, localized client-side icon validation) stay deferred to #7992 rather than widening this diff; the backend contract they need ships here.Maintainer decision requested
This PR reverses #1211, which removed the folder icon system. Issue #6586 demonstrates demand for the feature but is not itself a maintainer decision — the Design and First Principles review lanes both flagged that the reversal needs explicit maintainer sign-off. Calling it out here so it is decided deliberately rather than implied by a green board.
Related Issues
Fixes #6586
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)