Skip to content

Conversation

ArnabChatterjee20k
Copy link
Contributor

@ArnabChatterjee20k ArnabChatterjee20k commented Sep 17, 2025

…, point, and polygon components

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

  • Bug Fixes
    • Toggling Required now only clears/hides the default value when enabling Required. Disabling Required no longer auto-restores or alters the default, preventing unintended changes.
    • For point columns, the “Default” toggle now accurately reflects whether a default value is set, staying in sync with the actual default state.

Copy link

appwrite bot commented Sep 17, 2025

Console

Project ID: 688b7bf400350cbd60e9

Sites (2)
Site Status Logs Preview QR
 console-stage
688b7cf6003b1842c9dc
Ready Ready View Logs Preview URL QR Code
 console-cloud
688b7c18002b9b871a8f
Ready Ready View Logs Preview URL QR Code

Note

Cursor pagination performs better than offset pagination when loading further pages.

Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

Updates in three Svelte components adjust reactive handling of the Required flag for spatial columns. In line.svelte and polygon.svelte, handleDefaultState is now invoked only when $required is true, preventing automatic default restoration when $required becomes false. In point.svelte, the same conditional invocation is applied, and a new reactive effect synchronizes defaultChecked with whether data.default is non-null. No exported/public APIs were changed.

Possibly related PRs

  • Spatial type attributes #2338: Introduces spatial type attributes and related default-handling functions used by point.svelte, line.svelte, and polygon.svelte; this PR modifies that same reactive default-state logic.

Suggested reviewers

  • HarshMN2345
  • ItzNotABug

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely and accurately summarizes the primary change—a refactor to handleDefaultState logic for required fields—and aligns with the modifications in the line, point, and polygon components shown in the diff.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-spatial-col-fix

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]/columns/polygon.svelte (1)

83-92: Bug: deleting breaks ring closure and can throw on undefined ring.

ring may be undefined; current code removes the closing coordinate (last index) and doesn’t re-close the ring, yielding invalid polygons. Fix by guarding, removing the last real point, and re-closing; drop the ring if it would fall below the minimal 4-point ring.

-    function deleteCoordinate(ringIndex: number) {
-        const ring = data.default?.at(ringIndex);
-
-        ring.splice(ring.length - 1, 1);
-        if (ring.length === 0) {
-            data.default.splice(ringIndex, 1);
-        }
-
-        data.default = [...(data.default || [])];
-    }
+    function deleteCoordinate(ringIndex: number) {
+        const ring = data.default?.at(ringIndex);
+        if (!ring) return;
+
+        // Minimal valid ring: 4 points with last equal to first
+        if (ring.length <= 4) {
+            data.default?.splice(ringIndex, 1);
+        } else {
+            // Remove last real point (second-to-last index) and re-close the ring
+            ring.splice(ring.length - 2, 1);
+            ring[ring.length - 1] = [...ring[0]];
+        }
+        data.default = [...(data.default || [])];
+    }
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte (1)

87-92: Call on required=true only: good change; add defaultChecked sync to avoid UI drift on programmatic changes.

If $required flips true from upstream, data.default is cleared but defaultChecked may stay true. Mirror point.svelte by syncing defaultChecked from data.default.

     $effect(() => {
         data.required = $required;
         if ($required) {
             handleDefaultState(true);
         }
     });
+
+    // Keep the toggle in sync with actual default presence
+    $effect(() => {
+        defaultChecked = data.default !== null;
+    });
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte (1)

98-103: Add defaultChecked sync for consistency with point.svelte.

Keep the Default toggle reflecting actual data.default when $required changes programmatically.

     $effect(() => {
         data.required = $required;
         if ($required) {
             handleDefaultState(true);
         }
     });
+
+    $effect(() => {
+        defaultChecked = data.default !== null;
+    });
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/point.svelte (1)

80-82: Nice: reactive sync of defaultChecked.

Prevents UI drift; please replicate this in line/polygon for consistency.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e45f35a and f481358.

📒 Files selected for processing (3)
  • src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte (1 hunks)
  • src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/point.svelte (1 hunks)
  • src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte (1 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 (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte (2)

100-102: Call on required=true only: good change.

Prevents unintended restoration when Required is unchecked. Matches intent of the PR.


111-114: Redundant else branch — remove or align across components.

File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte (lines 111–114)

    on:change={(e) => {
        if (e.detail) defaultChecked = false;
        else data.default = null;
    }}

When Required toggles off this handler nulls data.default; Line/Point do not. Either remove the else branch for neutrality or implement the same nulling behavior in the Line and Point components so behavior is consistent.

src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/point.svelte (1)

75-78: Call on required=true only: good change.

Matches desired behavior; no restoration when Required is unchecked.

src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte (1)

100-103: Minor parity nit: align Required on-change behavior with polygon/point.

When Required toggles off, also set data.default = null (like polygon) so all three components behave identically.

File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte (around lines 100–103). Automated search failed (rg/grep error); unable to confirm handlers—manually verify and apply the change above.

@abnegate abnegate merged commit 3e0d1da into main Sep 18, 2025
5 checks passed
@abnegate abnegate deleted the update-spatial-col-fix branch September 18, 2025 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants