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 |
WalkthroughThe PR refactors preference management in two Svelte components (team and user). Tuple-based prefs ([key, value][]) are replaced by a PrefRow type with ids and explicit key/value fields. A normalize function and deep-equality checks are introduced to compare current vs original prefs. onMount initializes PrefRow arrays (ensuring at least one empty row). updatePrefs sanitizes rows (removing empty entries) and produces an object or {} when empty. UI bindings switch from 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.
There was a problem hiding this comment.
index is fine. we're not reordering the list, just adding/deleting. the bug is the bind:value on tuple indices and not on loop keys 👍
…dashboard-user-team-prefs-deletion
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/routes/`(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte:
- Line 32: The variable prefs is declared as PrefRow[] but initialized to null,
causing a type mismatch; either change the declaration to allow null (use a
nullable type for prefs) or initialize prefs to an empty array so it matches
PrefRow[]; update the declaration where prefs is defined in updatePrefs.svelte
(symbol: prefs, type: PrefRow[]) to one of these fixes and adjust any downstream
checks that rely on null vs empty-array semantics accordingly.
In
`@src/routes/`(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte:
- Line 32: The declaration let prefs: PrefRow[] = null; causes a TypeScript type
mismatch because null is not assignable to PrefRow[]; either initialize prefs to
an empty array (e.g., set initial value to []) or broaden the type to allow null
by changing the annotation to PrefRow[] | null; update any downstream code that
checks for null accordingly (look for the prefs variable in this module and any
functions that read it) so the chosen approach is consistent across usages.
♻️ Duplicate comments (1)
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte (1)
110-110: Whitespace validation inconsistency (same as user preferences).The trim validation inconsistency flagged in past review comments still applies. Apply
trim()to the disabled checks for consistency withupdatePrefssanitization logic.Also applies to: 124-128
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte (1)
109-109: Whitespace validation inconsistency between UI and save logic.The
disabledchecks on lines 109 and 123-127 use rawpref.key/pref.valuetruthiness, whileupdatePrefs(lines 48-50) usestrim(). This allows whitespace-only entries to appear valid in the UI but get filtered on save.For consistency, apply
trim()in the disabled checks:♻️ Suggested fix
<Button icon compact - disabled={(!pref.key || !pref.value) && index === 0} + disabled={(!pref.key.trim() || !pref.value.trim()) && index === 0} on:click={() => {disabled={ !!prefs && prefs.length > 0 && - !(prefs[prefs.length - 1].key && prefs[prefs.length - 1].value) + !(prefs[prefs.length - 1].key.trim() && prefs[prefs.length - 1].value.trim()) }Also applies to: 123-127
| } | ||
| let prefs: [string, string][] = null; | ||
| let prefs: PrefRow[] = null; |
There was a problem hiding this comment.
Type annotation mismatch: null is not assignable to PrefRow[].
Same issue as the user preferences file. The variable is typed as PrefRow[] but initialized with null.
🛠️ Suggested fix
- let prefs: PrefRow[] = null;
+ let prefs: PrefRow[] | null = null;📝 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.
| let prefs: PrefRow[] = null; | |
| let prefs: PrefRow[] | null = null; |
🤖 Prompt for AI Agents
In
`@src/routes/`(console)/project-[region]-[project]/auth/teams/team-[team]/updatePrefs.svelte
at line 32, The variable prefs is declared as PrefRow[] but initialized to null,
causing a type mismatch; either change the declaration to allow null (use a
nullable type for prefs) or initialize prefs to an empty array so it matches
PrefRow[]; update the declaration where prefs is defined in updatePrefs.svelte
(symbol: prefs, type: PrefRow[]) to one of these fixes and adjust any downstream
checks that rely on null vs empty-array semantics accordingly.
| } | ||
| let prefs: [string, string][] = null; | ||
| let prefs: PrefRow[] = null; |
There was a problem hiding this comment.
Type annotation mismatch: null is not assignable to PrefRow[].
The variable is typed as PrefRow[] but initialized with null. This is a TypeScript type mismatch.
🛠️ Suggested fix
- let prefs: PrefRow[] = null;
+ let prefs: PrefRow[] | null = null;📝 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.
| let prefs: PrefRow[] = null; | |
| let prefs: PrefRow[] | null = null; |
🤖 Prompt for AI Agents
In
`@src/routes/`(console)/project-[region]-[project]/auth/user-[user]/updatePrefs.svelte
at line 32, The declaration let prefs: PrefRow[] = null; causes a TypeScript
type mismatch because null is not assignable to PrefRow[]; either initialize
prefs to an empty array (e.g., set initial value to []) or broaden the type to
allow null by changing the annotation to PrefRow[] | null; update any downstream
code that checks for null accordingly (look for the prefs variable in this
module and any functions that read it) so the chosen approach is consistent
across usages.
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.