feat: bucket folders #62
Conversation
WalkthroughAdds a new design document (021-bucket-folders.md) specifying folder support for storage buckets. Defines lightweight, non-nested folders as Folder documents alongside file documents (discriminator), with a folderId field on files to denote root vs specific folder. Describes folder CRUD APIs, file endpoint changes to accept/filter by folderId, required indexes, a migration plan to annotate existing files, implementation steps (create/list/delete with optional force), permission inheritance, testing/benchmarking notes, UI/UX and docs requirements, and future roadmap items (nested folders, advanced permissions, optimizations). Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
021-bucket-folders.md (1)
37-51: Add fence languages for API examplesMarkdown linters (MD040) are flagging the unlabeled code fences in this section. Tag them with an appropriate language (e.g.,
http,json, ortext) so automated checks pass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
021-bucket-folders.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
021-bucket-folders.md
10-10: Link and image reference definitions should be needed
Unused link or image reference definition: "summary"
(MD053, link-image-reference-definitions)
16-16: Link fragments should be valid
(MD051, link-fragments)
16-16: Link and image reference definitions should be needed
Unused link or image reference definition: "problem-statement"
(MD053, link-image-reference-definitions)
24-24: Link fragments should be valid
(MD051, link-fragments)
24-24: Link and image reference definitions should be needed
Unused link or image reference definition: "design-proposal"
(MD053, link-image-reference-definitions)
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Link and image reference definitions should be needed
Unused link or image reference definition: "prior-art"
(MD053, link-image-reference-definitions)
196-196: Link and image reference definitions should be needed
Unused link or image reference definition: "unresolved-questions"
(MD053, link-image-reference-definitions)
205-205: Link and image reference definitions should be needed
Unused link or image reference definition: "future-possibilities"
(MD053, link-image-reference-definitions)
| - `(bucketInternalId, type, name)` - unique constraint for folder names | ||
| - `(bucketInternalId, folderId)` - list files in folder | ||
| - `(bucketInternalId, type)` - list all folders |
There was a problem hiding this comment.
Unique index will break existing file uploads
The proposed unique index (bucketInternalId, type, name) applies to all documents in the bucket collection. Once the migration sets type = 'file' on every file, this index will forbid two files in the same bucket sharing the same name—something Appwrite currently allows. That’s a backward-incompatible change that will cause uploads (and even the migration itself) to fail in any bucket that already contains duplicate filenames. Please limit the uniqueness to folders only (e.g., partial index type = 'folder') or pick another discriminator so files remain unaffected.
🤖 Prompt for AI Agents
In 021-bucket-folders.md around lines 93-95, the suggested unique index
`(bucketInternalId, type, name)` would make filenames unique across all
documents once files get `type = 'file'`, breaking existing uploads; change the
index to only enforce uniqueness for folders by creating a partial/filtered
unique index that applies when `type = 'folder'` (or use a folder-specific
discriminator field), and update the migration steps to set `type` only for
folder documents (or populate the discriminator safely) so files are not
affected; ensure the migration creates the partial unique index after ensuring
all folder documents meet the constraint and include a fallback plan to detect
and resolve existing duplicate folder names before index creation.
| Delete folder. Fails if folder contains files unless `force=true`. | ||
|
|
||
| **Modified: POST /v1/storage/buckets/:bucketId/files** | ||
| Add `folderId` parameter (optional, defaults to null = root level). |
There was a problem hiding this comment.
should this really be folder ID? maybe path for better DX? we can generate the hierarchy on creation or simpler throw 404 if path doesn't exists and send to create a folder..
| - `(bucketInternalId, type)` - list all folders | ||
|
|
||
| **Migration:** | ||
| 1. Add `folderId` field to all existing file documents (default null) |
There was a problem hiding this comment.
We should get DB team (specifically Shmuel to help with this), we have a few migrations upcoming, could save time.
| - Folder-level permissions (override bucket) | ||
| - Bulk operations (move all files in folder) | ||
| - Folder templates (create with predefined structure) | ||
| - Shared folders (special permission model) |
There was a problem hiding this comment.
In theory this would just be a feature of the permission model, no? just adding a team id or a role in permissions
| - Bulk operations (move all files in folder) | ||
| - Folder templates (create with predefined structure) | ||
| - Shared folders (special permission model) | ||
| - Folder hooks/events for automation |
There was a problem hiding this comment.
Would this be similar to our current events? Can't we already expose them in phase 1?
| **Phase 4 - Performance:** | ||
| - Materialized paths for fast hierarchy queries | ||
| - Denormalized file counts (already included) | ||
| - Folder statistics (total size, last modified) |
There was a problem hiding this comment.
This would actually be nice in step 1, should be simple to implement and will save data migration later.
|
A few more questions:
|
loks0n
force-pushed
the
feat-bucket-folders
branch
from
371b619 to
1b40ab0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
021-bucket-folders.md (3)
37-37: Add language identifier to code blocks for consistency.Lines 37 and 46 have unmarked code blocks. Per markdown best practices, specify the language for syntax highlighting.
Suggested fixes:
- Line 37: Change
```to```json- Line 46: Change
```to```json(Also applies to line 72–83 which uses
```phpcorrectly.)Apply these diffs:
**POST /v1/storage/buckets/:bucketId/folders** Create a folder. - ``` + ```json bucketId: string (required)**GET /v1/storage/buckets/:bucketId/folders** List folders in a bucket. - ``` + ```json bucketId: string (required)Also applies to: 46-46
10-10: Remove unused markdown reference definitions.The document defines anchor references (lines 10, 16, 24, 184, 196, 205) but they are not actively used in the markdown structure or as links. Remove these definitions to clean up the file:
- Line 10:
[summary]: #summary- Line 16:
[problem-statement]: #problem-statement- Line 24:
[design-proposal]: #design-proposal- Line 184:
[prior-art]: #prior-art- Line 196:
[unresolved-questions]: #unresolved-questions- Line 205:
[future-possibilities]: #future-possibilitiesThese can be automatically generated by markdown renderers if needed.
Apply this diff to remove all unused definitions:
## Summary - [summary]: #summary - Add folder support to Appwrite Storage for organizing files. Folders are lightweight organizational containers - they don't have independent permissions, don't support nesting initially, and exist purely to help users sort and filter files within buckets. ## Problem Statement (Step 1) - [problem-statement]: #problem-statement - **What problem are you trying to solve?** ... ## Design proposal (Step 2) - [design-proposal]: #design-proposal - ### Design decisions(Continue removing at lines 184, 196, 205 similarly.)
Also applies to: 16-16, 24-24, 184-184, 196-196, 205-205
106-111: Creating a folder: missing error handling and edge cases.The creation steps on lines 106–111 are high-level but lack detail on:
- What happens if
bucket exists and user has CREATE permissionbut the bucket itself is deleted concurrently?- Is the duplicate folder name check (line 107) performed before or after ID generation (line 108)? If concurrent creates occur, can a race condition cause duplicate names?
- Line 109 says "Inherit bucket permissions if none specified"—does this mean a deep copy, a reference, or auto-inheritance at runtime?
Recommendation: Add a note on the concurrency model (e.g., unique index prevents duplicates) and clarify permission inheritance strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
021-bucket-folders.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
021-bucket-folders.md
10-10: Link and image reference definitions should be needed
Unused link or image reference definition: "summary"
(MD053, link-image-reference-definitions)
16-16: Link fragments should be valid
(MD051, link-fragments)
16-16: Link and image reference definitions should be needed
Unused link or image reference definition: "problem-statement"
(MD053, link-image-reference-definitions)
24-24: Link fragments should be valid
(MD051, link-fragments)
24-24: Link and image reference definitions should be needed
Unused link or image reference definition: "design-proposal"
(MD053, link-image-reference-definitions)
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Link and image reference definitions should be needed
Unused link or image reference definition: "prior-art"
(MD053, link-image-reference-definitions)
196-196: Link and image reference definitions should be needed
Unused link or image reference definition: "unresolved-questions"
(MD053, link-image-reference-definitions)
205-205: Link and image reference definitions should be needed
Unused link or image reference definition: "future-possibilities"
(MD053, link-image-reference-definitions)
🔇 Additional comments (3)
021-bucket-folders.md (3)
62-62: Document thefolderId='root'convention in endpoint specifications or refactor for clarity.The documentation gap is confirmed. Line 62 states that
folderId"defaults to null = root level," but it doesn't document the special case at line 115 where passing the string literal'root'converts toNULL. This implementation detail is described only in the Implementation Details section, not in the API endpoint specifications themselves (lines 35-65), creating potential user confusion.However, I cannot verify the referenced prior comment from eldadfux, as the file contains only the current design proposal without embedded review history.
Resolution options:
- Add to line 65 (GET endpoint spec):
folderIdquery parameter: "Pass'root'to list files at root level (converts to NULL internally)"- Simplify implementation to use
null/absent parameter only, removing the'root'string special case
169-174: The review comment is based on incorrect premises and should be disregarded.The referenced "eldadfux feedback on line 20" does not exist in the document—line 20 is part of the generic problem statement, not a prior review comment. Additionally, breadcrumb navigation does not inherently conflict with the "flat structure initially" design (line 29). Breadcrumbs in a single-level folder UI simply show: root → current folder. They are a navigation aid, not a nesting implementation.
The design explicitly defers nested folders to Phase 2, as stated in the Unresolved Questions section. The Phase 1 UI requirements (lines 169–174) are consistent with this constraint. While "Search across folders" could be more explicit about scope, it does not contradict the flat structure—it reasonably means searching all files regardless of folder assignment.
Likely an incorrect or invalid review comment.
12-12: Review comment cites non-existent prior feedback; cannot be verified against codebase.The design document accurately reflects the deferral of nested folders to Phase 2 (line 207) and explicitly lists "Nested folders: Defer or include v1?" as an unresolved design question (line 199). However, the review comment references "eldadfux's past comment (line 12 of prior reviews)" and "eldadfux's comment on search (line 20 of prior reviews)" along with specific quotes that do not appear anywhere in the repository. These prior reviews are not accessible in the codebase—no prior review files, PR comments, or documentation containing the cited feedback was found despite comprehensive searches. The design decision to defer nesting is properly documented; the concern should be validated against the actual source documents that contain eldadfux's feedback.
Likely an incorrect or invalid review comment.
| 'name' => 'Invoices', | ||
| '$permissions' => [...], // same as bucket unless overridden | ||
| 'search' => 'folder_id Invoices', | ||
| 'filesCount' => 0, // cached count |
There was a problem hiding this comment.
Cached filesCount field lacks consistency guarantees for force delete.
Line 82 introduces a denormalized filesCount field on folder documents. However, the delete logic on lines 120–125 does not mention updating filesCount when files are moved to root during a force delete.
Additionally, there is no strategy described for keeping filesCount in sync:
- When files are uploaded to a folder, who increments the count?
- When files are deleted, who decrements it?
- What happens if a count becomes stale due to errors?
Recommendation: Clarify in the Implementation Details section how filesCount is maintained during all file operations (create, delete, move between folders). Consider whether denormalization is worth the complexity, or rely on a count query instead for Phase 1.
Also applies to: 120-125
🤖 Prompt for AI Agents
In 021-bucket-folders.md around lines 82 and 120-125, the denormalized
'filesCount' field is introduced but no update/consistency strategy is specified
and force-delete file moves do not adjust the count; update the Implementation
Details to (1) state which operations and services update filesCount (uploads
increment, deletes decrement, moves decrement source/increment dest,
force-delete moving files to root must decrement original folder and increment
root if root tracks count), (2) require updates be done in the same transaction
where possible or via an atomic two-step (update document + compensating retry)
and describe a retry/backfill job to repair stale counts, (3) list failure modes
and mitigation (idempotent updates, optimistic locking or DB increment ops), and
(4) include a justification to drop denormalization for Phase 1 in favor of
realtime count queries if the added complexity is undesirable.
| - `(bucketInternalId, type, name)` - unique constraint for folder names | ||
| - `(bucketInternalId, folderId)` - list files in folder | ||
| - `(bucketInternalId, type)` - list all folders | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
The proposed unique index (bucketInternalId, type, name) applies to all documents in the bucket collection. Once the migration sets type = 'file' on existing files (line 99), this index will forbid two files with the same name—something Appwrite currently allows. This will cause the migration itself to fail in any bucket with duplicate filenames, and future uploads will be blocked.
Recommendation: Create a partial/filtered unique index that applies only when type = 'folder', or separate the folder name constraint from the file namespace. Update line 99's migration to set type only for new folder documents, not existing files.
This was flagged in previous review comments but remains unaddressed. Please confirm the approach before proceeding.
I'll examine the RFC file and verify the critical index uniqueness concern.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate the RFC file
fd "021-bucket-folders.md" --type f
Length of output: 21
🏁 Script executed:
#!/bin/bash
# Extract and examine the index definitions and migration strategy
fd "021-bucket-folders.md" --type f --exec cat {} \; | head -120 | tail -40Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Get the full data structure and implementation sections
fd "021-bucket-folders.md" --type f --exec cat {} \; | grep -n -A 20 "Data Structure\|Implementation\|migration"Length of output: 2247
🏁 Script executed:
#!/bin/bash
# Look for any index, constraint, or database-related documentation
fd "021-bucket-folders.md" --type f --exec cat {} \;Length of output: 7168
Critical index design will break migrations on buckets with duplicate filenames.
The unique index (bucketInternalId, type, name) on lines 93-95 combined with the migration logic on lines 97-101 creates an unsolvable constraint conflict:
- Existing files receive
type = 'file'during migration - Unique constraint on
(bucketInternalId, type, name)forbids two files with identical names in the same bucket - Appwrite currently permits duplicate file names (by design)
- Result: Migration fails on any production bucket with duplicate filenames; future uploads are blocked
Fix: Implement a partial/filtered unique index applying only to folders (type = 'folder'), or separate folder names into a distinct namespace. Update migration to exclude setting type on existing files—apply it only to new folder documents.
This concern was flagged previously and remains unaddressed.
🤖 Prompt for AI Agents
In 021-bucket-folders.md around lines 93-96, the proposed unique index
`(bucketInternalId, type, name)` will break migrations because existing files
get `type='file'` and Appwrite allows duplicate filenames; adjust the schema and
migration so folder name uniqueness applies only to folders: create a
partial/filtered unique index that includes `WHERE type = 'folder'` (or maintain
a separate namespace/column for folder names) and change the migration to avoid
assigning `type` to existing file documents (only set `type='folder'` for newly
created folder records), ensuring existing duplicate file names are not
constrained during migration or afterwards.
| **Migration:** | ||
| 1. Add `folderId` field to all existing file documents (default null) | ||
| 2. Add `type` field to all existing file documents (default 'file') | ||
| 3. Create indexes | ||
| 4. Deploy |
There was a problem hiding this comment.
Migration strategy lacks validation and rollback plan.
The migration on lines 97–101 is too brief:
- No validation that all files will have
folderId = nullafter migration - No check for existing duplicate folder names (the unique index depends on this)
- No rollback procedure if the migration fails mid-deployment
- No data consistency checks post-migration
Recommendation: Expand the migration plan to include:
- Pre-migration validation (check for duplicate folder names, if any exist pre-migration)
- Atomic application of
folderIdandtypefields - Index creation with constraint validation
- Post-migration verification (sample check that all files have
folderIdandtype) - Rollback steps if indexes fail to create
🤖 Prompt for AI Agents
In 021-bucket-folders.md around lines 97–101, the migration plan is
insufficient: add a pre-migration validation step to scan for duplicate folder
names and report/resolve conflicts before proceeding; implement an atomic update
strategy (e.g., staged updates with a transactional batch or write a new temp
field and switch pointers) to set folderId=null and type='file' together; when
creating indexes, use non-blocking/validated creation (build indexes in the
background or validate constraints against a sample) and include a pre-check
that the uniqueness constraint won’t be violated; add post-migration
verification scripts to sample and assert that all documents have folderId and
type set and that counts match pre-migration totals; and document explicit
rollback steps (how to drop partial fields, revert to backups or restore from
snapshot, and drop newly created indexes) to execute if any step fails
mid-deployment.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit