feat(dashboard): streamline api key permission setup#358
feat(dashboard): streamline api key permission setup#358omaroubari wants to merge 7 commits intodatabuddy-analytics:mainfrom
Conversation
… are selected for the api key
|
@omaroubari 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
|
Greptile SummaryThis PR streamlines the API key creation dialog by replacing manual scope selects with three preset permission tabs (All / Restricted / Read data only), swapping the website add-button pattern for a combobox picker, and carrying the selected global scopes forward as the default for newly added websites. UI polish is also applied to Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User opens Create API Key sheet]) --> B[Enter key name]
B --> C{Select global permission preset}
C -->|All| D[globalScopes = ALL_SCOPES]
C -->|Restricted| E[globalScopes = DEFAULT_SCOPES\nthen toggle individual scopes]
C -->|Read data only| F[globalScopes = DEFAULT_SCOPES]
D & E & F --> G{Add website restrictions?}
G -->|No| H["resources.global = globalScopes\n(global access)"]
G -->|Yes — open combobox| I[Select/deselect websites\nwebsiteAccess entries store snapshot of globalScopes]
I --> J{Submit}
J -->|websiteAccess.length > 0| K["resources[website:X] = globalScopes ⚠️\n(uses live globalScopes, not snapshot)"]
J -->|websiteAccess.length = 0| H
H & K --> L[mutation.mutate → API call]
L --> M([API key created — show secret])
Reviews (1): Last reviewed commit: "docs(api-keys): documented permission pr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <Popover onOpenChange={setPopoverOpen} open={popoverOpen}> | ||
| <PopoverTrigger asChild> | ||
| <Button | ||
| aria-expanded={open} |
There was a problem hiding this comment.
aria-expanded uses sheet open prop instead of popoverOpen
The aria-expanded attribute on the combobox trigger is bound to the component's open prop (whether the sheet/drawer is open), not popoverOpen (whether the website picker combobox is open). This means aria-expanded will always be true while the sheet is visible, breaking accessibility for screen readers.
| aria-expanded={open} | |
| aria-expanded={popoverOpen} |
| for (const entry of websiteAccess) { | ||
| if (entry.resourceId && entry.scopes.length > 0) { | ||
| resources[`website:${entry.resourceId}`] = entry.scopes; | ||
| resources[`website:${entry.resourceId}`] = globalScopes; | ||
| } | ||
| } |
There was a problem hiding this comment.
entry.scopes is captured as a snapshot of globalScopes at the time a website was added, but in onSubmit it is only used as a guard (entry.scopes.length > 0) while the actual value written to resources is the current globalScopes. If the user later switches to the "Restricted" tab and de-selects all scopes (making globalScopes = []), the guard still passes (the snapshot is non-empty), but every website entry is written with an empty scope array. The API would receive website:X = [], silently creating a key with no permissions for the selected websites.
The condition should also verify the live value, or use entry.scopes as the authoritative value:
for (const entry of websiteAccess) {
if (entry.resourceId && globalScopes.length > 0) {
resources[`website:${entry.resourceId}`] = globalScopes;
}
}| <Command> | ||
| <CommandList> | ||
| <CommandEmpty>No websites found.</CommandEmpty> | ||
| <CommandGroup> | ||
| {(websites as Website[]).map((website) => ( | ||
| <CommandItem | ||
| key={website.id} | ||
| onSelect={(currentValue) => { | ||
| isWebsiteSelected(currentValue) | ||
| ? removeWebsite(currentValue) | ||
| : addWebsite(currentValue); | ||
| setPopoverOpen(false); | ||
| }} | ||
| value={website.id} | ||
| > | ||
| <TrashIcon size={14} /> | ||
| </Button> | ||
| </div> | ||
| <div className="grid grid-cols-2 gap-1"> | ||
| {SCOPE_OPTIONS.slice(0, 6).map((scope) => { | ||
| const isSelected = entry.scopes.includes( | ||
| scope.value | ||
| ); | ||
| return ( | ||
| <button | ||
| className={`flex items-center gap-2 rounded px-2 py-1.5 text-left text-xs ${ | ||
| isSelected | ||
| ? "bg-primary/20 text-foreground" | ||
| : "hover:bg-muted" | ||
| }`} | ||
| key={scope.value} | ||
| onClick={() => | ||
| toggleWebsiteScope( | ||
| entry.resourceId ?? "", | ||
| scope.value | ||
| ) | ||
| } | ||
| type="button" | ||
| > | ||
| <div | ||
| className={`flex size-3 shrink-0 items-center justify-center rounded-sm border ${ | ||
| isSelected | ||
| ? "border-primary bg-primary text-primary-foreground" | ||
| : "border-muted-foreground/30" | ||
| }`} | ||
| > | ||
| {isSelected && ( | ||
| <CheckIcon size={8} weight="bold" /> | ||
| )} | ||
| </div> | ||
| <span className="truncate"> | ||
| {scope.label} | ||
| </span> | ||
| </button> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
| )} | ||
| {website.name || website.domain} | ||
| <CheckIcon | ||
| className={`ml-auto size-3.5 ${ | ||
| isWebsiteSelected(website.id) | ||
| ? "opacity-100" | ||
| : "opacity-0" | ||
| }`} | ||
| /> | ||
| </CommandItem> | ||
| ))} | ||
| </CommandGroup> | ||
| </CommandList> | ||
| </Command> | ||
| </PopoverContent> |
There was a problem hiding this comment.
CommandInput — picker is not actually searchable
The PR description advertises a "searchable website picker", and the CommandEmpty ("No websites found.") implies filtering is expected. However, no CommandInput component is rendered inside Command, so users have no way to type a search query. The cmdk Command component filters items based on text typed into a CommandInput; without it, the filter never activates and CommandEmpty can never appear.
Consider adding a search input:
<Command>
<CommandInput placeholder="Search websites..." />
<CommandList>
<CommandEmpty>No websites found.</CommandEmpty>
...
</CommandList>
</Command>| <TabsPrimitive.Trigger | ||
| className={cn( | ||
| "inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5 whitespace-nowrap rounded-md border border-transparent px-2 py-1", | ||
| "z-5 inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5 whitespace-nowrap rounded-md border border-transparent px-2 py-1", |
There was a problem hiding this comment.
z-5 is not a standard Tailwind utility
Tailwind's default z-index scale only includes z-0, z-10, z-20, z-30, z-40, z-50. z-5 will be silently ignored unless the project extends the scale with a custom config. The intent here is to ensure the trigger buttons stack above the z-0 sliding indicator <div>, but using an undefined utility class means there is no actual stacking guarantee.
Use z-10 (or another value from the configured scale) instead:
| "z-5 inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5 whitespace-nowrap rounded-md border border-transparent px-2 py-1", | |
| "z-10 inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5 whitespace-nowrap rounded-md border border-transparent px-2 py-1", |
| onSelect={(currentValue) => { | ||
| isWebsiteSelected(currentValue) | ||
| ? removeWebsite(currentValue) | ||
| : addWebsite(currentValue); | ||
| setPopoverOpen(false); | ||
| }} |
There was a problem hiding this comment.
setPopoverOpen(false) is called unconditionally inside onSelect, so the dropdown closes immediately after adding or removing a website. Users who want to select multiple websites must reopen the combobox for each one. Keeping the popover open during multi-select (and closing it on blur/outside-click via Radix's default behavior) would be more ergonomic.
Consider only closing on removal (or not closing at all and relying on Radix's outside-click dismiss):
onSelect={(currentValue) => {
if (isWebsiteSelected(currentValue)) {
removeWebsite(currentValue);
} else {
addWebsite(currentValue);
}
// let Radix handle close-on-outside-click instead
}}2 participants
Description
Checklist