fix(platform): polish logo/favicon upload button hover and feedback (#1961)

[larryro]

Summary

Polishes the logo/favicon upload control in branding settings (image-upload-field.tsx) to give it visible interaction feedback and align it with the design system. Resolves #1961.

Changes

• Hover/focus/active feedback on the upload button: border + background lift on hover (hover:border-border-strong hover:bg-bg-elevated), a visible focus ring (focus-visible:ring-1 focus-visible:ring-ring with offset) matching the Button primitive, and an active:scale-[0.97] press, all motion-reduce-safe.
• Replace affordance: hovering/focusing a populated control reveals a dim overlay with an upload icon, signalling the image can be replaced. Empty-state + icon brightens on hover.
• Drag-and-drop: drop an image file directly onto the control to upload it, with a highlighted drag-over state. File handling is shared between the file input and drop via a single uploadFile helper; dropped non-image files are ignored.
• Remove button gains hover scale and a focus ring.
• Added a WithImage Storybook story demonstrating the populated/hover state.

Verification

• tsc --noEmit (platform) — green
• oxlint on the touched files — 0 warnings, 0 errors
• oxfmt --check — formatted
• Branding component tests (branding-form, branding-preview) — 21 passed

No new user-facing strings or schema changes (interaction-only polish; aria-labels unchanged).

[larryro]

Desk Review — NOT READY (changes required)

CI: ✅ all required checks green (Build, Lint, Format, Type check, Knip, Unit, UI, Browser, Migrations, Storybook, SAST/Opengrep, CodeQL, all 16 Playwright platform shards + web/docs). Skipped jobs are fork-PR only; Trivy is NEUTRAL. No red, no pending.

The styling/feedback polish is well done — hover/focus-visible/active/drag states, valid design tokens (border-border-strong, bg-bg-elevated, accent-base, ring-offset-background), behavior-preserving uploadFile extraction, sound object-URL lifecycle, and a corner remove-button polish. Security is fine: the client file.type check is UX-only and the server action (convex/branding/file_actions.ts) re-validates auth + MIME allowlist + 2 MB cap + safe paths.

🚫 Blocking — missing test for the new drag-and-drop behavior

This PR adds genuinely new interactive logic with branches but no test exercises any of it:

• happy: drop a valid image → uploadFile runs (image-upload-field.tsx L135–144)
• edge: drop while isUploading → ignored (L139)
• error: drop a non-image (!file.type.startsWith('image/')) → silently ignored (L141)

There is no image-upload-field.test.tsx (its siblings branding-form, branding-preview, color-picker-input all have one). branding-form.test.tsx mocks this component, and the e2e settings-depth.spec.ts branding block tests only the brand-color field — neither drives upload or drag-drop.

This fails the repo's own DoD:

• AGENTS.md L77 — "Tests carry the change: unit (happy + one edge + one error) … — or N/A."
• AGENTS.md L195 — "Every feature and fix carries its test — happy path, one edge, one error condition minimum."
• Ripple Map L90 ("A new interactive UI element") → a11y + manual guide + e2e.

The new WithImage Storybook story is a visual variant only — it asserts no behavior.

To clear: add image-upload-field.test.tsx covering (a) valid image dropped → upload invoked, (b) drop while uploading → no second upload, (c) non-image dropped → no upload, plus an accessibility describe block via checkAccessibility() (house pattern). Pure styling changes don't need tests; the drag-drop logic does.

Non-blocking polish (optional)

• Drag-highlight flicker: the <img>/<Image> (L184–200) and empty-state <Plus> (L206) lack pointer-events-none (only the overlay <span> has it, L201). Dragging across a child fires dragleave on the button and briefly clears isDragging; dragover restores it → possible flicker.
• Stuck drag ring: handleDragLeave has no isUploading guard, so isDragging can stay true (ring visible) if a click-upload starts mid-drag, until the next dragleave.
• Type-check inconsistency: drop path filters on file.type.startsWith('image/') (L141) but the <input> path doesn't; a .ico with empty reported MIME would work via picker yet be silently rejected on drop.
• Convention nits: focus-visible:ring-1 ring-offset-2 vs the house ring-2 ring-offset-2; off-scale bg-foreground/55 (L201).

Verdict: NOT READY — add the drag-and-drop component test (the single gating item); the polish notes are optional.

[larryro]

Desk Review — PR #2113 · fix(platform): polish logo/favicon upload button hover and feedback (#1961)

Verdict: NOT READY — CHANGES REQUIRED (one blocking test gap; everything else is sound)

Branch tale/xs7ds2dhms1xdcb48gx82qn195896m5n. Reviewed the whole implementation, not just the diff. The change is genuinely good: both acceptance criteria are met, CI is green, and the existing tests pass. The single blocking item is a missing test for the one piece of new logic the PR introduces.

CI state

gh pr checks 2113 → 45 pass, 0 fail, 0 pending, 5 skipping (fork-PR/conditional jobs only: Validate images, Docs container test, Smoke test, Trivy, Opengrep OSS). Key gates green: Build platform, UI, Type check, Lint, Format, Knip, all Playwright (platform 1–16 / web / docs).

Tests

Ran the project's own suite once, locally, from services/platform:
bunx vitest --run --project client app/features/settings/branding/components/image-upload-field.test.tsx
→ Test Files 1 passed (1), Tests 5 passed (5) (renders accessible control; drop happy path; drop ignored while uploading; non-image drop rejected; axe a11y audit).

What's verified good

• AC1 (visible hover/focus/active feedback): MET. Hover (hover:border-border-strong hover:bg-bg-elevated, group-hover replace overlay, Plus-icon color), focus (focus-visible:ring-ring ring-2 ring-offset-2, group-focus-visible overlay), active (active:scale-[0.97]), and drag highlight (isDragging && border-accent-base ring-accent-base) are all wired to real interactive states, each with motion-reduce: fallbacks.
• AC2 (matches design system): MET. Every utility resolves to a real @tale/ui token — confirmed in packages/ui/src/globals.css: --color-bg-elevated, --color-border-strong, --color-accent-base, --color-ring all defined (light+dark). No silently-no-op classes. The interaction cluster mirrors the DS buttonVariants base.
• Correctness/robustness: object-URL lifecycle is sound (revoked on re-upload, error, remove, and unmount cleanup — no leak found); drop-while-uploading is guarded (if (isUploading) return) plus disabled on the button; pointer-events-none on inner img/overlay/icon prevents drag-highlight flicker; keyboard upload still works (visible <button> is focusable; hidden input tabIndex=-1); aria-label present on button, remove button, and hidden input.
• Security boundary: mimeType/size are validated server-side in convex/branding/file_actions.ts (requireBrandingAdmin, mimeToExtension, MAX_IMAGE_SIZE_BYTES = 2MB), so the client gates are convenience, not a trust boundary.

BLOCKING — must fix

1. Untested new logic: the isAcceptedImage extension-fallback branch — services/platform/app/features/settings/branding/components/image-upload-field.tsx:21-25. The whole reason this helper exists (per its own comment) is to accept a valid file whose browser MIME is blank/non-image/* by falling back to the extension (e.g. a .ico reporting empty type). The tests cover only the image/png (MIME-true) and .txt/text/plain (both-false) cases — the middle branch (MIME not image/* AND name ends in an accepted extension → accepted) has zero coverage, so a regression that drops the fallback would keep all 5 tests green. Add a test in image-upload-field.test.tsx: drop makeFile('favicon.ico', '') (or 'application/octet-stream') and assert mockSaveImage/onUpload fire. (Verified by grepping the test file — no .ico/empty-MIME case exists.)

Recommended (non-blocking)

2. Drop path accepts more types than the picker or server allow — image-upload-field.tsx:21-22. isAcceptedImage returns true for ANY image/* (gif, bmp, avif, tiff…), but the picker's accept is .png,.svg,.jpg,.jpeg,.webp,.ico and the server's mimeToExtension only maps png/svg/jpeg/webp/ico (convex/branding/file_utils.ts:105). Dropping e.g. image/gif passes the client gate, then the server rejects it and the error is silently swallowed (see add --chat-max-width CSS variable #3) — confusing no-op. Consider validating the drop against the same accept list as the picker. The code comment claiming picker and drop are "in sync" is inaccurate.
3. Upload error is swallowed silently — image-upload-field.tsx:109-115. The catch resets preview/URL but never logs or rethrows; a failed upload gives the user (and console) no signal. This violates the repo rule in .claude/CLAUDE.md ("Never use an empty catch block — log or re-throw"). NOTE: pre-existing — the diff relocated this block byte-identically from handleFileChange into uploadFile; it is not newly introduced. Worth a one-line console.error while the code is open, and ideally a user-facing error state, but not a regression from this PR.
4. No tests for the upload-error path, the file-picker onChange path, or drag-over/leave — the shared uploadFile core is exercised via drop, but these branches are untested. Minor for a UI-polish PR.
5. Minor DS nit: main button uses focus-visible:ring-2 while the DS buttonVariants and the remove button (line 225) use ring-1. Pick one for consistency.

---

NOT READY — CHANGES REQUIRED

1. Add a test covering the isAcceptedImage extension-fallback branch in image-upload-field.test.tsx — drop a file whose MIME is empty/non-image/* but whose name ends in an accepted extension (e.g. favicon.ico) and assert the upload proceeds. This is the only branch of the PR's new logic with no coverage today.

[larryro]

Desk review — #1961 logo/favicon upload polish

Verdict: READY TO MERGE. CI is fully green and the change meets both acceptance criteria.

Verification

• CI: all required checks pass — Build platform, Type check, Lint, Format, Knip, UI, Unit, Storybook, Smoke test, Scan/Trivy/Opengrep, and all 16 Playwright (platform) shards + Playwright (web). The only non-pass entries are skipping (fork-PR / path-filtered jobs), which is expected.
• Tests (run locally): bunx vitest --run --config vitest.ui.config.ts app/features/settings/branding/components/image-upload-field.test.tsx → 6 passed (6).
• Acceptance criteria: ✅ visible hover/focus/active feedback (hover border-border-strong/bg-bg-elevated, focus-visible ring, active:scale, drag accent-base ring, hover/focus replace-overlay); ✅ styling matches the design system — every token (border-border-strong, bg-bg-elevated, accent-base, ring) is defined in packages/ui/src/globals.css for light + dark.

Implementation quality

• Drag-and-drop is added cleanly via an extracted uploadFile callback shared with the picker. The in-flight guard (if (isUploading) return) is correct — useCallback deps include isUploading, so the closure stays fresh (confirmed by the concurrent-drop test).
• isAcceptedImage() correctly mirrors the picker's accept list and is case-insensitive (name.toLowerCase()). The drop path is actually more defensive than the existing picker onChange, which validates nothing.
• New tests cover the new behavior well: drop happy path, concurrent-drop guard, extension fallback, non-image rejection, and an axe a11y audit.

Non-blocking observations (optional follow-ups, not required to merge)

1. Silent upload-failure — the catch {} in uploadFile clears the preview with no toast/log. This is pre-existing (identical try/catch existed in the prior handleFileChange) and out of this PR's scope, but worth a future ticket: e.g. a blank-MIME .ico accepted by the drop gate is rejected by the backend (saveImage derives the extension from MIME only; mimeToExtension('') → throws Unsupported image MIME type), and the user sees the preview silently vanish. In practice browsers usually report image/x-icon, which the backend accepts.
2. DRY nit — ACCEPTED_IMAGE_TYPES (string) and ACCEPTED_EXTENSIONS (array) duplicate the list; the string could be derived via ACCEPTED_EXTENSIONS.join(',').
3. Redundant aria-label on the hidden, tabIndex={-1} file input (the visible button already carries the label).

None of these block merge.