feat: per-connection icon & accent color override (#189)#241
feat: per-connection icon & accent color override (#189)#241NewtTheWolf wants to merge 28 commits into
Conversation
…TabularisDB#189) - Extract curated 30-icon subset into connectionIconPack.ts (src-of-truth for picker + resolver) - Replace import * as LucideIcons with named pack import to enable tree-shaking - Reset failed state on path change so ConnectionIconImage recovers from prior failures - Guard onError against post-unmount setState via mountedRef - Add aria-hidden to emoji span; narrow getConnectionIcon Pick to "appearance" only - Add ConnectionIconImage.test.tsx with regression test for sticky-failed fix - Update global lucide-react mock in tests/setup.ts with pack icons
…#189) Replace getDriverColor/getDriverIcon with getConnectionAccent/getConnectionIcon in OpenConnectionItem, ConnectionCard, ConnectionListItem, and VisualExplainModal so per-connection accent colours and icon overrides are honoured everywhere.
) Add set_connection_appearance Rust command to patch the stored appearance field after save. Mount AppearanceSection at the bottom of the General tab, wired with appearance state pre-populated from initialConnection on edit. For new connections a stable UUID is generated on mount and used as the icon upload target; after save the appearance (with its icon path) is persisted under the real backend-minted id via set_connection_appearance.
…ance (TabularisDB#189) - Delete previous image when user picks a new one in AppearanceSection (intra-session orphan) - Delete uploaded-but-unsaved image on modal cancel via useEffect cleanup + wasSavedRef - Extract set_appearance_impl pure helper; add 3 unit tests (update, clear, missing-id)
…DB#189) When the Custom hex input is opened after a palette swatch was picked, the draft now mirrors the current accentColor so closing the field can't silently revert it. Also drops i18n keys for errors that aren't wired up yet (tooLarge, invalidFormat) and the preview label that has no UI.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| let Some(crate::models::IconOverride::Image { path }) = a.icon.as_ref() else { return Ok(()) }; | ||
| let full = app_data_dir.join(path); | ||
| if full.exists() { | ||
| fs::remove_file(&full).map_err(|e| IconError::Io(e.to_string()))?; |
There was a problem hiding this comment.
CRITICAL: Path traversal vulnerability — cascade_delete_if_image deletes files without verifying the resolved path is inside the app data directory.
A maliciously crafted connections.json with an image path like ../../../sensitive-file.txt would cause arbitrary file deletion when the connection is removed. Unlike delete_connection_icon (which canonicalizes and checks starts_with), this function joins the raw stored path directly and calls fs::remove_file.
| group_id: original.group_id.clone(), // Copy to same group as original | ||
| sort_order: None, // Will be placed at end of group | ||
| detect_json_in_text_columns: original.detect_json_in_text_columns, | ||
| appearance: original.appearance.clone(), |
There was a problem hiding this comment.
WARNING: Duplicating a connection copies the image icon path by reference.
Both the original and the duplicate share the same icon file path. When either connection is deleted, cascade_delete_if_image removes the shared file, breaking the other connection's custom icon. Consider either copying the icon file with a new path on duplicate, or checking whether other connections still reference the path before cascade-deleting.
| value.icon?.type === "emoji" ? "emoji" : | ||
| value.icon?.type === "image" ? "image" : "default"; | ||
|
|
||
| const [tab, setTab] = useState<IconTab>(initialTab); |
There was a problem hiding this comment.
WARNING: tab state is initialized from value.icon?.type but never re-syncs when value changes externally.
If the parent resets the appearance (e.g., on modal re-open or form reset), initialTab is recomputed but tab stays on the old selection because it's held in local useState. This causes the UI to show the wrong tab (e.g., "pack" selected while value has no icon). Consider syncing tab with value.icon?.type via useEffect.
| sourcePath: picked, | ||
| }); | ||
| if (previousImagePath && previousImagePath !== stored) { | ||
| try { await invoke("delete_connection_icon", { relativePath: previousImagePath }); } |
There was a problem hiding this comment.
WARNING: Eager deletion of previous image file can cause data loss on modal cancel.
pickImage deletes the previous image file from disk immediately (before the connection is saved). If the user picks a new image and then cancels the modal, the cleanup effect in NewConnectionModal deletes the new file, but the old file was already deleted here — leaving connections.json pointing to a missing file with no way to recover the original icon. The same issue exists in removeImage. Consider deferring file deletion until the connection is actually saved.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Previously Flagged — Now Resolved
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)No issues found in unchanged code. Files Reviewed (8 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.6-20260420 · 1,516,149 tokens |
…#189) Replace the manual hex text input and regex validation in AppearanceSection with HexColorPicker (saturation/hue picker) + HexColorInput (lib-validated hex input), keeping the 12-swatch curated palette intact. Removes HEX_RE, customDraft, hexError, and commitCustom (~30 lines). Updates tests to cover the new panel open/close and live-emit behaviour.
…irectory The root cause of image upload silently breaking at display time: Tauri 2's assetProtocol was disabled (default), so convertFileSrc() produced URLs the webview refused to load. Fix: - Enable assetProtocol in tauri.conf.json, scoped to $APPDATA/connection-icons/** - Add fs:scope-appdata-recursive to capabilities so appDataDir() resolves - Add DEV-only console.error in ConnectionIconImage.onError to surface future failures
…rch, frimousse, image preview (TabularisDB#189) - Derive active tab reactively from value.icon.type (approach B: userTab ?? derivedTab) so reopening an edited connection shows the correct tab without stale useState - Add preview row at top showing current accent color dot + connection icon + name; threads driverManifest and connectionName props from NewConnectionModal - Add search input above icon pack grid (case-insensitive id filter) - Replace plain emoji <input> with Frimousse headless picker (built-in search, only emits valid single emoji) — removes graphemeCount, emojiDraft, emojiError, commitEmoji dead code - Show ConnectionIconImage thumbnail in Image tab after upload or when editing an existing image connection - Split pickImage() error handling into two distinct try/catch blocks with console.error logging so failures point to the exact failing step
… tab sync - Remove old emoji text-input tests (blur/validation no longer apply) - Add Frimousse mock to avoid async emoji data fetching in JSDOM - Add: emoji picker renders with search input, onEmojiSelect emits correctly - Add: pack icon search filters results by search term - Add: tab syncs to value.icon.type on external prop change (edit mode)
| value.icon?.type === "image" ? "image" : "default"; | ||
|
|
||
| const [tab, setTab] = useState<IconTab>(initialTab); | ||
| const [emojiDraft, setEmojiDraft] = useState(value.icon?.type === "emoji" ? value.icon.value : ""); |
There was a problem hiding this comment.
WARNING: emojiDraft state never re-syncs with external value changes
Similar to the tab state issue, emojiDraft is initialized from value.icon?.value but never updates when the value prop changes. If the modal switches to a different connection without unmounting AppearanceSection, the emoji input will display the previous connection's emoji.
| /> | ||
| <div className="flex items-center gap-2"> | ||
| <span className="text-zinc-500 text-sm">#</span> | ||
| <HexColorInput |
There was a problem hiding this comment.
WARNING: HexColorInput defaults to prefixed={true}, causing a double # prefix
The manual # span (line 157) combined with HexColorInput's default prefixed={true} behavior means the input displays ##abc123 when a value is present. Add prefixed={false} to HexColorInput and prepend # in the onChange handler (e.g. `#${c.toLowerCase()}`) so the display stays consistent with the intended #rrggbb placeholder.
…abularisDB#189) Tab state used to stay on the user's last click even after value.icon changed externally (parent re-render with new initialConnection). Resets userTab to null when value.icon.type differs from the previous render. Also enables the protocol-asset Cargo feature on tauri — required at the crate level for assetProtocol to actually serve files via tauri:// URLs. Without it, assetProtocol in tauri.conf.json is a no-op. Minor: memoize filteredPackIcons, route "No preview" through i18n.
…cons (TabularisDB#189) - Replace Frimousse with emoji-picker-react (dark theme, categories, skin tone selector, recent emojis, preview disabled) - Expand icon picker from 30 curated icons to all ~1900 lucide icons via dynamicIconImports with lazy loading + 120-result display cap - Add camelCase→kebab-case normalization for legacy stored icon ids in both resolver and picker - Wrap lazy icon components in Suspense in driverUI and AppearanceSection pack grid - Update test mocks: frimousse → emoji-picker-react, dynamicIconImports proxy in setup.ts - Add camelCase normalization tests in driverUI.test.tsx
| throw new Error(`Failed to save icon: ${e}`); | ||
| } | ||
| if (previousImagePath && previousImagePath !== stored) { | ||
| invoke("delete_connection_icon", { relativePath: previousImagePath }).catch(() => {}); |
There was a problem hiding this comment.
WARNING: Eager deletion of previous image file causes data loss on modal cancel
The old image file is deleted immediately after a new image is picked, but onChange with the new path isn't emitted until the next line. If the user cancels the modal after picking a new image, the old file is gone but connections.json still references it. Defer deletion until the connection is actually saved, or keep both files and garbage-collect orphans periodically.
| // Maps the previous 30-icon kebab-case ids to themselves (lucide uses kebab-case in dynamicIconImports). | ||
| // If callers used camelCase (e.g. "shieldCheck"), translate that here. | ||
| export const CONNECTION_ICON_PACK = new Proxy({} as Record<string, LazyExoticComponent<LucideIcon>>, { | ||
| get(_target, key: string) { |
There was a problem hiding this comment.
WARNING: Proxy get trap crashes on Symbol property access
The get trap types key as string, but JavaScript Proxy traps receive Symbol keys at runtime. camelToKebab(key) calls .replace() on the key, which throws TypeError for Symbols. Common operations like Object.prototype.toString.call(CONNECTION_ICON_PACK) or React DevTools inspecting the object can trigger this. Add a typeof key === 'string' guard at the top of the trap before calling string methods.
…_icon_for_duplicate (TabularisDB#189) - cascade_delete_if_image: canonicalize both paths and reject any file not under <app_data>/connection-icons/ before deletion - Add copy_icon_for_duplicate helper that copies icon files with the same path-containment guard, so duplicate_connection gives each duplicate its own icon file rather than sharing the original path - Wire copy_icon_for_duplicate into duplicate_connection in commands.rs; falls back to dropping the icon on copy failure instead of sharing - Add regression tests: cascade_delete_rejects_path_traversal, copy_icon_for_duplicate_produces_new_filename, copy_icon_for_duplicate_rejects_path_traversal
) Root cause of user data-loss bug: AppearanceSection eagerly called delete_connection_icon when the user picked a replacement image, so if the session was later cancelled the "previous" file was already gone but connections.json still referenced it. - AppearanceSection: remove eager delete_connection_icon from pickImage and removeImage; add onImageUploaded prop so the parent can track uploads - NewConnectionModal: track all uploaded paths in uploadedPathsRef - On cancel: delete every session upload except the original image (handles "pick A → B → C → cancel" correctly) - On save: delete every uploaded path except the final one, plus the original if the user replaced it - Add test: "does not eagerly delete previous image on pick"
…risDB#189) - CONNECTION_ICON_PACK proxy: guard get trap with typeof key !== "string" so Symbol accesses (Symbol.toStringTag, Symbol.iterator, etc.) from Object.prototype.toString.call(), React DevTools, etc. return undefined instead of throwing TypeError in camelToKebab's .replace() call - HexColorInput: add explicit prefixed={false} to future-proof against react-colorful changing its default - Add tests: Symbol.toStringTag and Symbol.iterator proxy access
1 participant
Summary
Lets users visually differentiate connections that share the same driver (e.g. "MySQL local" vs "MySQL prod") by picking a per-connection accent color and/or icon. Closes #189.
What's new
Architecture
appearance: { icon?, accentColor? }onSavedConnection(Rust + TS mirror). Backwards compatible — existingconnections.jsondeserializes unchanged.IconOverride { Pack { id } | Emoji { value } | Image { path } }.<app_data>/connection-icons/<id>-<sha8>.<ext>via new Tauri commandssave_connection_icon/delete_connection_icon. Validates MIME via magic bytes, rejects SVGs containing<script>/javascript:/on*=event handlers (incl. spaced and chained variants).set_connection_appearancecommand attaches appearance to a saved connection by id (needed because the backend mints its own id on create).getConnectionAccent/getConnectionIconinsrc/utils/driverUI.tsx. Wired into 4 components:OpenConnectionItem,ConnectionCard,ConnectionListItem,VisualExplainModal. Lucide pack lives insrc/utils/connectionIconPack.tsas an explicit 30-iconRecord(noimport *, so tree-shaking is preserved).<AppearanceSection>mounted insideNewConnectionModalwith collapsible color picker + 4-tab icon picker (Default / Pack / Emoji / Image).Out of scope (deferred)
Test plan
cd src-tauri && cargo test --lib— 598 passed (10 new module tests + 3 set_appearance tests + 1 update_connection regression test)pnpm vitest run— 2275 passed; 30 pre-existing failures (SettingsProvider/ThemeProvider/useSidebarResize localStorage mock issues) unchangedpnpm tsc --noEmit— clean<script>Spec & plan
Local-only docs (
/docsis gitignored):docs/superpowers/specs/2026-05-21-per-connection-icon-color-design.mddocs/superpowers/plans/2026-05-21-per-connection-icon-color.md