feat: favicons + custom port during dev#109
Merged
juliusmarminge merged 6 commits intomainfrom Feb 27, 2026
Merged
Conversation
|
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:
WalkthroughAdds a project favicon discovery and serving endpoint on the server, a Sidebar ProjectFavicon React component that fetches and displays project favicons, a workspace favicon validation ES module test script, new session-turn settlement logic, and updates the default dev server port from 5173 to 5733 across configs and scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DevHTTP as Dev HTTP Server
participant FS as FileSystem
participant Vite as Vite Dev Server
Browser->>DevHTTP: GET /api/project-favicon?cwd=/project
activate DevHTTP
DevHTTP->>FS: Phase 1 — check well-known favicon candidate paths
FS-->>DevHTTP: found / not found
alt Found in Phase 1
DevHTTP->>FS: read file bytes
FS-->>DevHTTP: file bytes
else Not found
DevHTTP->>FS: Phase 2 — read known source files (index.html, app files)
FS-->>DevHTTP: file contents
DevHTTP->>DevHTTP: extract href via regex and normalize
DevHTTP->>FS: resolve href in public/ then project root
FS-->>DevHTTP: resolved path / not found
alt Resolved
DevHTTP->>FS: read file bytes
FS-->>DevHTTP: file bytes
end
end
DevHTTP-->>Browser: 200 + favicon bytes (with Content-Type/Cache) or 404
deactivate DevHTTP
Browser->>Vite: dev server requests (unchanged) — dev redirect occurs elsewhere if not handled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/server/src/wsServer.ts (1)
314-320: Consider validatingcwdis an absolute path to limit exposure.The
cwdparameter is user-provided and passed directly topath.join. While the path traversal checks (lines 330, 376) prevent serving files outsideprojectCwd, a malicious client could potentially probe the filesystem structure by checking response times or errors for arbitrary paths.Consider adding validation that
cwdis an absolute path and optionally checking it against known project directories:Suggested validation
const projectCwd = url.searchParams.get("cwd"); if (!projectCwd) { res.writeHead(400, { "Content-Type": "text/plain" }); res.end("Missing cwd parameter"); return; } + + // Validate cwd is an absolute path + if (!path.isAbsolute(projectCwd)) { + res.writeHead(400, { "Content-Type": "text/plain" }); + res.end("Invalid cwd parameter: must be absolute path"); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/wsServer.ts` around lines 314 - 320, Validate the user-provided cwd before using it: in the request handler that checks url.pathname === "/api/project-favicon", ensure projectCwd (from url.searchParams.get("cwd")) is an absolute path (use path.isAbsolute equivalent) and reject non-absolute values with a 400; additionally, if your app has a list or resolver for allowed project roots (e.g., knownProjectDirs or a function that maps project IDs to paths), verify projectCwd is inside that allowlist or canonicalize and compare against allowed roots before calling path.join and the favicon-serving code, returning 403/400 for invalid/untrusted paths.apps/web/src/components/Sidebar.tsx (1)
167-185: Consider showing a placeholder during loading state.The component currently renders a hidden
<img>during loading, which means no visual feedback until the image loads or errors. While this avoids flicker for fast loads, it leaves an empty space in the layout.If this is intentional to prevent layout shift, the current approach is fine. Otherwise, consider showing the
FolderIconas a placeholder during loading:Optional: Show placeholder during loading
function ProjectFavicon({ cwd }: { cwd: string }) { const [status, setStatus] = useState<"loading" | "loaded" | "error">("loading"); const src = `${serverHttpOrigin}/api/project-favicon?cwd=${encodeURIComponent(cwd)}`; - if (status === "error") { + if (status === "error" || status === "loading") { return <FolderIcon className="size-3.5 shrink-0 text-muted-foreground/50" />; } return ( - <img - src={src} - alt="" - className={`size-3.5 shrink-0 rounded-sm object-contain ${status === "loading" ? "hidden" : ""}`} - onLoad={() => setStatus("loaded")} - onError={() => setStatus("error")} - /> + <> + <img + src={src} + alt="" + className="size-3.5 shrink-0 rounded-sm object-contain" + onLoad={() => setStatus("loaded")} + onError={() => setStatus("error")} + /> + </> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 167 - 185, ProjectFavicon currently renders a hidden <img> while loading which provides no visual feedback; update the ProjectFavicon component so it shows the FolderIcon placeholder when status is "loading" (and still when "error"), and only render the <img> when status is "loaded" (keeping the existing onLoad/onError handlers to flip status and preserving the size classes to avoid layout shift). This change should be made inside the ProjectFavicon function, toggling between FolderIcon and the img based on the status state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.test-favicon.mjs:
- Around line 52-54: The test file .test-favicon.mjs contains hardcoded
developer-specific paths in the test(...) calls; remove those absolute paths and
make the inputs configurable instead: either delete the two lines and only call
test(process.cwd()), or change the test invocation to read paths from CLI args
(process.argv) or an environment variable (e.g., process.env.TEST_PATHS) and
fall back to process.cwd(), or move these checks into a proper test with mocked
directories; update the test(...) calls to use the chosen configurable source so
no machine-specific absolute paths remain.
---
Nitpick comments:
In `@apps/server/src/wsServer.ts`:
- Around line 314-320: Validate the user-provided cwd before using it: in the
request handler that checks url.pathname === "/api/project-favicon", ensure
projectCwd (from url.searchParams.get("cwd")) is an absolute path (use
path.isAbsolute equivalent) and reject non-absolute values with a 400;
additionally, if your app has a list or resolver for allowed project roots
(e.g., knownProjectDirs or a function that maps project IDs to paths), verify
projectCwd is inside that allowlist or canonicalize and compare against allowed
roots before calling path.join and the favicon-serving code, returning 403/400
for invalid/untrusted paths.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 167-185: ProjectFavicon currently renders a hidden <img> while
loading which provides no visual feedback; update the ProjectFavicon component
so it shows the FolderIcon placeholder when status is "loading" (and still when
"error"), and only render the <img> when status is "loaded" (keeping the
existing onLoad/onError handlers to flip status and preserving the size classes
to avoid layout shift). This change should be made inside the ProjectFavicon
function, toggling between FolderIcon and the img based on the status state.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.test-favicon.mjsREADME.mdapps/desktop/scripts/dev-electron.mjsapps/server/package.jsonapps/server/src/wsServer.tsapps/web/src/components/Sidebar.tsxapps/web/vite.config.tsscripts/dev-runner.mjs
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| // Serve project favicons from their working directories. | ||
| // Handled before the dev redirect so it works in both modes. | ||
| if (url.pathname === "/api/project-favicon") { |
Contributor
There was a problem hiding this comment.
🟡 Medium src/wsServer.ts:314
The /api/project-favicon endpoint bypasses auth (only WebSocket upgrades check authToken) and reads user-controlled paths into memory without size limits. Consider adding auth validation and a file size check before fs.readFile to prevent memory exhaustion attacks.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/wsServer.ts around line 314:
The `/api/project-favicon` endpoint bypasses auth (only WebSocket upgrades check `authToken`) and reads user-controlled paths into memory without size limits. Consider adding auth validation and a file size check before `fs.readFile` to prevent memory exhaustion attacks.
Evidence trail:
apps/server/src/wsServer.ts lines 306-391 (HTTP server handler for /api/project-favicon - no auth check), lines 749-762 (WebSocket upgrade auth check using authToken), lines 288-303 (serveFaviconFile function with fs.readFile and no size limit), lines 315 (user-controlled cwd from query params), commit REVIEWED_COMMIT
- Add `isLatestTurnSettled` to avoid treating an active running turn as complete - Use the guard in `ChatView` for visited timestamps and completion summary/divider rendering - Add unit tests covering running, ready, and incomplete-timestamp cases
juliusmarminge
force-pushed
the
theo/favicons
branch
from
48a4b2d to
08ea222
Compare
- move `/api/project-favicon` handling into a dedicated server route with coverage for root and source-linked icons - wire `wsServer` to use the new route helper - treat any running session as unsettled in `isLatestTurnSettled` to prevent stale latest-turn banners, with test coverage
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/wsServer.ts`:
- Around line 315-331: The code trusts the user-supplied projectCwd and uses
brittle string-prefix containment checks in tryCandidates and trySourceFiles,
allowing path traversal; fix by normalizing/resolving and validating projectCwd
and candidate paths before any filesystem access: call
path.resolve/path.normalize on projectCwd and each candidate, ensure projectCwd
is absolute and exists (fs.stat) and then verify containment with path.relative
(reject if relative starts with '..' or resolves to outside projectCwd), and
replace string-prefix checks (e.g., candidate.startsWith(projectCwd + path.sep))
with this robust containment check in the functions tryCandidates and
trySourceFiles and where FAVICON_CANDIDATES are used.
- Around line 295-298: When handling the file read error in
apps/server/src/wsServer.ts (the branch that checks readErr after reading a
file), change the response to return 404 when readErr.code === 'ENOENT' instead
of a generic 500; otherwise keep the 500 behavior. Specifically, in the block
that currently does "if (readErr) { res.writeHead(500...); res.end('Read
error'); }", inspect readErr.code and call res.writeHead(404, { "Content-Type":
"text/plain" }) with a "Not Found" body for ENOENT, and fall back to 500 for
other errors. Ensure you reference the existing variables readErr and res so the
change is localized.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.test-favicon.mjsREADME.mdapps/desktop/scripts/dev-electron.mjsapps/server/package.jsonapps/server/src/wsServer.tsapps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/vite.config.tsscripts/dev-runner.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/dev-runner.mjs
- .test-favicon.mjs
- apps/web/src/components/Sidebar.tsx
- apps/server/package.json
- apps/web/vite.config.ts
- README.md
- apps/desktop/scripts/dev-electron.mjs
Comment on lines
+295
to
+298
| if (readErr) { | ||
| res.writeHead(500, { "Content-Type": "text/plain" }); | ||
| res.end("Read error"); | ||
| return; |
There was a problem hiding this comment.
Return 404 for ENOENT read races instead of generic 500.
If a file disappears between stat and read, this path currently reports a server error; that should be a not-found response.
🩹 Suggested error mapping
if (readErr) {
- res.writeHead(500, { "Content-Type": "text/plain" });
- res.end("Read error");
+ const code = (readErr as NodeJS.ErrnoException).code;
+ const status = code === "ENOENT" ? 404 : 500;
+ res.writeHead(status, { "Content-Type": "text/plain" });
+ res.end(status === 404 ? "Not Found" : "Read error");
return;
}📝 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
| if (readErr) { | |
| res.writeHead(500, { "Content-Type": "text/plain" }); | |
| res.end("Read error"); | |
| return; | |
| if (readErr) { | |
| const code = (readErr as NodeJS.ErrnoException).code; | |
| const status = code === "ENOENT" ? 404 : 500; | |
| res.writeHead(status, { "Content-Type": "text/plain" }); | |
| res.end(status === 404 ? "Not Found" : "Read error"); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/wsServer.ts` around lines 295 - 298, When handling the file
read error in apps/server/src/wsServer.ts (the branch that checks readErr after
reading a file), change the response to return 404 when readErr.code ===
'ENOENT' instead of a generic 500; otherwise keep the 500 behavior.
Specifically, in the block that currently does "if (readErr) {
res.writeHead(500...); res.end('Read error'); }", inspect readErr.code and call
res.writeHead(404, { "Content-Type": "text/plain" }) with a "Not Found" body for
ENOENT, and fall back to 500 for other errors. Ensure you reference the existing
variables readErr and res so the change is localized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/web/src/session-logic.test.ts (1)
156-202: Add coverage for the remainingisLatestTurnSettledbranches.The new cases cover the running-session regression well, but this suite still misses explicit checks for
session === null(should betruewhen timestamps are complete) andcompletedAt === null(should befalse).Suggested test additions
describe("isLatestTurnSettled", () => { @@ it("returns false when turn timestamps are incomplete", () => { @@ ).toBe(false); }); + + it("returns true when session is null and latest turn is complete", () => { + expect(isLatestTurnSettled(latestTurn, null)).toBe(true); + }); + + it("returns false when completedAt is missing", () => { + expect( + isLatestTurnSettled( + { + turnId: TurnId.makeUnsafe("turn-1"), + startedAt: "2026-02-27T21:10:00.000Z", + completedAt: null, + }, + null, + ), + ).toBe(false); + }); });Based on learnings: "For integration tests: Prefer deterministic inputs and explicit state checks; avoid relying on logs or timing assumptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/session-logic.test.ts` around lines 156 - 202, Add two missing test cases for isLatestTurnSettled: one where session is null and the latestTurn has both startedAt and completedAt (expect true), and one where latestTurn.completedAt is null with any session (expect false). Locate the describe("isLatestTurnSettled") suite in session-logic.test.ts and add an it(...) that passes session === null with the complete latestTurn timestamps and asserts true, and another it(...) that passes a latestTurn with completedAt: null (startedAt present) and asserts false; reference TurnId.makeUnsafe as used in other cases to construct turnIds.apps/server/src/projectFaviconRoute.ts (2)
5-10: Consider adding.jpegextension support.The MIME type mapping handles
.jpgbut not.jpeg, which is a valid alternative extension for JPEG images. Some projects may use.jpeg.Proposed fix
const FAVICON_MIME_TYPES: Record<string, string> = { ".png": "image/png", ".jpg": "image/jpeg", + ".jpeg": "image/jpeg", ".svg": "image/svg+xml", ".ico": "image/x-icon", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/projectFaviconRoute.ts` around lines 5 - 10, The FAVICON_MIME_TYPES mapping in projectFaviconRoute.ts is missing the ".jpeg" extension; update the FAVICON_MIME_TYPES constant to include the ".jpeg": "image/jpeg" entry so the route/function that reads this map (FAVICON_MIME_TYPES) correctly recognizes both .jpg and .jpeg favicon files.
82-92: Consider validatingprojectCwdagainst the known projects registry for defense-in-depth.The
cwdquery parameter accepts any filesystem path. WhileisPathWithinProjectprevents directory traversal within a selected project, it doesn't restrict which project directories can be targeted. The system maintains a registry of known projects in the projection snapshot (snapshot.projectswithworkspaceRootcomparison). Validating the incomingcwdagainst this registry would add an extra layer of security.Since this is a local development server, the current implementation is acceptable, but validation against known workspaces would be a beneficial safeguard against misconfigured clients or unintended filesystem access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/projectFaviconRoute.ts` around lines 82 - 92, Validate the incoming cwd in tryHandleProjectFaviconRequest against the known projects registry: look up snapshot.projects and ensure there is a project whose workspaceRoot equals (or is a parent of) the provided projectCwd (or use isPathWithinProject to confirm containment), and if no match is found respond with 400 and an error body; update the early-check that currently only verifies presence of projectCwd to also perform this registry check so only known workspace roots are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/projectFaviconRoute.test.ts`:
- Around line 35-43: The promise wrapper around server.listen is wrong because
the listen callback never receives an Error; instead attach an 'error' event
listener to the same server before calling server.listen and use the 'listening'
event (or the zero-arg listen callback) to resolve. Concretely: in the Promise
created around server.listen (the code that calls server.listen(0, "127.0.0.1",
...)), remove the error parameter check in the callback, add
server.once('error', err => { cleanup listeners; reject(err); }) before calling
server.listen, and on the listen callback (or server.once('listening', ...))
remove the error listener and resolve; also ensure you remove the listening
handler on error to avoid leaks.
In `@apps/server/src/projectFaviconRoute.ts`:
- Around line 43-45: The LINK_ICON_HTML_RE currently assumes rel appears before
href so tags like <link href="..." rel="icon"> are missed; update
LINK_ICON_HTML_RE to be order-independent by using lookahead assertions that
ensure the element has a rel=("icon"|"shortcut icon") attribute and a href=...
attribute in any order (e.g., a pattern using
<link\b(?=[^>]*\brel=...)(?=[^>]*\bhref=...)[^>]*> and capture the href value).
Do the same for LINK_ICON_OBJ_RE (use lookaheads to require both rel and href in
any order within the object-like text) or replace the regex approach with a
simple DOM-like parser if preferred; ensure the updated regexes still capture
the href value and remain case-insensitive like the originals.
---
Nitpick comments:
In `@apps/server/src/projectFaviconRoute.ts`:
- Around line 5-10: The FAVICON_MIME_TYPES mapping in projectFaviconRoute.ts is
missing the ".jpeg" extension; update the FAVICON_MIME_TYPES constant to include
the ".jpeg": "image/jpeg" entry so the route/function that reads this map
(FAVICON_MIME_TYPES) correctly recognizes both .jpg and .jpeg favicon files.
- Around line 82-92: Validate the incoming cwd in tryHandleProjectFaviconRequest
against the known projects registry: look up snapshot.projects and ensure there
is a project whose workspaceRoot equals (or is a parent of) the provided
projectCwd (or use isPathWithinProject to confirm containment), and if no match
is found respond with 400 and an error body; update the early-check that
currently only verifies presence of projectCwd to also perform this registry
check so only known workspace roots are accepted.
In `@apps/web/src/session-logic.test.ts`:
- Around line 156-202: Add two missing test cases for isLatestTurnSettled: one
where session is null and the latestTurn has both startedAt and completedAt
(expect true), and one where latestTurn.completedAt is null with any session
(expect false). Locate the describe("isLatestTurnSettled") suite in
session-logic.test.ts and add an it(...) that passes session === null with the
complete latestTurn timestamps and asserts true, and another it(...) that passes
a latestTurn with completedAt: null (startedAt present) and asserts false;
reference TurnId.makeUnsafe as used in other cases to construct turnIds.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/server/src/projectFaviconRoute.test.tsapps/server/src/projectFaviconRoute.tsapps/server/src/wsServer.tsapps/web/src/session-logic.test.tsapps/web/src/session-logic.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/session-logic.ts
- apps/server/src/wsServer.ts
Comment on lines
+35
to
+43
| await new Promise<void>((resolve, reject) => { | ||
| server.listen(0, "127.0.0.1", (error?: Error) => { | ||
| if (error) { | ||
| reject(error); | ||
| return; | ||
| } | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js http.Server.listen() callback signature error parameter
💡 Result:
http.Server extends net.Server, and server.listen(..., callback) simply adds callback as a listener for the 'listening' event—it is not an error-first callback and receives no parameters:
server.listen(3000, () => {
// no (err) argument here
console.log('listening');
});If binding fails (e.g., EADDRINUSE, EACCES), you must handle it via the 'error' event:
server.on('error', (err) => { /* handle err */ });
server.listen(3000);Sources: Node.js net.Server.listen() docs (callback is a 'listening' listener) [1]; Node.js http.Server.listen() is identical to net.Server.listen() [2].
🏁 Script executed:
# Find and examine the projectFaviconRoute.test.ts file
find . -name "projectFaviconRoute.test.ts" -type fRepository: pingdotgg/t3code
Length of output: 105
🏁 Script executed:
# Read the projectFaviconRoute.test.ts file, focusing on the relevant section
cat -n ./apps/server/src/projectFaviconRoute.test.ts | head -70 | tail -50Repository: pingdotgg/t3code
Length of output: 1693
server.listen callback does not receive an error parameter—errors are emitted via the 'error' event.
The Node.js server.listen() callback signature is () => void. It is registered as a listener for the 'listening' event and receives no parameters. Binding errors (e.g., EADDRINUSE, EACCES) are emitted via the 'error' event on the server, not passed to the callback. The if (error) check will never be true, and binding failures will not be caught.
Proposed fix to handle errors correctly
await new Promise<void>((resolve, reject) => {
+ server.once("error", reject);
- server.listen(0, "127.0.0.1", (error?: Error) => {
- if (error) {
- reject(error);
- return;
- }
+ server.listen(0, "127.0.0.1", () => {
resolve();
});
});📝 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
| await new Promise<void>((resolve, reject) => { | |
| server.listen(0, "127.0.0.1", (error?: Error) => { | |
| if (error) { | |
| reject(error); | |
| return; | |
| } | |
| resolve(); | |
| }); | |
| }); | |
| await new Promise<void>((resolve, reject) => { | |
| server.once("error", reject); | |
| server.listen(0, "127.0.0.1", () => { | |
| resolve(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/projectFaviconRoute.test.ts` around lines 35 - 43, The
promise wrapper around server.listen is wrong because the listen callback never
receives an Error; instead attach an 'error' event listener to the same server
before calling server.listen and use the 'listening' event (or the zero-arg
listen callback) to resolve. Concretely: in the Promise created around
server.listen (the code that calls server.listen(0, "127.0.0.1", ...)), remove
the error parameter check in the callback, add server.once('error', err => {
cleanup listeners; reject(err); }) before calling server.listen, and on the
listen callback (or server.once('listening', ...)) remove the error listener and
resolve; also ensure you remove the listening handler on error to avoid leaks.
- Update favicon regex parsing to support rel/href in either order - Add route tests for HTML link tags and object-style metadata with href-first ordering
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/projectFaviconRoute.ts (1)
5-10: Consider adding.jpegalias for completeness.The MIME type map includes
.jpgbut not.jpeg. Both extensions are common for JPEG images.Proposed addition
const FAVICON_MIME_TYPES: Record<string, string> = { ".png": "image/png", ".jpg": "image/jpeg", + ".jpeg": "image/jpeg", ".svg": "image/svg+xml", ".ico": "image/x-icon", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/projectFaviconRoute.ts` around lines 5 - 10, The FAVICON_MIME_TYPES map is missing the ".jpeg" alias; update the const FAVICON_MIME_TYPES to include an entry for ".jpeg": "image/jpeg" alongside ".jpg" so lookups for ".jpeg" return the correct MIME type (modify the FAVICON_MIME_TYPES Record in projectFaviconRoute.ts where it's declared).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/projectFaviconRoute.ts`:
- Around line 5-10: The FAVICON_MIME_TYPES map is missing the ".jpeg" alias;
update the const FAVICON_MIME_TYPES to include an entry for ".jpeg":
"image/jpeg" alongside ".jpg" so lookups for ".jpeg" return the correct MIME
type (modify the FAVICON_MIME_TYPES Record in projectFaviconRoute.ts where it's
declared).
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
2 participants
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.
Note
Add per-project favicon fetching via
wsServer.createServerGET/api/project-faviconand change default dev web port to 5733 across desktop, server, web, and scriptsIntroduce a server route that discovers and serves project favicons and add a sidebar UI that loads them; update dev tooling and configs to use port 5733 by default.
📍Where to Start
Start with the HTTP handler for GET
/api/project-faviconin wsServer.ts.Changes since #109 opened
/api/project-faviconroute handler with path validation, MIME type mapping, and favicon resolution from both well-known locations and source file metadata [2eaed39]isLatestTurnSettledinsession-logicmodule to return false wheneverorchestrationStatusis running, regardless of whether active turn matches latest turn [2eaed39]Macroscope summarized 08ea222.
Summary by CodeRabbit
New Features
Tests
Chores