refactor: replace folderPath with folders table and folderId#711
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Create explicit `folders` table with parent-child hierarchy for document organization. Documents now reference folders via `folderId` foreign key instead of implicit `folderPath` string field. - New `folders` table with `by_organizationId_and_parentId` index - Folder CRUD mutations (create, rename, delete with child checks) - `listFolders` and `getFolderBreadcrumb` queries - `getOrCreateFolderPath` helper for import path resolution - `folderId` field and `by_organizationId_and_folderId` index on documents - Updated document types and validators Refs #703
Replace all folderPath references with folderId across document backend: - Write paths: create, update, upload, and OneDrive import - Read paths: get_documents, cursor pagination, get_by_path (folder traversal) - Transform: output folderId instead of storagePath - OneDrive import: compute folderId from relativePath via getOrCreateFolderPath Refs #703
- documents-table: navigate by folderId, show folder rows, filter by exact match - breadcrumb-navigation: use getFolderBreadcrumb query with toFolderId helper - document-preview-dialog: remove storagePath, lookup by documentId only - documents route: use folderId URL search param Refs #703
- Tests for getOrCreateFolderPath (empty path, nested creation, idempotency) - Tests for buildBreadcrumb (single/nested/missing/broken chain) - Migration to backfill folderId from existing metadata.storagePath Refs #703
- New folder item in Import documents dropdown menu - CreateFolderDialog component with name input and validation - useFolders hook queries folders for current navigation level - Folder rows displayed at top of documents table - Documents filtered to current folder level (root shows only root docs) - useCreateFolder mutation hook Refs #703
…older - DocumentsActionMenu passes currentFolderId to DocumentUploadDialog - DocumentUploadDialog forwards folderId to uploadFiles options - useDocumentUpload passes folderId to createDocumentFromUpload mutation Refs #703
…ering server-side Add by_org_parent_name compound index to folders table so path traversal and folder creation can do direct lookups instead of scanning. Move folder-level document filtering from the client into the paginated query (server-side via folderId index). Introduce a dedicated deleteFolder mutation with separate UI dialogs for sync vs regular folders. Harden breadcrumb building with cycle detection and proper auth checks, and add storagePath fallback matching in get_document_by_path.
…resolution Add folder name validation (length, reserved names), duplicate name checks on create/rename, team-based access control on breadcrumbs, and paginated backfill migration. Remove storagePath fallback from document path lookup and use shared toId helper in breadcrumb navigation.
…stness - Always use folderId index for document queries instead of conditional dispatch - Add path separator validation to folder names and pass teamId through creation - Fix breadcrumb to use semantic nav/ol markup with focus-visible states - Navigate to root when current folder is deleted or breadcrumb resolves empty - Fix duplicate name check in rename to exclude current folder - Add depth limit to breadcrumb traversal and team access filtering to listFolders - Make backfill migration self-contained with full cursor loop and error handling - Fix i18n: interpolate folder name in delete confirmations, add missing keys
…n and tests Remove redundant by_organizationId_and_parentId index in favor of the compound by_org_parent_name index. Validate folder ownership in document create/update operations. Propagate teamId through getOrCreateFolderPath for OneDrive imports. Fix breadcrumb navigation callback stability with useRef and add accessibility aria-labels for document/folder rows.
…validation to folders Enforce team-based access control on folder mutations (create, rename, delete) and document uploads. Add folder depth limit (20), rate limiting for folder mutations, smarter index selection for document queries, and breadcrumb filtering by team access. Improve error logging in backfill migration and add comprehensive tests.
larryro
force-pushed
the
refactor/703-promote-folder-path
branch
from
4609793 to
a009b2c
Compare
Contributor
📝 WalkthroughWalkthroughThis PR introduces folder-based organization for documents by adding a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Replace unlisted sonner import with project toast hook and fix test type safety (no-explicit-any, no-non-null-assertion, implicit any).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/convex/documents/list_documents_paginated.ts (1)
78-99:⚠️ Potential issue | 🟠 MajorRoot pagination currently leaks nested documents.
filterArgs.folderIdbecomesundefinedfor root requests, soprimaryresolves to another filter or no folder filter at all and pagination runs across the whole organization. If this API backs the top-level folder view, documents with a realfolderIdwill still appear there. Distinguish “root only” from “all folders” before building the base query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/documents/list_documents_paginated.ts` around lines 78 - 99, The bug is that root requests (explicit "root only") are indistinguishable from omitted folder filters because filterArgs.folderId is coerced to undefined; change the folderId handling to preserve an explicit root marker: set filterArgs.folderId = null when args.folderId === null (keep String(args.folderId) for non-null values and leave undefined when omitted), then let buildBaseQuery/primary selection and subsequent FILTER_INDEXES filtering treat null as a valid folderId value so the query filters for q.eq(q.field('folderId'), null) for root-only requests rather than returning documents across all folders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/app/features/documents/components/breadcrumb-navigation.tsx`:
- Around line 47-53: The three button elements in the BreadcrumbNavigation
component (the back button with onClick={() => onNavigate(undefined)}, the other
breadcrumb navigation buttons around that same JSX and the one further down used
for navigation) are missing explicit type attributes; add type="button" to each
<button> to satisfy the a11y/useButtonType rule and ensure they don't default to
type="submit". Update the button elements referenced by the onClick handlers
(e.g., the button calling onNavigate) to include type="button".
In
`@services/platform/app/features/documents/components/document-delete-folder-dialog.tsx`:
- Around line 45-53: The unordered list in the DocumentDeleteFolderDialog
component is missing explicit list semantics; update the <ul> that renders the
delete-impact items (the element using className "text-muted-foreground ml-4
list-disc space-y-1 text-sm") to include role="list" so screen readers (e.g.,
Safari/VoiceOver) correctly announce it as a list while preserving existing
className and children.
In
`@services/platform/app/features/documents/components/document-row-actions.tsx`:
- Around line 72-90: The delete flow in handleDeleteFolderConfirm (which uses
useDeleteFolder -> deleteFolder) incorrectly calls the generic
folders.deleteFolder mutation for synced folders; update the handler to check
the folder payload for a syncConfigId (using documentId/context) and, when
present, call the dedicated sync-folder delete path instead of deleteFolder,
ensuring dialogs.setOpen.deleteFolder and onFolderDeleted are still invoked on
success and the same onError toast behavior is preserved; if the dedicated
sync-delete backend is not available yet, short-circuit to the existing
empty-folder flow (current deleteFolder) to avoid orphaned sync state or hard
failures.
In
`@services/platform/app/features/documents/components/documents-action-menu.tsx`:
- Around line 73-77: The folder creation action is always pushed into items but
the branch for hasMicrosoftAccount === false bypasses the menu and directly
triggers device upload, making CreateFolderDialog unreachable; update the
conditional logic so the FolderPlus item (and its onClick handler
handleCreateFolder) is rendered in the menu for both Microsoft-connected and
non-Microsoft workspaces, and limit the hasMicrosoftAccount check to only choose
upload behavior (e.g., showing OneDrive vs device upload) — inspect the items
array construction and the code paths around handleCreateFolder and the upload
logic (the branch currently around hasMicrosoftAccount and the block referenced
also at lines 90-102) and refactor so the folder action remains in the menu
while only upload/menu options change based on hasMicrosoftAccount.
In `@services/platform/app/features/documents/components/documents-table.tsx`:
- Around line 61-67: The paginator is being fed an organization-wide approximate
count (useApproxDocumentCount(organizationId)) while rows are scoped by
currentFolderId via useListDocumentsPaginated and paginatedResult, causing an
overestimate; update the call to useListPage so it receives a folder-aware count
(e.g., useApproxDocumentCount(organizationId, currentFolderId) or a new
folder-scoped count hook) or remove the approxRowCount/approxRowCount fallback
(and the prepended folderRows adjustment) until the count API matches the
folder-scoped results; look for useListPage, approxRowCount, folderRows,
useListDocumentsPaginated, useFolders and currentFolderId in the file and make
the change there.
- Around line 162-165: handleFolderDeleted currently always calls
navigateToFolder(undefined) which resets to root; instead thread the deleted
folder's parent id through the delete flow and navigate to that parent (or
undefined only if no parent). Update the handler(s) in useDocumentsTableConfig
(and the other occurrence around the 181-184 block) to accept/receive a
parentFolderId from the delete action and call navigateToFolder(parentFolderId
?? undefined) rather than hardcoding undefined; reference the
handleFolderDeleted and navigateToFolder symbols and ensure the delete action
supplies parentFolderId so nested deletions return to the correct parent folder.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/documents.tsx:
- Around line 10-13: The route should validate folderId before passing it to
DocumentsTable to avoid Convex v.id('folders') errors: update
validateSearch/searchSchema (or add a pre-parse check in the route handler) to
verify folderId matches the expected Convex folder-id format (or use an
isValidConvexId helper) and either reject or set folderId to undefined when it
fails validation; ensure the route uses the cleaned value (from validateSearch
or the route-level check) when rendering DocumentsTable so malformed deep links
like ?folderId=foo cannot reach the Convex query.
In `@services/platform/convex/documents/create_document.ts`:
- Around line 16-21: The folder check only verifies folder.organizationId but
misses team-scope alignment; update the validation in create_document.ts where
you call ctx.db.get(args.folderId) so that if folder exists you also compare
folder.teamId to args.teamId (treat null/undefined as org-wide consistently) and
either reject with an error when they mismatch or derive the document's
teamId/org by overwriting args.teamId with folder.teamId before persisting;
ensure this extra validation/derivation happens before the insert/write so
folder.organizationId, folder.teamId, args.organizationId and args.teamId are
always aligned (references: args.folderId, ctx.db.get, folder.organizationId,
folder.teamId, args.teamId).
In `@services/platform/convex/documents/get_documents_cursor.ts`:
- Around line 40-54: The current baseQuery in get_documents_cursor.ts uses
args.folderId truthiness so undefined means both "root" and "all folders";
update the logic to explicitly separate root-only vs org-wide searches: either
treat a sentinel (e.g. null or '') as "root" and query the
'by_organizationId_and_folderId' index with folderId set to that sentinel, or
add a new boolean arg (e.g. orgWide or includeSubfolders) and branch: when
orgWide use the 'by_organizationId' index, when root use
'by_organizationId_and_folderId' with folderId set to the explicit root value.
Change the baseQuery construction around the symbols baseQuery, args.folderId,
and the indexes 'by_organizationId_and_folderId'/'by_organizationId' accordingly
so undefined no longer conflates root and org-wide listings.
In `@services/platform/convex/documents/get_documents.ts`:
- Around line 32-46: The fallback branch currently queries all documents for an
organization when args.folderId is falsy, which includes nested-folder files;
change the fallback to explicitly filter for top-level documents by adding a
folderId === undefined condition so the query only returns root-level items.
Update the baseQuery construction where ctx.db.query('documents') is called (the
branch using withIndex('by_organizationId')) to include an additional
.eq('folderId', undefined) (or equivalent check supported by the query API)
alongside the existing .eq('organizationId', args.organizationId) before
.order('desc'), ensuring the root view only returns documents whose folderId is
undefined.
In `@services/platform/convex/documents/update_document_internal.ts`:
- Line 20: The update flow currently treats undefined as both “no change” and
“clear folder” so clearing a document’s folder never happens because undefined
fields are stripped before ctx.db.patch(); change the folderId contract to
accept an explicit null (e.g., folderId: Id<'folders'> | null) and update the
patch logic in updateDocumentInternal so that null is preserved and written to
the DB (set folderId: null in the object passed to ctx.db.patch()), while still
omitting undefined to preserve “no change” semantics; ensure callers are updated
to pass null when they want to clear the folder.
- Around line 33-37: The folder validation in update_document_internal.ts
currently only checks organizationId and can allow reparenting a team-scoped
document into a folder owned by a different team; modify the check that runs
when updateData.folderId is set (the block that calls
ctx.db.get(updateData.folderId)) to compute the effective team as const
effectiveTeamId = updateData.teamId ?? document.teamId and ensure the fetched
folder exists, matches document.organizationId and also has folder.teamId ===
effectiveTeamId (or appropriate null/undefined handling if folders can be
org-wide). Throw the same error if the team check fails so the document cannot
be reassigned into a folder owned by another team.
In `@services/platform/convex/folders/__tests__/build_breadcrumb.test.ts`:
- Around line 105-137: The tests only check upper bounds; make them assert exact
breadcrumb contents and ordering to catch truncation/ordering regressions: in
the A→B→A test, assert that buildBreadcrumb(ctx,'f1') returns exactly the two
entries for f1 (name "A") then f2 (name "B") (no duplicates or extra entries),
and in the depth-cap test, assert the result has length 20 and that the sequence
of folder IDs equals the expected ordered slice (for start 'f25' it should
contain exactly f25 down to f6 in the correct order), using the buildBreadcrumb
function and result array to compare IDs/names exactly.
In `@services/platform/convex/folders/__tests__/get_or_create_path.test.ts`:
- Around line 114-125: The test expectations for ctx.db.insert in
get_or_create_path.test.ts are missing the new teamId field, so update each
expected insert payload used by the getOrCreateFolderPath tests to include
teamId: undefined (or the appropriate value when testing a non-default team) —
specifically update the assertions that call ctx.db.insert('folders', { ... })
(e.g., the ones asserting inserts for 'docs' and 'reports' and the later
assertions around those blocks) to include teamId in the object so the expected
payload matches the implementation.
In `@services/platform/convex/folders/get_or_create_path.ts`:
- Around line 18-57: getOrCreateFolderPath currently creates new folder segments
without enforcing the same nesting cap as createFolder (20), allowing
imports/backfills to exceed the UI-allowed depth; fix by adding a depth guard
before creating a new folder: compute the current ancestor depth for parentId
(or call the shared validation used by createFolder if available), compare
against the MAX_FOLDER_DEPTH (20), and abort/stop creating further segments when
the limit is reached so that the code path that does ctx.db.insert('folders', {
... }) refuses to create beyond the allowed depth; use validateFolderName and
createFolder/shared validator to centralize the check if possible.
In `@services/platform/convex/folders/mutations.ts`:
- Around line 80-110: When creating a folder, tighten team scoping: if
args.teamId is present, load the Team record from the DB (via
ctx.db.get(args.teamId) or equivalent), verify team.organizationId ===
args.organizationId and throw if not; then obtain userTeamIds
(getUserTeamIds(ctx, String(authUser._id))) and only check membership after
confirming org ownership (throw if user not a member). Additionally, if
parent.teamId is set, require args.teamId to equal parent.teamId (or set
args.teamId to parent.teamId) so a child cannot omit or change team under a
team-scoped parent, and keep existing parent access checks (hasTeamAccess) and
depth checks (MAX_FOLDER_DEPTH) intact.
In `@services/platform/convex/folders/queries.ts`:
- Around line 29-45: Before iterating children, load the parent folder and
enforce access: call the folders query to fetch the parent folder by
args.parentId (e.g. using ctx.db.get / query on 'folders' for _id ==
args.parentId), then run hasTeamAccess(parent, userTeamIds) and immediately
return [] (or throw) if it fails; only then run the existing children
enumeration (the q loop that uses withIndex('by_org_parent_name')) so you don't
leak child names/IDs when the caller lacks access to the parent. Ensure you
reference getUserTeamIds, hasTeamAccess, args.parentId and the existing
'folders' query.
In `@services/platform/convex/onedrive/import_files.ts`:
- Around line 194-205: The code skips folder moves when content hash is
unchanged because folder resolution (building folderId via
deps.getOrCreateFolderPath using item.relativePath) happens after the early
`continue` for unchanged content; move the folder resolution logic (the block
that computes `folderId` with deps.getOrCreateFolderPath and segments from
item.relativePath) to run before the unchanged-content short-circuit and then
compare the resolved target folderId against the current document folder; if the
hash is unchanged but folderId differs, treat the record as an update (do not
continue) and proceed to update the document's folder association accordingly
(refer to the existing `folderId`, `deps.getOrCreateFolderPath`, and the
unchanged-content check logic to implement this).
In `@services/platform/public/openapi.json`:
- Around line 11994-11995: The public OpenAPI contract now requires an opaque
folderId (property "folderId") which prevents third-party clients from
discovering valid folders; revert this change by restoring "folderPath" as a
supported (or deprecated) filter alongside "folderId" in the schema so public
clients can construct requests, or alternatively add the missing folder
discovery endpoints and typed folder/document response fields to the public spec
so callers can obtain folderId values; update the schema to include both
"folderPath" (marked deprecated if desired) and "folderId" or add explicit
folder GET/list responses and their models to the OpenAPI file so consumers can
discover IDs.
---
Outside diff comments:
In `@services/platform/convex/documents/list_documents_paginated.ts`:
- Around line 78-99: The bug is that root requests (explicit "root only") are
indistinguishable from omitted folder filters because filterArgs.folderId is
coerced to undefined; change the folderId handling to preserve an explicit root
marker: set filterArgs.folderId = null when args.folderId === null (keep
String(args.folderId) for non-null values and leave undefined when omitted),
then let buildBaseQuery/primary selection and subsequent FILTER_INDEXES
filtering treat null as a valid folderId value so the query filters for
q.eq(q.field('folderId'), null) for root-only requests rather than returning
documents across all folders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6a4383e-dbc9-4f3c-aa04-e2e45d7d7244
⛔ Files ignored due to path filters (2)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**services/platform/convex/migrations/backfill_folders.tsis excluded by!**/migrations/**
📒 Files selected for processing (42)
services/platform/app/features/documents/components/breadcrumb-navigation.tsxservices/platform/app/features/documents/components/create-folder-dialog.tsxservices/platform/app/features/documents/components/document-delete-folder-dialog.tsxservices/platform/app/features/documents/components/document-preview-dialog.tsxservices/platform/app/features/documents/components/document-row-actions.tsxservices/platform/app/features/documents/components/document-upload-dialog.tsxservices/platform/app/features/documents/components/documents-action-menu.tsxservices/platform/app/features/documents/components/documents-table.tsxservices/platform/app/features/documents/hooks/mutations.tsservices/platform/app/features/documents/hooks/queries.tsservices/platform/app/features/documents/hooks/use-documents-table-config.tsxservices/platform/app/routes/dashboard/$id/_knowledge/documents.tsxservices/platform/convex/documents/__tests__/get_document_by_path.test.tsservices/platform/convex/documents/create_document.tsservices/platform/convex/documents/get_document_by_path.tsservices/platform/convex/documents/get_documents.tsservices/platform/convex/documents/get_documents_cursor.tsservices/platform/convex/documents/internal_mutations.tsservices/platform/convex/documents/list_documents_paginated.test.tsservices/platform/convex/documents/list_documents_paginated.tsservices/platform/convex/documents/mutations.tsservices/platform/convex/documents/queries.tsservices/platform/convex/documents/schema.tsservices/platform/convex/documents/transform_to_document_item.tsservices/platform/convex/documents/types.tsservices/platform/convex/documents/update_document_internal.tsservices/platform/convex/documents/validators.tsservices/platform/convex/folders/__tests__/build_breadcrumb.test.tsservices/platform/convex/folders/__tests__/get_or_create_path.test.tsservices/platform/convex/folders/__tests__/mutations.test.tsservices/platform/convex/folders/get_or_create_path.tsservices/platform/convex/folders/internal_mutations.tsservices/platform/convex/folders/mutations.tsservices/platform/convex/folders/queries.tsservices/platform/convex/folders/schema.tsservices/platform/convex/lib/rate_limiter/index.tsservices/platform/convex/onedrive/import_files.tsservices/platform/convex/onedrive/import_files_deps.tsservices/platform/convex/schema.tsservices/platform/messages/en.jsonservices/platform/public/openapi.jsonservices/platform/types/documents.ts
Comment on lines
+47
to
+53
| <button | ||
| onClick={() => onNavigate(undefined)} | ||
| className="text-muted-foreground hover:text-foreground/90 focus-visible:ring-ring size-4 shrink-0 cursor-pointer rounded-sm transition-colors focus-visible:ring-2 focus-visible:outline-none" | ||
| aria-label={tCommon('aria.backTo', { | ||
| page: t('breadcrumb.documents'), | ||
| })} | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/platform/app/features/documents/components/breadcrumb-navigation.tsxRepository: tale-project/tale
Length of output: 4232
🏁 Script executed:
fd -t f 'biome\.json' -o 'biome\.jsonc' | head -20Repository: tale-project/tale
Length of output: 204
🏁 Script executed:
find . -name "biome.json*" -o -name ".biome*" | head -20Repository: tale-project/tale
Length of output: 73
🏁 Script executed:
cat -n biome.jsonRepository: tale-project/tale
Length of output: 2354
Add type="button" to the breadcrumb buttons.
Biome's enabled accessibility rules (a11y/useButtonType) require explicit type attributes on all button elements. These three buttons are missing the type attribute.
Proposed fix
<button
+ type="button"
onClick={() => onNavigate(undefined)}
className="text-muted-foreground hover:text-foreground/90 focus-visible:ring-ring size-4 shrink-0 cursor-pointer rounded-sm transition-colors focus-visible:ring-2 focus-visible:outline-none"
aria-label={tCommon('aria.backTo', {
page: t('breadcrumb.documents'),
})}
>
@@
<button
+ type="button"
onClick={() => onNavigate(undefined)}
className="text-muted-foreground hover:text-foreground/90 focus-visible:ring-ring cursor-pointer rounded-sm text-xs font-medium whitespace-nowrap transition-colors focus-visible:ring-2 focus-visible:outline-none"
>
@@
<button
+ type="button"
onClick={() => onNavigate(folder._id)}
className="text-muted-foreground hover:text-foreground/90 focus-visible:ring-ring cursor-pointer rounded-sm text-xs font-medium whitespace-nowrap transition-colors focus-visible:ring-2 focus-visible:outline-none"
aria-label={t('aria.navigateToFolder', {
name: folder.name,
})}
>Also applies to: lines 57 and 85.
🧰 Tools
🪛 Biome (2.4.4)
[error] 47-53: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/documents/components/breadcrumb-navigation.tsx`
around lines 47 - 53, The three button elements in the BreadcrumbNavigation
component (the back button with onClick={() => onNavigate(undefined)}, the other
breadcrumb navigation buttons around that same JSX and the one further down used
for navigation) are missing explicit type attributes; add type="button" to each
<button> to satisfy the a11y/useButtonType rule and ensure they don't default to
type="submit". Update the button elements referenced by the onClick handlers
(e.g., the button calling onNavigate) to include type="button".
Comment on lines
+45
to
+53
| <ul className="text-muted-foreground ml-4 list-disc space-y-1 text-sm"> | ||
| <li> | ||
| {tDocuments('deleteSyncFolder.willDelete.filesAndSubfolders')} | ||
| </li> | ||
| <li> | ||
| {tDocuments('deleteSyncFolder.willDelete.autoSyncConfig')} | ||
| </li> | ||
| <li>{tDocuments('deleteSyncFolder.willDelete.syncHistory')}</li> | ||
| </ul> |
Contributor
There was a problem hiding this comment.
Add explicit list semantics to the delete-impact list.
Without role="list", screen readers can stop announcing this <ul> as a list in this codebase, which makes the destructive consequences harder to parse.
♿ Add the missing list role
- <ul className="text-muted-foreground ml-4 list-disc space-y-1 text-sm">
+ <ul
+ role="list"
+ className="text-muted-foreground ml-4 list-disc space-y-1 text-sm"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ul className="text-muted-foreground ml-4 list-disc space-y-1 text-sm"> | |
| <li> | |
| {tDocuments('deleteSyncFolder.willDelete.filesAndSubfolders')} | |
| </li> | |
| <li> | |
| {tDocuments('deleteSyncFolder.willDelete.autoSyncConfig')} | |
| </li> | |
| <li>{tDocuments('deleteSyncFolder.willDelete.syncHistory')}</li> | |
| </ul> | |
| <ul | |
| role="list" | |
| className="text-muted-foreground ml-4 list-disc space-y-1 text-sm" | |
| > | |
| <li> | |
| {tDocuments('deleteSyncFolder.willDelete.filesAndSubfolders')} | |
| </li> | |
| <li> | |
| {tDocuments('deleteSyncFolder.willDelete.autoSyncConfig')} | |
| </li> | |
| <li>{tDocuments('deleteSyncFolder.willDelete.syncHistory')}</li> | |
| </ul> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/documents/components/document-delete-folder-dialog.tsx`
around lines 45 - 53, The unordered list in the DocumentDeleteFolderDialog
component is missing explicit list semantics; update the <ul> that renders the
delete-impact items (the element using className "text-muted-foreground ml-4
list-disc space-y-1 text-sm") to include role="list" so screen readers (e.g.,
Safari/VoiceOver) correctly announce it as a list while preserving existing
className and children.
Comment on lines
72
to
+90
| const handleDeleteFolderConfirm = useCallback(() => { | ||
| deleteDocument( | ||
| { documentId: toId<'documents'>(documentId) }, | ||
| deleteFolder( | ||
| { folderId: toId<'folders'>(documentId) }, | ||
| { | ||
| onSuccess: () => dialogs.setOpen.deleteFolder(false), | ||
| onSuccess: () => { | ||
| dialogs.setOpen.deleteFolder(false); | ||
| onFolderDeleted?.(); | ||
| }, | ||
| onError: (error) => { | ||
| console.error('Failed to delete folder:', error); | ||
| toast({ | ||
| title: tDocuments('actions.deleteFolderFailed'), | ||
| description: error instanceof Error ? error.message : undefined, | ||
| variant: 'destructive', | ||
| }); | ||
| }, | ||
| }, | ||
| ); | ||
| }, [deleteDocument, documentId, dialogs.setOpen, tDocuments]); | ||
| }, [deleteFolder, documentId, dialogs.setOpen, tDocuments, onFolderDeleted]); |
Contributor
There was a problem hiding this comment.
Sync-folder delete is still wired to the empty-folder mutation.
useDeleteFolder() points at the generic folders.deleteFolder mutation, and that backend only deletes empty folders after checking for child folders/documents. It never removes the sync config/history that the new sync-folder label and dialog promise to delete, so synced folders will either hard-fail on delete or leave orphaned sync state. Route syncConfigId folders to the dedicated sync-delete path, or keep the normal empty-folder flow until that backend exists.
Also applies to: 147-149, 188-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/components/document-row-actions.tsx`
around lines 72 - 90, The delete flow in handleDeleteFolderConfirm (which uses
useDeleteFolder -> deleteFolder) incorrectly calls the generic
folders.deleteFolder mutation for synced folders; update the handler to check
the folder payload for a syncConfigId (using documentId/context) and, when
present, call the dedicated sync-folder delete path instead of deleteFolder,
ensuring dialogs.setOpen.deleteFolder and onFolderDeleted are still invoked on
success and the same onError toast behavior is preserved; if the dedicated
sync-delete backend is not available yet, short-circuit to the existing
empty-folder flow (current deleteFolder) to avoid orphaned sync state or hard
failures.
Comment on lines
+73
to
+77
| items.push({ | ||
| label: tDocuments('folder.newFolder'), | ||
| icon: FolderPlus, | ||
| onClick: handleCreateFolder, | ||
| }); |
Contributor
There was a problem hiding this comment.
The "New folder" action disappears when Microsoft isn't connected.
menuItems now always includes the folder action, but the hasMicrosoftAccount === false branch bypasses the menu and opens device upload directly. That makes CreateFolderDialog unreachable for non-Microsoft workspaces.
📁 Render the action menu in both cases
- {hasMicrosoftAccount ? (
- <DataTableActionMenu
- label={tDocuments('upload.importDocuments')}
- icon={Plus}
- menuItems={menuItems}
- />
- ) : (
- <DataTableActionMenu
- label={tDocuments('upload.importDocuments')}
- icon={Plus}
- onClick={handleDeviceUpload}
- />
- )}
+ <DataTableActionMenu
+ label={tDocuments('upload.importDocuments')}
+ icon={Plus}
+ menuItems={menuItems}
+ />Also applies to: 90-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/documents/components/documents-action-menu.tsx`
around lines 73 - 77, The folder creation action is always pushed into items but
the branch for hasMicrosoftAccount === false bypasses the menu and directly
triggers device upload, making CreateFolderDialog unreachable; update the
conditional logic so the FolderPlus item (and its onClick handler
handleCreateFolder) is rendered in the menu for both Microsoft-connected and
non-Microsoft workspaces, and limit the hasMicrosoftAccount check to only choose
upload behavior (e.g., showing OneDrive vs device upload) — inspect the items
array construction and the code paths around handleCreateFolder and the upload
logic (the branch currently around hasMicrosoftAccount and the block referenced
also at lines 90-102) and refactor so the folder action remains in the menu
while only upload/menu options change based on hasMicrosoftAccount.
Comment on lines
61
to
+67
| const paginatedResult = useListDocumentsPaginated({ | ||
| organizationId, | ||
| folderId: currentFolderId, | ||
| initialNumItems: 20, | ||
| }); | ||
|
|
||
| const { data: folders } = useFolders(organizationId, currentFolderId); |
Contributor
There was a problem hiding this comment.
Keep the table count aligned with the folder-scoped dataset.
The list query is now scoped by currentFolderId, but useListPage still receives the organization-wide useApproxDocumentCount(organizationId) at Line 214. Once the user drills into a folder, the paginator/virtualizer will overestimate how many rows exist, and prepending folderRows here makes the mismatch worse even at the root. Please switch to a folder-aware count or stop passing approxRowCount until the count API matches the rows being rendered.
Also applies to: 69-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/components/documents-table.tsx`
around lines 61 - 67, The paginator is being fed an organization-wide
approximate count (useApproxDocumentCount(organizationId)) while rows are scoped
by currentFolderId via useListDocumentsPaginated and paginatedResult, causing an
overestimate; update the call to useListPage so it receives a folder-aware count
(e.g., useApproxDocumentCount(organizationId, currentFolderId) or a new
folder-scoped count hook) or remove the approxRowCount/approxRowCount fallback
(and the prepended folderRows adjustment) until the count API matches the
folder-scoped results; look for useListPage, approxRowCount, folderRows,
useListDocumentsPaginated, useFolders and currentFolderId in the file and make
the change there.
Comment on lines
+18
to
+57
| let parentId: Id<'folders'> | undefined; | ||
|
|
||
| for (const segment of segments) { | ||
| let validName: string; | ||
| try { | ||
| validName = validateFolderName(segment); | ||
| } catch { | ||
| // Intentionally stop on invalid segments rather than throwing. | ||
| // Callers (migration backfill, OneDrive import) rely on partial | ||
| // path creation with their own error handling wrappers. | ||
| console.warn( | ||
| `[getOrCreateFolderPath] Stopped at invalid segment "${segment}" in path [${pathSegments.join('/')}]`, | ||
| ); | ||
| break; | ||
| } | ||
|
|
||
| const existing = await ctx.db | ||
| .query('folders') | ||
| .withIndex('by_org_parent_name', (q) => | ||
| q | ||
| .eq('organizationId', organizationId) | ||
| .eq('parentId', parentId) | ||
| .eq('name', validName), | ||
| ) | ||
| .first(); | ||
|
|
||
| if (existing) { | ||
| parentId = existing._id; | ||
| } else { | ||
| parentId = await ctx.db.insert('folders', { | ||
| organizationId, | ||
| name: validName, | ||
| parentId, | ||
| createdBy, | ||
| teamId, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return parentId; |
Contributor
There was a problem hiding this comment.
Don’t bypass the folder depth limit in the path helper.
createFolder caps nesting at 20, but this helper never counts ancestors while creating new segments. That means a backfill/import can build a path the public folder APIs would later reject, leaving branches the UI can’t manage consistently. Please apply the same depth guard here or extract the depth check into shared folder-creation validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/folders/get_or_create_path.ts` around lines 18 - 57,
getOrCreateFolderPath currently creates new folder segments without enforcing
the same nesting cap as createFolder (20), allowing imports/backfills to exceed
the UI-allowed depth; fix by adding a depth guard before creating a new folder:
compute the current ancestor depth for parentId (or call the shared validation
used by createFolder if available), compare against the MAX_FOLDER_DEPTH (20),
and abort/stop creating further segments when the limit is reached so that the
code path that does ctx.db.insert('folders', { ... }) refuses to create beyond
the allowed depth; use validateFolderName and createFolder/shared validator to
centralize the check if possible.
Comment on lines
+80
to
+110
| if (args.parentId) { | ||
| const parent = await ctx.db.get(args.parentId); | ||
| if (!parent || parent.organizationId !== args.organizationId) { | ||
| throw new Error('Parent folder not found'); | ||
| } | ||
| if (parent.teamId) { | ||
| const userTeamIds = await getUserTeamIds(ctx, String(authUser._id)); | ||
| if (!hasTeamAccess(parent, userTeamIds)) { | ||
| throw new Error('Parent folder not accessible'); | ||
| } | ||
| } | ||
|
|
||
| let depth = 1; | ||
| let ancestorId = parent.parentId; | ||
| while (ancestorId && depth < MAX_FOLDER_DEPTH) { | ||
| const ancestor = await ctx.db.get(ancestorId); | ||
| if (!ancestor) break; | ||
| depth++; | ||
| ancestorId = ancestor.parentId; | ||
| } | ||
| if (depth >= MAX_FOLDER_DEPTH) { | ||
| throw new Error('Maximum folder nesting depth exceeded'); | ||
| } | ||
| } | ||
|
|
||
| if (args.teamId) { | ||
| const userTeamIds = await getUserTeamIds(ctx, String(authUser._id)); | ||
| if (!userTeamIds.includes(args.teamId)) { | ||
| throw new Error('Cannot create folder in a team you do not belong to'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten team scoping before inserting the folder.
args.teamId is only checked against the caller’s memberships. That still lets someone reuse a team ID from another organization, and it also allows an org-wide/different-team child under a team-scoped parent by omitting or changing args.teamId. Both cases break folder hierarchy and access invariants. Load the requested team, verify it belongs to args.organizationId, and when parent.teamId is set require the child to inherit that same team.
Based on learnings: addMember validates the target team belongs to the provided organization before checking/creating membership; keep this org-ownership validation for team-scoped mutations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/folders/mutations.ts` around lines 80 - 110, When
creating a folder, tighten team scoping: if args.teamId is present, load the
Team record from the DB (via ctx.db.get(args.teamId) or equivalent), verify
team.organizationId === args.organizationId and throw if not; then obtain
userTeamIds (getUserTeamIds(ctx, String(authUser._id))) and only check
membership after confirming org ownership (throw if user not a member).
Additionally, if parent.teamId is set, require args.teamId to equal
parent.teamId (or set args.teamId to parent.teamId) so a child cannot omit or
change team under a team-scoped parent, and keep existing parent access checks
(hasTeamAccess) and depth checks (MAX_FOLDER_DEPTH) intact.
Comment on lines
+29
to
+45
| const userTeamIds = await getUserTeamIds(ctx, String(authUser._id)); | ||
| const folders: Doc<'folders'>[] = []; | ||
|
|
||
| const q = ctx.db | ||
| .query('folders') | ||
| .withIndex('by_org_parent_name', (qb) => | ||
| qb | ||
| .eq('organizationId', args.organizationId) | ||
| .eq('parentId', args.parentId), | ||
| ); | ||
|
|
||
| for await (const folder of q) { | ||
| if (!hasTeamAccess(folder, userTeamIds)) continue; | ||
| folders.push(folder); | ||
| } | ||
|
|
||
| return folders; |
Contributor
There was a problem hiding this comment.
Validate parent access before enumerating its children.
This query checks organization membership, but if parentId points at a team-restricted folder it never verifies the caller can see that parent. A leaked parent ID can therefore expose child folder names/IDs whenever the child itself is org-wide or otherwise accessible. Load the parent first and short-circuit if hasTeamAccess(parent, userTeamIds) fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/folders/queries.ts` around lines 29 - 45, Before
iterating children, load the parent folder and enforce access: call the folders
query to fetch the parent folder by args.parentId (e.g. using ctx.db.get / query
on 'folders' for _id == args.parentId), then run hasTeamAccess(parent,
userTeamIds) and immediately return [] (or throw) if it fails; only then run the
existing children enumeration (the q loop that uses
withIndex('by_org_parent_name')) so you don't leak child names/IDs when the
caller lacks access to the parent. Ensure you reference getUserTeamIds,
hasTeamAccess, args.parentId and the existing 'folders' query.
Comment on lines
+194
to
+205
| let folderId: Id<'folders'> | undefined; | ||
| if (deps.getOrCreateFolderPath && item.relativePath) { | ||
| const segments = item.relativePath.split('/').slice(0, -1); | ||
| if (segments.length > 0) { | ||
| folderId = await deps.getOrCreateFolderPath( | ||
| args.organizationId, | ||
| segments, | ||
| args.userId, | ||
| args.teamId, | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't skip folder moves when the content hash is unchanged.
This new folderId path only runs after the unchanged-content continue above. If a OneDrive/SharePoint file moves to a different folder without changing bytes, sync will skip it and leave the document attached to the old folder indefinitely. Resolve the target folder before the hash-equality short-circuit and treat folder changes as updates.
Also applies to: 209-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/onedrive/import_files.ts` around lines 194 - 205,
The code skips folder moves when content hash is unchanged because folder
resolution (building folderId via deps.getOrCreateFolderPath using
item.relativePath) happens after the early `continue` for unchanged content;
move the folder resolution logic (the block that computes `folderId` with
deps.getOrCreateFolderPath and segments from item.relativePath) to run before
the unchanged-content short-circuit and then compare the resolved target
folderId against the current document folder; if the hash is unchanged but
folderId differs, treat the record as an update (do not continue) and proceed to
update the document's folder association accordingly (refer to the existing
`folderId`, `deps.getOrCreateFolderPath`, and the unchanged-content check logic
to implement this).
Comment on lines
+11994
to
11995
| "folderId": { | ||
| "type": "string" |
Contributor
There was a problem hiding this comment.
Public clients can no longer discover a valid folderId.
Switching this filter from folderPath to folderId makes the public contract depend on an opaque identifier, but this OpenAPI file still exposes no folder endpoints or typed document responses that would let API consumers obtain those IDs. As documented here, third-party clients cannot construct this request anymore.
Consider either keeping folderPath as a deprecated compatibility field until folder discovery endpoints are published, or exposing the folder APIs / response fields in this public spec at the same time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/public/openapi.json` around lines 11994 - 11995, The public
OpenAPI contract now requires an opaque folderId (property "folderId") which
prevents third-party clients from discovering valid folders; revert this change
by restoring "folderPath" as a supported (or deprecated) filter alongside
"folderId" in the schema so public clients can construct requests, or
alternatively add the missing folder discovery endpoints and typed
folder/document response fields to the public spec so callers can obtain
folderId values; update the schema to include both "folderPath" (marked
deprecated if desired) and "folderId" or add explicit folder GET/list responses
and their models to the OpenAPI file so consumers can discover IDs.
This was referenced Mar 7, 2026
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
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
folderstable with proper schema, queries, mutations, rate limiting, depth validation, and team access checks — replacing the string-basedfolderPathon documentsfolderIdforeign key referencesfolderPathvalues into folder recordsTest plan
convex/folders/__tests__/mutations.test.ts,build_breadcrumb.test.ts,get_or_create_path.test.tsconvex/documents/__tests__/get_document_by_path.test.tsandlist_documents_paginated.test.ts🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes