fix(dashboard): correct user/team prefs deletion and binding issues #2772
Conversation
Console (appwrite/console)Project ID: Sites (1)
Tip Every Git commit and branch gets its own deployment URL automatically |
WalkthroughThese changes refactor preference management across two Svelte components. The existing tuple-based preference arrays ([key, value][]) are replaced with a typed PrefRow structure that includes unique identifiers. The refactor introduces normalization logic for comparing preferences, replaces bind:value directives with explicit on:input event handlers for better reactivity control, updates the updatePrefs function to sanitize input and convert results to objects, and transitions array mutations to immutable patterns. Similar structural changes are applied to both team and user preference components. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte (1)
120-135: Minor: trim validation inconsistency.The add button validation checks
prefs[prefs.length - 1].key && prefs[prefs.length - 1].valuewithout trimming, but the updatePrefs function usespref.key.trim() !== ''for sanitization. This means a user could enter whitespace-only values, the add button would enable, but those preferences would be filtered out on save, potentially causing confusion.🔧 Suggested fix to align validation
disabled={prefs?.length && - prefs[prefs.length - 1].key && - prefs[prefs.length - 1].value + prefs[prefs.length - 1].key.trim() && + prefs[prefs.length - 1].value.trim() ? false : true} on:click={() => { - if (prefs[prefs.length - 1].key && prefs[prefs.length - 1].value) { + if (prefs[prefs.length - 1].key.trim() && prefs[prefs.length - 1].value.trim()) { prefs = [...prefs, createPrefRow()]; } }}>
🤖 Fix all issues with AI agents
In
@src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte:
- Around line 123-131: The add-button validation uses raw prefs[prefs.length -
1].key/value so whitespace-only entries slip through; update both the disabled
expression and the on:click check to trim() the latest pref's key and value
before validating (e.g., replace prefs[prefs.length - 1].key and .value with
prefs[prefs.length - 1].key.trim() and prefs[prefs.length - 1].value.trim()),
and keep the existing createPrefRow() push logic unchanged so empty-but-nonblank
whitespace entries are rejected consistently like in updatePrefs.
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte (1)
82-117: Consider a more immutable update pattern.The current approach mutates the PrefRow object and then reassigns the array with spread syntax. While this works in Svelte 5, a more idiomatic pattern would create new objects:
♻️ More idiomatic immutable pattern
on:input={(e) => { - prefs[index].key = e.target.value; - prefs = [...prefs]; + prefs = prefs.map((p, i) => + i === index ? { ...p, key: e.target.value } : p + ); }}Similarly for value updates:
on:input={(e) => { - prefs[index].value = e.target.value; - prefs = [...prefs]; + prefs = prefs.map((p, i) => + i === index ? { ...p, value: e.target.value } : p + ); }}This avoids in-place mutation entirely, though the current pattern is functionally correct.
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte (1)
83-117: Optional: Consider immutable update pattern (same as user preferences).The same mutation + reassignment pattern is used here. While functionally correct, consider the more idiomatic immutable pattern suggested in the user preferences review for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
src/routes/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use SvelteKit file conventions: +page.svelte for components, +page.ts for data loaders, +layout.svelte for wrappers, +error.svelte for error handling, and dynamic route params in square brackets like [param]
Files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Configure dynamic routes using SvelteKit convention with [param] syntax in route directory names
Files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
🧠 Learnings (1)
📚 Learning: 2025-12-05T09:24:15.846Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2670
File: src/routes/(console)/organization-[organization]/+layout.ts:134-150
Timestamp: 2025-12-05T09:24:15.846Z
Learning: In the Appwrite console codebase, `prefs.organization` is maintained to always reference an organization with Platform.Appwrite, so when using preferences.organization as a fallback in redirect logic, no additional platform validation is required.
Applied to files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
🔇 Additional comments (5)
src/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte (4)
15-28: LGTM! Robust normalization and comparison logic.The PrefRow type definition and normalize function are well-designed. The function correctly handles both legacy tuple arrays and the new PrefRow structure, filters empty entries using trim, sorts for consistent comparison, and uses JSON.stringify for deep equality checking. This approach effectively detects when preferences have changed and enables/disables the Update button accordingly.
30-43: LGTM! Clean initialization pattern.The state initialization and createPrefRow helper are well-structured. Using an incremental counter for unique IDs provides stable keys for Svelte's {#each} blocks, which prevents binding issues. The onMount logic correctly initializes from existing preferences or starts with a single empty row for a better UX.
45-73: LGTM! Proper sanitization and error handling.The updatePrefs function correctly sanitizes preferences by filtering out empty entries and explicitly sends
{}when no preferences remain, which addresses the core issue in #1458. The error handling and cache invalidation are properly implemented.
108-108: Good UX: preserving at least one row.The disabled logic
(!pref.key || !pref.value) && index === 0ensures users can always delete non-empty rows and prevents deletion of the first row when it's empty, maintaining at least one input row for better UX.src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte (1)
1-143: LGTM! Excellent consistency with user preferences component.The team preferences component mirrors the user preferences implementation with identical logic for PrefRow management, normalization, sanitization, and UI interactions. This consistency makes the codebase more maintainable and ensures users have the same experience when managing team vs. user preferences.
| disabled={prefs?.length && | ||
| prefs[prefs.length - 1][0] && | ||
| prefs[prefs.length - 1][1] | ||
| prefs[prefs.length - 1].key && | ||
| prefs[prefs.length - 1].value | ||
| ? false | ||
| : true} | ||
| on:click={() => { | ||
| if (prefs[prefs.length - 1][0] && prefs[prefs.length - 1][1]) { | ||
| prefs.push(['', '']); | ||
| prefs = prefs; | ||
| if (prefs[prefs.length - 1].key && prefs[prefs.length - 1].value) { | ||
| prefs = [...prefs, createPrefRow()]; | ||
| } |
There was a problem hiding this comment.
Minor: trim validation inconsistency (same as user preferences).
The same whitespace-only validation issue exists here as in the user preferences file. The add button doesn't trim values before validation, but updatePrefs does. Consider applying the same trim() fix suggested in the user preferences review.
🤖 Prompt for AI Agents
In
@src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte
around lines 123 - 131, The add-button validation uses raw prefs[prefs.length -
1].key/value so whitespace-only entries slip through; update both the disabled
expression and the on:click check to trim() the latest pref's key and value
before validating (e.g., replace prefs[prefs.length - 1].key and .value with
prefs[prefs.length - 1].key.trim() and prefs[prefs.length - 1].value.trim()),
and keep the existing createPrefRow() push logic unchanged so empty-but-nonblank
whitespace entries are rejected consistently like in updatePrefs.
| const normalize = (entries: [string, string][] | PrefRow[]) => | ||
| entries | ||
| .map(item => Array.isArray(item) ? item : [item.key, item.value]) | ||
| .filter(([k, v]: [string, string]) => k.trim() && v.trim()) | ||
| .sort(([a]: [string, string], [b]: [string, string]) => a.localeCompare(b)); |
There was a problem hiding this comment.
lets move this out. no need to create a function on every reactive change.
| const currentNormalized = normalize(prefs); | ||
| const originalNormalized = normalize(Object.entries($team.prefs as Record<string, string>)); | ||
| arePrefsDisabled = JSON.stringify(currentNormalized) === JSON.stringify(originalNormalized); |
There was a problem hiding this comment.
we also have a deepEqual utility, see if that helps for better comparison
| let prefs: [string, string][] = null; | ||
| let prefs: PrefRow[] = null; | ||
| let arePrefsDisabled = true; | ||
| let nextId = 0; |
There was a problem hiding this comment.
same here as well, not needed.
| <svelte:fragment slot="aside"> | ||
| {#if prefs} | ||
| {#each prefs as [key, value], index} | ||
| {#each prefs as pref, index (pref.id)} |
There was a problem hiding this comment.
using just the index should be fine for the loop's unique id.
There was a problem hiding this comment.
ok that index keys reduce complexity, but I was initially using stable keys because the original bug was caused by index shifts after deletions and since we’ve now removed bind:value and are using immutable updates, index keys should be safe but I just want to confirm that this tradeoff is acceptable, as it technically weakens DOM stability for delete/reorder scenarios.
If you’re okay with it, I’ll proceed with index keys and remove nextId, otherwise I can keep stable keys to fully avoid that class of bugs.
2 participants
What does this PR do?
This PR fixes incorrect behavior when deleting user/team preferences from the Appwrite Dashboard.
Previously, deleting the first or last preference caused:
Fake deletion by inserting an empty key-value pair (['', ''])
Serialization into { "": "" }, corrupting stored preferences
Update button not enabling correctly
Preferences appearing again after refresh
Ghost bindings due to index-based bind:value in Svelte
This PR resolves the issue by:
Replacing tuple-based prefs with a stable PrefRow { id, key, value } structure
Removing fake deletion logic and deleting preference rows for real
Fixing Svelte binding issues by removing bind:value on array indices
Using immutable updates and stable keys in {#each}
Sanitizing preferences before saving (filtering empty keys/values)
Explicitly sending {} when no preferences remain
Normalizing preference comparison so deletions are detected correctly
No backend or SDK behavior was changed.
Test Plan
Manually verified using the Appwrite Dashboard:
Open a user and add one or more preferences
Delete the first preference → Update button enables correctly
Click Update → refresh page → preferences remain correct
Delete the last remaining preference → click Update → refresh → prefs are empty
Add, edit, and delete multiple preferences in different orders
Verified no { "": "" } payload is sent in API calls
Verified same behavior for team preferences
Tested on Appwrite Cloud Dashboard.
Related PRs and Issues
Fixes:
#1458
Have you read the Contributing Guidelines on issues
?
Yes ✅
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.