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 static permission rendering with a Popover-driven, hover-activated permissions row component. Adds permission parsing (user|team|other) into a ParsedPermission shape and per-permission caching with Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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.
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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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, ARIA attributes, and keyboard support.The button is missing critical accessibility features flagged in previous reviews:
- No
type="button"(can trigger unintended form submits)- No keyboard handlers (excludes keyboard-only users)
- No ARIA attributes (confuses screen readers)
Apply this diff:
- <button - onmouseenter={() => { + <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, false)} + onkeydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + show(); + } + if (e.key === 'Escape') { + hide(); + } + }}>
📜 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] 235-239: Cannot redeclare block-scoped variable 'isUser'. svelte-check reports duplicate declaration in row.svelte (duplicate 'isUser' variable).
[error] 236-240: Cannot redeclare block-scoped variable 'isAnonymous'. svelte-check reports duplicate declaration in row.svelte (duplicate 'isAnonymous' variable).
⏰ 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 (4)
src/lib/components/permissions/row.svelte (4)
25-34: Good addition: caching layer implemented.The
permissionDataCacheaddresses the reviewer's request to cache user/team data, preventing redundant fetches on hover. The type signature properly covers user, team, and fallback cases.
51-76: LGTM: robust permission parsing.The
parsePermissionfunction safely handles malformed strings and returns structured data with validity flags, making downstream logic clearer and safer.
78-122: Excellent caching and data fetching implementation.The
getDatafunction implements a proper cache-first strategy, handles both user and team types, and provides sensible fallbacks for not-found cases. This addresses the past review comment about caching.
318-328: LGTM: popover styling is appropriate.The fixed-width styling with proper spacing ensures consistent tooltip layout.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/components/permissions/row.svelte (1)
152-182: Add required accessibility attributes and keyboard support.The button is still missing critical accessibility features that were flagged in previous reviews:
- Missing
type="button"- will cause unintended form submissions if used inside forms- Missing ARIA attributes (
aria-haspopup,aria-expanded,aria-label)- No keyboard support - keyboard users cannot open/navigate the popover
Apply this diff:
<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, false)} + onkeydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + show(); + } + if (e.key === 'Escape') { + hide(); + } + }}>
🧹 Nitpick comments (5)
src/lib/components/permissions/row.svelte (5)
25-34: Consider cache invalidation for failed lookups.Failed permission lookups (returning
notFound: true) are cached permanently inpermissionDataCache. If a user or team is created after a failed lookup, the cache will continue serving stalenotFoundresults until the page reloads. Additionally, the cache has no size limit and could grow indefinitely.Consider adding:
- A TTL or manual invalidation mechanism for failed lookups
- A maximum cache size with LRU eviction
51-76: Remove unnecessary try-catch block.The
try-catchblock wrappingparsePermissionis unnecessary since none of the operations inside (split, array access) throw exceptions. Thecatchblock will never execute.function parsePermission(permission: string): ParsedPermission { - try { - const [type, rest] = permission.split(':'); - if (!rest) { - return { type: 'other', id: permission, isValid: false }; - } + 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 }; - } + 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 - }; - } + 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 }; - } + return { type: 'other', id: permission, isValid: false }; }
78-122: Consider adding error logging for failed lookups.When user or team fetches fail (lines 101-103, 112-114), the errors are silently caught and returned as
notFound. This makes debugging production issues difficult.Consider adding console warnings or structured logging:
} catch (error) { + console.warn(`Failed to fetch user ${parsed.id}:`, error); return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; }
167-174: Optimize repeated getData calls with a reactive variable.
getData(role)is called twice—once in the button content (line 167) and again in the tooltip (line 194). While the cache prevents duplicate network requests, the function is still invoked multiple times unnecessarily.Create a reactive promise variable:
let { role, placement = 'bottom-start', children }: Props = $props(); + +let dataPromise = $derived(getData(role));Then use
dataPromisein both locations:-{#await getData(role)} +{#await dataPromise} {role} {:then data}Also applies to: 194-308
315-325: Verify the negative margin doesn't cause layout issues.Line 323 applies a negative margin of
-1remto the popover, which appears to be a positioning adjustment. Negative margins can cause unexpected overflow or clipping issues in some layouts.Consider whether this can be replaced with proper positioning via the Popover component's built-in positioning props, or document why this offset is needed.
📜 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: build
- GitHub Check: e2e
🔇 Additional comments (1)
src/lib/components/permissions/row.svelte (1)
287-305: Email is already displayed in the main UI—this is not a new exposure specific to the tooltip.The review raises concerns about PII display, but the analysis overlooked that email is already shown in the primary button text (line 269:
data.name ?? data?.email ?? data?.phone ?? '-'). The tooltip at line 294 repeats the same data. Phone is tooltip-only, which is the only genuinely tooltip-specific detail.Additionally, this component is part of an admin console permission management UI—not public-facing code. Users accessing this interface are already authorized staff managing permissions. The underlying data is fetched via the SDK, which provides its own authentication barrier. The review's framing about "displaying PII without access control" overlooks that access control exists at the route/SDK level, not necessarily required at the component level.
While phone display in a tooltip could warrant consideration for sensitive deployments, presenting email exposure as a critical compliance issue ignores that it's already displayed prominently in the main UI.
Likely an incorrect or invalid review comment.
| import { formatName } from '$lib/helpers/string'; | ||
| type PermissionData = Partial< | ||
| Models.User<Record<string, unknown>> & Models.Team<Record<string, unknown>> |
There was a problem hiding this comment.
we can remove Record<...> from both as shown before in the review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/components/permissions/row.svelte (1)
143-173: Add accessibility attributes and keyboard support.The button is missing critical accessibility features that were previously requested in past review comments but not yet implemented:
- Missing
type="button"— can trigger unintended form submissions- Missing ARIA attributes (
aria-haspopup,aria-label)- Missing keyboard event handlers — keyboard users cannot interact with the popover
Apply this diff to add accessibility support:
<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?.()}
🧹 Nitpick comments (1)
src/lib/components/permissions/row.svelte (1)
32-32: Consider adding cache size management.The
permissionDataCacheMap has no size limit or invalidation strategy. In long-running sessions with many unique permissions, this could grow unbounded.Consider implementing a simple size limit or LRU eviction in the future:
const MAX_CACHE_SIZE = 100; async function getData(permission: string): Promise<PermissionData> { const cached = permissionDataCache.get(permission); if (cached) return cached; const parsed = parsePermission(permission); const fetchPromise = fetchPermissionData(parsed); if (permissionDataCache.size >= MAX_CACHE_SIZE) { const firstKey = permissionDataCache.keys().next().value; permissionDataCache.delete(firstKey); } permissionDataCache.set(permission, fetchPromise); return fetchPromise; }
📜 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 comments (3)
src/lib/components/permissions/row.svelte (3)
25-40: LGTM! Clean type definitions and props interface.The
PermissionDatatype andPropsinterface are well-structured and follow the reviewer's suggestions from past comments. The use of Svelte 5's$props()is correct.
49-113: LGTM! Solid parsing and caching implementation.The permission parsing logic is robust with proper error handling, and the caching mechanism (
getData→fetchPermissionData) is clean and efficient as requested in past reviews.
175-303: LGTM! Well-structured tooltip content.The Popover tooltip implementation handles loading states, not-found cases, and displays user/team information clearly. The use of
Link.Anchorinside the tooltip (not nested in the button) is appropriate, and the mouse handlers properly manage hover state.
| {formatName( | ||
| data.name ?? data?.email ?? data?.phone ?? '-', | ||
| $isSmallViewport ? 5 : 7 | ||
| )} |
There was a problem hiding this comment.
Consider increasing truncation limit for small viewports.
Line 163 truncates to only 7 characters on small viewports, which may be too aggressive and could make names difficult to recognize. For comparison, custom permissions (line 153) use 8 characters minimum, and the tooltip (line 265) uses 12 characters minimum.
Consider increasing the small viewport truncation to at least 8-10 characters for consistency and better UX:
{formatName(
data.name ?? data?.email ?? data?.phone ?? '-',
- $isSmallViewport ? 5 : 7
+ $isSmallViewport ? 8 : 12
)}📝 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.
| {formatName( | |
| data.name ?? data?.email ?? data?.phone ?? '-', | |
| $isSmallViewport ? 5 : 7 | |
| )} | |
| {formatName( | |
| data.name ?? data?.email ?? data?.phone ?? '-', | |
| $isSmallViewport ? 8 : 12 | |
| )} |
🤖 Prompt for AI Agents
In src/lib/components/permissions/row.svelte around lines 161 to 164, the
small-viewport truncation currently uses 5 characters which is too aggressive
and inconsistent with other components; change the ternary so small viewports
use at least 8 characters (e.g., replace the 5 with 8) to align with custom
permissions and improve UX while keeping the existing large-viewport value.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/components/permissions/row.svelte (1)
160-163: Increase truncation limits for readability (small viewports too aggressive)Current 5/7 chars are hard to recognize.
- $isSmallViewport ? 5 : 7 + $isSmallViewport ? 8 : 12
🧹 Nitpick comments (4)
src/lib/components/permissions/row.svelte (4)
166-170: Use parsePermission for type instead of string prefix checksAvoid brittle
startsWith('user'); you already haveparsePermission.@@ - <Badge + {@const parsedForBadge = parsePermission(role)} + <Badge size="xs" variant="secondary" - content={role.startsWith('user') ? 'User' : 'Team'} /> + content={parsedForBadge.type === 'user' ? 'User' : 'Team'} /> @@ - {@const isUser = role.startsWith('user')} - {@const isAnonymous = - !data.email && !data.phone && !data.name && isUser} - {@const parsed = parsePermission(role)} - {@const id = parsed.id} + {@const parsed = parsePermission(role)} + {@const isUser = parsed.type === 'user'} + {@const isAnonymous = !data.email && !data.phone && !data.name && isUser} + {@const id = parsed.id}Also applies to: 225-229
196-199: Improve avatar alt text semanticsDecorative avatars should use empty alt; anonymous should be descriptive.
- <Avatar alt="avatar" size="m"> + <Avatar alt="" size="m"> @@ - <Avatar alt="avatar" size="m"> + <Avatar alt="Anonymous user" size="m"> @@ - <Avatar alt="avatar" size="m"> + <Avatar alt="" size="m">Also applies to: 238-241, 244-246
149-149: Avoid passing interactive children into the trigger button
{@render children?.()}inside a<button>can nest focusable content if consumers pass links/controls. Either document thatchildrenmust be non‑interactive or render it outside the button.Would you prefer we change this prop to a “prefix” text/icon slot and enforce non‑interactive content?
75-101: Differentiate not‑found vs transient errors for fetchesAll errors map to
{ notFound: true }, so network/5xx will appear as “not found.” Consider distinguishing 404 from others and exposing a retry.Example tweak:
- If error.status === 404 → notFound
- Else → { transient: true } and show a retry/Spinner
| const permissionDataCache: Map<string, Promise<PermissionData>> = new Map(); | ||
There was a problem hiding this comment.
Critical: cache key ignores project/region → cross‑project data leakage risk
Same user/team id in different projects can resolve to different entities. Caching by permission alone can show stale/wrong PII after switching project/region. Scope cache by project+region.
Apply:
@@
-const permissionDataCache: Map<string, Promise<PermissionData>> = new Map();
+const permissionDataCache: Map<string, Promise<PermissionData>> = new Map();
+
+function cacheKey(permission: string) {
+ return `${page.params.region}:${page.params.project}:${permission}`;
+}
@@
-async function getData(permission: string): Promise<PermissionData> {
- const cached = permissionDataCache.get(permission);
+async function getData(permission: string): Promise<PermissionData> {
+ const key = cacheKey(permission);
+ const cached = permissionDataCache.get(key);
if (cached) return cached;
const parsed = parsePermission(permission);
const fetchPromise = fetchPermissionData(parsed);
- permissionDataCache.set(permission, fetchPromise);
+ permissionDataCache.set(key, fetchPromise);
return fetchPromise;
}Optional follow‑up: add TTL/LRU to prevent unbounded growth across navigations.
Also applies to: 103-112
| let isMouseOverTooltip = $state(false); | ||
| function hidePopover(hideTooltip: () => void, timeout = true) { | ||
| if (!timeout) { | ||
| isMouseOverTooltip = false; | ||
| return hideTooltip(); | ||
| } | ||
| setTimeout(() => { | ||
| if (!isMouseOverTooltip) { | ||
| hideTooltip(); | ||
| } | ||
| }, 150); | ||
| } |
There was a problem hiding this comment.
Fix event binding + a11y + hover timing (keyboard + prevent implicit submit)
Use Svelte on: directives (current attributes won’t bind reliably), add keyboard/focus handlers and type="button", and make hover timers cancellable to avoid flicker.
@@
- let isMouseOverTooltip = $state(false);
+ let isMouseOverTooltip = $state(false);
+ let hoverTimer: number | null = null;
function hidePopover(hideTooltip: () => void, timeout = true) {
if (!timeout) {
isMouseOverTooltip = false;
+ if (hoverTimer) {
+ clearTimeout(hoverTimer);
+ hoverTimer = null;
+ }
return hideTooltip();
}
- setTimeout(() => {
- if (!isMouseOverTooltip) {
- hideTooltip();
- }
- }, 150);
+ if (hoverTimer) clearTimeout(hoverTimer);
+ hoverTimer = window.setTimeout(() => {
+ if (!isMouseOverTooltip) hideTooltip();
+ hoverTimer = null;
+ }, 150);
}
@@
- <button
- onmouseenter={() => {
+ <button
+ type="button"
+ aria-haspopup="dialog"
+ aria-label="View permission details"
+ on:mouseenter={() => {
if (!$menuOpen) {
- setTimeout(show, 150);
+ if (hoverTimer) clearTimeout(hoverTimer);
+ hoverTimer = window.setTimeout(show, 150);
}
}}
- onmouseleave={() => 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(); }
+ }}>
@@
- onmouseenter={() => (isMouseOverTooltip = true)}
- onmouseleave={() => hidePopover(hide, false)}>
+ on:mouseenter={() => (isMouseOverTooltip = true)}
+ on:mouseleave={() => hidePopover(hide, false)}>Optional: wire aria-expanded/aria-describedby to the popover if the API exposes an ID/“showing” state on the trigger.
Also applies to: 141-149, 180-182
🤖 Prompt for AI Agents
In src/lib/components/permissions/row.svelte around lines 114 to 126, the
popover/tooltip logic uses non‑Svelte attribute event binding, lacks
keyboard/focus handlers, omits type="button" (causing implicit submit), and uses
non‑cancellable hover timers which cause flicker; replace attribute bindings
with Svelte on: directives for mouseenter/mouseleave/click, add on:focus and
on:blur and keyboard handlers (Enter/Space) to open/close for a11y, set the
trigger button element to type="button" to prevent form submit, and change the
timeout logic to use a cancellable timer (store the timer id and clearTimeout on
opposite events) so hover enter/leave cancels pending hides; apply the same
fixes to the other occurrences at lines 141-149 and 180-182 and optionally wire
aria-expanded/aria-describedby to the popover state/ID if available.
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
Chores