feat(desktop): add project icon support with custom protocol#1379
Merged
saddlepaddle merged 1 commit intomainfrom Feb 11, 2026
Merged
feat(desktop): add project icon support with custom protocol#1379saddlepaddle merged 1 commit intomainfrom
saddlepaddle merged 1 commit intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements comprehensive project icon management for the desktop application. It adds favicon discovery from project directories, icon upload/deletion via TRPC procedures, disk-based icon persistence, protocol-based icon serving, database schema updates, and UI components for displaying and managing icons across the sidebar and settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ProjectSettings UI
participant TRPC as TRPC Router
participant IconMgr as Icon Manager
participant FS as File System
participant DB as Database
User->>UI: Selects image file
UI->>UI: Convert to data URL
User->>UI: Clicks upload
UI->>TRPC: setProjectIcon({id, icon: dataUrl})
TRPC->>IconMgr: saveProjectIconFromDataUrl()
IconMgr->>FS: Decode & validate data URL
FS->>FS: Check size ≤ 512KB
IconMgr->>FS: Write to PROJECT_ICONS_DIR/{id}.{ext}
FS-->>IconMgr: Protocol URL (superset-icon://)
IconMgr-->>TRPC: Return URL
TRPC->>DB: Update project.iconUrl
DB-->>TRPC: Success
TRPC-->>UI: Return iconUrl
UI->>UI: Invalidate cache & refresh
UI-->>User: Display new icon
sequenceDiagram
participant App as Desktop App
participant Startup as App Init
participant Registry as Protocol Registry
participant User as User Request
participant Handler as Icon Handler
participant FS as File System
participant Browser as Browser Window
Startup->>App: App starts
App->>Startup: ensureProjectIconsDir()
Startup->>FS: Create PROJECT_ICONS_DIR
App->>Registry: Register superset-icon protocol
Registry-->>App: Protocol registered (privileged)
App->>Handler: Set up protocol handler
Handler->>Handler: Map superset-icon:// to disk path
User->>Browser: Load project with iconUrl
Browser->>Handler: Request superset-icon://projectId
Handler->>FS: Get icon file path
FS-->>Handler: File exists
Handler->>FS: Read file (pathToFileURL)
FS-->>Handler: File contents
Handler-->>Browser: Serve icon image
Browser->>Browser: Render icon in UI
sequenceDiagram
participant User
participant UI as Project UI
participant TRPC as TRPC Router
participant Favicon as Favicon Discovery
participant FS as File System
participant IconMgr as Icon Manager
participant DB as Database
User->>UI: Opens project (first time)
UI->>TRPC: triggerFaviconDiscovery({id})
TRPC->>Favicon: discoverAndSaveProjectIcon()
Favicon->>FS: Scan for favicon patterns (glob)
FS->>FS: Exclude node_modules, .git, dist, build
FS-->>Favicon: Match found (e.g., favicon.png)
Favicon->>FS: Check file size ≤ 256KB
FS-->>Favicon: Size OK
Favicon->>IconMgr: saveProjectIconFromFile()
IconMgr->>FS: Copy icon to PROJECT_ICONS_DIR
FS-->>IconMgr: Protocol URL
IconMgr-->>Favicon: URL
Favicon-->>TRPC: Return iconUrl
TRPC->>DB: Update project.iconUrl
DB-->>TRPC: Success
TRPC-->>UI: { iconUrl }
UI->>UI: Display discovered icon
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/projects/utils/favicon-discovery.ts`:
- Around line 47-58: fast-glob returns files in filesystem order, so using
matches[0] can pick a lower-priority pattern; after the fg(...) call in
favicon-discovery.ts, re-sort the matches array according to the priority of
FAVICON_PATTERNS before selecting the iconPath: for each matched path (matches),
determine which FAVICON_PATTERNS entry it satisfies (check patterns in order and
pick the first that matches), assign that pattern index as its priority key,
then sort matches by that priority key (and optionally by filename length/newer
tie-breaker) and set iconPath = sortedMatches[0]; reference the symbols
FAVICON_PATTERNS, fg(...), matches and iconPath when making the change.
In `@apps/desktop/src/main/index.ts`:
- Around line 247-264: The superset-icon protocol handler (iconProtocolHandler)
returns the file response for getProjectIconPath(projectId) with a stable URL,
which can be cached by Chromium; update iconProtocolHandler so responses include
cache-busting (either append a query param like ?v={timestampOrHash} derived
from the file mtime/hash stored alongside iconUrl, or set response headers such
as Cache-Control: no-cache) before returning the Response from protocol.handle
and session.fromPartition(...).protocol.handle to ensure updated icons are
fetched immediately after replacement.
In `@apps/desktop/src/main/lib/project-icons.ts`:
- Around line 91-95: The regex used to parse the data URL in project-icons.ts
rejects MIME subtypes like "svg+xml" and "x-icon"; update the pattern used in
the dataUrl.match(...) call (the variable `match`) to allow plus signs, hyphens
and dots in the subtype (e.g., use a character class that permits letters,
digits, plus, dot and hyphen instead of `\w+`) so
`data:image/svg+xml;base64,...` and `data:image/x-icon;base64,...` succeed and
the existing error throw on invalid formats remains unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/ProjectSettings.tsx:
- Around line 104-120: The FileReader in handleFileChange lacks an onerror
handler so read failures are silently ignored; add reader.onerror to capture the
error, call setProjectIcon.mutate or a provided error handler/state with an
error result (or call a UI notification) and log the error so the user receives
feedback, and ensure you still clear e.target.value after handling failures;
reference handleFileChange, reader, and setProjectIcon in your changes.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectThumbnail/ProjectThumbnail.tsx`:
- Line 47: The iconError state (created via useState in ProjectThumbnail) is not
reset when iconUrl changes, so if an earlier icon load failed the component will
keep skipping the icon branch after a new icon is provided; fix by watching
iconUrl and resetting iconError to false when it changes (add a useEffect that
calls setIconError(false) whenever iconUrl updates) and ensure you add useRef to
the React imports if you end up using it elsewhere; also apply the same reset
logic for any other error states referenced in the 70-89 block that depend on
iconUrl.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
1100-1106: Add a max-length constraint on theiconinput string.A base64-encoded 512 KB image produces ~700 KB of text. Without a schema-level bound, an arbitrarily large string could be sent before the server-side size check in
saveProjectIconFromDataUrlrejects it. Adding a limit here protects the tRPC transport layer.Suggested fix
.input( z.object({ id: z.string(), - icon: z.string().nullable(), + icon: z.string().max(1_000_000).nullable(), }), )apps/desktop/src/main/lib/project-icons.ts (1)
59-74:saveProjectIconFromFiledoesn't enforce the size limit.
saveProjectIconFromDataUrlandsaveProjectIconFromBufferboth validate againstMAX_ICON_SIZE, butsaveProjectIconFromFilecopies the file without any size check. When called fromfavicon-discovery.ts, the caller enforces a 256 KB limit viastat, but a future caller could bypass this.Suggested fix
export async function saveProjectIconFromFile({ projectId, sourcePath, }: { projectId: string; sourcePath: string; }): Promise<string> { ensureProjectIconsDir(); removeExistingIcon(projectId); + const { size } = await import("node:fs/promises").then((fs) => fs.stat(sourcePath)); + if (size > MAX_ICON_SIZE) { + throw new Error( + `Icon file too large (${Math.round(size / 1024)}KB). Maximum is ${MAX_ICON_SIZE / 1024}KB.`, + ); + } + const ext = extname(sourcePath) || ".png";Or simply use the already-imported
statfromnode:fs/promises(not currently imported — you'd need to add it):+import { copyFile, stat, writeFile } from "node:fs/promises";
Comment on lines
+47
to
+58
| try { | ||
| const matches = await fg(FAVICON_PATTERNS, { | ||
| cwd: repoPath, | ||
| absolute: true, | ||
| ignore: ["**/node_modules/**", "**/.git/**", "**/dist/**", "**/build/**"], | ||
| onlyFiles: true, | ||
| }); | ||
|
|
||
| if (matches.length === 0) return null; | ||
|
|
||
| // Use the first match (ordered by FAVICON_PATTERNS priority) | ||
| const iconPath = matches[0]; |
Contributor
There was a problem hiding this comment.
fast-glob does not return matches ordered by input pattern priority.
The comment on line 57 says "ordered by FAVICON_PATTERNS priority," but fast-glob returns results in filesystem traversal order, not in the order the patterns were supplied. A lower-priority match (e.g., assets/favicon.ico) may be returned before a higher-priority one (favicon.ico).
Suggested fix — re-sort matches by pattern priority
const matches = await fg(FAVICON_PATTERNS, {
cwd: repoPath,
absolute: true,
ignore: ["**/node_modules/**", "**/.git/**", "**/dist/**", "**/build/**"],
onlyFiles: true,
});
if (matches.length === 0) return null;
-// Use the first match (ordered by FAVICON_PATTERNS priority)
-const iconPath = matches[0];
+// Sort matches by FAVICON_PATTERNS priority order
+const iconPath = matches.sort((a, b) => {
+ const aIndex = FAVICON_PATTERNS.findIndex((p) => a.endsWith(p));
+ const bIndex = FAVICON_PATTERNS.findIndex((p) => b.endsWith(p));
+ return (aIndex === -1 ? Infinity : aIndex) - (bIndex === -1 ? Infinity : bIndex);
+})[0];🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/projects/utils/favicon-discovery.ts` around
lines 47 - 58, fast-glob returns files in filesystem order, so using matches[0]
can pick a lower-priority pattern; after the fg(...) call in
favicon-discovery.ts, re-sort the matches array according to the priority of
FAVICON_PATTERNS before selecting the iconPath: for each matched path (matches),
determine which FAVICON_PATTERNS entry it satisfies (check patterns in order and
pick the first that matches), assign that pattern index as its priority key,
then sort matches by that priority key (and optionally by filename length/newer
tie-breaker) and set iconPath = sortedMatches[0]; reference the symbols
FAVICON_PATTERNS, fg(...), matches and iconPath when making the change.
Comment on lines
+247
to
+264
| // Register protocol handler for superset-icon:// URLs | ||
| // Must register on BOTH default session and the app's custom partition | ||
| const iconProtocolHandler = (request: Request) => { | ||
| const url = new URL(request.url); | ||
| // superset-icon://projects/{projectId} → file on disk | ||
| const projectId = url.pathname.replace(/^\//, ""); | ||
| const iconPath = getProjectIconPath(projectId); | ||
| if (!iconPath) { | ||
| return new Response("Not found", { status: 404 }); | ||
| } | ||
| return net.fetch(pathToFileURL(iconPath).toString()); | ||
| }; | ||
| protocol.handle("superset-icon", iconProtocolHandler); | ||
| session | ||
| .fromPartition("persist:superset") | ||
| .protocol.handle("superset-icon", iconProtocolHandler); | ||
|
|
||
| ensureProjectIconsDir(); |
Contributor
There was a problem hiding this comment.
Potential stale cache when icons are replaced.
The protocol URL is stable (superset-icon://projects/{projectId}) and never changes even when the underlying icon file is replaced. Chromium/Electron may cache the old response, causing the UI to show the previous icon after an upload/replace until the session cache is cleared or the app restarts.
Consider appending a cache-busting query param (e.g., a timestamp or hash stored alongside iconUrl) or setting Cache-Control: no-cache on the response:
Suggested response with cache-control header
const iconProtocolHandler = (request: Request) => {
const url = new URL(request.url);
const projectId = url.pathname.replace(/^\//, "");
const iconPath = getProjectIconPath(projectId);
if (!iconPath) {
return new Response("Not found", { status: 404 });
}
- return net.fetch(pathToFileURL(iconPath).toString());
+ return net.fetch(pathToFileURL(iconPath).toString()).then((res) => {
+ const headers = new Headers(res.headers);
+ headers.set("Cache-Control", "no-cache");
+ return new Response(res.body, {
+ status: res.status,
+ statusText: res.statusText,
+ headers,
+ });
+ });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Register protocol handler for superset-icon:// URLs | |
| // Must register on BOTH default session and the app's custom partition | |
| const iconProtocolHandler = (request: Request) => { | |
| const url = new URL(request.url); | |
| // superset-icon://projects/{projectId} → file on disk | |
| const projectId = url.pathname.replace(/^\//, ""); | |
| const iconPath = getProjectIconPath(projectId); | |
| if (!iconPath) { | |
| return new Response("Not found", { status: 404 }); | |
| } | |
| return net.fetch(pathToFileURL(iconPath).toString()); | |
| }; | |
| protocol.handle("superset-icon", iconProtocolHandler); | |
| session | |
| .fromPartition("persist:superset") | |
| .protocol.handle("superset-icon", iconProtocolHandler); | |
| ensureProjectIconsDir(); | |
| // Register protocol handler for superset-icon:// URLs | |
| // Must register on BOTH default session and the app's custom partition | |
| const iconProtocolHandler = (request: Request) => { | |
| const url = new URL(request.url); | |
| // superset-icon://projects/{projectId} → file on disk | |
| const projectId = url.pathname.replace(/^\//, ""); | |
| const iconPath = getProjectIconPath(projectId); | |
| if (!iconPath) { | |
| return new Response("Not found", { status: 404 }); | |
| } | |
| return net.fetch(pathToFileURL(iconPath).toString()).then((res) => { | |
| const headers = new Headers(res.headers); | |
| headers.set("Cache-Control", "no-cache"); | |
| return new Response(res.body, { | |
| status: res.status, | |
| statusText: res.statusText, | |
| headers, | |
| }); | |
| }); | |
| }; | |
| protocol.handle("superset-icon", iconProtocolHandler); | |
| session | |
| .fromPartition("persist:superset") | |
| .protocol.handle("superset-icon", iconProtocolHandler); | |
| ensureProjectIconsDir(); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/index.ts` around lines 247 - 264, The superset-icon
protocol handler (iconProtocolHandler) returns the file response for
getProjectIconPath(projectId) with a stable URL, which can be cached by
Chromium; update iconProtocolHandler so responses include cache-busting (either
append a query param like ?v={timestampOrHash} derived from the file mtime/hash
stored alongside iconUrl, or set response headers such as Cache-Control:
no-cache) before returning the Response from protocol.handle and
session.fromPartition(...).protocol.handle to ensure updated icons are fetched
immediately after replacement.
Comment on lines
+91
to
+95
| // Parse data URL: data:image/png;base64,<data> | ||
| const match = dataUrl.match(/^data:image\/(\w+);base64,(.+)$/); | ||
| if (!match) { | ||
| throw new Error("Invalid data URL format"); | ||
| } |
Contributor
There was a problem hiding this comment.
Data URL regex rejects SVG and ICO uploads — \w+ doesn't match svg+xml or x-icon.
The file picker accepts image/svg+xml and image/x-icon, but the regex /^data:image\/(\w+);base64,(.+)$/ only matches word characters ([a-zA-Z0-9_]) for the MIME subtype. Data URLs for SVGs (data:image/svg+xml;base64,...) and ICOs (data:image/x-icon;base64,...) will fail with "Invalid data URL format".
Suggested fix
-const match = dataUrl.match(/^data:image\/(\w+);base64,(.+)$/);
+const match = dataUrl.match(/^data:image\/([\w+\-.]+);base64,(.+)$/);
if (!match) {
throw new Error("Invalid data URL format");
}
-const ext = match[1] === "jpeg" ? "jpg" : match[1];
+const subtype = match[1];
+const EXT_MAP: Record<string, string> = {
+ jpeg: "jpg",
+ "svg+xml": "svg",
+ "x-icon": "ico",
+};
+const ext = EXT_MAP[subtype] ?? subtype;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Parse data URL: data:image/png;base64,<data> | |
| const match = dataUrl.match(/^data:image\/(\w+);base64,(.+)$/); | |
| if (!match) { | |
| throw new Error("Invalid data URL format"); | |
| } | |
| // Parse data URL: data:image/png;base64,<data> | |
| const match = dataUrl.match(/^data:image\/([\w+\-.]+);base64,(.+)$/); | |
| if (!match) { | |
| throw new Error("Invalid data URL format"); | |
| } | |
| const subtype = match[1]; | |
| const EXT_MAP: Record<string, string> = { | |
| jpeg: "jpg", | |
| "svg+xml": "svg", | |
| "x-icon": "ico", | |
| }; | |
| const ext = EXT_MAP[subtype] ?? subtype; |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/project-icons.ts` around lines 91 - 95, The regex
used to parse the data URL in project-icons.ts rejects MIME subtypes like
"svg+xml" and "x-icon"; update the pattern used in the dataUrl.match(...) call
(the variable `match`) to allow plus signs, hyphens and dots in the subtype
(e.g., use a character class that permits letters, digits, plus, dot and hyphen
instead of `\w+`) so `data:image/svg+xml;base64,...` and
`data:image/x-icon;base64,...` succeed and the existing error throw on invalid
formats remains unchanged.
Comment on lines
+104
to
+120
| const handleFileChange = useCallback( | ||
| (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const file = e.target.files?.[0]; | ||
| if (!file) return; | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| const dataUrl = reader.result as string; | ||
| setProjectIcon.mutate({ id: projectId, icon: dataUrl }); | ||
| }; | ||
| reader.readAsDataURL(file); | ||
|
|
||
| // Reset input so the same file can be re-selected | ||
| e.target.value = ""; | ||
| }, | ||
| [projectId, setProjectIcon], | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing reader.onerror handler — FileReader failures are silently swallowed.
If readAsDataURL fails (e.g., the file can't be read), no feedback is given to the user.
Suggested fix
const reader = new FileReader();
reader.onload = () => {
const dataUrl = reader.result as string;
setProjectIcon.mutate({ id: projectId, icon: dataUrl });
};
+reader.onerror = () => {
+ console.error("[project-settings/icon] Failed to read file");
+};
reader.readAsDataURL(file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleFileChange = useCallback( | |
| (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0]; | |
| if (!file) return; | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| const dataUrl = reader.result as string; | |
| setProjectIcon.mutate({ id: projectId, icon: dataUrl }); | |
| }; | |
| reader.readAsDataURL(file); | |
| // Reset input so the same file can be re-selected | |
| e.target.value = ""; | |
| }, | |
| [projectId, setProjectIcon], | |
| ); | |
| const handleFileChange = useCallback( | |
| (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0]; | |
| if (!file) return; | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| const dataUrl = reader.result as string; | |
| setProjectIcon.mutate({ id: projectId, icon: dataUrl }); | |
| }; | |
| reader.onerror = () => { | |
| console.error("[project-settings/icon] Failed to read file"); | |
| }; | |
| reader.readAsDataURL(file); | |
| // Reset input so the same file can be re-selected | |
| e.target.value = ""; | |
| }, | |
| [projectId, setProjectIcon], | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/ProjectSettings.tsx
around lines 104 - 120, The FileReader in handleFileChange lacks an onerror
handler so read failures are silently ignored; add reader.onerror to capture the
error, call setProjectIcon.mutate or a provided error handler/state with an
error result (or call a UI notification) and log the error so the user receives
feedback, and ensure you still clear e.target.value after handling failures;
reference handleFileChange, reader, and setProjectIcon in your changes.
| className, | ||
| }: ProjectThumbnailProps) { | ||
| const [imageError, setImageError] = useState(false); | ||
| const [iconError, setIconError] = useState(false); |
Contributor
There was a problem hiding this comment.
iconError is never reset when iconUrl changes — a replaced icon won't render until remount.
If the first icon fails to load (iconError = true) and the user then uploads a new icon (new iconUrl), the component still skips the icon branch because iconError remains true.
Suggested fix — reset error state when iconUrl changes
const [iconError, setIconError] = useState(false);
+
+// Reset icon error when the URL changes (e.g. after upload/replace)
+const prevIconUrl = useRef(iconUrl);
+if (prevIconUrl.current !== iconUrl) {
+ prevIconUrl.current = iconUrl;
+ setIconError(false);
+}You'll need to add useRef to the import from react.
Also applies to: 70-89
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectThumbnail/ProjectThumbnail.tsx`
at line 47, The iconError state (created via useState in ProjectThumbnail) is
not reset when iconUrl changes, so if an earlier icon load failed the component
will keep skipping the icon branch after a new icon is provided; fix by watching
iconUrl and resetting iconError to false when it changes (add a useEffect that
calls setIconError(false) whenever iconUrl updates) and ensure you add useRef to
the React imports if you end up using it elsewhere; also apply the same reset
logic for any other error states referenced in the 70-89 block that depend on
iconUrl.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
featicons-rework
branch
from
82f2bed to
555cd34
Compare
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
Rework project icons to store files on disk and serve via superset-icon:// custom Electron protocol instead of base64 in SQLite. - Add project-icons.ts utility for disk storage at ~/.superset/project-icons/ - Register superset-icon:// protocol on both default and persist:superset sessions - Add favicon auto-discovery from project directories using fast-glob - Add triggerFaviconDiscovery and setProjectIcon tRPC mutations - Add icon upload/replace/remove UI in Project Settings - Thread iconUrl through sidebar component chain - Schema: add single icon_url column to projects table Co-authored-by: Ipriyankrajai <priyank73@hotmail.com>
saddlepaddle
force-pushed
the
featicons-rework
branch
from
2f07e84 to
ad0c04d
Compare
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/project-icons.ts`:
- Around line 40-45: removeExistingIcon currently uses getProjectIconPath which
returns only the first match, so leftover icon files with other extensions can
remain; modify removeExistingIcon to enumerate the project icons directory
(e.g., with fs.readdirSync or a glob) and delete all files whose basename
matches the projectId (instead of calling getProjectIconPath and unlinking a
single file). Keep using unlinkSync for removal and reference getProjectIconPath
only if you need to compute the directory path or reuse its logic, but ensure
all matching files (projectId.*) are removed.
- Around line 59-74: saveProjectIconFromFile currently copies the source file
without enforcing MAX_ICON_SIZE like the other save paths; before calling
copyFile in saveProjectIconFromFile, use statSync(sourcePath).size to check the
file size against MAX_ICON_SIZE and bail out (throw or return an error) if it
exceeds the limit, keeping the existing ensureProjectIconsDir and
removeExistingIcon calls and preserving extname/PROJECT_ICONS_DIR usage; also
add statSync to the file imports if not already imported.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
1115-1160: Errors fromsaveProjectIconFromDataUrlsurface as INTERNAL_SERVER_ERROR.If the data URL is malformed or the icon exceeds 512KB,
saveProjectIconFromDataUrlthrows a plainError. tRPC will wrap this asINTERNAL_SERVER_ERROR, but these are client input validation failures that should beBAD_REQUEST.Proposed fix: catch and rethrow as BAD_REQUEST
// Save icon from data URL - const iconUrl = await saveProjectIconFromDataUrl({ - projectId: input.id, - dataUrl: input.icon, - }); + let iconUrl: string; + try { + iconUrl = await saveProjectIconFromDataUrl({ + projectId: input.id, + dataUrl: input.icon, + }); + } catch (error) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: error instanceof Error ? error.message : "Failed to save icon", + }); + }apps/desktop/src/main/lib/project-icons.ts (1)
25-35: No input sanitization onprojectIdused in file path operations.While projectIds are UUIDs in practice, this function and the save functions use
projectIddirectly in file paths without validation. A defensive UUID format check would harden against misuse if these functions are ever called with untrusted input.Optional: add a UUID format guard
+const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +function assertValidProjectId(projectId: string): void { + if (!UUID_REGEX.test(projectId)) { + throw new Error(`Invalid project ID format: ${projectId}`); + } +}Then call
assertValidProjectId(projectId)at the top of each public function.
Comment on lines
+40
to
+45
| function removeExistingIcon(projectId: string): void { | ||
| const existing = getProjectIconPath(projectId); | ||
| if (existing) { | ||
| unlinkSync(existing); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
removeExistingIcon only removes the first matching file.
getProjectIconPath returns the first match. If somehow multiple icon files exist for the same project with different extensions (e.g., from a race condition or a bug), only the first is cleaned up. The leftover file could be picked up later by getProjectIconPath after the new icon is saved with a different extension.
Proposed fix: remove all matching files
function removeExistingIcon(projectId: string): void {
- const existing = getProjectIconPath(projectId);
- if (existing) {
- unlinkSync(existing);
+ if (!existsSync(PROJECT_ICONS_DIR)) return;
+ const files = readdirSync(PROJECT_ICONS_DIR);
+ for (const f of files) {
+ const name = f.substring(0, f.lastIndexOf("."));
+ if (name === projectId) {
+ unlinkSync(join(PROJECT_ICONS_DIR, f));
+ }
}
}🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/project-icons.ts` around lines 40 - 45,
removeExistingIcon currently uses getProjectIconPath which returns only the
first match, so leftover icon files with other extensions can remain; modify
removeExistingIcon to enumerate the project icons directory (e.g., with
fs.readdirSync or a glob) and delete all files whose basename matches the
projectId (instead of calling getProjectIconPath and unlinking a single file).
Keep using unlinkSync for removal and reference getProjectIconPath only if you
need to compute the directory path or reuse its logic, but ensure all matching
files (projectId.*) are removed.
Comment on lines
+59
to
+74
| export async function saveProjectIconFromFile({ | ||
| projectId, | ||
| sourcePath, | ||
| }: { | ||
| projectId: string; | ||
| sourcePath: string; | ||
| }): Promise<string> { | ||
| ensureProjectIconsDir(); | ||
| removeExistingIcon(projectId); | ||
|
|
||
| const ext = extname(sourcePath) || ".png"; | ||
| const destPath = join(PROJECT_ICONS_DIR, `${projectId}${ext}`); | ||
| await copyFile(sourcePath, destPath); | ||
|
|
||
| return getProjectIconProtocolUrl(projectId); | ||
| } |
Contributor
There was a problem hiding this comment.
saveProjectIconFromFile skips the MAX_ICON_SIZE check that other save paths enforce.
Both saveProjectIconFromDataUrl and saveProjectIconFromBuffer validate against MAX_ICON_SIZE, but saveProjectIconFromFile copies the source file without any size check. If favicon discovery finds a large file (e.g., a high-res logo.png), it will be saved and served without limit.
Proposed fix: add size validation before copying
+import { statSync } from "node:fs";
+
export async function saveProjectIconFromFile({
projectId,
sourcePath,
}: {
projectId: string;
sourcePath: string;
}): Promise<string> {
ensureProjectIconsDir();
+
+ const fileSize = statSync(sourcePath).size;
+ if (fileSize > MAX_ICON_SIZE) {
+ throw new Error(
+ `Icon file too large (${Math.round(fileSize / 1024)}KB). Maximum is ${MAX_ICON_SIZE / 1024}KB.`,
+ );
+ }
+
removeExistingIcon(projectId);
const ext = extname(sourcePath) || ".png";
const destPath = join(PROJECT_ICONS_DIR, `${projectId}${ext}`);
await copyFile(sourcePath, destPath);
return getProjectIconProtocolUrl(projectId);
}Note: statSync is already imported on line 1 (via existsSync from node:fs) — just add it to the import list.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/project-icons.ts` around lines 59 - 74,
saveProjectIconFromFile currently copies the source file without enforcing
MAX_ICON_SIZE like the other save paths; before calling copyFile in
saveProjectIconFromFile, use statSync(sourcePath).size to check the file size
against MAX_ICON_SIZE and bail out (throw or return an error) if it exceeds the
limit, keeping the existing ensureProjectIconsDir and removeExistingIcon calls
and preserving extname/PROJECT_ICONS_DIR usage; also add statSync to the file
imports if not already imported.
2 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rework of #1168 by @Ipriyankrajai — stores project icons on disk and serves them via a custom
superset-icon://Electron protocol instead of base64 in SQLite.~/.superset/project-icons/{projectId}.{ext}and serve viasuperset-icon://protocol — making<img src={project.iconUrl}>work transparentlyfavicon.ico,logo.png, etc.)iconUrlthrough the sidebar component chain (WorkspaceSidebar→ProjectSection→ProjectHeader→ProjectThumbnail)Changes
Backend (main process):
project-icons.ts— new utility for icon disk storage (save from file/data URL/buffer, delete, lookup)favicon-discovery.ts— auto-discovers favicons in project repos usingfast-globwith sensible ignore patternsmain/index.ts— registerssuperset-icon://protocol on both default andpersist:supersetsessionsprojects.ts— addstriggerFaviconDiscoveryandsetProjectIcontRPC mutationsSchema:
icon_urltext column to projects table (migration 0020)Frontend:
ProjectThumbnail— renders icon with priority:iconUrl→ GitHub avatar → first letter fallbackProjectSettings— adds "Project Icon" section with upload/replace/remove controlsWorkspaceSidebar→ProjectSection→ProjectHeaderTest plan
favicon.icoin its root → triggers auto-discovery, icon appears in sidebarbun run typecheckpasses (verified)bun run testpasses (verified — 1205 pass, 0 fail)Closes #1168
Summary by CodeRabbit
Release Notes