Open
Conversation
… Figma imports and API interactions, add a seed script and callback analyzer, and update the backup prompt.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
Contributor
Reviewer's GuideImplements a full user-facing asset library backed by the existing files table, including new API routes, React-query powered UI, DB schema extensions, and upload adapter integration, plus some env/script tweaks and tests around the new assets API. Sequence diagram for Tauri/local asset upload and library refreshsequenceDiagram
actor User
participant Editor
participant AssetLibrary
participant UseUpload as useUpload_hook
participant UploadAdapter as upload_adapter
participant SyncToDb as syncToDatabase
participant AssetsPost as POST_api_assets
participant ApiAuth as api_auth_requireMutation
participant DB as Database
participant ReactQuery as ReactQuery_client
User->>Editor: choose file to upload
Editor->>AssetLibrary: render with onSelect
User->>AssetLibrary: click Upload and pick files
AssetLibrary->>UseUpload: startUpload(files)
UseUpload->>UploadAdapter: uploadWithTauriFs(file)
UploadAdapter->>UploadAdapter: writeFile_to_local_fs
UploadAdapter->>SyncToDb: syncToDatabase(result, tauri, file)
SyncToDb->>AssetsPost: POST /api/assets {url, name, originalName, size, type, storageProvider}
AssetsPost->>ApiAuth: requireMutation()
ApiAuth-->>AssetsPost: userId
AssetsPost->>DB: insert into files
DB-->>AssetsPost: new row
AssetsPost-->>SyncToDb: 201 Created
SyncToDb-->>UploadAdapter: void
UploadAdapter-->>UseUpload: UploadResult(url, name)
UseUpload-->>AssetLibrary: onUploadComplete callback
AssetLibrary->>ReactQuery: invalidateQueries(assets)
ReactQuery->>AssetLibrary: refetch getAssets
AssetLibrary-->>User: updated asset list with new item
ER diagram for updated files table and user relationerDiagram
user ||--o{ files : owns
user {
text id PK
text email
text name
bigint created_at
}
files {
text id PK
text user_id FK
text url
text name
text original_name
integer size
text type
text storage_provider
boolean is_public
bigint created_at
}
Class diagram for asset library UI and server APIclassDiagram
class AssetLibrary {
+string view
+number page
+string search
+AssetSort sort
+string editingId
+string editName
+constructor AssetLibrary(onSelect, className)
+handleRename(id, currentName) void
+saveRename() Promise~void~
+handleFileUpload(event) Promise~void~
+copyToClipboard(url) void
}
class AssetMenu {
+constructor AssetMenu(item, onRename, onDelete, onCopy)
}
class GetAssetsParams {
+number page
+number limit
+string search
+AssetSort sort
+AssetOrder order
}
class AssetsResponse {
+File[] items
+number total
+number page
+number limit
+number totalPages
}
class getAssets {
+getAssets(page, limit, search, sort, order) Promise~AssetsResponse~
}
class updateAsset {
+updateAsset(id, name, isPublic) Promise~void~
}
class destroyFile {
+destroyFile(id) Promise~void~
}
class File {
+string id
+string userId
+string url
+string name
+string originalName
+number size
+string type
+string storageProvider
+boolean isPublic
+number createdAt
}
class AssetsApiList {
+GET(request) Promise~Response~
}
class AssetsApiMutate {
+POST(request) Promise~Response~
+DELETE(request) Promise~Response~
}
class AssetApiItem {
+GET(request, params) Promise~Response~
+PATCH(request, params) Promise~Response~
}
class AssetApiRedownload {
+POST(request, params) Promise~Response~
}
class UploadAdapter {
+syncToDatabase(result, storageProvider, file) Promise~void~
+uploadWithUploadThing(file) Promise~UploadResult~
+uploadWithTauriFs(file) Promise~UploadResult~
+uploadWithLocalStorage(file) Promise~UploadResult~
}
class UploadResult {
+string url
+string name
}
class AssetSort {
<<enumeration>>
createdAt
name
size
}
class AssetOrder {
<<enumeration>>
asc
desc
}
AssetLibrary --> getAssets : uses
AssetLibrary --> updateAsset : renames_updates
AssetLibrary --> destroyFile : deletes
AssetLibrary --> AssetMenu : renders
AssetLibrary --> File : displays
getAssets --> File : returns
AssetsApiList --> File : queries
AssetsApiMutate --> File : inserts_deletes
AssetApiItem --> File : reads_updates
AssetApiRedownload --> File : reads
UploadAdapter --> UploadResult : returns
UploadAdapter --> AssetsApiMutate : calls_POST_api_assets
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 10 issues, and left some high level feedback:
- The asset listing logic (both in
apps/web/app/api/assets/route.tsandfeatures/media/api/queries/get-assets.ts) derivestotalviacountResult.length, but that query currently selects{ count: files.id }which will return one row per file; consider switching to a proper aggregate (e.g.count(*)) and reading the numeric count instead of using the array length so pagination metadata is correct. - Server-only modules (
features/media/api/queries/get-assets.tsandfeatures/media/api/mutations/update-asset.ts, both marked'use server'and usingauth/requireMutation) are imported directly into the clientAssetLibrarycomponent and used inside React Query; this will either fail at build time or at runtime—consider exposing these as HTTP endpoints or client-safe wrappers instead of invoking server actions from the browser. - The shebang line in
apps/web/scripts/seed-identity-guide.tswas accidentally changed to include leading whitespace and spaces inside#!/usr/bin/env bun, which will break execution; it should be restored to a valid, unindented shebang.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The asset listing logic (both in `apps/web/app/api/assets/route.ts` and `features/media/api/queries/get-assets.ts`) derives `total` via `countResult.length`, but that query currently selects `{ count: files.id }` which will return one row per file; consider switching to a proper aggregate (e.g. `count(*)`) and reading the numeric count instead of using the array length so pagination metadata is correct.
- Server-only modules (`features/media/api/queries/get-assets.ts` and `features/media/api/mutations/update-asset.ts`, both marked `'use server'` and using `auth`/`requireMutation`) are imported directly into the client `AssetLibrary` component and used inside React Query; this will either fail at build time or at runtime—consider exposing these as HTTP endpoints or client-safe wrappers instead of invoking server actions from the browser.
- The shebang line in `apps/web/scripts/seed-identity-guide.ts` was accidentally changed to include leading whitespace and spaces inside `#!/usr/bin/env bun`, which will break execution; it should be restored to a valid, unindented shebang.
## Individual Comments
### Comment 1
<location> `apps/web/features/media/components/AssetLibrary.tsx:5-14` </location>
<code_context>
+import { getAssets, type AssetSort, type AssetOrder } from '../api/queries/get-assets'
</code_context>
<issue_to_address>
**issue (bug_risk):** Client component is calling a `use server` function directly via React Query, which will not bundle correctly.
`getAssets` is a `'use server'` function, but `AssetLibrary` is a client component and is using it as a React Query `queryFn`. Server actions/server-only modules can’t be called directly from client components; this will likely fail to bundle or break at runtime (e.g. no `auth`/DB access on the client).
Instead, either:
- Call your `/api/assets` REST endpoint via `fetch` in the `queryFn`, or
- Load the data in a server component and pass the results into a client-only presentation component.
</issue_to_address>
### Comment 2
<location> `apps/web/app/api/assets/route.ts:29-34` </location>
<code_context>
+ : eq(files.userId, userId)
+
+ // Get total count
+ const countResult = await db
+ .select({ count: files.id })
+ .from(files)
+ .where(whereClause)
+
+ const total = countResult.length
+
+ // Get paginated results
</code_context>
<issue_to_address>
**issue (bug_risk):** Total count computation in the assets API is incorrect for paginated responses.
This uses `select({ count: files.id })` and then `countResult.length` as `total`, which will always be `1` for an aggregate `COUNT` query. Instead, select an aggregate (e.g. `count(*)`) and read the value from the single row (e.g. `countResult[0].count`), then compute `totalPages` from that value.
</issue_to_address>
### Comment 3
<location> `apps/web/features/media/components/media-picker.tsx:12-14` </location>
<code_context>
- </div>
- ))}
- </div>
+ <AssetLibrary
+ onSelect={onSelect}
+ className={className}
+ />
)
</code_context>
<issue_to_address>
**issue (bug_risk):** The `onSelect` callback types between `MediaPicker` and `AssetLibrary` are incompatible in TypeScript.
`MediaPicker` exposes `onSelect: (url: string) => void`, but `AssetLibrary` expects `onSelect?: (url: string, file: any) => void` and calls it as `onSelect?.(item.url, item)`. In TS, a function with fewer params is not assignable to one with more, so passing `onSelect` through will fail type-checking.
To fix this, make the second parameter optional in `AssetLibrary`:
```ts
type AssetLibraryProps = {
onSelect?: (url: string, file?: any) => void
}
```
This preserves runtime behavior while making the types compatible.
</issue_to_address>
### Comment 4
<location> `apps/web/scripts/seed-identity-guide.ts:1` </location>
<code_context>
-#!/usr/bin/env bun
+ #!/usr/bin / env bun
/**
</code_context>
<issue_to_address>
**issue (bug_risk):** The shebang line has extra whitespace and indentation, which will break execution as a CLI script.
The shebang was changed from `#!/usr/bin/env bun` to ` #!/usr/bin / env bun`, which adds whitespace inside `/usr/bin/env` and before the `#!`. It needs to be reverted to exactly:
```sh
#!/usr/bin/env bun
```
so the interpreter path is valid and the script stays executable as a CLI.
</issue_to_address>
### Comment 5
<location> `apps/web/app/api/assets/[id]/route.ts:5-8` </location>
<code_context>
+import { requireMutation } from '@/lib/api-auth'
+import { getDatabase, files, eq, and } from '@skriuw/db'
+
+type RouteParams = { params: Promise<{ id: string }> }
+
+// PATCH /api/assets/:id - Rename or toggle visibility
+export async function PATCH(request: NextRequest, { params }: RouteParams) {
+ try {
+ const auth = await requireMutation()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Route handler `params` is incorrectly typed as a `Promise`, and then awaited unnecessarily.
`params` is passed as a plain object, so typing it as `Promise<{ id: string }>` and awaiting it is incorrect and misleading for both consumers and tooling.
You can correct this to:
```ts
type RouteParams = { params: { id: string } }
export async function PATCH(request: NextRequest, { params }: RouteParams) {
const { id } = params
// ...
}
```
Please update the `redownload` route similarly for consistency.
Suggested implementation:
```typescript
type RouteParams = { params: { id: string } }
```
```typescript
// PATCH /api/assets/:id - Rename or toggle visibility
export async function PATCH(request: NextRequest, { params }: RouteParams) {
const { id } = params
try {
const auth = await requireMutation()
if (!auth.authenticated) return auth.response
const { userId } = auth
if (!id) {
return NextResponse.json({ error: 'ID is required' }, { status: 400 })
}
const body = await request.json()
const { name, isPublic } = body
```
You should apply the same pattern to the `redownload` route:
1. Ensure its `RouteParams` (or equivalent) type uses `params: { id: string }` (not a Promise).
2. Destructure `id` directly from `params` without `await`, ideally at the top of the route handler:
`export async function redownload(request: NextRequest, { params }: RouteParams) { const { id } = params; ... }`.
</issue_to_address>
### Comment 6
<location> `apps/web/__tests__/media/assets-api.test.ts:2` </location>
<code_context>
+import { describe, it, expect, mock, beforeEach } from "bun:test";
+import { GET, POST, DELETE } from "@/app/api/assets/route";
+import { PATCH, GET as GET_ONE } from "@/app/api/assets/[id]/route";
+import { NextRequest } from "next/server";
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for POST /api/assets to cover asset creation and validation failures.
`POST` is imported but never exercised. Since the handler writes to the DB and enforces required fields and auth, please add tests that:
- Cover a successful creation for an authenticated user, including returned shape and defaulted fields.
- Assert `400` responses for missing required fields (e.g. `url`, `name`) and expected error payload.
- Assert unauthenticated/unauthorized responses when `requireMutation` reports `authenticated: false`.
Suggested implementation:
```typescript
// Mock auth
const mockRequireMutation = mock();
const mockAllowReadAccess = mock();
const mockGetSession = mock();
mock.module("@/lib/api-auth", () => ({
requireMutation: mockRequireMutation,
allowReadAccess: mockAllowReadAccess,
GUEST_USER_ID: "guest-id"
}));
describe("POST /api/assets", () => {
const baseUrl = "http://localhost/api/assets";
const makePostRequest = (body: unknown) =>
new NextRequest(baseUrl, {
method: "POST",
body: JSON.stringify(body),
headers: {
"content-type": "application/json"
}
});
beforeEach(() => {
mockRequireMutation.mockReset();
mockAllowReadAccess.mockReset();
mockGetSession.mockReset();
});
it("creates a new asset for an authenticated user and returns the created asset with defaulted fields", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: true,
userId: "user-123"
});
const payload = {
name: "Example asset",
url: "https://example.com/asset.png"
};
const req = makePostRequest(payload);
const res = await POST(req);
expect(res.status).toBe(201);
const json = await res.json();
// Basic shape: echo user-specified fields and include server-generated defaults
expect(json).toEqual(
expect.objectContaining({
name: payload.name,
url: payload.url
})
);
// Defaulted / server-populated fields
expect(json).toHaveProperty("id");
expect(json.id).toBeTruthy();
expect(json).toHaveProperty("createdAt");
expect(json).toHaveProperty("updatedAt");
});
it("returns 400 and validation errors when required fields are missing", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: true,
userId: "user-123"
});
// Missing both `name` and `url`
const invalidPayload = {};
const req = makePostRequest(invalidPayload);
const res = await POST(req);
expect(res.status).toBe(400);
const json = await res.json();
// Expect a structured validation error payload
expect(json).toHaveProperty("error");
expect(json.error).toEqual(
expect.objectContaining({
message: expect.any(String)
})
);
// If the handler returns field-specific errors, assert they include `name` and `url`
if ("fieldErrors" in json.error) {
expect(json.error.fieldErrors).toEqual(
expect.objectContaining({
name: expect.any(Array),
url: expect.any(Array)
})
);
}
});
it("returns an unauthenticated/unauthorized response when requireMutation reports authenticated: false", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: false
});
const payload = {
name: "Example asset",
url: "https://example.com/asset.png"
};
const req = makePostRequest(payload);
const res = await POST(req);
// Use whatever status code the handler uses for unauthenticated requests (401 or 403 are typical)
expect([401, 403]).toContain(res.status);
const json = await res.json();
expect(json).toHaveProperty("error");
expect(json.error).toEqual(
expect.objectContaining({
message: expect.any(String)
})
);
});
```
The above tests assume:
1. `requireMutation` is `async` and returns an object with at least `{ authenticated: boolean, userId?: string }`.
2. The `POST` handler:
- Returns `201` on success with a JSON body that includes `id`, `name`, `url`, `createdAt`, and `updatedAt`.
- Returns `400` on validation errors with a JSON payload containing an `error` object, optionally with `fieldErrors` keyed by field name.
- Returns `401` or `403` when `authenticated: false`.
If your actual handler differs, adjust:
- The expected status codes in the assertions.
- The shape of the JSON payloads (`json.error`, `json.error.fieldErrors`, defaulted fields).
Also, if this test file already defines shared helpers for building `NextRequest` objects or common auth mocks, prefer reusing those instead of `makePostRequest` and/or the local `beforeEach` to stay consistent with the rest of the test suite.
</issue_to_address>
### Comment 7
<location> `apps/web/__tests__/media/assets-api.test.ts:22-31` </location>
<code_context>
+}));
+
+// Mock DB
+const mockDb = {
+ select: mock(() => mockDb),
+ from: mock(() => mockDb),
+ where: mock(() => mockDb),
+ limit: mock(() => mockDb),
+ offset: mock(() => mockDb),
+ orderBy: mock(() => mockDb),
+ insert: mock(() => mockDb),
+ values: mock(() => mockDb),
+ update: mock(() => mockDb),
+ set: mock(() => mockDb),
+ delete: mock(() => mockDb),
+ returning: mock(() => [])
+};
+
</code_context>
<issue_to_address>
**issue (testing):** The current DB mocking pattern is likely to break the query chaining used in the handlers.
`mockDb.select` is initially a chainable mock (`mock(() => mockDb)`), but in `"should query db for authenticated user"` you later override it with `mockReturnValueOnce([...])`. At that point `select` returns an array instead of a query builder, so `db.select().from().where()` will fail when it tries to access `from` on the array, and the test will throw instead of exercising the handler logic.
To fix this, keep `select` returning a chainable object and only mock the terminal step of the chain. For example, either:
- Keep `select` returning `mockDb` and add a terminal method (e.g. `execute`/`all`) whose return value you control per test; or
- Have `select` return an object with a custom `from`/`where` chain whose final method returns the desired array.
The key is that mocks must preserve the Drizzle query-builder chaining contract while still allowing per-test data control.
</issue_to_address>
### Comment 8
<location> `apps/web/__tests__/media/assets-api.test.ts:89` </location>
<code_context>
+ });
+ });
+
+ describe("DELETE /api/assets", () => {
+ it("should delete file if owned by user", async () => {
+ mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative-path tests for DELETE /api/assets (missing id, unauthorized, or not owned).
Since the handler has explicit branches for missing `id` (400), auth failure (unauthenticated from `requireMutation`), and file not found / not owned (404), please add tests to cover these cases so error handling and status codes are verified and protected against regressions.
Suggested implementation:
```typescript
describe("DELETE /api/assets", () => {
it("should delete file if owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
mockDb.select.mockReturnValue([{ id: "file-1", storageProvider: "local-fs" }]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(200);
expect(data.success).toBe(true);
expect(mockDb.delete).toHaveBeenCalled();
});
it("should return 400 if id is missing", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets");
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
expect(mockDb.select).not.toHaveBeenCalled();
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 401 if user is not authenticated", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: false });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(401);
expect(data.success).toBe(false);
expect(mockDb.select).not.toHaveBeenCalled();
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 404 if file not found", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-unknown");
mockDb.select.mockReturnValue([]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 404 if file is not owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
mockDb.select.mockReturnValue([
{ id: "file-1", storageProvider: "local-fs", userId: "other-user" },
]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
expect(mockDb.delete).not.toHaveBeenCalled();
});
```
These tests assume:
- The DELETE handler returns a JSON body with `{ success: boolean }` for both success and error cases.
- The status codes are `400` for missing `id`, `401` for unauthenticated, and `404` for file not found or not owned.
- `mockDb.select` returns an array of file records, optionally including a `userId` field used by the handler to check ownership.
If the actual handler uses different response shapes, field names (e.g., `ownerId` instead of `userId`), or status codes, adjust the expectations and mocked record shape accordingly to match the real implementation.
</issue_to_address>
### Comment 9
<location> `apps/web/__tests__/media/assets-api.test.ts:3` </location>
<code_context>
+import { describe, it, expect, mock, beforeEach } from "bun:test";
+import { GET, POST, DELETE } from "@/app/api/assets/route";
+import { PATCH, GET as GET_ONE } from "@/app/api/assets/[id]/route";
+import { NextRequest } from "next/server";
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for GET /api/assets/[id] or removing the unused import.
The test file imports `GET as GET_ONE` from `@/app/api/assets/[id]/route`, but doesn’t exercise the single-asset GET handler. Please either add coverage for `GET /api/assets/:id` (at least a happy path and a failure case such as not owned/not found/unauthorized/missing id) or remove the unused import to keep the tests focused.
```suggestion
import { PATCH } from "@/app/api/assets/[id]/route";
```
</issue_to_address>
### Comment 10
<location> `apps/web/__tests__/media/assets-api.test.ts:104` </location>
<code_context>
+ });
+ });
+
+ describe("PATCH /api/assets/[id]", () => {
+ it("should update file name", async () => {
+ mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for PATCH /api/assets/[id] validation and ownership checks.
The current test only covers the successful rename. Please also add tests for the other branches in this handler: missing `id` (400), no valid fields to update (both `name` and `isPublic` undefined → 400), file not found or not owned (404), and unauthorized access via `requireMutation`. This will better exercise the validation and ownership checks, especially for metadata-only updates and failure cases.
Suggested implementation:
```typescript
describe("PATCH /api/assets/[id]", () => {
it("should update file name", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
mockDb.select.mockReturnValue([
{ id: "file-1", userId: "user-1", storageProvider: "local-fs", name: "old.jpg", isPublic: false },
]);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(200);
expect(data.success).toBe(true);
expect(mockDb.update).toHaveBeenCalledWith(
expect.objectContaining({ name: "new-name.jpg" })
);
});
it("should return 400 when id param is missing", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
const res = await PATCH(req, { params: {} as any });
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
});
it("should return 400 when no valid fields are provided", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({}),
} as any);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
});
it("should return 404 when asset is not found or not owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
mockDb.select.mockReturnValue([]); // not found / not owned
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
});
it("should return 401 when user is not authenticated", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: false, userId: null });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(401);
expect(data.success).toBe(false);
});
```
These tests assume:
1. `PATCH` is imported from the corresponding route file and accepts `(req, { params })`.
2. The handler returns `400` for missing `id` and when both `name` and `isPublic` are absent, `404` when the asset is not found / not owned, and `401` when `requireMutation` indicates an unauthenticated user.
3. The JSON response includes a `success` boolean.
If the actual handler uses different status codes or response shapes (e.g. 403 instead of 401, or no `success` flag), adjust the `expect(res.status)` and `expect(data.success)` assertions accordingly, and align the `mockDb.update` expectation with your real update call signature.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+5
to
+14
| import { getAssets, type AssetSort, type AssetOrder } from '../api/queries/get-assets' | ||
| import { updateAsset } from '../api/mutations/update-asset' | ||
| import { destroyFile } from '../api/mutations/destroy-file' | ||
| import { | ||
| Loader2, Search, Grid, List as ListIcon, MoreVertical, | ||
| File as FileIcon, Trash2, Edit2, Link as LinkIcon, Download, Eye | ||
| } from 'lucide-react' | ||
| import { | ||
| Dialog, DialogContent, DialogHeader, DialogTitle, | ||
| Button, Input, DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigger, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Client component is calling a use server function directly via React Query, which will not bundle correctly.
getAssets is a 'use server' function, but AssetLibrary is a client component and is using it as a React Query queryFn. Server actions/server-only modules can’t be called directly from client components; this will likely fail to bundle or break at runtime (e.g. no auth/DB access on the client).
Instead, either:
- Call your
/api/assetsREST endpoint viafetchin thequeryFn, or - Load the data in a server component and pass the results into a client-only presentation component.
Comment on lines
+29
to
+34
| const countResult = await db | ||
| .select({ count: files.id }) | ||
| .from(files) | ||
| .where(whereClause) | ||
|
|
||
| const total = countResult.length |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Total count computation in the assets API is incorrect for paginated responses.
This uses select({ count: files.id }) and then countResult.length as total, which will always be 1 for an aggregate COUNT query. Instead, select an aggregate (e.g. count(*)) and read the value from the single row (e.g. countResult[0].count), then compute totalPages from that value.
Comment on lines
+12
to
+14
| <AssetLibrary | ||
| onSelect={onSelect} | ||
| className={className} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The onSelect callback types between MediaPicker and AssetLibrary are incompatible in TypeScript.
MediaPicker exposes onSelect: (url: string) => void, but AssetLibrary expects onSelect?: (url: string, file: any) => void and calls it as onSelect?.(item.url, item). In TS, a function with fewer params is not assignable to one with more, so passing onSelect through will fail type-checking.
To fix this, make the second parameter optional in AssetLibrary:
type AssetLibraryProps = {
onSelect?: (url: string, file?: any) => void
}This preserves runtime behavior while making the types compatible.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env bun | |||
| #!/usr/bin / env bun | |||
Contributor
There was a problem hiding this comment.
issue (bug_risk): The shebang line has extra whitespace and indentation, which will break execution as a CLI script.
The shebang was changed from #!/usr/bin/env bun to #!/usr/bin / env bun, which adds whitespace inside /usr/bin/env and before the #!. It needs to be reverted to exactly:
#!/usr/bin/env bunso the interpreter path is valid and the script stays executable as a CLI.
Comment on lines
+5
to
+8
| type RouteParams = { params: Promise<{ id: string }> } | ||
|
|
||
| // PATCH /api/assets/:id - Rename or toggle visibility | ||
| export async function PATCH(request: NextRequest, { params }: RouteParams) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Route handler params is incorrectly typed as a Promise, and then awaited unnecessarily.
params is passed as a plain object, so typing it as Promise<{ id: string }> and awaiting it is incorrect and misleading for both consumers and tooling.
You can correct this to:
type RouteParams = { params: { id: string } }
export async function PATCH(request: NextRequest, { params }: RouteParams) {
const { id } = params
// ...
}Please update the redownload route similarly for consistency.
Suggested implementation:
type RouteParams = { params: { id: string } } // PATCH /api/assets/:id - Rename or toggle visibility
export async function PATCH(request: NextRequest, { params }: RouteParams) {
const { id } = params
try {
const auth = await requireMutation()
if (!auth.authenticated) return auth.response
const { userId } = auth
if (!id) {
return NextResponse.json({ error: 'ID is required' }, { status: 400 })
}
const body = await request.json()
const { name, isPublic } = bodyYou should apply the same pattern to the redownload route:
- Ensure its
RouteParams(or equivalent) type usesparams: { id: string }(not a Promise). - Destructure
iddirectly fromparamswithoutawait, ideally at the top of the route handler:
export async function redownload(request: NextRequest, { params }: RouteParams) { const { id } = params; ... }.
| @@ -0,0 +1,122 @@ | |||
| import { describe, it, expect, mock, beforeEach } from "bun:test"; | |||
| import { GET, POST, DELETE } from "@/app/api/assets/route"; | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for POST /api/assets to cover asset creation and validation failures.
POST is imported but never exercised. Since the handler writes to the DB and enforces required fields and auth, please add tests that:
- Cover a successful creation for an authenticated user, including returned shape and defaulted fields.
- Assert
400responses for missing required fields (e.g.url,name) and expected error payload. - Assert unauthenticated/unauthorized responses when
requireMutationreportsauthenticated: false.
Suggested implementation:
// Mock auth
const mockRequireMutation = mock();
const mockAllowReadAccess = mock();
const mockGetSession = mock();
mock.module("@/lib/api-auth", () => ({
requireMutation: mockRequireMutation,
allowReadAccess: mockAllowReadAccess,
GUEST_USER_ID: "guest-id"
}));
describe("POST /api/assets", () => {
const baseUrl = "http://localhost/api/assets";
const makePostRequest = (body: unknown) =>
new NextRequest(baseUrl, {
method: "POST",
body: JSON.stringify(body),
headers: {
"content-type": "application/json"
}
});
beforeEach(() => {
mockRequireMutation.mockReset();
mockAllowReadAccess.mockReset();
mockGetSession.mockReset();
});
it("creates a new asset for an authenticated user and returns the created asset with defaulted fields", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: true,
userId: "user-123"
});
const payload = {
name: "Example asset",
url: "https://example.com/asset.png"
};
const req = makePostRequest(payload);
const res = await POST(req);
expect(res.status).toBe(201);
const json = await res.json();
// Basic shape: echo user-specified fields and include server-generated defaults
expect(json).toEqual(
expect.objectContaining({
name: payload.name,
url: payload.url
})
);
// Defaulted / server-populated fields
expect(json).toHaveProperty("id");
expect(json.id).toBeTruthy();
expect(json).toHaveProperty("createdAt");
expect(json).toHaveProperty("updatedAt");
});
it("returns 400 and validation errors when required fields are missing", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: true,
userId: "user-123"
});
// Missing both `name` and `url`
const invalidPayload = {};
const req = makePostRequest(invalidPayload);
const res = await POST(req);
expect(res.status).toBe(400);
const json = await res.json();
// Expect a structured validation error payload
expect(json).toHaveProperty("error");
expect(json.error).toEqual(
expect.objectContaining({
message: expect.any(String)
})
);
// If the handler returns field-specific errors, assert they include `name` and `url`
if ("fieldErrors" in json.error) {
expect(json.error.fieldErrors).toEqual(
expect.objectContaining({
name: expect.any(Array),
url: expect.any(Array)
})
);
}
});
it("returns an unauthenticated/unauthorized response when requireMutation reports authenticated: false", async () => {
mockRequireMutation.mockResolvedValue({
authenticated: false
});
const payload = {
name: "Example asset",
url: "https://example.com/asset.png"
};
const req = makePostRequest(payload);
const res = await POST(req);
// Use whatever status code the handler uses for unauthenticated requests (401 or 403 are typical)
expect([401, 403]).toContain(res.status);
const json = await res.json();
expect(json).toHaveProperty("error");
expect(json.error).toEqual(
expect.objectContaining({
message: expect.any(String)
})
);
});The above tests assume:
requireMutationisasyncand returns an object with at least{ authenticated: boolean, userId?: string }.- The
POSThandler:- Returns
201on success with a JSON body that includesid,name,url,createdAt, andupdatedAt. - Returns
400on validation errors with a JSON payload containing anerrorobject, optionally withfieldErrorskeyed by field name. - Returns
401or403whenauthenticated: false.
- Returns
If your actual handler differs, adjust:
- The expected status codes in the assertions.
- The shape of the JSON payloads (
json.error,json.error.fieldErrors, defaulted fields).
Also, if this test file already defines shared helpers for buildingNextRequestobjects or common auth mocks, prefer reusing those instead ofmakePostRequestand/or the localbeforeEachto stay consistent with the rest of the test suite.
Comment on lines
+22
to
+31
| const mockDb = { | ||
| select: mock(() => mockDb), | ||
| from: mock(() => mockDb), | ||
| where: mock(() => mockDb), | ||
| limit: mock(() => mockDb), | ||
| offset: mock(() => mockDb), | ||
| orderBy: mock(() => mockDb), | ||
| insert: mock(() => mockDb), | ||
| values: mock(() => mockDb), | ||
| update: mock(() => mockDb), |
Contributor
There was a problem hiding this comment.
issue (testing): The current DB mocking pattern is likely to break the query chaining used in the handlers.
mockDb.select is initially a chainable mock (mock(() => mockDb)), but in "should query db for authenticated user" you later override it with mockReturnValueOnce([...]). At that point select returns an array instead of a query builder, so db.select().from().where() will fail when it tries to access from on the array, and the test will throw instead of exercising the handler logic.
To fix this, keep select returning a chainable object and only mock the terminal step of the chain. For example, either:
- Keep
selectreturningmockDband add a terminal method (e.g.execute/all) whose return value you control per test; or - Have
selectreturn an object with a customfrom/wherechain whose final method returns the desired array.
The key is that mocks must preserve the Drizzle query-builder chaining contract while still allowing per-test data control.
| }); | ||
| }); | ||
|
|
||
| describe("DELETE /api/assets", () => { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add negative-path tests for DELETE /api/assets (missing id, unauthorized, or not owned).
Since the handler has explicit branches for missing id (400), auth failure (unauthenticated from requireMutation), and file not found / not owned (404), please add tests to cover these cases so error handling and status codes are verified and protected against regressions.
Suggested implementation:
describe("DELETE /api/assets", () => {
it("should delete file if owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
mockDb.select.mockReturnValue([{ id: "file-1", storageProvider: "local-fs" }]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(200);
expect(data.success).toBe(true);
expect(mockDb.delete).toHaveBeenCalled();
});
it("should return 400 if id is missing", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets");
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
expect(mockDb.select).not.toHaveBeenCalled();
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 401 if user is not authenticated", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: false });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(401);
expect(data.success).toBe(false);
expect(mockDb.select).not.toHaveBeenCalled();
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 404 if file not found", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-unknown");
mockDb.select.mockReturnValue([]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
expect(mockDb.delete).not.toHaveBeenCalled();
});
it("should return 404 if file is not owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets?id=file-1");
mockDb.select.mockReturnValue([
{ id: "file-1", storageProvider: "local-fs", userId: "other-user" },
]);
const res = await DELETE(req);
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
expect(mockDb.delete).not.toHaveBeenCalled();
});These tests assume:
- The DELETE handler returns a JSON body with
{ success: boolean }for both success and error cases. - The status codes are
400for missingid,401for unauthenticated, and404for file not found or not owned. mockDb.selectreturns an array of file records, optionally including auserIdfield used by the handler to check ownership.
If the actual handler uses different response shapes, field names (e.g., ownerId instead of userId), or status codes, adjust the expectations and mocked record shape accordingly to match the real implementation.
| @@ -0,0 +1,122 @@ | |||
| import { describe, it, expect, mock, beforeEach } from "bun:test"; | |||
| import { GET, POST, DELETE } from "@/app/api/assets/route"; | |||
| import { PATCH, GET as GET_ONE } from "@/app/api/assets/[id]/route"; | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for GET /api/assets/[id] or removing the unused import.
The test file imports GET as GET_ONE from @/app/api/assets/[id]/route, but doesn’t exercise the single-asset GET handler. Please either add coverage for GET /api/assets/:id (at least a happy path and a failure case such as not owned/not found/unauthorized/missing id) or remove the unused import to keep the tests focused.
Suggested change
| import { PATCH, GET as GET_ONE } from "@/app/api/assets/[id]/route"; | |
| import { PATCH } from "@/app/api/assets/[id]/route"; |
| }); | ||
| }); | ||
|
|
||
| describe("PATCH /api/assets/[id]", () => { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for PATCH /api/assets/[id] validation and ownership checks.
The current test only covers the successful rename. Please also add tests for the other branches in this handler: missing id (400), no valid fields to update (both name and isPublic undefined → 400), file not found or not owned (404), and unauthorized access via requireMutation. This will better exercise the validation and ownership checks, especially for metadata-only updates and failure cases.
Suggested implementation:
describe("PATCH /api/assets/[id]", () => {
it("should update file name", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
mockDb.select.mockReturnValue([
{ id: "file-1", userId: "user-1", storageProvider: "local-fs", name: "old.jpg", isPublic: false },
]);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(200);
expect(data.success).toBe(true);
expect(mockDb.update).toHaveBeenCalledWith(
expect.objectContaining({ name: "new-name.jpg" })
);
});
it("should return 400 when id param is missing", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
const res = await PATCH(req, { params: {} as any });
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
});
it("should return 400 when no valid fields are provided", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({}),
} as any);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(400);
expect(data.success).toBe(false);
});
it("should return 404 when asset is not found or not owned by user", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: true, userId: "user-1" });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
mockDb.select.mockReturnValue([]); // not found / not owned
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(404);
expect(data.success).toBe(false);
});
it("should return 401 when user is not authenticated", async () => {
mockRequireMutation.mockResolvedValue({ authenticated: false, userId: null });
const req = new NextRequest("http://localhost/api/assets/file-1", {
method: "PATCH",
body: JSON.stringify({ name: "new-name.jpg" }),
} as any);
const res = await PATCH(req, { params: { id: "file-1" } as any });
const data = await res.json();
expect(res.status).toBe(401);
expect(data.success).toBe(false);
});These tests assume:
PATCHis imported from the corresponding route file and accepts(req, { params }).- The handler returns
400for missingidand when bothnameandisPublicare absent,404when the asset is not found / not owned, and401whenrequireMutationindicates an unauthenticated user. - The JSON response includes a
successboolean.
If the actual handler uses different status codes or response shapes (e.g. 403 instead of 401, or no success flag), adjust the expect(res.status) and expect(data.success) assertions accordingly, and align the mockDb.update expectation with your real update call signature.
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 by Sourcery
Introduce a full-featured user asset management system and integrate it into the web app’s media workflows.
New Features:
Enhancements:
Build:
Documentation:
Tests: