fix(emoji-picker): surface silent upload failures + fix stale grid refresh#303
Conversation
…efresh closure Custom emoji upload from the Set Project Icon picker's Custom tab could silently fail: empty-pendingFile and invalid-slug branches in doUpload() returned with no user feedback (no toast, no error). Additionally, the post-success/post-delete grid refresh called an orphaned renderCustomGrid closure captured before buildCustomPanel() rebuilt the panel, instead of relying on buildCustomPanel()'s own fresh render. - doUpload(): showToast on both previously-silent early-return paths - proceed()/delete handler: drop stale renderCustomGrid() call; buildCustomPanel() already repaints the live grid via its own closure No repro environment (headless browser) available in this sandbox to confirm which branch is load-bearing in production, so both are fixed as cheap, non-conflicting defense in depth per dispatch brief.
![clagentic-reviewer[bot]](https://avatars.githubusercontent.com/in/3944489?s=60&v=4){kind=small}
There was a problem hiding this comment.
PEACHES — clean
- lib/public/modules/emoji-picker.js:259 — Surfaces silent no-op (missing pendingFile) with toast "Couldn't add custom emoji"; reuses existing showToast import.
- lib/public/modules/emoji-picker.js:267 — Surfaces silent no-op (invalid slug) with toast "Enter a valid name"; same pattern.
- lib/public/modules/emoji-picker.js:296 — Stale-closure fix verified: buildCustomPanel() creates new customGrid element + renderCustomGrid closure, calls renderCustomGrid() before return (lines 399, 401). Removing the orphaned captured renderCustomGrid() is correct — it would paint detached DOM node.
- lib/public/modules/emoji-picker.js:377 — Same stale-closure fix applied to delete path; reasoning is consistent.
Stale-closure hazard explained accurately in comments (lines 252-257, 289-294, 372-375). All silent-return paths in doUpload() now surfaced. Brand rules (clagentic-console.brand-product-name) compliant. Reuse discipline maintained (uploadCustomIcon() + server route untouched). No new dependencies. npm test 619/619 passing per PR description.
![clagentic-security[bot]](https://avatars.githubusercontent.com/in/4134208?s=60&v=4){kind=small}
There was a problem hiding this comment.
BOBBIE — clean
Audited HEAD 0b58f5e (live GitHub head.sha matches expected; no drift).
Scope: lib/public/modules/emoji-picker.js only — confirmed via diff. custom-icons.js (uploadCustomIcon, slug validation) and server-settings.js (POST /api/custom-emoji/:slug) are unmodified by this PR.
Review findings:
- doUpload() early returns (line ~258, ~266) now call showToast() with static string literals only ("Couldn't add custom emoji", "Enter a valid name") — no user-controlled input reaches these calls.
- The pre-existing showToast(result.error || "Upload failed", "warn") call at line ~286 is unchanged by this PR (server-controlled error string, same as before).
- showToast() (lib/public/modules/utils.js:1) sets textContent, not innerHTML — no XSS vector for toast text, unchanged by this PR.
- The stale-closure fix (removing the extra renderCustomGrid() call after fetchCustomList(...buildCustomPanel())) is a rendering-correctness change with no security surface — it does not touch what is POSTed, only which grid re-render function runs.
- Confirmed no change to the upload payload, slug construction/validation regex, or the fetch('/api/custom-emoji/'+encodeURIComponent(slug)) call in custom-icons.js.
No findings against RULEBOOK.md (bobbie.secret., bobbie.bleed., bobbie.dep., bobbie.sast.) or .crew/bobbie.yaml project rules (no-push-token-in-source, no-api-key-in-yoke, postinstall-safe — none apply to this file).
Scanners:
- gitleaks: 6 findings, all in .claude/worktrees/*/lib/certs/privkey.pem (pre-existing local dev TLS certs, unrelated to this diff's file scope) — not citable against this PR.
- semgrep (p/javascript, p/security-audit) on emoji-picker.js: 0 findings.
- osv-scanner on package-lock.json: pre-existing dependency vulnerabilities (handlebars, nodemailer, undici, ws, etc.), none introduced by this PR (no package.json/package-lock.json change in diff) — not citable against this PR.
scanners_run: ["gitleaks:ran", "semgrep:ran", "osv-scanner:ran"]
early_exit_reason: single-file client UI change, no new sinks introduced, upload payload and server route unmodified — reasoning bounded to the diff's actual surface.
clagentic gate-note — authorized
Authorize rationale: NAOMI authorize: PEACHES clean (0 findings), BOBBIE clean (0 findings), both audited head_sha 0b58f5e, no drift. No LORE task_id — ad-hoc merge, Archivist unreachable this session. |
Fixes a silent-failure bug in the emoji pickers Custom tab upload flow (Set Project Icon -> emoji picker -> Custom -> upload). Reported: adding a custom emoji from Server Settings works, but from the pickers Custom tab it silently does nothing (no icon, no error, no toast).
MILLER root-cause diagnosis (ad-hoc dispatch, no live-browser repro available in this sandbox) identified two candidate branches in lib/public/modules/emoji-picker.js doUpload()/proceed(), both fixed as cheap non-conflicting defense in depth:
Silent early returns in doUpload() (line ~251, ~257 pre-fix): the empty-pendingFile and invalid-slug checks returned with zero user feedback. Now both call showToast(), already used for server-side upload failures at the neighboring line, with plain copy: Couldnt add custom emoji / Enter a valid name.
Stale-closure grid refresh (line ~279 pre-fix): after a successful upload or delete, the code called buildCustomPanel() followed by an outer renderCustomGrid() call captured by an OLDER buildCustomPanel() invocation, repainting an orphaned detached grid the moment buildCustomPanel() rebuilds the panel. buildCustomPanel() already repaints the live grid via its own fresh renderCustomGrid closure, so the stale outer call is removed at both call sites (upload success + icon delete).
No changes to uploadCustomIcon() (custom-icons.js) or the server route (server-settings.js) -- diagnosis showed both are caller-independent and correct; only the pickers own async upload-flow wiring needed fixing.
Ad-hoc task (no LORE task_id; Archivist unreachable at dispatch time). Diagnosis basis: MILLER confidence 0.55, root-cause region proven via code reading; this sandbox has no headless-browser tool to confirm which of the two branches was actually load-bearing in production, so both are fixed.
Tests: npm test -- 619/619 passing, 0 failures. No new dependencies. No DOM test harness exists in this repo for picker UI flows (see test/custom-icons-lr-0847.test.js note on this); the fix reuses the same shared uploadCustomIcon()/showToast() surfaces already covered indirectly by that test files pure-function coverage.