Fix: states not updated by ItzNotABug · Pull Request #2698 · appwrite/console

ItzNotABug · 2025-12-11T08:10:35Z

What does this PR do?

(Provide a description of what this PR does.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

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?

(Write your answer here.)

Summary by CodeRabbit

Refactor
- Improved internal state management for database table row editing, permissions modification, and related row operations by refactoring to reactive state patterns, enhancing code consistency and maintainability across the editing interface.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

appwrite · 2025-12-11T08:10:40Z

Console (appwrite/console)

Project ID: 688b7bf400350cbd60e9

Sites (1)

Site	Status	Logs	Preview	QR
console-stage `688b7cf6003b1842c9dc`	Ready	View Logs	Preview URL

Tip

Git integration provides automatic deployments with optional PR comments

coderabbitai · 2025-12-11T08:10:49Z

Walkthrough

This PR refactors disabled state management across four row editing components. The changes replace imperative function-based disable checks (isDisabled(), disableSubmit()) with a declarative reactive prop-and-effect pattern. The main layout component introduces three disabled state flags (editRowDisabled, editRelatedRowDisabled, editRowPermissionsDisabled) that are bound to child components as props. Child components now receive these disabled states and use reactive effects to synchronize them with internal validation conditions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Reactive effect logic validation: Each of the three child components (edit.svelte, editPermissions.svelte, editRelated.svelte) introduces new $effect blocks that need careful review to ensure they correctly replicate the behavior of their removed function counterparts, particularly the symmetricDifference comparison in editPermissions.svelte.
Prop binding wiring: The main layout component binds three disabled state flags to child components via bind:disabled, bind:disabledState, and bind:arePermsDisabled. Verify all bindings are correctly connected and named consistently.
onMount hook patterns: Each child component adds an onMount import and lifecycle hook solely to reference the disabled prop (to silence not-read warnings). Verify this pattern is intentional and follow established project conventions.
State synchronization: Review the reactive effects that synchronize disabled states in editPermissions.svelte and editRelated.svelte to ensure they maintain identical behavior to the original functions they replaced.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix: states not updated' is vague and doesn't clearly describe which states are being fixed or what the actual changes accomplish.	Provide a more specific title that clarifies what states are being fixed and the primary improvement, such as 'Refactor: Convert disabled state checks to reactive flags' or 'Fix: Update disabled state management in row editors'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-disabled-states

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte (1)
309-317: Remove redundant disabled state update.

The disabledState is updated in two places:

Line 314: Manually in handleFormUpdate

Line 342: Reactively in $effect

This creates duplicate work and potential timing issues. The reactive $effect already tracks all necessary dependencies (via workData reactivity when stores are updated), making the manual update redundant.

Remove the manual update from handleFormUpdate:
     function handleFormUpdate(rowId: string) {
         return (updatedFormValues: object) => {
             const workStore = workData.get(rowId);
             if (workStore) {
                 workStore.set(updatedFormValues as Models.Row);
-                disabledState = calculateAndCompareDisabledState();
             }
         };
     }
The $effect at line 341-343 will automatically recalculate when workData stores change, since calculateAndCompareDisabledState() reads from those stores via get(work).

Also applies to: 341-343

🧹 Nitpick comments (4)

src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte (1)
38-41: Remove the onMount anti-pattern.

Using onMount solely to reference a variable to silence compiler warnings is an anti-pattern. If the prop is truly unused, the warning is legitimate. However, disabledState is actually consumed by the parent via $bindable, so the warning is incorrect.

In Svelte 5, bindable props that are only written (not read) by the component can trigger false "not read" warnings. The proper solution is to either:

Add // @ts-expect-error - bindable prop written by reactive effect above the prop declaration, or

Use $inspect(disabledState) in development mode

The current approach adds unnecessary lifecycle overhead.

Remove the onMount block entirely and add a suppression comment:
-    onMount(() => {
-        /* silences the not read error warning */
-        disabledState;
-    });
-
     function isSingleStore() {
Then add a comment on line 24:
     let {
         rows,
         tableId,
+        // Bindable prop - written by reactive effects, consumed by parent
         disabledState = $bindable(true)
     }: {
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editPermissions.svelte (1)
26-29: Remove the onMount anti-pattern.

Same issue as in editRelated.svelte: using onMount solely to silence a compiler warning is an anti-pattern. The arePermsDisabled prop is bindable and consumed by the parent via two-way binding, so it's not actually unused.

Remove the onMount block and add a clarifying comment instead:
-    onMount(() => {
-        /* silences the not read error warning */
-        arePermsDisabled;
-    });
-
     export async function updatePermissions() {
Add a comment on the prop declaration (line 17):
     let {
         row = $bindable(null),
+        // Bindable prop - written by reactive effect, consumed by parent
         arePermsDisabled = $bindable(true)
     }: {
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/edit.svelte (1)
44-47: Remove the onMount anti-pattern.

Same issue as the other components: using onMount solely to reference a bindable prop to silence warnings is an anti-pattern. The disabled prop is written by the reactive effect and consumed by the parent.

Remove the onMount block:
-    onMount(() => {
-        /* silences the not read error warning */
-        disabled;
-    });
-
     function initWork() {
Add a clarifying comment on the prop declaration (line 31):
     let {
         row = $bindable(),
         rowId = $bindable(null),
         autoFocus = true,
+        // Bindable prop - written by reactive effect, consumed by parent
         disabled = $bindable(true)
     }: {
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+layout.svelte (1)
443-444: Verify consistent disabled state bindings across all editors.

The bindings look correct, but ensure all child components properly implement the bindable prop contract:

EditRow expects bind:disabled

EditRelatedRow expects bind:disabledState

EditRowPermissions expects bind:arePermsDisabled

Note the naming inconsistency: disabled vs disabledState vs arePermsDisabled. While this works, it would be more maintainable if all three used the same prop name (e.g., disabled).

Consider standardizing the prop name across all three components for consistency:
// Instead of:
bind:disabled={editRowDisabled}
bind:disabledState={editRelatedRowDisabled}
bind:arePermsDisabled={editRowPermissionsDisabled}

// Use:
bind:disabled={editRowDisabled}
bind:disabled={editRelatedRowDisabled}
bind:disabled={editRowPermissionsDisabled}
This would require renaming disabledState → disabled in editRelated.svelte and arePermsDisabled → disabled in editPermissions.svelte.

Also applies to: 508-509, 519-520, 525-526, 551-552, 554-557

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e26e9bb and 55ad9a6.

📒 Files selected for processing (4)

src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+layout.svelte (5 hunks)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/edit.svelte (2 hunks)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editPermissions.svelte (1 hunks)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte (2 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{ts,tsx,js,jsx,svelte}