inital implementation, requires auth changes#5
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds in-memory session management and admin authentication (hooks, login/logout endpoints, admin pages), a PhotoTagger UI and APIs to list untagged photos and update image EXIF GPS data using piexifjs, increases image cache expiry, and applies global layout/CSS adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Server
participant Drive
User->>Browser: Request /admin/panel
Browser->>Server: GET /admin/panel (with cookies)
Server->>Server: handle hook -> getSession from cookie
alt session missing/invalid
Server-->>Browser: 303 Redirect /admin/login
else session valid
Server-->>Browser: Render admin panel
end
sequenceDiagram
participant User
participant PhotoTagger
participant Server
participant Drive
PhotoTagger->>Server: GET /api/photos/untagged
Server->>Drive: List files in folder
Drive-->>Server: Files list
Server-->>PhotoTagger: JSON array of untagged photos
User->>PhotoTagger: Edit photo -> provide lat/lon -> Save
PhotoTagger->>Server: POST /api/photos/image/update {fileId, lat, lon}
Server->>Drive: Download image (media)
Drive-->>Server: Image bytes
Server->>Server: Parse EXIF (piexifjs), set GPS tags, inject EXIF
Server->>Drive: Upload updated image (replace)
Drive-->>Server: Updated file metadata
Server-->>PhotoTagger: 200 { updated metadata }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
src/routes/api/photos/image/[fileId]/+server.ts (1)
89-89: Simplify the cache expiry calculation.The expression
10 * 1000 * 12 * 60 * 60is unnecessarily complex and makes it difficult to determine the intended duration at a glance.Apply this diff to make the 5-day duration explicit:
- expires: now + 10 * 1000 * 12 * 60 * 60, + expires: now + 5 * 24 * 60 * 60 * 1000, // 5 dayssrc/routes/api/photos/image/update/+server.ts (1)
13-13: Consider using the singleton drive instance for consistency.This handler calls
initDrive()to create a new Drive client, while other routes (e.g.,src/routes/api/photos/image/[fileId]/+server.ts) import the singletondriveinstance from$lib/server/drive. Creating multiple instances may lead to inconsistent behavior or unnecessary overhead.Apply this diff to use the singleton:
-import { initDrive } from "$lib/server/drive"; +import { drive } from "$lib/server/drive"; import type { RequestHandler } from "@sveltejs/kit"; import { Readable } from "node:stream"; import piexif from "piexifjs"; export const POST: RequestHandler = async ({ request }) => { const { fileId, lat, lon } = await request.json(); if (!fileId || lat == null || lon == null) { return new Response("Missing parameters", { status: 400 }); } - const drive = initDrive(); - try {src/routes/admin/panel/+page.svelte (1)
9-12: Add error handling to the logout flow.The
handleLogoutfunction doesn't handle fetch errors or check the response status. If the logout request fails, the user is still navigated to/admin/login, potentially leaving the session active.Apply this diff to add error handling:
async function handleLogout() { - await fetch("/api/logout", { method: "POST" }); - goto("/admin/login"); + try { + const res = await fetch("/api/logout", { method: "POST" }); + if (!res.ok) { + console.error('Logout failed:', res.status); + return; + } + goto("/admin/login"); + } catch (err) { + console.error('Logout error:', err); + } }src/routes/api/photos/untagged/+server.ts (1)
8-14: Replaceanytypes with proper type definitions.Using
anydefeats TypeScript's type safety. Define proper types for the Drive API response and file objects.Apply this diff:
+type DriveFile = { + id: string; + name: string; + createdTime: string; + imageMediaMetadata?: { + time?: string; + location?: { + latitude: number; + longitude: number; + }; + }; +}; + +type DriveListResponse = { + data: { + files: DriveFile[]; + }; +}; + export const GET: RequestHandler = async () => { const drive = initDrive(); - const listRes: any = await drive.files.list({ + const listRes = await drive.files.list({ q: `'${GOOGLE_FOLDER_ID}' in parents`, fields: 'files(id,name,createdTime,imageMediaMetadata)', pageSize: 100 - }); + }) as DriveListResponse; - const untagged = listRes.data.files.filter((f: any) => !f.imageMediaMetadata?.location); + const untagged = listRes.data.files.filter((f) => !f.imageMediaMetadata?.location);src/lib/components/dashboard/PhotoTagger.svelte (2)
74-82: Use SvelteKit navigation instead ofwindow.location.href.Directly setting
window.location.hrefcauses a full page reload and bypasses SvelteKit's client-side routing. For opening the image in a new tab/window, usewindow.open()instead, or if you want same-tab navigation, use SvelteKit'sgoto().If you want to open in a new tab:
- on:click={() => window.location.href = `/api/photos/image/${photo.id}`} + on:click={() => window.open(`/api/photos/image/${photo.id}`, '_blank')}Or if you want same-tab navigation using SvelteKit:
+ import { goto } from '$app/navigation'; + // ... rest of script on:click={() => window.location.href = `/api/photos/image/${photo.id}`} + on:click={() => goto(`/api/photos/image/${photo.id}`)}
92-92: Specify input type as "number" for better UX.Using
type="text"for latitude/longitude inputs doesn't provide optimal UX on mobile devices (which show text keyboards). Consider usingtype="number"to show numeric keyboards, orinputmode="decimal"for better mobile support.Apply this diff:
Latitude: - <input type="text" bind:value={lat} placeholder="e.g. 51.5074" /> + <input type="text" inputmode="decimal" bind:value={lat} placeholder="e.g. 51.5074" /> </label> <label> Longitude: - <input type="text" bind:value={lon} placeholder="e.g. -0.1278" /> + <input type="text" inputmode="decimal" bind:value={lon} placeholder="e.g. -0.1278" /> </label>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
package.json(2 hunks)src/hooks.server.ts(1 hunks)src/lib/components/dashboard/PhotoTagger.svelte(1 hunks)src/routes/+layout.svelte(1 hunks)src/routes/+page.svelte(1 hunks)src/routes/admin/login/+page.svelte(1 hunks)src/routes/admin/panel/+page.svelte(1 hunks)src/routes/api/login/+server.ts(1 hunks)src/routes/api/logout/+server.ts(1 hunks)src/routes/api/photos/image/[fileId]/+server.ts(1 hunks)src/routes/api/photos/image/update/+server.ts(1 hunks)src/routes/api/photos/untagged/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/api/photos/untagged/+server.ts (2)
src/routes/api/photos/image/[fileId]/+server.ts (1)
GET(29-103)src/lib/server/drive.ts (2)
drive(18-18)initDrive(8-16)
src/routes/api/login/+server.ts (1)
src/routes/api/logout/+server.ts (1)
POST(3-8)
src/routes/api/photos/image/update/+server.ts (1)
src/lib/server/drive.ts (2)
drive(18-18)initDrive(8-16)
src/routes/api/logout/+server.ts (1)
src/routes/api/login/+server.ts (1)
POST(4-25)
🔇 Additional comments (4)
src/routes/+layout.svelte (1)
8-19:overflow: hiddenmay prevent scrolling on pages with overflowing content.Setting
overflow: hiddenonhtmlandbody(line 9) prevents vertical and horizontal scrolling globally. If any page content exceeds the viewport dimensions, users won't be able to scroll to view it.Verify that all pages (including the admin panel and login page) fit within the viewport at various screen sizes, or consider using
overflow: autoor removing this constraint::global(html, body) { - overflow: hidden; + overflow: auto; padding: 0;src/routes/api/logout/+server.ts (1)
3-8: LGTM!The logout implementation correctly deletes the session cookie with the same path used during login, ensuring proper cleanup.
src/routes/api/photos/image/[fileId]/+server.ts (1)
45-45: Verify cache duration alignment intentionality.The code does have a cache mismatch: internal cache expires after 5 days (
expires: now + 10 * 1000 * 12 * 60 * 60= 432,000 seconds), while both HTTP responses usemax-age=3600(1 hour). This means internal cache serves images for 5 days while clients refresh hourly.Confirm whether this mismatch is intentional (allowing server-side caching while forcing client refresh rates) or needs alignment per the suggested options: increase HTTP cache to 432000 seconds to match internal cache, or decrease internal cache to 3600 seconds to match HTTP headers. Also consider the impact of the
/api/photos/image/updateendpoint on cache invalidation strategy.src/routes/+page.svelte (1)
186-209: LGTM - Clean refactoring.The removal of the container wrapper and inline component rendering is a straightforward refactoring that aligns with the global layout approach mentioned in the PR context.
AlexTrebs
force-pushed
the
feature/tagging-dev
branch
from
f54f293 to
d54e6d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
package.json (1)
49-49: Duplicatevitedependency already flagged.This issue was previously identified:
viteappears in bothdevDependenciesanddependenciesbut should only be indevDependenciesas it's a build tool.src/routes/admin/login/+page.svelte (1)
30-30: Past issue with invalid input type already resolved.The previous comment about
type="username"being invalid is no longer applicable; the code now correctly usestype="text".src/routes/api/login/+server.ts (1)
5-21: Previously flagged issues still present.Multiple critical and major issues were identified in past reviews:
- Missing input validation for
usernameandpassword(could be undefined)- Plaintext password comparison is insecure
- No rate limiting to prevent brute-force attacks
secure: truecookie flag breaks local developmentsrc/routes/api/photos/image/update/+server.ts (1)
6-54: Previously flagged critical and major issues still present.Multiple issues were identified in past reviews:
- Missing authentication check - any unauthenticated user can modify images
- Potential memory exhaustion with large images (downloads entire file to memory)
- Cache invalidation missing after update (stale images served from cache in
src/routes/api/photos/image/[fileId]/+server.ts)
🧹 Nitpick comments (6)
src/routes/api/logout/+server.ts (1)
3-8: Consider adding authentication check to prevent unauthorized logout.Currently, any client can call this endpoint without authentication. While the security risk is low (only deletes the caller's own session cookie), it's good practice to verify the session exists before attempting logout.
Apply this diff:
import type { RequestHandler } from '@sveltejs/kit'; -import { destroySession } from '$lib/server/session'; +import { getSession, destroySession } from '$lib/server/session'; export const POST: RequestHandler = async ({ cookies }) => { const sessionId = cookies.get('session'); - if (sessionId) destroySession(sessionId); + if (sessionId && getSession(sessionId)) { + destroySession(sessionId); + } cookies.delete('session', { path: '/' }); return new Response(JSON.stringify({ success: true }), { headers: { 'Content-Type': 'application/json' } }); };src/routes/admin/panel/+page.svelte (1)
65-81: Global button styles may affect unintended elements.The
:global(button)selector applies styles to all<button>elements across the entire application, potentially overriding styles in other components or pages (e.g., the "Show Most Recent" button on the main page).Consider scoping the styles to this component's buttons:
- :global(button) { + button { background: #333; /* ... */ } - :global(button:hover){ + button:hover { background: #555; /* ... */ }src/routes/api/login/+server.ts (1)
23-35: Reconsider GET and DELETE handler necessity.The GET handler exposes session data (line 24-27) without an apparent use case in the admin UI. The DELETE handler (line 30-35) duplicates logout functionality already provided by
src/routes/api/logout/+server.ts. Consider removing unused endpoints to reduce attack surface.src/lib/server/session.ts (3)
1-1: Avoid exporting the sessions Map directly.Exporting
sessionsallows external code to mutate the Map directly, bypassing any validation or business logic. This breaks encapsulation and could lead to inconsistent state.Apply this diff:
-export const sessions = new Map<string, { expires: number }>(); +const sessions = new Map<string, { expires: number }>();
1-24: In-memory sessions have operational limitations for production.This implementation works for development but has limitations for production:
- Sessions are lost on server restart
- No support for horizontal scaling (load balancers, multiple instances)
- Expired sessions are only cleaned on access, leading to gradual memory growth
For production, consider using a persistent session store (Redis, database) or implement a periodic cleanup task for expired sessions.
17-17: Consider making session expiry configurable.The 1-hour expiry is hardcoded. Making it configurable via environment variables would improve flexibility for different deployment environments.
Example:
const SESSION_DURATION = parseInt(env.SESSION_DURATION_MS ?? '3600000'); // default 1h const expires = Date.now() + SESSION_DURATION;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
package.json(1 hunks)src/hooks.server.ts(1 hunks)src/lib/components/dashboard/PhotoTagger.svelte(1 hunks)src/lib/server/session.ts(1 hunks)src/routes/+layout.svelte(1 hunks)src/routes/+page.svelte(1 hunks)src/routes/admin/login/+page.svelte(1 hunks)src/routes/admin/panel/+page.svelte(1 hunks)src/routes/api/login/+server.ts(1 hunks)src/routes/api/logout/+server.ts(1 hunks)src/routes/api/photos/image/[fileId]/+server.ts(1 hunks)src/routes/api/photos/image/update/+server.ts(1 hunks)src/routes/api/photos/untagged/+server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/components/dashboard/PhotoTagger.svelte
- src/routes/api/photos/untagged/+server.ts
- src/hooks.server.ts
- src/routes/+layout.svelte
- src/routes/api/photos/image/[fileId]/+server.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/api/logout/+server.ts (1)
src/routes/api/login/+server.ts (1)
POST(5-21)
src/routes/api/photos/image/update/+server.ts (1)
src/lib/server/drive.ts (2)
drive(18-18)initDrive(8-16)
src/routes/api/login/+server.ts (2)
src/routes/api/logout/+server.ts (1)
POST(3-8)src/lib/server/session.ts (3)
createSession(15-20)getSession(3-13)destroySession(22-24)
🔇 Additional comments (2)
src/routes/+page.svelte (1)
186-209: LGTM - Clean layout simplification.The removal of the container wrapper and direct inlining of components simplifies the DOM structure without affecting functionality. Map initialization and event handling remain intact.
package.json (1)
47-47: LGTM - Appropriate dependency for EXIF manipulation.The
piexifjslibrary is correctly added to support GPS metadata updates insrc/routes/api/photos/image/update/+server.ts.
| } else { | ||
| const data = await res.json(); | ||
| error = data.error || "Login failed"; | ||
| } |
There was a problem hiding this comment.
Potential error on non-JSON error responses.
Line 18 calls res.json() without checking the Content-Type header. If the login endpoint returns a non-JSON error response (e.g., plain text), this will throw an exception and leave error unset.
Apply this diff:
if (res.ok) {
goto("/admin/panel");
} else {
- const data = await res.json();
- error = data.error || "Login failed";
+ try {
+ const data = await res.json();
+ error = data.error || "Login failed";
+ } catch {
+ error = "Login failed";
+ }
}📝 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.
| } else { | |
| const data = await res.json(); | |
| error = data.error || "Login failed"; | |
| } | |
| } else { | |
| try { | |
| const data = await res.json(); | |
| error = data.error || "Login failed"; | |
| } catch { | |
| error = "Login failed"; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/routes/admin/login/+page.svelte around lines 17 to 20, the code calls
res.json() unconditionally which will throw if the response is not JSON; change
it to safely attempt to parse JSON and fall back to text by first checking
res.headers.get('content-type') for 'application/json' or wrapping res.json() in
try/catch, and set error to the parsed JSON error field if present or to the
response text or a default "Login failed" message on parse failure.
| async function handleLogout() { | ||
| await fetch("/api/logout", { method: "POST" }); | ||
| goto("/admin/login"); | ||
| } |
There was a problem hiding this comment.
Add error handling to logout flow.
The logout handler doesn't await or check the response status before navigating. If the logout request fails, the user will still be redirected but the session may not be properly cleared.
Apply this diff:
async function handleLogout() {
- await fetch("/api/logout", { method: "POST" });
- goto("/admin/login");
+ try {
+ const res = await fetch("/api/logout", { method: "POST" });
+ if (res.ok) {
+ goto("/admin/login");
+ }
+ } catch (err) {
+ console.error("Logout failed:", err);
+ }
}🤖 Prompt for AI Agents
In src/routes/admin/panel/+page.svelte around lines 9 to 12, the logout flow
currently doesn't verify the API response before redirecting; update
handleLogout to wrap the fetch in try/catch, await the POST, check response.ok
(or status === 200), and only call goto("/admin/login") when the logout
succeeded; on failure log the error, surface a user-facing message (toast/alert)
and avoid redirecting so the session state remains consistent.
| return new Response("ok"); | ||
| } | ||
|
|
||
| return new Response("unauthorized", { status: 401 }); |
There was a problem hiding this comment.
Inconsistent response format causes client-side errors.
Line 17 returns plain text "ok" on success, while line 20 returns plain text "unauthorized" on failure. However, the login page at src/routes/admin/login/+page.svelte (line 18) expects a JSON response with an error field on failure. This mismatch will cause the error parsing to fail.
Apply this diff for consistency:
- return new Response("ok");
+ return new Response(JSON.stringify({ success: true }), {
+ headers: { "Content-Type": "application/json" }
+ });
}
- return new Response("unauthorized", { status: 401 });
+ return new Response(JSON.stringify({ error: "Invalid credentials" }), {
+ status: 401,
+ headers: { "Content-Type": "application/json" }
+ });
};📝 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.
| return new Response("ok"); | |
| } | |
| return new Response("unauthorized", { status: 401 }); | |
| return new Response(JSON.stringify({ success: true }), { | |
| headers: { "Content-Type": "application/json" } | |
| }); | |
| } | |
| return new Response(JSON.stringify({ error: "Invalid credentials" }), { | |
| status: 401, | |
| headers: { "Content-Type": "application/json" } | |
| }); |
🤖 Prompt for AI Agents
In src/routes/api/login/+server.ts around lines 17 to 20, the endpoint currently
returns plain text ("ok" on success and "unauthorized" on failure) which
mismatches the client expectation of a JSON object with an error field; change
both responses to JSON: on success return a JSON body (e.g., { success: true })
with Content-Type: application/json and status 200, and on failure return JSON {
error: "unauthorized" } with Content-Type: application/json and status 401 so
the login page can parse the error field consistently.
| export const POST: RequestHandler = async ({ cookies }) => { | ||
| cookies.delete('session', { path: '/' }); | ||
| return new Response(JSON.stringify({ success: true }), { | ||
| headers: { 'Content-Type': 'application/json' } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Missing server-side session cleanup causes memory leak.
The endpoint deletes the client-side cookie but does not call destroySession() to remove the session from the server-side sessions Map (in src/lib/server/session.ts). Over time, expired sessions will accumulate in memory.
Apply this diff to clean up the session:
import type { RequestHandler } from '@sveltejs/kit';
+import { destroySession } from '$lib/server/session';
export const POST: RequestHandler = async ({ cookies }) => {
+ const sessionId = cookies.get('session');
+ if (sessionId) destroySession(sessionId);
cookies.delete('session', { path: '/' });
return new Response(JSON.stringify({ success: true }), {
headers: { 'Content-Type': 'application/json' }
});
};📝 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.
| export const POST: RequestHandler = async ({ cookies }) => { | |
| cookies.delete('session', { path: '/' }); | |
| return new Response(JSON.stringify({ success: true }), { | |
| headers: { 'Content-Type': 'application/json' } | |
| }); | |
| }; | |
| import type { RequestHandler } from '@sveltejs/kit'; | |
| import { destroySession } from '$lib/server/session'; | |
| export const POST: RequestHandler = async ({ cookies }) => { | |
| const sessionId = cookies.get('session'); | |
| if (sessionId) destroySession(sessionId); | |
| cookies.delete('session', { path: '/' }); | |
| return new Response(JSON.stringify({ success: true }), { | |
| headers: { 'Content-Type': 'application/json' } | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In src/routes/api/logout/+server.ts around lines 3 to 8, the handler only
deletes the client cookie and must also remove the server-side session to avoid
memory leaks; read the 'session' cookie value before deleting it, import
destroySession from src/lib/server/session.ts, and call (await if destroySession
is async) destroySession(sessionId) with that value prior to or immediately
after cookies.delete so the session Map entry is removed on logout; keep the
JSON response and content-type header unchanged.
| if (!fileId || lat == null || lon == null) { | ||
| return new Response("Missing parameters", { status: 400 }); | ||
| } |
There was a problem hiding this comment.
Add type validation for latitude and longitude.
The validation checks for null but doesn't verify that lat and lon are valid numbers. A client could send strings or other types, leading to incorrect EXIF data or runtime errors.
Apply this diff:
const { fileId, lat, lon } = await request.json();
-if (!fileId || lat == null || lon == null) {
+if (!fileId || typeof lat !== 'number' || typeof lon !== 'number') {
return new Response("Missing parameters", { status: 400 });
}
+
+if (lat < -90 || lat > 90 || lon < -180 || lon > 180) {
+ return new Response("Invalid coordinates", { status: 400 });
+}📝 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.
| if (!fileId || lat == null || lon == null) { | |
| return new Response("Missing parameters", { status: 400 }); | |
| } | |
| if (!fileId || typeof lat !== 'number' || typeof lon !== 'number') { | |
| return new Response("Missing parameters", { status: 400 }); | |
| } | |
| if (lat < -90 || lat > 90 || lon < -180 || lon > 180) { | |
| return new Response("Invalid coordinates", { status: 400 }); | |
| } |
🤖 Prompt for AI Agents
In src/routes/api/photos/image/update/+server.ts around lines 9 to 11, the
current guard only checks for null but doesn't ensure lat and lon are valid
numbers; update the validation to coerce or parse lat and lon into numbers
(e.g., Number or parseFloat), verify they are not NaN, and enforce valid ranges
(latitude between -90 and 90, longitude between -180 and 180); if parsing fails
or values are out of range, return a 400 response with a descriptive error
message so invalid types/values are rejected before using them in EXIF updates.
| // --- 1. Download the image --- | ||
| const res = await drive.files.get({ fileId, alt: "media" }, { responseType: "arraybuffer" }); | ||
| const buffer = Buffer.from(res.data); | ||
| const base64 = buffer.toString("binary"); |
There was a problem hiding this comment.
Use "latin1" encoding instead of deprecated "binary".
Line 19 uses toString("binary") which is deprecated in Node.js. The recommended replacement is "latin1", which is an alias for the same encoding.
Apply this diff:
const buffer = Buffer.from(res.data);
-const base64 = buffer.toString("binary");
+const base64 = buffer.toString("latin1");Also update line 30:
const newBase64 = piexif.insert(exifBytes, base64);
-const newBuffer = Buffer.from(newBase64, "binary");
+const newBuffer = Buffer.from(newBase64, "latin1");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/routes/api/photos/image/update/+server.ts around line 19, replace
buffer.toString("binary") with buffer.toString("latin1") because "binary" is
deprecated; also inspect and update the corresponding use on line 30 to use
"latin1" wherever "binary" was used (e.g., Buffer.from(..., "latin1") or other
encodings) so both encoding and decoding use the latin1 alias consistently.
| const updated = await drive.files.update({ | ||
| fileId, | ||
| media: { | ||
| mimeType: "image/jpeg", |
There was a problem hiding this comment.
Hardcoded MIME type may not match original image format.
Line 39 assumes all images are JPEG (mimeType: "image/jpeg"), but the image could be PNG, HEIC, or another format. This could cause corruption or format conversion issues.
Retrieve and preserve the original MIME type:
const drive = initDrive();
try {
+ // Get original MIME type
+ const metadata = await drive.files.get({ fileId, fields: 'mimeType' });
+ const originalMimeType = metadata.data.mimeType || 'image/jpeg';
+
// --- 1. Download the image ---
const res = await drive.files.get({ fileId, alt: "media" }, { responseType: "arraybuffer" });
// ...
// --- 4. Update file in place ---
const updated = await drive.files.update({
fileId,
media: {
- mimeType: "image/jpeg",
+ mimeType: originalMimeType,
body: stream,
},📝 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.
| mimeType: "image/jpeg", | |
| const drive = initDrive(); | |
| try { | |
| // Get original MIME type | |
| const metadata = await drive.files.get({ fileId, fields: 'mimeType' }); | |
| const originalMimeType = metadata.data.mimeType || 'image/jpeg'; | |
| // --- 1. Download the image --- | |
| const res = await drive.files.get({ fileId, alt: "media" }, { responseType: "arraybuffer" }); | |
| // ... (rest of the download and processing code) | |
| // --- 4. Update file in place --- | |
| const updated = await drive.files.update({ | |
| fileId, | |
| media: { | |
| mimeType: originalMimeType, | |
| body: stream, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In src/routes/api/photos/image/update/+server.ts around line 39, the MIME type
is hardcoded as "image/jpeg"; change the code to read and preserve the original
MIME type from the incoming file instead of hardcoding. Fetch the uploaded
file's MIME via the request (e.g., formData file.type or file.mimetype) or
derive it from the file buffer using a MIME detection utility, then assign that
value to mimeType when saving/returning the image; also add a safe fallback
(e.g., "application/octet-stream") if detection fails.
AlexTrebs
force-pushed
the
feature/tagging-dev
branch
from
d54e6d8 to
da498a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/routes/api/photos/untagged/+server.ts (4)
7-7: Reuse the singleton drive instance.Calling
initDrive()on every request is inefficient. Thesrc/lib/server/drive.tsalready exports a singletondriveinstance.Apply this diff:
-import { initDrive } from '$lib/server/drive'; +import { drive } from '$lib/server/drive'; export const GET: RequestHandler = async () => { - const drive = initDrive(); const listRes: any = await drive.files.list({
6-24: Admin endpoint lacks authentication.This endpoint exposes untagged photos (admin functionality) but has no authentication. The
hooks.server.tsonly protects/admin/*routes, leaving/api/photos/untaggedpublicly accessible.Add session validation:
-export const GET: RequestHandler = async () => { +import { getSession } from '$lib/server/session'; +import { error } from '@sveltejs/kit'; + +export const GET: RequestHandler = async ({ cookies }) => { + const session = getSession(cookies.get('session')); + if (!session) { + throw error(401, 'Unauthorized'); + } + const drive = initDrive();
6-12: Drive API call lacks error handling.The
drive.files.list()call can fail due to network issues, quota limits, or invalid credentials, causing unhandled promise rejections.Wrap in try-catch:
export const GET: RequestHandler = async () => { const drive = initDrive(); - const listRes: any = await drive.files.list({ - q: `'${GOOGLE_FOLDER_ID}' in parents`, - fields: 'files(id,name,createdTime,imageMediaMetadata)', - pageSize: 100 - }); - - const untagged = listRes.data.files.filter((f: any) => !f.imageMediaMetadata?.location); - - return json( - untagged.map((f: any) => ({ - id: f.id, - name: f.name, - takenAt: f.imageMediaMetadata?.time || f.createdTime, - url: `/api/photos/image/${f.id}` - })) - ); + + try { + const listRes: any = await drive.files.list({ + q: `'${GOOGLE_FOLDER_ID}' in parents`, + fields: 'files(id,name,createdTime,imageMediaMetadata)', + pageSize: 100 + }); + + const untagged = listRes.data.files.filter((f: any) => !f.imageMediaMetadata?.location); + + return json( + untagged.map((f: any) => ({ + id: f.id, + name: f.name, + takenAt: f.imageMediaMetadata?.time || f.createdTime, + url: `/api/photos/image/${f.id}` + })) + ); + } catch (err) { + console.error('Failed to fetch untagged photos:', err); + return json({ error: 'Failed to fetch photos' }, { status: 500 }); + } };
11-11: Pagination missing for large photo collections.The hardcoded
pageSize: 100limits results to the first 100 untagged photos. Collections exceeding this won't be fully visible in the admin panel.Implement pagination by fetching all pages:
export const GET: RequestHandler = async () => { const drive = initDrive(); - const listRes: any = await drive.files.list({ - q: `'${GOOGLE_FOLDER_ID}' in parents`, - fields: 'files(id,name,createdTime,imageMediaMetadata)', - pageSize: 100 - }); - - const untagged = listRes.data.files.filter((f: any) => !f.imageMediaMetadata?.location); + + let allFiles: any[] = []; + let pageToken: string | undefined; + + do { + const listRes: any = await drive.files.list({ + q: `'${GOOGLE_FOLDER_ID}' in parents`, + fields: 'files(id,name,createdTime,imageMediaMetadata),nextPageToken', + pageSize: 1000, + pageToken + }); + allFiles.push(...listRes.data.files); + pageToken = listRes.data.nextPageToken; + } while (pageToken); + + const untagged = allFiles.filter((f: any) => !f.imageMediaMetadata?.location);src/routes/api/photos/image/update/+server.ts (3)
26-26: Replace deprecated "binary" encoding with "latin1".Lines 26 and 39 use the deprecated
"binary"encoding. Node.js recommends using"latin1"instead.Apply this diff:
const buffer = Buffer.from(res.data); -const base64 = buffer.toString("binary"); +const base64 = buffer.toString("latin1"); // ... EXIF manipulation ... const newBase64 = piexif.insert(exifBytes, base64); -const newBuffer = Buffer.from(newBase64, "binary"); +const newBuffer = Buffer.from(newBase64, "latin1");Also applies to: 39-39
13-15: Input validation insufficient for coordinates.The validation checks for
nullbut doesn't verifylatandlonare valid numbers within correct ranges (latitude: -90 to 90, longitude: -180 to 180).Add type and range validation:
const { fileId, lat, lon } = await request.json(); -if (!fileId || lat == null || lon == null) { +if (!fileId || typeof lat !== 'number' || typeof lon !== 'number') { return new Response("Missing parameters", { status: 400 }); } + +if (lat < -90 || lat > 90 || lon < -180 || lon > 180) { + return new Response("Invalid coordinates", { status: 400 }); +}
48-48: Hardcoded MIME type may corrupt non-JPEG images.Line 48 assumes all images are JPEG, but the image could be PNG, HEIC, or another format, potentially causing corruption.
Retrieve and preserve the original MIME type:
try { + // Get original MIME type + const metadata = await drive.files.get({ fileId, fields: 'mimeType' }); + const originalMimeType = metadata.data.mimeType || 'image/jpeg'; + // --- 1. Download the image --- const res = await drive.files.get( { fileId, alt: "media" }, { responseType: "arraybuffer" }, ); // ... EXIF manipulation ... // --- 4. Update file in place --- const updated = await drive.files.update({ fileId, media: { - mimeType: "image/jpeg", + mimeType: originalMimeType, body: stream, },src/routes/api/logout/+server.ts (1)
3-8: Server-side session cleanup still missing.The endpoint deletes the client cookie but doesn't call
destroySession()to remove the session from the in-memory Map, causing sessions to accumulate indefinitely.Apply this diff:
import type { RequestHandler } from '@sveltejs/kit'; +import { destroySession } from '$lib/server/session'; export const POST: RequestHandler = async ({ cookies }) => { + const sessionId = cookies.get('session'); + if (sessionId) destroySession(sessionId); cookies.delete('session', { path: '/' }); return new Response(JSON.stringify({ success: true }), { headers: { 'Content-Type': 'application/json' } }); };
🧹 Nitpick comments (1)
src/routes/api/photos/image/update/+server.ts (1)
17-17: Use the singleton drive instance.Calling
initDrive()on every request is inefficient whensrc/lib/server/drive.tsexports a singleton.Apply this diff:
-import { initDrive } from "$lib/server/drive"; +import { drive } from "$lib/server/drive"; export const POST: RequestHandler = async ({ request, cookies }) => { // ... validation ... - const drive = initDrive(); try {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
package.json(1 hunks)src/hooks.server.ts(1 hunks)src/lib/components/dashboard/PhotoTagger.svelte(1 hunks)src/lib/server/session.ts(1 hunks)src/routes/+layout.svelte(1 hunks)src/routes/+page.svelte(1 hunks)src/routes/admin/login/+page.svelte(1 hunks)src/routes/admin/panel/+page.svelte(1 hunks)src/routes/api/login/+server.ts(1 hunks)src/routes/api/logout/+server.ts(1 hunks)src/routes/api/photos/image/[fileId]/+server.ts(1 hunks)src/routes/api/photos/image/update/+server.ts(1 hunks)src/routes/api/photos/untagged/+server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/routes/api/login/+server.ts
- src/routes/admin/panel/+page.svelte
- package.json
- src/lib/server/session.ts
- src/routes/admin/login/+page.svelte
- src/routes/+layout.svelte
- src/routes/api/photos/image/[fileId]/+server.ts
- src/lib/components/dashboard/PhotoTagger.svelte
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/api/logout/+server.ts (2)
src/routes/api/login/+server.ts (1)
POST(5-21)src/routes/api/photos/image/update/+server.ts (1)
POST(7-63)
src/hooks.server.ts (1)
src/lib/server/session.ts (1)
getSession(3-13)
src/routes/api/photos/image/update/+server.ts (4)
src/routes/api/login/+server.ts (1)
POST(5-21)src/routes/api/logout/+server.ts (1)
POST(3-8)src/lib/server/session.ts (1)
destroySession(22-24)src/lib/server/drive.ts (2)
drive(18-18)initDrive(8-16)
src/routes/api/photos/untagged/+server.ts (2)
src/routes/api/photos/image/[fileId]/+server.ts (1)
GET(29-103)src/lib/server/drive.ts (2)
drive(18-18)initDrive(8-16)
🔇 Additional comments (2)
src/routes/+page.svelte (1)
186-209: The original review comment is incorrect and should be disregarded.The layout structure is properly maintained. The global styles and container wrapper were not removed—they remain in
src/routes/+layout.svelte:
- Global styles for
htmlandbodyare applied via:global()declarations, settingheight: 100%,width: 100%, and other essential properties.- The
#containerwrapper withheight: 100%,width: 100%, and flex display is still present in the layout file.- The simplified template at lines 186-209 is the expected SvelteKit pattern: the page renders directly within the layout's
<slot />, which is wrapped by the container and inherits the global styles.The map div's
height: 100%will work correctly because the parent elements (layout container and html/body) all have explicit height set. No layout breakage will occur.src/hooks.server.ts (1)
6-24: Good implementation of session-based authentication.The hook properly validates sessions using
getSession()and redirects unauthenticated requests to/admin/login. This addresses the security concerns from previous reviews by using proper session tokens instead of hardcoded values.
| export const POST: RequestHandler = async ({ request }) => { | ||
| const sessionId = cookies.get("session"); | ||
| if (sessionId) destroySession(sessionId); |
There was a problem hiding this comment.
Missing cookies parameter causes runtime error.
Line 8 references cookies.get("session") but the handler signature on line 7 doesn't include cookies in the destructured parameters, causing a ReferenceError at runtime.
Apply this diff:
-export const POST: RequestHandler = async ({ request }) => {
+export const POST: RequestHandler = async ({ request, cookies }) => {
const sessionId = cookies.get("session");
if (sessionId) destroySession(sessionId);📝 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.
| export const POST: RequestHandler = async ({ request }) => { | |
| const sessionId = cookies.get("session"); | |
| if (sessionId) destroySession(sessionId); | |
| export const POST: RequestHandler = async ({ request, cookies }) => { | |
| const sessionId = cookies.get("session"); | |
| if (sessionId) destroySession(sessionId); |
🤖 Prompt for AI Agents
In src/routes/api/photos/image/update/+server.ts around lines 7 to 9, the POST
handler destructures only { request } but then uses cookies.get("session"),
causing a ReferenceError; update the handler signature to destructure cookies as
well (e.g., async ({ request, cookies }) => ...) so cookies is defined before
calling cookies.get("session"), leaving the rest of the logic unchanged.
| const sessionId = cookies.get("session"); | ||
| if (sessionId) destroySession(sessionId); |
There was a problem hiding this comment.
Session destruction logic is misplaced.
This endpoint updates image EXIF data but immediately destroys the session, which seems incorrect. Image update operations should validate the session for authentication, not destroy it. Session destruction belongs in the logout endpoint.
Replace with authentication check:
+import { getSession } from '$lib/server/session';
+import { error } from '@sveltejs/kit';
+
export const POST: RequestHandler = async ({ request, cookies }) => {
- const sessionId = cookies.get("session");
- if (sessionId) destroySession(sessionId);
+ const session = getSession(cookies.get("session"));
+ if (!session) {
+ throw error(401, 'Unauthorized');
+ }
const { fileId, lat, lon } = await request.json();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/routes/api/photos/image/update/+server.ts around lines 8-9, the code
currently destroys the session after reading the cookie which is wrong for an
update endpoint; replace the destroySession call with proper authentication:
retrieve the sessionId from cookies, validate it (e.g., fetch session data or
call isValidSession(sessionId)), and if invalid or missing return an
unauthorized response (401); only proceed with the EXIF update when the session
is valid — leave session destruction to the logout endpoint.
1 participant
Summary by CodeRabbit
New Features
Chores
Style