feat(dashboard): streamline api key permission setup#359
feat(dashboard): streamline api key permission setup#359omaroubari wants to merge 9 commits intodatabuddy-analytics:mainfrom
Conversation
… are selected for the api key
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@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 |
|
|
Greptile SummaryThis PR streamlines the API key creation dialog by replacing manual scope selects with three preset tabs ("All", "Restricted", "Read data only") and swapping the website dropdown + trash-button pattern for a searchable Key issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Open Create Dialog]) --> B[Enter Key Name]
B --> C{Choose Preset Tab}
C -->|All| D[globalScopes = ALL_SCOPES]
C -->|Restricted| E[globalScopes = DEFAULT_SCOPES\nShow scope checkboxes]
E --> E2[User toggles individual scopes]
E2 --> F
C -->|Read data only| F[globalScopes = DEFAULT_SCOPES]
D --> G{Select Websites?}
F --> G
G -->|No websites selected| H[resources.global = globalScopes]
G -->|One or more websites| I[resources[website:id] = globalScopes\nfor each selected website\nglobal key omitted]
H --> J[mutation.mutate]
I --> J
J --> K([API Key Created — show secret])
Reviews (1): Last reviewed commit: "fix(dashboard): various bug fixes in api..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const assignGlobalScopes = (value: string) => { | ||
| switch (value) { | ||
| case "all": | ||
| setGlobalScopes(ALL_SCOPES); | ||
| break; | ||
| case "restricted": | ||
| setGlobalScopes(DEFAULT_SCOPES); | ||
| break; | ||
| case "read-data": | ||
| setGlobalScopes(DEFAULT_SCOPES); | ||
| break; | ||
| default: | ||
| setGlobalScopes(DEFAULT_SCOPES); | ||
| } | ||
| }; |
There was a problem hiding this comment.
assignGlobalScopes resets scope state on every tab change. If a user is on the "Restricted" tab and customises their scopes (e.g., toggling on extra permissions), then accidentally clicks "All" and back to "Restricted", their work is silently discarded — setGlobalScopes(DEFAULT_SCOPES) is called again.
Consider preserving the restricted selection in a separate ref/state so it can be restored when the user returns to that tab, similar to how form field values are kept across focus changes.
| <p className="text-muted-foreground text-xs"> | ||
| These permissions apply to all websites. Read Data is included | ||
| by default. | ||
| </p> |
There was a problem hiding this comment.
The helper text still says "Read Data is included by default", but the initial value of globalScopes is now ALL_SCOPES (all scopes) and the tabs default to "all". The description misleads users into thinking they are starting with a minimal permission set.
| <p className="text-muted-foreground text-xs"> | |
| These permissions apply to all websites. Read Data is included | |
| by default. | |
| </p> | |
| <p className="text-muted-foreground text-xs"> | |
| These permissions apply to all websites unless you restrict to | |
| specific ones below. | |
| </p> |
| "focus-visible:ring-ring/50 disabled:pointer-events-none disabled:opacity-50 _data-[state=active]:border-accent-foreground/10", | ||
| "_data-[state=active]:bg-secondary-brightest [&_svg:not([class*='size-'])]:size-4 [&_svg]:pointer-events-none [&_svg]:shrink-0", | ||
| // disable trigger active styling so TabList default `absolute` indicator animation is visible |
There was a problem hiding this comment.
_data-[state=active]:border-accent-foreground/10 and _data-[state=active]:bg-secondary-brightest are not valid Tailwind utility classes — the leading _ makes them unrecognisable to the compiler, effectively commenting them out without actually removing them. While the inline comment explains the intent, this approach will confuse future contributors who don't know the convention, and any Tailwind purge / scan step that processes strings will include these as false positives.
A more idiomatic approach is to use data-[state=inactive]:... to set the desired state explicitly, or (if you need the classes to stay visible for documentation) leave the originals as a plain code comment above the line.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <Command> | ||
| <CommandList> | ||
| <CommandEmpty>No websites found.</CommandEmpty> | ||
| <CommandInput placeholder="Search websites..." /> | ||
| <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 placed inside CommandList
CommandInput is nested inside CommandList, but the cmdk library (which the shadcn Command is built on) requires it to be a direct child of Command, outside the list. The list element is the scrollable container, so placing the input there causes the search box to scroll alongside the results and breaks the built-in filtering — CommandEmpty will not respond to input changes correctly.
The correct structure should be:
<Command>
<CommandInput placeholder="Search websites..." />
<CommandList>
<CommandEmpty>No websites found.</CommandEmpty>
<CommandGroup>
...
</CommandGroup>
</CommandList>
</Command>
omaroubari
changed the title
|
Fixed a bunch of bugs. Ready for review. |
2 participants
Description
databuddy.api.key.ux.mp4
Checklist