feat: move file listing to dedicated api#420
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new CORS-enabled GET /api/files route plus supporting libs: query validation and auth checks, Supabase file fetching, path/recursive filtering, file enrichment with owner emails, and route caching disabled. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as API Route
participant Handler as Files Handler
participant Validator as Query Validator
participant Auth as Auth Service
participant DB as Files DB
participant Accounts as Accounts Service
Client->>Route: GET /api/files?artist_account_id=...&path=...&recursive=...
Route->>Handler: forward request
Handler->>Validator: validateGetFilesQuery(request)
Validator->>Auth: validateAuthContext() / checkAccountArtistAccess()
Auth-->>Validator: auth context / access result
Validator-->>Handler: ValidatedGetFilesQuery or NextResponse(error)
Handler->>DB: listFilesByArtist(artistAccountId, path, recursive)
DB->>DB: getFilesByArtistId(...)
DB-->>Handler: files[]
Handler->>Accounts: selectAccountEmails(owner_ids)
Accounts-->>Handler: account email rows
Handler->>Handler: enrichFiles(files)
Handler-->>Client: NextResponse.json({ files }) + CORS headers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
1 issue found across 9 files
Confidence score: 2/5
- High-confidence, high-severity issue in
lib/files/validateGetFilesQuery.ts: usingz.coerce.boolean()will convert the string"false"totrue, which can invert request behavior in a user-facing query path. - This creates a concrete regression risk (severity 8/10, confidence 10/10), so merge risk is elevated until boolean parsing is corrected.
- A safer approach is to use the existing
lib/content/booleanFromString.tshelper for explicit string-to-boolean handling. - Pay close attention to
lib/files/validateGetFilesQuery.ts- query boolean coercion can misinterpret"false"and return incorrect results.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/files/validateGetFilesQuery.ts">
<violation number="1" location="lib/files/validateGetFilesQuery.ts:10">
P1: `z.coerce.boolean()` will parse the query string `"false"` as `true` (since `Boolean("false") === true`). Use the existing `booleanFromString` helper from `lib/content/booleanFromString.ts` which was created for exactly this case.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Handler as getFilesHandler (API)
participant Auth as validateGetFilesQuery
participant Logic as listFilesByArtist
participant DB as Supabase (Database)
Note over Client,DB: NEW: Dedicated File Listing Flow
Client->>Handler: GET /api/files?artist_account_id=...&path=...
Handler->>Auth: NEW: validateGetFilesQuery(request)
Auth->>Auth: validateAuthContext()
Auth->>DB: checkAccountArtistAccess(userId, artistId)
DB-->>Auth: access allowed
alt NEW: Access Denied or Invalid Input
Auth-->>Handler: 400/401/403 Error Response
Handler-->>Client: Error JSON
end
Auth-->>Handler: Validated Query + requesterAccountId
Handler->>Logic: NEW: listFilesByArtist(artistId, path, recursive)
Logic->>DB: NEW: getFilesByArtistId(artistId)
DB-->>Logic: File rows (all files for artist)
alt recursive is false
Logic->>Logic: NEW: filterFilesByPath(rows, path)
else recursive is true
Logic->>Logic: Filter by path prefix
end
Logic-->>Handler: Filtered File records
opt If files found
Handler->>Handler: Extract unique owner_account_ids
Handler->>DB: NEW: selectAccountEmails(accountIds)
DB-->>Handler: Email rows
Handler->>Handler: NEW: Enrich file objects with owner_email
end
Handler-->>Client: 200 OK (JSON with enriched files)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/files/listFilesByArtist.ts (1)
7-9: Consider co-locatingListedFileRecordwith its usage.The
ListedFileRecordtype extendsFileRecordwithowner_email, but the enrichment that adds this field happens ingetFilesHandler.ts. While exporting it here works, consider whether this type belongs ingetFilesHandler.tsor a shared types file, sincelistFilesByArtistreturnsFileRecord[](without the enrichment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/files/listFilesByArtist.ts` around lines 7 - 9, The ListedFileRecord type (which extends FileRecord with owner_email) is declared in listFilesByArtist.ts but the enrichment adding owner_email is performed in getFilesHandler.ts; move or re-export the type to be colocated with its usage by either relocating ListedFileRecord into getFilesHandler.ts or into a shared types module used by both files, update imports in listFilesByArtist (and any consumers) to reference the new location, and ensure listFilesByArtist's return type reflects whether it returns FileRecord[] (unenriched) or ListedFileRecord[] (enriched) so the type accurately matches the implementation.lib/files/filterFilesByPath.ts (1)
14-14: Consider extracting the duplicated regex pattern to a shared constant.The regex
/^files\/[^\/]+\/[^\/]+\/(.+)$/is duplicated here and inlistFilesByArtist.ts(line 30). Extracting it to a shared constant (e.g.,STORAGE_KEY_RELATIVE_PATH_REGEX) would improve maintainability and ensure consistency if the storage key format ever changes.♻️ Proposed refactor
Create a shared constant in a new file
lib/files/constants.ts:/** Extracts the relative path portion from a storage_key (e.g., "files/owner/artist/path/to/file.txt" → "path/to/file.txt") */ export const STORAGE_KEY_RELATIVE_PATH_REGEX = /^files\/[^\/]+\/[^\/]+\/(.+)$/;Then import and use it in both
filterFilesByPath.tsandlistFilesByArtist.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/files/filterFilesByPath.ts` at line 14, Extract the duplicated regex /^files\/[^\/]+\/[^\/]+\/(.+)$/ into a shared constant (e.g., STORAGE_KEY_RELATIVE_PATH_REGEX) exported from a new module (suggested lib/files/constants.ts with the provided docstring) and replace the inline literal in filterFilesByPath (where const match = file.storage_key.match(...)) and in listFilesByArtist (the similar match on line 30) with imports of STORAGE_KEY_RELATIVE_PATH_REGEX; update both files to import the constant and use file.storage_key.match(STORAGE_KEY_RELATIVE_PATH_REGEX) so the pattern is defined in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/files/getFilesHandler.ts`:
- Line 27: Remove the unnecessary defensive filter by changing the ownerIds
calculation to use the direct mapping of owner_account_id without filtering:
update the expression that defines ownerIds (the Array.from(new Set(...)) call
using files.map(file => file.owner_account_id)) to remove .filter(Boolean); keep
the Set and Array.from logic intact so duplicates are still removed.
In `@lib/files/validateGetFilesQuery.ts`:
- Line 10: The recursive query field currently uses z.coerce.boolean() which
treats the string "false" as truthy; update the schema in
validateGetFilesQuery.ts for the recursive field to parse query string booleans
explicitly by replacing z.coerce.boolean().optional().default(false) with a
string-based transform that accepts 'true'|'false' and maps to a boolean (e.g.,
z.string().pipe(z.enum(['true','false']).transform(v => v ===
'true')).optional().default(false)), keeping the same optional/default behavior
so ?recursive=false yields false.
---
Nitpick comments:
In `@lib/files/filterFilesByPath.ts`:
- Line 14: Extract the duplicated regex /^files\/[^\/]+\/[^\/]+\/(.+)$/ into a
shared constant (e.g., STORAGE_KEY_RELATIVE_PATH_REGEX) exported from a new
module (suggested lib/files/constants.ts with the provided docstring) and
replace the inline literal in filterFilesByPath (where const match =
file.storage_key.match(...)) and in listFilesByArtist (the similar match on line
30) with imports of STORAGE_KEY_RELATIVE_PATH_REGEX; update both files to import
the constant and use file.storage_key.match(STORAGE_KEY_RELATIVE_PATH_REGEX) so
the pattern is defined in one place.
In `@lib/files/listFilesByArtist.ts`:
- Around line 7-9: The ListedFileRecord type (which extends FileRecord with
owner_email) is declared in listFilesByArtist.ts but the enrichment adding
owner_email is performed in getFilesHandler.ts; move or re-export the type to be
colocated with its usage by either relocating ListedFileRecord into
getFilesHandler.ts or into a shared types module used by both files, update
imports in listFilesByArtist (and any consumers) to reference the new location,
and ensure listFilesByArtist's return type reflects whether it returns
FileRecord[] (unenriched) or ListedFileRecord[] (enriched) so the type
accurately matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4175eba8-35b7-4323-9519-305cbda0cf0a
⛔ Files ignored due to path filters (3)
lib/files/__tests__/getFilesHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/listFilesByArtist.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/validateGetFilesQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
app/api/files/route.tslib/files/filterFilesByPath.tslib/files/getFilesHandler.tslib/files/listFilesByArtist.tslib/files/validateGetFilesQuery.tslib/supabase/files/getFilesByArtistId.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/files/listFilesByArtist.ts (1)
16-37: Extract recursive filtering into a reusable utility.
listFilesByArtistis over 20 lines and duplicates path parsing logic already present inlib/files/filterFilesByPath.ts. Split recursive filtering into a shared helper so this function stays orchestration-focused.As per coding guidelines, "Flag functions longer than 20 lines" and "Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/files/listFilesByArtist.ts` around lines 16 - 37, The function listFilesByArtist contains recursive path-parsing/filtering logic that duplicates code in filterFilesByPath; extract that recursion-aware logic into a reusable helper (e.g., filterFilesByPathRecursive or extend filterFilesByPath) that accepts (files: FileRecord[], path?: string, recursive: boolean) and encapsulates the STORAGE_KEY_RELATIVE_PATH_REGEX matching and pathPrefix logic, then replace the inline block in listFilesByArtist (which uses getFilesByArtistId and STORAGE_KEY_RELATIVE_PATH_REGEX) with a single call to the new helper so listFilesByArtist stays orchestration-focused and under 20 lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/files/enrichFiles.ts`:
- Around line 21-24: The current loop over ownerEmailRows sets ownerEmails to
the first-seen row per account_id, which is non-deterministic; change the logic
to deterministically pick the row with the most recent updated_at for each
account_id: when iterating ownerEmailRows (symbol ownerEmailRows) skip rows with
falsy row.account_id, and for each row compare row.updated_at against the
existing timestamp stored for that account_id in ownerEmails (or a parallel map
like ownerEmailTimestamps); if the current row.updated_at is newer (parse as
Date or compare ISO strings) replace the stored email (row.email ?? null) and
timestamp, otherwise keep the existing one, so ownerEmails always ends up with
the most-recent email per account_id.
In `@lib/files/listFilesByArtist.ts`:
- Around line 21-36: The current listFilesByArtist implementation loads all
files via getFilesByArtistId and then filters in-memory (using
STORAGE_KEY_RELATIVE_PATH_REGEX and filterFilesByPath), which will not scale;
instead modify getFilesByArtistId (or add a new function like
getFilesByArtistIdWithPath) to accept path and recursive flags and perform the
path/recursive filtering in the Supabase query (use storage_key LIKE 'prefix%'
or the DB regex equivalent to implement the pathPrefix logic used when
path.endsWith("/") and ensure non-recursive behavior matches filterFilesByPath),
and add server-side pagination parameters so listFilesByArtist calls the DB with
path and recursive to return only matching rows rather than fetching all files
into memory.
---
Nitpick comments:
In `@lib/files/listFilesByArtist.ts`:
- Around line 16-37: The function listFilesByArtist contains recursive
path-parsing/filtering logic that duplicates code in filterFilesByPath; extract
that recursion-aware logic into a reusable helper (e.g.,
filterFilesByPathRecursive or extend filterFilesByPath) that accepts (files:
FileRecord[], path?: string, recursive: boolean) and encapsulates the
STORAGE_KEY_RELATIVE_PATH_REGEX matching and pathPrefix logic, then replace the
inline block in listFilesByArtist (which uses getFilesByArtistId and
STORAGE_KEY_RELATIVE_PATH_REGEX) with a single call to the new helper so
listFilesByArtist stays orchestration-focused and under 20 lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5910e33f-5770-45cd-875d-5695249631e9
⛔ Files ignored due to path filters (4)
lib/files/__tests__/enrichFiles.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/getFilesHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/listFilesByArtist.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/validateGetFilesQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
lib/files/constants.tslib/files/enrichFiles.tslib/files/filterFilesByPath.tslib/files/getFilesHandler.tslib/files/listFilesByArtist.tslib/files/validateGetFilesQuery.ts
✅ Files skipped from review due to trivial changes (1)
- lib/files/constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/files/getFilesHandler.ts
- lib/files/filterFilesByPath.ts
- lib/files/validateGetFilesQuery.ts
| const allFiles = await getFilesByArtistId(artistAccountId); | ||
|
|
||
| if (recursive) { | ||
| if (path) { | ||
| const pathPrefix = path.endsWith("/") ? path : `${path}/`; | ||
| return allFiles.filter(file => { | ||
| const match = file.storage_key.match(STORAGE_KEY_RELATIVE_PATH_REGEX); | ||
| if (!match) return false; | ||
| return match[1].startsWith(pathPrefix); | ||
| }); | ||
| } | ||
|
|
||
| return allFiles; | ||
| } | ||
|
|
||
| return filterFilesByPath(allFiles, path); |
There was a problem hiding this comment.
Avoid full artist-file reads for path-filtered requests.
Line 21 pulls all files, then Lines 23-36 filter in memory. This will degrade badly for large artist libraries (latency + memory). Push path/recursive filtering into the Supabase query (and ideally paginate) so only matching rows are loaded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/files/listFilesByArtist.ts` around lines 21 - 36, The current
listFilesByArtist implementation loads all files via getFilesByArtistId and then
filters in-memory (using STORAGE_KEY_RELATIVE_PATH_REGEX and filterFilesByPath),
which will not scale; instead modify getFilesByArtistId (or add a new function
like getFilesByArtistIdWithPath) to accept path and recursive flags and perform
the path/recursive filtering in the Supabase query (use storage_key LIKE
'prefix%' or the DB regex equivalent to implement the pathPrefix logic used when
path.endsWith("/") and ensure non-recursive behavior matches filterFilesByPath),
and add server-side pagination parameters so listFilesByArtist calls the DB with
path and recursive to return only matching rows rather than fetching all files
into memory.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/files/__tests__/listFilesByArtist.test.ts">
<violation number="1" location="lib/files/__tests__/listFilesByArtist.test.ts:89">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The updated assertion includes file `"2"` (the `reports/` directory entry itself) in results described as "all descendants". Tracing the implementation, `"reports/".startsWith("reports/")` is `true`, so the filter accidentally includes the queried directory. The old assertion `["3", "4"]` was correct — this looks like a regression being normalized in the test rather than a bug being caught. Consider adding an `&&` guard in the filter (e.g., `match[1] !== pathPrefix`) or adjusting the test name if this inclusion is intentional.</violation>
</file>
<file name="lib/files/enrichFiles.ts">
<violation number="1" location="lib/files/enrichFiles.ts:22">
P2: The first-seen-wins deduplication here is non-deterministic when an account has multiple email rows, because query result ordering is not guaranteed. Consider sorting `ownerEmailRows` by a stable criterion (e.g. most recent `updated_at`) before populating the map, so `owner_email` is consistent across requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
Smoke test summary:
Note: this worked because the preview API deployment is reading the same Supabase project that the current |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/files/enrichFiles.ts (1)
16-36: RefactorenrichFilesinto smaller helpers to satisfy length and DRY constraints.
enrichFilesis currently 21 lines. Please extract the owner-email map construction into a small utility (for example,buildLatestOwnerEmailMap) so the exported function remains focused on orchestration.Suggested refactor
import type { Tables } from "@/types/database.types"; import selectAccountEmails from "@/lib/supabase/account_emails/selectAccountEmails"; type FileRecord = Tables<"files">; export type ListedFileRecord = FileRecord & { owner_email: string | null; }; + +function buildLatestOwnerEmailMap(ownerEmailRows: Tables<"account_emails">[]) { + const ownerEmails = new Map<string, string | null>(); + const ownerEmailUpdatedAts = new Map<string, string>(); + + for (const row of ownerEmailRows) { + if (!row.account_id) continue; + const existingUpdatedAt = ownerEmailUpdatedAts.get(row.account_id); + if (existingUpdatedAt && row.updated_at <= existingUpdatedAt) continue; + ownerEmails.set(row.account_id, row.email ?? null); + ownerEmailUpdatedAts.set(row.account_id, row.updated_at); + } + + return ownerEmails; +} export async function enrichFiles(files: FileRecord[]): Promise<ListedFileRecord[]> { const ownerIds = Array.from(new Set(files.map(file => file.owner_account_id))); const ownerEmailRows = ownerIds.length ? await selectAccountEmails({ accountIds: ownerIds }) : []; - - const ownerEmails = new Map<string, string | null>(); - const ownerEmailUpdatedAts = new Map<string, string | null>(); - for (const row of ownerEmailRows) { - if (!row.account_id) continue; - - const existingUpdatedAt = ownerEmailUpdatedAts.get(row.account_id); - if (existingUpdatedAt && row.updated_at <= existingUpdatedAt) continue; - - ownerEmails.set(row.account_id, row.email ?? null); - ownerEmailUpdatedAts.set(row.account_id, row.updated_at); - } + const ownerEmails = buildLatestOwnerEmailMap(ownerEmailRows); return files.map(file => ({ ...file, owner_email: ownerEmails.get(file.owner_account_id) ?? null, })); }As per coding guidelines,
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: "Flag functions longer than 20 lines" and "Keep functions small and focused."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/files/enrichFiles.ts` around lines 16 - 36, Extract the owner-email map construction from enrichFiles into a new helper (e.g., buildLatestOwnerEmailMap) that accepts ownerIds and ownerEmailRows (or calls selectAccountEmails internally) and returns a Map<string, string | null> (and optionally updatedAt map internally) containing the latest email per account_id; then simplify enrichFiles to compute ownerIds, call selectAccountEmails (or the new helper), receive the ownerEmails map, and return the mapped files using ownerEmails.get(file.owner_account_id) ?? null so enrichFiles only orchestrates and the email-merging logic lives in buildLatestOwnerEmailMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/files/enrichFiles.ts`:
- Around line 16-36: Extract the owner-email map construction from enrichFiles
into a new helper (e.g., buildLatestOwnerEmailMap) that accepts ownerIds and
ownerEmailRows (or calls selectAccountEmails internally) and returns a
Map<string, string | null> (and optionally updatedAt map internally) containing
the latest email per account_id; then simplify enrichFiles to compute ownerIds,
call selectAccountEmails (or the new helper), receive the ownerEmails map, and
return the mapped files using ownerEmails.get(file.owner_account_id) ?? null so
enrichFiles only orchestrates and the email-merging logic lives in
buildLatestOwnerEmailMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ed37c57-06c4-4a2a-ab10-4ab86e3144ae
⛔ Files ignored due to path filters (2)
lib/files/__tests__/enrichFiles.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/files/__tests__/listFilesByArtist.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (2)
lib/files/enrichFiles.tslib/files/listFilesByArtist.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/files/listFilesByArtist.ts
Summary
GET /api/filesendpoint in the API serviceowner_emailpathfiltering and optional recursive listingSummary by CodeRabbit