fix: getDriverIcon never checked for a URL/data: manifest icon (#632)

[aesslinger]

Closes #632

Summary

Every external plugin's icon fell through to the generic Plug icon in 7 of the 8 places the app renders a driver/connection icon, regardless of what its .tabularium icon field was set to. getDriverIcon (src/utils/driverUI.tsx) only ever compared the icon string against 6 hardcoded literals — a URL or data: URI (a documented, valid manifest value per the Tabularium docs) could never match any of them and always fell to the default Plug case. Only EngineCard.tsx's own separate, duplicated icon-rendering logic (used solely by the "Choose a database" picker) had a working URL check.

Every currently-published external plugin on the registry is affected identically — confirmed in the issue.

Priority order

@debba confirmed the intended 3-tier priority on the issue: Connection Custom Icon > Manifest Icon (if present) > Plugin Icon.

• getConnectionIcon already enforced the outer tier (per-connection appearance.icon override checked before falling back to getDriverIcon) — unchanged here.
• This PR completes the middle tier (manifest-supplied URL/data: icon), which was previously dead code — no manifest value could ever reach it.
• The inner tier (built-in brand SVG / legacy lucide names / generic fallback) is getDriverIcon's existing literal-string switch, now only reached once the URL check has been ruled out.

What changed

Two commits:

1. refactor: move RegistryDriverIcon out of the connection-modal folder — moves the existing <img>-with-fallback component from src/components/modals/connection/ to src/components/ (matching ConnectionIconImage's sibling placement), so the general-purpose driverUI.tsx can reuse it without reaching into one modal's private folder. No behavior change, just updates the 2 existing importers.

2. fix: getDriverIcon never checked for a URL/data: manifest icon — adds the URL/data: check as the first branch in getDriverIcon, fixing all 7 affected call sites at once (they all route through getDriverIcon/getConnectionIcon). Also corrects PluginManifest.icon's doc comment, which never documented URL/data: URI as valid values.

Explicitly out of scope

EngineCard.tsx's own renderIcon is left untouched beyond its import path — it has its own separate Database fallback for the URL-check branch (vs. getDriverIcon's Plug), and unifying those two fallbacks is a small but real behavior change beyond what this issue asks for.

Verification

• pnpm tsc --noEmit — clean
• pnpm lint — clean
• pnpm vitest run — 3734/3767 (33 pre-existing/unrelated failures — a localStorage.clear() jsdom gap in this local environment, confirmed present on a clean checkout, unrelated to this change)
• New tests/utils/driverUI.test.tsx (9 tests) covers: URL icon → <img>, data: icon → <img>, literal-string brand/lucide/fallback cases still work unchanged, and the full 3-tier priority order via getConnectionIcon (connection override beats manifest URL, manifest URL beats built-in fallback)
• Existing tests/components/connection/EngineCard.test.tsx (3 tests) still pass unchanged

[aesslinger]

Local verification notes

Ran a local review pass against .rules/react.md, .rules/typescript.md, .rules/general.md and independently verified every claim in the PR description against the actual code on this branch (not just skimmed the diff).

Rules compliance — all clean: no react.md #3 (Fast Refresh) concern (driverUI.tsx isn't a Context/component-export file), no any usage (the as PluginManifest casts in the new tests mirror an existing pattern already in EngineCard.tsx, not something new), English-only comments throughout.

Claims independently reproduced, not taken on faith:

• Re-grepped every getDriverIcon/getConnectionIcon caller fresh — confirmed exactly 7 external call sites plus EngineCard.tsx's own separate call (the 8th), matching the PR description precisely. None missed, none orphaned after the RegistryDriverIcon move.
• Read getConnectionIcon's current implementation directly — confirmed it's genuinely unchanged and already enforces the "connection override checked before falling back to getDriverIcon" behavior the PR description describes.
• Confirmed EngineCard.tsx's renderIcon is untouched beyond its import path, and its Database-vs-Plug fallback divergence from getDriverIcon (called out as an intentional scope boundary) is real, not accidental.
• Test counts, tsc, and pnpm lint all reproduced exactly: 12/12 on the two targeted test files (9 new + 3 existing EngineCard tests, unchanged), full suite 3734/3767 with the same 33 pre-existing/unrelated failures cited in the description, tsc --noEmit clean, pnpm lint clean.

One thing noted, not a blocker: found 2 other icon-rendering sites in the codebase not covered by this PR's "8 places" framing — PluginInstallConfirmModal.tsx's local PluginIcon and PaletteResults.tsx's command-palette icons. Verified both are genuinely unaffected: PluginIcon already renders any string unconditionally as an <img> (no literal-string bug to begin with), and PaletteResults.tsx's icons are unrelated lucide-name icons for palette actions, not plugin manifest icons. No gap — just noting that "8 places" was scoped to getDriverIcon's actual callers, not a literal count of every icon anywhere in the app.

Manual test plan (real data, no synthetic fixture needed — the real tabularis-postgresql-plugin manifest already declares a URL icon, exactly the scenario #632 describes):

Step	Where	Before this fix	After this fix
Choose a database picker	EngineCard	✅ branded SVG (already worked)	✅ unchanged
Sidebar entry	OpenConnectionItem / ConnectionGroupItem	❌ generic Plug	✅ branded SVG
Connections page, card view	ConnectionCard	❌ generic Plug	✅ branded SVG
Connections page, list view	ConnectionListItem	❌ generic Plug	✅ branded SVG
Appearance settings preview	AppearanceSection	❌ generic Plug	✅ branded SVG
Visual explain modal	VisualExplainModal	❌ generic Plug	✅ branded SVG

No known blockers.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review of the two follow-up commits (21a30b3, e00a1891) on top of the previously-reviewed ebd269c. The new shared isUrlIcon helper (src/utils/driverUI.tsx) is a faithful, case-insensitive extraction of the pre-existing inline http(s)/data: scheme checks, now reused by NewConnectionModal, EngineCard, InstallGate, and getDriverIcon. Verified the helper only matches http(s):// and data: schemes and correctly rejects built-in lookup keys — including database (no colon at the scheme position, so it is not mistaken for a data: URI), ftp://, and the empty string. As a result javascript: / vbscript: URIs still fall through to the built-in switch and render <Plug>, preserving the existing XSS-safe <img> rendering path. The added case-insensitivity is a correct RFC 3986 fix, so HTTPS:// and DATA: manifest icons now resolve too. Test consolidation is clean: every import in the renamed connectionIconPack.test.tsx remains in use, the ConnectionIconImage mock is only required in driverUI.test.tsx (where the image override path is exercised), and there is no test duplication.

Files Reviewed (8 files)

• src/utils/driverUI.tsx — isUrlIcon helper + URL/data: branch
• src/components/modals/NewConnectionModal.tsx — switched to shared isUrlIcon
• src/components/modals/connection/EngineCard.tsx — switched to shared isUrlIcon
• src/components/modals/connection/InstallGate.tsx — switched to shared isUrlIcon
• src/components/RegistryDriverIcon.tsx — rename/move (unchanged since prior review)
• src/types/plugins.ts — icon doc comment (unchanged since prior review)
• tests/utils/driverUI.test.tsx — consolidated tests incl. new isUrlIcon cases
• tests/utils/connectionIconPack.test.tsx — renamed; icon-pack/legacy-id + Proxy Symbol-safety tests

Previous Review Summary (commit ebd269c)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit ebd269c)

Status: No Issues Found | Recommendation: Merge

The fix adds a URL/data: branch as the first tier in getDriverIcon (src/utils/driverUI.tsx), mirroring the same check already working in EngineCard.tsx's renderIcon (line 36). Verified: the Plug fallback and RegistryDriverIcon onError fallback are wired correctly; the documented 3-tier priority — per-connection override (getConnectionIcon, unchanged) → manifest URL/data: icon → built-in brand/lucide/Plug — is preserved. Rendering a manifest icon URL via <img> is not a new pattern; EngineCard.tsx already did so for the database picker. The new tests/utils/driverUI.test.tsx assertions are valid against the tests/setup.ts lucide-react null mock (brand SVGs come from ./driverIcons, not mocked), and the IconOverride union matches getConnectionIcon's emoji/pack/image switch.

Files Reviewed (6 files)

• src/components/RegistryDriverIcon.tsx — pure rename/move, no content change
• src/components/modals/connection/EngineCard.tsx — import path only
• src/components/modals/connection/InstallGate.tsx — import path only
• src/types/plugins.ts — icon doc comment correction
• src/utils/driverUI.tsx — URL/data: icon branch + RegistryDriverIcon import
• tests/utils/driverUI.test.tsx — new tests (9 cases)

---

_{Reviewed by glm-5.2 · Input: 76.1K · Output: 18.1K · Cached: 179.3K}

[debba]

Hi @aesslinger, while reviewing I pushed two small commits on top of your branch:

• 21a30b3 extracts a shared isUrlIcon helper in driverUI.tsx and makes the URL/data: scheme check case-insensitive, so manifests with e.g. HTTPS:// or DATA: icons are handled the same way everywhere (NewConnectionModal, EngineCard, InstallGate).
• e00a189 consolidates the driverUI-related tests under tests/utils/driverUI.test.tsx to match the repo's testing conventions.

Both test files pass locally (25/25). Let me know if you agree with the changes, happy to adjust or revert if you'd prefer a different approach.