feat: add feature flag folders for organization#330
feat: add feature flag folders for organization#330maoshuorz wants to merge 4 commits intodatabuddy-analytics:mainfrom
Conversation
Extract regex literals to top-level constants and remove non-null assertion to satisfy biome lint rules.
|
@maoshuorz 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)
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 SummaryThis PR adds a folder organisation system for feature flags, consisting of a nullable Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant FS as FolderSidebar
participant P as FlagsPage
participant FSelector as FolderSelector
participant Sheet as FlagSheet
participant API as flags.list / flags.update
U->>P: Load page
P->>API: flags.list({ websiteId })
API-->>P: all active flags (with folder field)
P->>FS: flags=activeFlags
FS-->>U: Renders folder tree (counts via buildFolderTree)
U->>FS: Click folder node
FS->>P: onSelectFolder("billing")
P->>P: filteredFlags = activeFlags.filter(startsWith "billing/")
P-->>U: FlagsList re-renders with filtered flags
U->>Sheet: Open "Create / Edit flag"
Sheet->>FSelector: existingFlags (for dropdown)
U->>FSelector: Select or type new folder
FSelector-->>Sheet: onChange(folderPath | null)
Sheet->>API: flags.create / flags.update { folder: "billing/plans" }
API-->>Sheet: updated flag
P->>API: flags.list (cache invalidated)
API-->>P: refreshed flags
Last reviewed commit: a2bbfe3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) : activeFlags.length === 0 ? ( | ||
| <div className="flex flex-1 items-center justify-center py-16"> | ||
| <EmptyState | ||
| action={{ | ||
| label: "Create Your First Flag", | ||
| onClick: handleCreateFlag, | ||
| }} | ||
| description="Create your first feature flag to start controlling feature rollouts and A/B testing across your application." | ||
| icon={<FlagIcon weight="duotone" />} | ||
| title="No feature flags yet" |
There was a problem hiding this comment.
No empty state for filtered folder view
When a folder is selected but contains no flags (i.e., filteredFlags is empty), the FlagsList is rendered with an empty array. There is no user-facing message like "No flags in this folder", because the existing empty state is only shown when activeFlags.length === 0. A user looking at an empty folder gets no feedback, which can be confusing.
Consider adding an additional check:
} : filteredFlags.length === 0 ? (
<div className="flex flex-1 items-center justify-center py-16">
<EmptyState
description="No flags in this folder."
icon={<FolderIcon weight="duotone" />}
title="No flags here"
variant="minimal"
/>
</div>
) : (
| folder: z | ||
| .string() | ||
| .max(100) | ||
| .regex(/^[a-zA-Z0-9_\-/]*$/) | ||
| .nullable() | ||
| .optional(), |
There was a problem hiding this comment.
Empty string passes folder validation but creates inconsistency
The regex /^[a-zA-Z0-9_\-/]*$/ with z.string() allows an empty string "" to pass validation (since * matches zero or more characters). This creates a subtle inconsistency:
- In the create and restore paths:
input.folder || nullconverts""tonull, so the DB storesNULL. - In the update path:
updatesis spread directly, sofolder: ""is stored as the literal empty string""in the DB. - In the list filter path:
folder === ""triggersisNull(flags.folder), which won't match a row wherefolder = "".
So a flag updated via API with folder: "" would be invisible to the folder: "" list filter (which expects NULL). Adding .min(1) (but only when the value is not null) would close this gap:
| folder: z | |
| .string() | |
| .max(100) | |
| .regex(/^[a-zA-Z0-9_\-/]*$/) | |
| .nullable() | |
| .optional(), | |
| folder: z | |
| .string() | |
| .min(1) | |
| .max(100) | |
| .regex(/^[a-zA-Z0-9_\-/]*$/) | |
| .nullable() | |
| .optional(), |
| environment: z.string().nullable().optional(), | ||
| folder: z | ||
| .string() | ||
| .max(100) | ||
| .regex(/^[a-zA-Z0-9_\-/]*$/) | ||
| .nullable() |
There was a problem hiding this comment.
Same empty-string gap in shared schema
This flagFormSchema validation has the same issue as updateFlagSchema in flags.ts — the regex ^[a-zA-Z0-9_\-/]*$ allows an empty string. Adding .min(1) prevents a form submission of folder: "" which would behave differently from folder: null:
| environment: z.string().nullable().optional(), | |
| folder: z | |
| .string() | |
| .max(100) | |
| .regex(/^[a-zA-Z0-9_\-/]*$/) | |
| .nullable() | |
| folder: z | |
| .string() | |
| .min(1) | |
| .max(100) | |
| .regex(/^[a-zA-Z0-9_\-/]*$/) | |
| .nullable() | |
| .optional(), |
| className="h-8 text-sm" | ||
| onChange={(e) => setNewFolderInput(e.target.value)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| e.preventDefault(); | ||
| handleCreateFolder(); | ||
| } | ||
| }} | ||
| placeholder="New folder path..." |
There was a problem hiding this comment.
"New folder" button enabled for slash-only input
The button's disabled check evaluates newFolderInput.trim() as the truthy guard, but handleCreateFolder strips leading/trailing slashes before the emptiness check. This means a user can type / (or ///), see the + button become enabled, click it, and nothing happens silently — because trimmed becomes "" after the replace.
Aligning the disabled logic with the same strip-then-check:
| className="h-8 text-sm" | |
| onChange={(e) => setNewFolderInput(e.target.value)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| e.preventDefault(); | |
| handleCreateFolder(); | |
| } | |
| }} | |
| placeholder="New folder path..." | |
| disabled={ | |
| !(() => { | |
| const t = newFolderInput.trim().replace(/^\/+|\/+$/g, ""); | |
| return t && FOLDER_PATH_REGEX.test(t); | |
| })() | |
| } |
packages/rpc/src/routers/flags.ts
Outdated
| conditions.push(eq(flags.status, input.status)); | ||
| } | ||
|
|
||
| if (input.folder !== undefined) { | ||
| if (input.folder === "") { | ||
| conditions.push(isNull(flags.folder)); | ||
| } else { | ||
| conditions.push(eq(flags.folder, input.folder)); |
There was a problem hiding this comment.
List folder filter only supports exact match, not sub-folder hierarchy
The API filter uses eq(flags.folder, input.folder) for an exact match. This is inconsistent with the client-side filtering in page.tsx, which also returns flags in child folders via f.folder?.startsWith(${selectedFolder}/).
If anyone calls the API directly with a parent folder path (e.g., folder: "billing"), flags stored in billing/plans will be silently omitted. The API should either document this limitation or match the client's behaviour.
|
recheck |
- Add .min(1) to folder schema to prevent empty-string vs NULL inconsistency - Add empty state message when selected folder has no flags - Fix folder create button being enabled for slash-only input
API now returns flags in sub-folders when filtering by parent folder, consistent with the client-side startsWith behavior.
|
Thanks for the thorough review! All 4 issues have been addressed:
|
2 participants
Summary
Resolves #271
Adds a folder system to organize feature flags in the dashboard UI. Uses a simple string field approach (Option A) for maximum simplicity and backward compatibility.
Changes
Database
foldertext field toflagstable with btree indexAPI (ORPC)
folderfilter parameter toflags.listfolderfield toflags.createandflags.updateShared Validation
foldertoflagFormSchemawith path validation regexDashboard UI
billing/plans)Design
Screenshots
(Will add after deployment)
/claim #271