Feat: enhance permission display and handling in permissions table #2466
Conversation
|
This PR was not deployed automatically as @HarshMN2345 does not have access to the Railway project. In order to get automatic PR deploys, please add @HarshMN2345 to your workspace on Railway. |
WalkthroughReplaces the simple role tooltip in the permissions row with an interactive Popover-driven UI. Adds Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 (4)
src/lib/components/permissions/row.svelte (4)
102-115: Stabilize hover behavior: clear pending show timer to avoid flickerCurrently, show() is delayed but not canceled on quick mouseleave, causing a popover “flash.” Track and clear the timer.
@@ - let isMouseOverTooltip = false; + let isMouseOverTooltip = false; + let showTimer: number | undefined; @@ - <button - on:mouseenter={() => { - if (!$menuOpen) { - setTimeout(show, 150); - } - }} - on:mouseleave={() => hidePopover(hide)}> + <button + on:mouseenter={() => { + if (!$menuOpen) { + if (showTimer) clearTimeout(showTimer); + showTimer = window.setTimeout(show, 150); + } + }} + on:mouseleave={() => { + if (showTimer) clearTimeout(showTimer); + hidePopover(hide); + }}> @@ - on:mouseenter={() => (isMouseOverTooltip = true)} + on:mouseenter={() => (isMouseOverTooltip = true)} on:mouseleave={() => hidePopover(hide, false)}>Also applies to: 131-137, 169-171
101-101: Reduce API calls and hardening: cache fetch + encode id in URLs
- Avoid double fetch by reusing a single promise.
- Don’t parse the role repeatedly in markup; derive reactive values.
- URL-encode the id to prevent broken links with special chars.
Add reactive derivations:
@@ - } + } + // Reuse a single fetch per role and derive helpers + let dataPromise: ReturnType<typeof getData>; + $: dataPromise = getData(role); + $: parsed = parsePermission(role); + $: isUser = parsed.type === 'user'; + $: id = parsed.isValid ? parsed.id : ''; + $: encodedId = id ? encodeURIComponent(id) : '';Reuse the promise in compact view:
@@ - {#await getData(role)} + {#await dataPromise} {role} {:then data} {formatName( data.name ?? data?.email ?? data?.phone ?? '-', $isSmallViewport ? 5 : 12 )} {/await}Reuse the promise in tooltip:
@@ - {#await getData(role)} + {#await dataPromise}Stop re-parsing role in markup:
@@ - {@const isUser = role.startsWith('user')} - {@const isAnonymous = - !data.email && !data.phone && !data.name && isUser} - {@const id = role.split(':')[1].split('/')[0]} + {@const isAnonymous = + !data.email && !data.phone && !data.name && isUser}Encode id in hrefs and use derived isUser:
@@ - href={role.startsWith('user') - ? `${base}/project-${page.params.region}-${page.params.project}/auth/user-${id}` - : `${base}/project-${page.params.region}-${page.params.project}/auth/teams/team-${id}`}> + href={isUser + ? `${base}/project-${page.params.region}-${page.params.project}/auth/user-${encodedId}` + : `${base}/project-${page.params.region}-${page.params.project}/auth/teams/team-${encodedId}`}>Also applies to: 145-152, 173-173, 214-218, 241-244
35-60: Parser resilience: avoid try/catch and handle edge casesNo code here throws; try/catch adds overhead and hides parse mistakes. Consider a small regex and named groups; also trim input to avoid whitespace bugs.
Example:
const re = /^(?<type>user|team):(?<id>[^/\s]+)(?:\/(?<roleName>.+))?$/; export function parsePermission(permission: string): ParsedPermission { const m = permission.trim().match(re); return m?.groups ? { type: m.groups.type as 'user'|'team', id: m.groups.id, roleName: m.groups.roleName, isValid: true } : { type: 'other', id: permission, isValid: false }; }
122-128: Consider i18n for user‑facing strings'Users', 'Guests', 'Any', 'User', 'Team', 'Email', 'Phone' are hardcoded. If the console is localized, move these to the translation layer.
Also applies to: 155-158, 265-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/permissions/row.svelte(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/lib/components/permissions/row.svelte
[error] 1-1: Prettier formatting check failed. Code style issues found in src/lib/components/permissions/row.svelte. Run 'prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (1)
src/lib/components/permissions/row.svelte (1)
1-303: CI failure: Prettier formatFormatting check failed for this file. Please run the repo’s formatter (e.g., pnpm format or prettier --write) and re‑commit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/components/permissions/row.svelte (6)
35-60: Harden parsing with a single regex (handles edges, removes try/catch noise)Use a regex to robustly capture type, id, and optional roleName; avoids misparsing and unnecessary try/catch.
- function parsePermission(permission: string): ParsedPermission { - try { - const [type, rest] = permission.split(':'); - if (!rest) { - return { type: 'other', id: permission, isValid: false }; - } - - const [id, roleName] = rest.split('/'); - if (!id) { - return { type: 'other', id: permission, isValid: false }; - } - - if (type === 'user' || type === 'team') { - return { - type: type as 'user' | 'team', - id, - roleName, - isValid: true - }; - } - - return { type: 'other', id: permission, isValid: false }; - } catch (error) { - return { type: 'other', id: permission, isValid: false }; - } - } + function parsePermission(permission: string): ParsedPermission { + const m = permission.match(/^(user|team):([^/]+)(?:\/(.+))?$/); + if (m) { + const [, type, id, roleName] = m; + return { + type: type as 'user' | 'team', + id, + roleName, + isValid: true + }; + } + return { type: 'other', id: permission, isValid: false }; + }
62-99: Avoid duplicate network calls: cachegetData(permission)resultsThe component awaits
getData(role)twice (trigger + tooltip). Add a tiny cache to prevent repeated fetches per role.Apply this diff to memoize per permission:
- async function getData(permission: string): Promise< + async function getData(permission: string): Promise< Partial<Models.User<Record<string, unknown>> & Models.Team<Record<string, unknown>>> & { notFound?: boolean; roleName?: string; customName?: string; } > { - const parsed = parsePermission(permission); + const parsed = parsePermission(permission); + + // Return cached promise if present + if (dataCache.has(permission)) { + return dataCache.get(permission)!; + } - if (!parsed.isValid || parsed.type === 'other') { - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; - } + const p = (async () => { + if (!parsed.isValid || parsed.type === 'other') { + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + } - if (parsed.type === 'user') { - try { - const user = await sdk - .forProject(page.params.region, page.params.project) - .users.get({ userId: parsed.id }); - return user; - } catch (error) { - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; - } - } + if (parsed.type === 'user') { + try { + const user = await sdk + .forProject(page.params.region, page.params.project) + .users.get({ userId: parsed.id }); + return user; + } catch { + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + } + } - if (parsed.type === 'team') { - try { - const team = await sdk - .forProject(page.params.region, page.params.project) - .teams.get({ teamId: parsed.id }); - return team; - } catch (error) { - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; - } - } + if (parsed.type === 'team') { + try { + const team = await sdk + .forProject(page.params.region, page.params.project) + .teams.get({ teamId: parsed.id }); + return team; + } catch { + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + } + } - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + })(); + + dataCache.set(permission, p); + return p; }Add this outside the function (top-level in the script):
// Simple in-memory cache for per-permission fetches const dataCache = new Map< string, Promise< Partial<Models.User<Record<string, unknown>> & Models.Team<Record<string, unknown>>> & { notFound?: boolean; roleName?: string; customName?: string; } > >();Optionally hoist a promise for the current role to reuse in both await blocks:
let dataPromise: ReturnType<typeof getData>; $: dataPromise = getData(role);Then replace both
{#await getData(role)}with{#await dataPromise}(see comments below on exact lines).
100-113: Minor: clear pending hide timer on re-entry/unmountPrevent stale timeouts by clearing any previous hide timer; clear on destroy.
Example changes:
+import { onDestroy } from 'svelte'; + let isMouseOverTooltip = false; +let hideTimer: ReturnType<typeof setTimeout> | null = null; function hidePopover(hideTooltip: () => void, timeout = true) { if (!timeout) { isMouseOverTooltip = false; - return hideTooltip(); + if (hideTimer) clearTimeout(hideTimer); + hideTimer = null; + return hideTooltip(); } - setTimeout(() => { + if (hideTimer) clearTimeout(hideTimer); + hideTimer = setTimeout(() => { if (!isMouseOverTooltip) { hideTooltip(); } - }, 150); + hideTimer = null; + }, 150); } + +onDestroy(() => { + if (hideTimer) clearTimeout(hideTimer); +});
171-285: Reduce duplicate fetches and use parser-derived id + safer href encoding
- Reuse the hoisted
dataPromisehere.- Avoid brittle
role.split(':')[1].split('/')[0]; useparsePermission(role).- Encode
idin href to be safe.- Minor: set more meaningful avatar alt text for a11y.
- {#await getData(role)} + {#await dataPromise} <Layout.Stack alignItems="center"> <Spinner /> </Layout.Stack> {:then data} {#if data.notFound} @@ - {@const isUser = role.startsWith('user')} + {@const isUser = parsePermission(role).type === 'user'} {@const isAnonymous = !data.email && !data.phone && !data.name && isUser} - {@const id = role.split(':')[1].split('/')[0]} + {@const id = parsePermission(role).id} @@ - {#if isAnonymous} - <Avatar alt="avatar" size="m"> + {#if isAnonymous} + <Avatar alt="Anonymous user" size="m"> <Icon icon={IconAnonymous} size="s" /> </Avatar> {:else if data.name} - <AvatarInitials name={data.name} size="m" /> + <AvatarInitials name={data.name} size="m" /> {:else} - <Avatar alt="avatar" size="m"> + <Avatar alt="No avatar" size="m"> <Icon icon={IconMinusSm} size="s" /> </Avatar> {/if} @@ - <Link.Anchor + <Link.Anchor variant="quiet" - href={role.startsWith('user') - ? `${base}/project-${page.params.region}-${page.params.project}/auth/user-${id}` - : `${base}/project-${page.params.region}-${page.params.project}/auth/teams/team-${id}`}> + href={isUser + ? `${base}/project-${page.params.region}-${page.params.project}/auth/user-${encodeURIComponent(id)}` + : `${base}/project-${page.params.region}-${page.params.project}/auth/teams/team-${encodeURIComponent(id)}`}>
153-156: Prefer parser over string prefix for type detectionUse
parsePermission(role).typeto decide label; more robust thanstartsWith.- <Badge - size="xs" - variant="secondary" - content={role.startsWith('user') ? 'User' : 'Team'} /> + <Badge + size="xs" + variant="secondary" + content={parsePermission(role).type === 'user' ? 'User' : 'Team'} />
291-301: Popover style looks fineFixed width and spacing are reasonable. Consider using design-token vars for margin if available to avoid magic -1rem, but not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/permissions/row.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (3)
src/lib/components/permissions/row.svelte (3)
26-26: Good addition:placementpropExposing
placementwith a sensible default improves flexibility. LGTM.
114-117: Helper reads clearly
isCustomPermissionis straightforward and correct. LGTM.
239-241: Routes match existing user/team detail patterns Verified thatauth/user-…andauth/teams/team-…URLs are used consistently across the codebase; no changes needed.
| <Popover let:show let:hide {placement} portal> | ||
| <button | ||
| on:mouseenter={() => { | ||
| if (!$menuOpen) { | ||
| setTimeout(show, 150); | ||
| } | ||
| }} | ||
| on:mouseleave={() => hidePopover(hide)}> | ||
| <slot> | ||
| {#if isCustomPermission(role)} | ||
| <Link.Anchor> | ||
| {formatName(role, $isSmallViewport ? 8 : 15)} | ||
| </Link.Anchor> | ||
| {:else} | ||
| <Layout.Stack direction="row" gap="s" alignItems="center" inline> | ||
| <Typography.Text> | ||
| {#await getData(role)} | ||
| {role} | ||
| {:then data} | ||
| {formatName( | ||
| data.name ?? data?.email ?? data?.phone ?? '-', | ||
| $isSmallViewport ? 5 : 12 | ||
| )} | ||
| {/await} | ||
| </Typography.Text> | ||
| <Badge | ||
| size="xs" | ||
| variant="secondary" | ||
| content={role.startsWith('user') ? 'User' : 'Team'} /> | ||
| </Layout.Stack> | ||
| {:then data} | ||
| {@const isUser = role.startsWith('user')} | ||
| {@const isTeam = role.startsWith('team')} | ||
| {@const isAnonymous = !data.email && !data.phone && !data.name && isUser} | ||
| <Layout.Stack> | ||
| <Layout.Stack direction="row" gap="s" alignItems="center"> | ||
| {#if isAnonymous} | ||
| <Avatar alt="avatar" size="xs"> | ||
| <Icon icon={IconAnonymous} size="s" /> | ||
| </Avatar> | ||
| {:else if data.name} | ||
| <AvatarInitials name={data.name} size="xs" /> | ||
| {:else} | ||
| <Avatar alt="avatar" size="xs"> | ||
| <Icon icon={IconMinusSm} size="s" /> | ||
| </Avatar> | ||
| {/if} | ||
| <Typography.Text truncate color="--fgcolor-neutral-primary"> | ||
| {data.name ?? data?.email ?? data?.phone ?? '-'} | ||
| </Typography.Text> | ||
| {/if} | ||
| </slot> | ||
| </button> |
There was a problem hiding this comment.
Accessibility: fix nested interactive, add keyboard support, and prevent implicit form submit
- Avoid nesting
<a>inside<button>(invalid, confusing for AT). - Add type="button" to avoid unintended form submits.
- Add focus/blur/keydown handlers so keyboard users can open/close the popover.
- <Popover let:show let:hide {placement} portal>
- <button
- on:mouseenter={() => {
+ <Popover let:show let:hide {placement} portal>
+ <button
+ type="button"
+ aria-haspopup="dialog"
+ aria-label="View permission details"
+ on:mouseenter={() => {
if (!$menuOpen) {
setTimeout(show, 150);
}
}}
- on:mouseleave={() => hidePopover(hide)}>
+ on:mouseleave={() => hidePopover(hide)}
+ on:focus={() => { if (!$menuOpen) show(); }}
+ on:blur={() => hidePopover(hide)}
+ on:keydown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ show();
+ }
+ if (e.key === 'Escape') {
+ hide();
+ }
+ }}>
<slot>
{#if isCustomPermission(role)}
- <Link.Anchor>
- {formatName(role, $isSmallViewport ? 8 : 15)}
- </Link.Anchor>
+ <Typography.Text>
+ {formatName(role, $isSmallViewport ? 8 : 15)}
+ </Typography.Text>
{:else}
<Layout.Stack direction="row" gap="s" alignItems="center" inline>
<Typography.Text>
- {#await getData(role)}
+ {#await dataPromise}
{role}
{:then data}
{formatName(
data.name ?? data?.email ?? data?.phone ?? '-',
$isSmallViewport ? 5 : 12
)}
{/await}
</Typography.Text>
<Badge
size="xs"
variant="secondary"
- content={role.startsWith('user') ? 'User' : 'Team'} />
+ content={parsePermission(role).type === 'user' ? 'User' : 'Team'} />
</Layout.Stack>
{/if}
</slot>
</button>To support {#await dataPromise} above, add:
let dataPromise: ReturnType<typeof getData>;
$: dataPromise = getData(role);near the other script declarations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/components/permissions/row.svelte (1)
152-182: Fix accessibility: add type, keyboard support, and ARIA attributes.The button element has accessibility gaps similar to those flagged in past reviews:
- Missing
type="button"may cause unintended form submission.- No keyboard support (focus/blur/keydown handlers) prevents keyboard-only navigation.
- Missing ARIA attributes (
aria-haspopup,aria-label) reduce screen reader usability.- Line 179 uses
role.startsWith('user')instead ofparsePermission(role).typefor type checking, which is inconsistent with the rest of the codebase.Apply this diff to address accessibility:
<Popover let:show let:hide {placement} portal> <button + type="button" + aria-haspopup="dialog" + aria-label="View permission details" onmouseenter={() => { if (!$menuOpen) { setTimeout(show, 150); } }} - onmouseleave={() => hidePopover(hide)}> + onmouseleave={() => hidePopover(hide)} + onfocus={() => { if (!$menuOpen) show(); }} + onblur={() => hidePopover(hide)} + onkeydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + show(); + } + if (e.key === 'Escape') { + hide(); + } + }}> {@render children?.()} {#if isCustomPermission(role)} <Typography.Text style="text-decoration: underline;"> {formatName(role, $isSmallViewport ? 8 : 15)} </Typography.Text> {:else} <Layout.Stack direction="row" gap="s" alignItems="center" inline> <Typography.Text> {#await getData(role)} {role} {:then data} {formatName( data.name ?? data?.email ?? data?.phone ?? '-', $isSmallViewport ? 5 : 7 )} {/await} </Typography.Text> <Badge size="xs" variant="secondary" - content={role.startsWith('user') ? 'User' : 'Team'} /> + content={parsePermission(role).type === 'user' ? 'User' : 'Team'} /> </Layout.Stack> {/if} </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/permissions/row.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (3)
src/lib/components/permissions/row.svelte (3)
36-42: LGTM!Props interface is well-defined with appropriate types and defaults. The Svelte 5 runes syntax is correctly used.
51-76: LGTM!The
parsePermissionfunction correctly parses permission strings, validates types, and handles edge cases with appropriate fallbacks.
25-34: Verify cache memory management in long-running sessions.The
permissionDataCacheMap caches promises indefinitely without any eviction strategy. In long-running sessions with many permissions, this could accumulate stale entries and consume memory.Consider implementing a cache eviction policy (e.g., LRU, time-based expiration) or clearing the cache when navigating between projects.
Based on past review comments about caching.
Also applies to: 78-122
| {@const isUser = role.startsWith('user')} | ||
| {@const isAnonymous = | ||
| !data.email && !data.phone && !data.name && isUser} | ||
| {@const id = role.split(':')[1].split('/')[0]} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use parsePermission helper instead of manual parsing.
Line 238 manually parses the permission string with role.split(':')[1].split('/')[0], which duplicates logic and is error-prone. Use the parsePermission helper for consistency and safety.
Apply this diff:
+ {@const parsed = parsePermission(role)}
{@const isUser = role.startsWith('user')}
{@const isAnonymous =
!data.email && !data.phone && !data.name && isUser}
- {@const id = role.split(':')[1].split('/')[0]}
+ {@const id = parsed.id}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {@const id = role.split(':')[1].split('/')[0]} | |
| {@const parsed = parsePermission(role)} | |
| {@const isUser = role.startsWith('user')} | |
| {@const isAnonymous = | |
| !data.email && !data.phone && !data.name && isUser} | |
| {@const id = parsed.id} |
🤖 Prompt for AI Agents
In src/lib/components/permissions/row.svelte around line 238, the code manually
parses the permission id via role.split(':')[1].split('/')[0]; replace this with
the parsePermission helper to avoid duplicated, brittle parsing. Import or
ensure parsePermission is available in this file, call it with the role string
and extract the id from its return (e.g., const { id } = parsePermission(role)),
and remove the manual split expression so the component uses the helper
consistently and safely.
2 participants
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
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?
yes
Summary by CodeRabbit
New Features
Style