Fix: few permissions UI improvements #2559
Conversation
ConsoleProject ID: Tip You can use Avatars API to generate QR code for any text or URLs. |
WalkthroughThe PR replaces a div-based table wrapper in the permissions view with a TableScroll component (imported from '$lib/elements/table') and removes the internal Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
| let { role, placement = 'bottom-start', children }: Props = $props(); | ||
| const dispatch = createEventDispatcher<{ notFound: string }>(); |
There was a problem hiding this comment.
lets avoid dispatch as its marked deprecated, we can use callbacks.
Co-authored-by: Darshan <itznotabug@gmail.com>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/permissions/permissions.svelte(4 hunks)src/lib/components/permissions/row.svelte(5 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 (7)
src/lib/components/permissions/row.svelte (4)
2-2: LGTM: Event dispatcher setup.The import and initialization of the event dispatcher for notifying parent components about missing roles follows standard Svelte patterns.
Also applies to: 41-41
180-180: LGTM: Spacing adjustment for small viewports.The increase from 18 to 20 characters for name display on small viewports should improve readability while maintaining responsive design.
299-306: LGTM: Improved typography hierarchy for email.The change from
Typography.TexttoTypography.Captionwith increased character limits (24/32) appropriately styles email as secondary information in the tooltip while allowing more of the email address to be visible.
309-313: LGTM: Consistent typography for phone display.The typography change maintains consistency with the email field styling, appropriately presenting phone numbers as secondary contact information.
src/lib/components/permissions/permissions.svelte (3)
23-23: LGTM: TableScroll component import.The import is correctly structured and the component path appears valid.
128-128: LGTM: Improved table scrolling with TableScroll component.Replacing the custom
div.table-wrapperwith a dedicatedTableScrollcomponent improves consistency and maintainability across the application's table implementations.Also applies to: 178-178
143-143: Verify that silent deletion of missing roles is the intended UX.The
on:notFoundhandler automatically removes roles that cannot be found without user notification. While this cleans up stale/invalid roles automatically, users receive no feedback about which roles were removed or why.Consider whether users should be notified when roles are automatically deleted, especially if this could affect their permission configuration unexpectedly. If silent deletion is intentional, ensure this behavior is documented.
| async function verifyExistence() { | ||
| try { | ||
| const data = await getData(role); | ||
| if (data?.notFound) { | ||
| dispatch('notFound', role); | ||
| } | ||
| } catch {} | ||
| } | ||
| onMount(() => { | ||
| verifyExistence(); | ||
| }); |
There was a problem hiding this comment.
Empty catch block silently suppresses errors.
The empty catch block at line 122 will silently swallow all errors during role verification, making it difficult to debug issues when getData or the dispatcher fails. At minimum, consider logging errors for development/debugging purposes.
Additionally, if the permissions table contains many roles, the onMount hook will trigger simultaneous API calls for each row. Consider whether this could impact performance or if debouncing/batching might be beneficial.
Apply this diff to add error logging:
async function verifyExistence() {
try {
const data = await getData(role);
if (data?.notFound) {
dispatch('notFound', role);
}
- } catch {}
+ } catch (error) {
+ console.error('Failed to verify role existence:', role, error);
+ }
}📝 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.
| async function verifyExistence() { | |
| try { | |
| const data = await getData(role); | |
| if (data?.notFound) { | |
| dispatch('notFound', role); | |
| } | |
| } catch {} | |
| } | |
| onMount(() => { | |
| verifyExistence(); | |
| }); | |
| async function verifyExistence() { | |
| try { | |
| const data = await getData(role); | |
| if (data?.notFound) { | |
| dispatch('notFound', role); | |
| } | |
| } catch (error) { | |
| console.error('Failed to verify role existence:', role, error); | |
| } | |
| } | |
| onMount(() => { | |
| verifyExistence(); | |
| }); |
🤖 Prompt for AI Agents
In src/lib/components/permissions/row.svelte around lines 116 to 127, the empty
catch block in verifyExistence silently swallows errors and onMount triggers a
verify for every row which can cause many simultaneous API calls; update the
catch to log the error (e.g., console.error with a clear message and the role)
so failures are visible, and move per-row verification into a batched or
debounced mechanism (for example expose a parent-level verifier or queue
requests) so row mounts do not fire N concurrent getData calls — implement the
logging immediately and refactor verification to a shared/batched call to
prevent performance spikes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/permissions/permissions.svelte(4 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). (1)
- GitHub Check: e2e
🔇 Additional comments (2)
src/lib/components/permissions/permissions.svelte (2)
23-23: LGTM! Import addresses previous feedback.The TableScroll import is correctly added and addresses the previous review suggestion.
128-128: Good replacement with TableScroll for improved scrolling UX.Using the TableScroll component instead of a custom div wrapper provides consistent scrolling behavior and better responsive design support.
Also applies to: 178-178
| <Table.Row.Base {root}> | ||
| <Table.Cell column="role" {root}> | ||
| <Row {role} /> | ||
| <Row {role} on:notFound={() => deleteRole(role)} /> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that notFound only fires for genuinely missing roles, not transient errors.
The automatic deletion via the notFound event handler is a good UX improvement for cleaning up stale permissions. However, ensure that the Row component's verifyExistence logic properly distinguishes between a role that genuinely doesn't exist and temporary errors (network failures, API timeouts, etc.) to prevent unexpected deletions.
Run the following script to examine the Row component's error handling:
🏁 Script executed:
#!/bin/bash
# Description: Check Row component's notFound dispatch logic and error handling
# Find and display the Row component implementation
fd -t f "row.svelte" src/lib/components/permissions --exec cat {}
# Search for getData implementation and error handling patterns
rg -A 10 -B 2 "getData|notFound|verifyExistence" src/lib/components/permissions/row.svelteLength of output: 18150
notFound event does not distinguish between missing roles and transient errors—fix required.
The Row component's error handling is problematic. In fetchPermissionData, all errors (network failures, timeouts, API errors) are caught and treated identically as notFound: true, making them indistinguishable from genuine 404s. This causes any transient API failure during verifyExistence() to trigger automatic deletion via the notFound event.
Examples that would incorrectly delete roles:
- Network timeout or temporary API unavailability
- Rate limiting (429)
- Server errors (5xx)
The SDK calls need error handling that distinguishes between:
- HTTP 404 → dispatch
notFound(genuine deletion) - Other errors → log and retry, or surface error to user without automatic deletion
🤖 Prompt for AI Agents
In src/lib/components/permissions/permissions.svelte around line 143, the Row
on:notFound handler is triggered by fetchPermissionData treating all errors as
notFound, causing transient network/API errors to delete roles; update
fetchPermissionData/verifyExistence to differentiate errors so only HTTP 404
dispatches notFound, while other errors (network timeouts, 429, 5xx, SDK/network
exceptions) are logged/surfaced and retried or emit a distinct error event
(e.g., dispatch('error', ...)) instead of notFound; ensure Row listens for that
error event and does not call deleteRole on non-404 errors, and add limited
retry/backoff or surface UI message for transient failures.
…ub.com/appwrite/console into fix-SER-414-more-permissions-ui-issues
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/permissions/row.svelte (1)
76-102: Critical: All API errors treated as "not found."The
catchblocks at lines 87 and 97 returnnotFound: truefor every error, making network timeouts, 5xx responses, and rate limits indistinguishable from genuine 404s. This causes transient failures to trigger automatic role deletion.Fix: Inspect the error object to check for a 404 status code. Only return
notFound: truefor actual 404 responses; for other errors, either log and rethrow, or return a distinct error state that does not invokeonNotFound.Apply this pattern to distinguish errors:
if (parsed.type === 'user') { try { return await sdk .forProject(page.params.region, page.params.project) .users.get({ userId: parsed.id }); } catch (error) { - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + // Check if error is a 404 (adjust based on SDK error structure) + if (error?.code === 404 || error?.status === 404) { + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + } + console.error('Failed to fetch user permission data:', parsed.id, error); + throw error; // Rethrow non-404 errors } } if (parsed.type === 'team') { try { return await sdk .forProject(page.params.region, page.params.project) .teams.get({ teamId: parsed.id }); } catch (error) { - return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + if (error?.code === 404 || error?.status === 404) { + return { notFound: true, roleName: parsed.roleName, customName: parsed.id }; + } + console.error('Failed to fetch team permission data:', parsed.id, error); + throw error; } }
♻️ Duplicate comments (2)
src/lib/components/permissions/permissions.svelte (1)
143-143: Duplicate concern: Transient errors still trigger automatic deletion.The callback pattern is an improvement over the deprecated dispatch, but the underlying critical issue persists: the Row component's
verifyExistencelogic (lines 115-122 in row.svelte) treats all errors—network failures, timeouts, rate limits, 5xx—identically asnotFound: true. This causesdeleteRoleto fire for transient API failures, not just genuinely missing roles.src/lib/components/permissions/row.svelte (1)
115-126: Duplicate concern: Empty catch block and inadequate error handling.This duplicates the previously flagged issues:
- The empty
catchblock silently suppresses all errors, making debugging impossible.- More critically,
fetchPermissionData(lines 76-102) treats all errors—network failures, timeouts, 5xx, rate limiting—identically asnotFound: true. This causes transient API failures to trigger automatic role deletion via theonNotFoundcallback.- Each row mounting fires a separate
getDatacall, causing N simultaneous API requests.Required fix: In
fetchPermissionData, inspect the error object or HTTP status to distinguish genuine 404s from other failures. Only returnnotFound: truefor actual 404 responses; log or surface other errors without invokingonNotFound.Check whether the Appwrite SDK exposes error types or status codes that can distinguish 404s:
#!/bin/bash # Search for SDK error handling patterns and status code inspection rg -nP -A5 -B2 '\bAppwriteException\b|\berror\.code\b|\berror\.status\b|\b404\b' --type ts -g '!node_modules/**'
🧹 Nitpick comments (3)
src/lib/components/permissions/permissions.svelte (1)
128-178: LGTM: TableScroll standardizes the table wrapper.The refactor to use
TableScrollfrom the design system library improves consistency and eliminates custom scrollbar styling.src/lib/components/permissions/row.svelte (2)
298-312: LGTM: Caption typography improves visual hierarchy.Switching email and phone details to
Typography.Captionproperly establishes them as secondary metadata within the tooltip.
179-179: LGTM: Minor formatting adjustment.The increased width (18 → 20 characters) reduces truncation for slightly longer names on larger viewports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/permissions/permissions.svelte(4 hunks)src/lib/components/permissions/row.svelte(5 hunks)
🧰 Additional context used
🪛 ESLint
src/lib/components/permissions/row.svelte
[error] 121-121: Empty block statement.
(no-empty)
⏰ 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 (1)
src/lib/components/permissions/row.svelte (1)
37-40: LGTM: Callback pattern replaces deprecated dispatch.The optional
onNotFoundcallback prop correctly replaces the deprecated event dispatcher, as suggested in past reviews.
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
Bug Fixes