feat: add folder system for feature flag organization (#271)#346
feat: add folder system for feature flag organization (#271)#346nifanpinc wants to merge 15 commits intodatabuddy-analytics:mainfrom
Conversation
|
|
|
@nifanpinc is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryAdds a folder organization system for feature flags using a simple string-based approach with While the database layer (migration, schema, shared schema) is clean and backward-compatible, there are several critical issues that need to be addressed before merging:
Confidence Score: 1/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[FlagsPage] -->|fetch| B[flags.list API]
A -->|extract folders| C[FolderTree Sidebar]
C -->|select folder| D{Filter Logic}
D -->|null| E[All Active Flags]
D -->|__uncategorized__| F[Flags without folder]
D -->|folder path| G[Flags matching folder + children]
E --> H[FlagsList Component]
F --> H
G --> H
H -->|edit| I[setEditingFlagAtom + setIsFlagSheetOpen]
H -->|delete| J[invalidateQueries only]
I -->|BUG: no FlagSheet rendered| K[Nothing happens]
J -->|BUG: no DeleteDialog/mutation| L[Nothing happens]
subgraph New API Endpoints
M[flags.listFolders] -->|BUG: notDeleted not called| N[Returns folders including deleted flags]
O[flags.moveToFolder] -->|BUG: no scope check on flagId| P[Updates flag folder]
P -->|BUG: wrong args| Q[invalidateFlagCache fails]
end
subgraph DB Layer
R[flags table] -->|new column| S[folder: text nullable]
S -->|indexed| T[idx_flags_folder]
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import type { Flag, TargetGroup } from "./_components/types"; | ||
| import { | ||
| isFlagSheetOpenAtom, | ||
| editingFlagAtom, |
There was a problem hiding this comment.
editingFlagAtom does not exist
This import will fail at build time. The flagsAtoms.ts file only exports isFlagSheetOpenAtom and isGroupSheetOpenAtom — there is no editingFlagAtom. You'll need to either add this atom to the store file, or revert to using local useState as the old code did.
| return ( | ||
| <div className="flex h-full min-h-0 gap-0"> | ||
| {/* Folder Sidebar */} | ||
| {folders.length > 0 && ( | ||
| <div className="w-56 shrink-0 border-r border-border overflow-y-auto py-2"> | ||
| <FolderTree | ||
| folders={folders} | ||
| selectedFolder={selectedFolder} | ||
| onSelectFolder={setSelectedFolder} | ||
| onCreateFolder={handleCreateFolder} | ||
| /> | ||
| </div> | ||
| </ErrorBoundary> | ||
| </FeatureGate> | ||
| )} | ||
|
|
||
| {/* Flags List */} | ||
| <div className="flex-1 min-w-0 overflow-y-auto"> | ||
| <FlagsList | ||
| flags={filteredFlags as Flag[]} | ||
| groups={groupsMap} | ||
| onEdit={(flag) => { | ||
| setEditingFlag(flag); | ||
| setIsFlagSheetOpen(true); | ||
| }} | ||
| onDelete={() => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: orpc.flags.list.queryOptions({ input: { websiteId } }).queryKey, | ||
| }); | ||
| }} | ||
| /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
FlagSheet and DeleteDialog removed — edit/delete broken
The old page rendered <FlagSheet> and <DeleteDialog>, but this rewrite removed both without relocating them. Setting setIsFlagSheetOpen(true) / setEditingFlag(flag) on line 108-109 will update atoms, but no component listens to render the sheet on this page. The same applies to delete — onDelete (line 111-114) now only invalidates queries but never actually performs a delete mutation or shows a confirmation dialog.
This means:
- Clicking "Edit" on a flag does nothing visible
- Clicking "Delete" on a flag silently does nothing (no mutation, no confirmation)
You need to re-add the FlagSheet and DeleteDialog components (or lift them into the layout if that was the intention).
packages/rpc/src/routers/flags.ts
Outdated
| const result = await context.db | ||
| .selectDistinct({ folder: flags.folder }) | ||
| .from(flags) | ||
| .where(and(notDeleted, scopeCondition)); |
There was a problem hiding this comment.
notDeleted used as value instead of function call
notDeleted is a function that takes a table argument (see packages/db/src/utils.ts), but here it's passed as a bare reference. Since a function reference is truthy, Drizzle will receive a non-SQL value in the and() call — this means the deleted-flag filter is silently skipped, and listFolders will return folders from soft-deleted flags.
| .where(and(notDeleted, scopeCondition)); | |
| .where(and(notDeleted(flags), scopeCondition)); |
packages/rpc/src/routers/flags.ts
Outdated
| .where(eq(flags.id, flagId)) | ||
| .returning(); | ||
| if (result) { | ||
| await invalidateFlagCache(result); |
There was a problem hiding this comment.
invalidateFlagCache called with wrong arguments
invalidateFlagCache expects (id: string, websiteId?, organizationId?, flagKey?) but result is a full flag row object. Passing the object as the first argument coerces it to "[object Object]" for the id parameter, and websiteId/organizationId are left undefined — so the cache will not be properly invalidated.
| await invalidateFlagCache(result); | |
| await invalidateFlagCache(result.id, result.websiteId, result.organizationId, result.key); |
packages/rpc/src/routers/flags.ts
Outdated
| for (const flagId of input.flagIds) { | ||
| const [result] = await context.db | ||
| .update(flags) | ||
| .set({ folder: input.folder, updatedAt: new Date() }) | ||
| .where(eq(flags.id, flagId)) |
There was a problem hiding this comment.
moveToFolder doesn't scope-check individual flags
The update query filters only on eq(flags.id, flagId) without verifying the flag belongs to the provided websiteId/organizationId. A user with update permission on website A could move flags belonging to website B by providing arbitrary flag IDs. Other mutation endpoints (e.g., update) verify the flag's scope before modifying it.
Consider adding a scope condition to the WHERE clause:
| for (const flagId of input.flagIds) { | |
| const [result] = await context.db | |
| .update(flags) | |
| .set({ folder: input.folder, updatedAt: new Date() }) | |
| .where(eq(flags.id, flagId)) | |
| for (const flagId of input.flagIds) { | |
| const scopeCondition = getScopeCondition(input.websiteId, input.organizationId); | |
| const [result] = await context.db | |
| .update(flags) | |
| .set({ folder: input.folder, updatedAt: new Date() }) | |
| .where(and(eq(flags.id, flagId), scopeCondition, isNull(flags.deletedAt))) | |
| .returning(); |
| import { orpc } from "@/lib/orpc"; | ||
| import { cn } from "@/lib/utils"; | ||
| import { FlagKey } from "./flag-key"; | ||
| import { FolderIcon } from "@phosphor-icons/react"; |
There was a problem hiding this comment.
Unused import
FolderIcon is imported but never used in this file. The PR description mentions showing a folder badge on each flag item, but no badge rendering was added to the FlagRow component.
| import { FolderIcon } from "@phosphor-icons/react"; |
- Remove non-existent editingFlagAtom import, use useState instead - Restore FlagSheet, DeleteDialog, FeatureGate, ErrorBoundary components - Fix notDeleted used as value instead of function call in listFolders - Fix invalidateFlagCache called with wrong arguments in moveToFolder - Add scope check (websiteId/organizationId) to moveToFolder update query - Remove unused FolderIcon import from flags-list.tsx
|
Addressed all 6 issues from the automated review:
All folder functionality (sidebar, filtering, drag-to-folder) is preserved. |
|
Hi @databuddy-analytics/maintainers, I've addressed all 6 issues identified by the automated code review in commit bb0ab37:
The PR is now ready for re-review. All changes are backward compatible and follow the existing code patterns. Thanks! |
|
Hi @databuddy-analytics/maintainers, Following up on this PR - I've addressed all feedback from the automated review 24+ hours ago. The folder system implementation is complete and ready for human review. Is there anything else needed from my side to move this forward? Thanks! |
2 participants
Summary
Implements #271 — Feature Flag Folders for Organization.
Changes
Database (
packages/db)foldertext field toflagstable (nullable, backward compatible)idx_flags_folderindex for query performance0001_add_flag_folders.sqlShared Schema (
packages/shared)folderfield toflagFormSchema(nullable, optional)API (
packages/rpc)flags.list: Added optionalfolderfilter parameterflags.create/update: Accept and persistfolderfieldflags.listFolders: New endpoint — returns distinct folder names for a website/orgflags.moveToFolder: New endpoint — batch move flags to a folderDashboard UI (
apps/dashboard)FolderTreecomponent: Hierarchical folder navigation with expand/collapse, nested folder support via/separatorpage.tsx: Integrated folder sidebar — shows when folders exist, filters flags by selected folderflags-list.tsx: Shows folder badge on each flag itemflag-sheet.tsx: Added folder input field for create/edit with nesting hinttypes.ts: Addedfolderto Flag typeDesign Decisions
folderis a plain text field storing paths likeauth/login/separator (no separate folder table needed)Screenshots
The folder sidebar appears on the left when folders exist, with a tree view supporting nested folders. Each flag shows its folder as a small badge.