Fix: data not persisting, options not loading. #2390
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughThis PR makes coordinated Svelte UI changes: columnForm.svelte stops exporting props and sources inputs via Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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: 2
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/create.svelte (1)
46-52: Type: id is assigned null but declared as optional string.CreateRow.id?: string is later initialized with null (Line 50). Either allow null in the type or initialize as undefined to keep TS happy.
- type CreateRow = { - id?: string; + type CreateRow = { + id?: string | null;src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/columnItem.svelte (1)
63-63: Unsubscribe the manual store subscription to avoid leaks.formStore.subscribe(...) isn’t auto‑unsubscribed. Register onDestroy to clean up.
+ import { onDestroy } from 'svelte'; … - formStore.subscribe((values) => onUpdateFormValues?.(values)); + const unsubscribe = formStore.subscribe((values) => onUpdateFormValues?.(values)); + onDestroy(unsubscribe);src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (1)
90-91: Option load size seems too low.Query.limit(2) severely limits initial options and may feel broken. Consider 10–25 for better UX.
- queries: [Query.select(displayNames), Query.limit(2)] + queries: [Query.select(displayNames), Query.limit(10)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/columnForm.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/columnItem.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/create.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(2 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 (10)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/columnItem.svelte (2)
31-34: In-place store updates fix array reset bug.The switch to formStore.update with targeted key mutation should prevent list resets while preserving reactivity.
Also applies to: 39-42
57-58: Explicit default for unknown format.Returning 'String' reads clearer than implicit fallthrough. LGTM.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (2)
7-7: Import alignment with new pagination control.InputSelect is correctly sourced from $lib/elements/forms and used in the footer pagination. No issues spotted.
1089-1095: Confirm: Confirm.svelte exports confirmDeletion and confirmDeletionLabelRepo search returned no matches — ensure Confirm.svelte exports
export let confirmDeletionandexport let confirmDeletionLabel, or update spreadsheet.svelte to use the actual prop names.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (3)
73-74: Initialize single-rel selection consistently.Setting both newItemValue and singleRel to row.$id ensures Select/ComboBox stay in sync on mount. LGTM.
372-392: Keyed ComboBox to refresh options state is a good call.Using {#key availableOptions} prevents stale option lists and fixes “options not loading” edge cases.
200-231: noResultsOption shape: confirm Input.Select/ComboBox APIsearchNoResultsOption is set to { disabled: true, message: 'Searching...' } but Select/ComboBox options elsewhere use { label, value, disabled }. Input.Select/ComboBox come from @appwrite.io/pink-svelte (not defined in this repo), so verify upstream accepts a "message" key — otherwise change to { label: 'Searching...', value: null, disabled: true }.
Location: src/routes/(console)/.../relationship.svelte — set ≈ lines 200–231; used ≈ lines 382, 418, 444.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/columnForm.svelte (2)
8-16: $props/$bindable migration looks correct.Inputs now flow through internal bindings; matches how the parent binds in create.svelte. No issues.
18-18: Local state with $state is fine.showCustomId via $state(false) is consistent with runes usage elsewhere.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/create.svelte (1)
113-114: Approve: Dependencies.ROWS is the correct invalidation target for create.Confirmed: Dependencies.ROWS exists and is used for create/delete (list refresh); single-row updates use Dependencies.ROW — this is consistent across the codebase. Examples: src/routes/.../rows/create.svelte:113 (create → invalidate(Dependencies.ROWS)), src/routes/.../table-[table]/spreadsheet.svelte:415 (delete → invalidate(Dependencies.ROWS)), src/routes/.../rows/edit.svelte:131 (update → invalidate(Dependencies.ROW)), src/routes/.../table-[table]/+page.ts:12-13 (depends on ROW and ROWS).
| {#key options} | ||
| {#if limited} | ||
| <!-- for unlink badge --> | ||
| <Input.Select | ||
| {id} | ||
| {options} | ||
| autofocus={limited} | ||
| bind:value={newItemValue} | ||
| required={column.required} | ||
| label={limited ? undefined : label} | ||
| placeholder={`Select ${column.key}`} | ||
| noResultsOption={searchNoResultsOption} | ||
| on:change={() => { | ||
| if (newItemValue === null) { | ||
| value = null; | ||
| singleRel = null; | ||
| } else { | ||
| const selectedRow = rowList.rows.find( | ||
| (row) => row.$id === newItemValue | ||
| ); | ||
|
|
||
| if (selectedRow) { | ||
| value = selectedRow; | ||
| singleRel = newItemValue; | ||
| } | ||
| } | ||
| }} | ||
| leadingIcon={!limited ? IconRelationship : undefined} /> | ||
| {:else} | ||
| <Input.ComboBox | ||
| {id} | ||
| {options} | ||
| autofocus={limited} | ||
| bind:value={newItemValue} | ||
| required={column.required} | ||
| label={limited ? undefined : label} | ||
| placeholder={`Select ${column.key}`} | ||
| noResultsOption={searchNoResultsOption} | ||
| on:change={() => { | ||
| if (newItemValue === null) { | ||
| value = null; | ||
| singleRel = null; | ||
| } else { | ||
| const selectedRow = rowList.rows.find( | ||
| (row) => row.$id === newItemValue | ||
| ); | ||
|
|
||
| if (selectedRow) { | ||
| value = selectedRow; | ||
| singleRel = newItemValue; | ||
| } | ||
| } | ||
| }} | ||
| leadingIcon={!limited ? IconRelationship : undefined} /> | ||
| {/if} |
There was a problem hiding this comment.
Type mismatches: newItemValue/singleRel/value can be null.
Code assigns null to newItemValue, singleRel, and value, but their types exclude null. This will trip TS and tooling.
- let singleRel: string;
+ let singleRel: string | null = null;
- let newItemValue: string = '';
+ let newItemValue: string | null = '';
- export let value: object | string[];
+ export let value: object | string[] | null;Also guard against rowList being undefined when resolving selectedRow to avoid rare NPEs during slow loads:
- const selectedRow = rowList.rows.find(
+ const selectedRow = rowList?.rows?.find(
(row) => row.$id === newItemValue
);Committable suggestion skipped: line range outside the PR's diff.
...[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte
Show resolved
Hide resolved
fix: query limit on fetching relationship items. fix: show no items when load returns 0 items for relationships.
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 (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (2)
246-246: Undeclared reactive vars: declareoptions(and peers) to satisfy TS
$: options = …assigns to an undeclared identifier; TS/Svelte will error andoptions.lengthis used later. Declare/initialize these.Apply:
+let options: SelectOption[] = []; +let paginatedItems: string[] = []; +let totalCount = 0; +let hasItems = false; +let showTopAddButton = false; +let showBottomAddButton = false; +let showEmptyInput = false;
258-260:label = undefinedconflicts withexport let label: stringWiden the prop type to allow undefined.
-export let label: string; +export let label: string | undefined;
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (2)
469-482: Nulls assigned to non‑nullable vars (singleRel,newItemValue,value)Handlers set
value = nullandnewItemValue = singleRel = null, but current types exclude null. This will trip TS and tooling.Update declarations:
-export let value: object | string[]; +export let value: Models.Row | string[] | null; -let singleRel: string; +let singleRel: string | null = null; -let newItemValue: string = ''; +let newItemValue: string | null = null;Additionally, ensure callers expect
valueto benullwhen unlinked.
411-467: Possible NPE:rowList.rowsaccessed before load
rowListmay be undefined when the user changes selection quickly;rowList.rows.find(...)will crash.- const selectedRow = rowList.rows.find( + const selectedRow = rowList?.rows?.find( (row) => row.$id === newItemValue );Repeat the same change in the non‑limited ComboBox handler below.
- const selectedRow = rowList.rows.find( + const selectedRow = rowList?.rows?.find( (row) => row.$id === newItemValue );
🧹 Nitpick comments (6)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (2)
159-159: Simplify/clarify nested ternary for emptyCells.Readability nit. Parenthesize and flip the last ternary for clarity.
-const emptyCells = $derived(($isSmallViewport ? 14 : $isTabletViewport ? 17 : 24) + (!$expandTabs ? 2 : 0)); +const emptyCells = $derived( + ($isSmallViewport ? 14 : ($isTabletViewport ? 17 : 24)) + + ($expandTabs ? 0 : 2) +);
65-74: Guard against negative overlay height; initialize on mount.If the header sits below the viewport bottom,
dynamicHeightcan go negative; also, the first value is only set after a resize. Clamp and initialize once on mount.- const dynamicHeight = viewportHeight - headerRect.bottom; + const dynamicHeight = Math.max(0, viewportHeight - headerRect.bottom); @@ - onMount(async () => { + onMount(async () => { if (spreadsheetContainer) { resizeObserver = new ResizeObserver(debouncedUpdateOverlayHeight); resizeObserver.observe(spreadsheetContainer); } + // Initialize once; ResizeObserver may not fire immediately on all browsers. + updateOverlayHeight(); });Also applies to: 80-85
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (4)
90-90: Reasonable: increase initial fetch size to 5Improves UX without overloading the sheet. Consider extracting to a constant if reused.
349-352: Bind to the source array element (clearer and more explicit)Binding directly to
relatedList[i]avoids ambiguity with the loop alias.- bind:value={item} + bind:value={relatedList[i]}
411-439: Limited mode unlink option + requiredWhen
column.requiredis true, selecting the injectedUnlink(value: null) conflicts with required. Ensure upstream validation allows null here or gate adding the unlink option behind!column.required.- if (!isRelationshipToMany(column) && baseOptions.length && value && limited) { + if (!isRelationshipToMany(column) && baseOptions.length && value && limited && !column.required) {
200-231: Optional: cancel pending debounced searches on unmountPrevents late updates after navigation.
-import { type ComponentType, onMount } from 'svelte'; +import { type ComponentType, onMount, onDestroy } from 'svelte'; @@ }, 500); + onDestroy(() => (debouncedSearch as any)?.cancel?.());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/console#2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
🔇 Additional comments (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (2)
11-11: Viewport stores: confirm mutual exclusivity (small vs tablet).The ternary at Line 159 assumes
isSmallViewportandisTabletViewportdon’t overlap. Please confirm the store definitions enforce exclusivity; otherwise tablet will never be selected when both are true.
338-345: Align JS viewport logic with CSS breakpoints.CSS uses 768/1024/1280/1440px thresholds; ensure
isSmallViewport/isTabletViewportmatch these to avoid JS/CSS divergence in layout.Also applies to: 362-389
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (2)
72-74: Good: single‑relation state is initialized from provided rowSetting both
newItemValueandsingleReltorow.$idkeeps Select/ComboBox in sync. Ensure their types accept null elsewhere (see separate comment).
441-451: Guard when no options available
{@const noOptions = options.length <= 0}assumesoptionsis defined. Declaringlet options: SelectOption[] = [](see above) resolves this. Also good call disabling the input whennoOptions.Please confirm
optionsis declared as suggested so this expression never dereferencesundefined.
...[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte
Show resolved
Hide resolved
update: rabbit's comment.
There was a problem hiding this comment.
Actionable comments posted: 2
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/columns/types/relationship.svelte (1)
345-353: Creation mode: binding to loop variable ‘item’ doesn’t persist back to relatedListBinding to the each-loop variable won’t update the backing array, so selections won’t persist. Bind to the indexed array slot.
- bind:value={item} + bind:value={relatedList[i]}
♻️ Duplicate comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (3)
26-26: Nullability mismatch: value/singleRel/newItemValue assigned null but typed non-nullable — fix typesCurrently these variables are declared as non-nullable but multiple branches assign null, causing TS errors and fragile runtime assumptions. Make them nullable.
Apply:
-export let value: object | string[]; +export let value: object | string[] | null; -let singleRel: string; +let singleRel: string | null = null; -let newItemValue: string = ''; +let newItemValue: string | null = null;With this change, the null assignments in add/cancel/unlink handlers (Lines 144, 150, 425–439, 453–466, 470–483) become type-safe.
Also applies to: 36-36, 39-39, 140-147, 149-152, 425-439, 453-466, 470-483
115-123: getAvailableOptions(): always return an array and narrow option.value before includes()Optional-chaining on source after nullish-coalescing is redundant, and passing string|null to string[].includes() is a type error. Also ensure we never return undefined.
- function getAvailableOptions(excludeIndex?: number): SelectOption[] { - const source = options ?? []; - return source?.filter((option) => { - const otherItems = - excludeIndex !== undefined - ? relatedList.filter((_, idx) => idx !== excludeIndex) - : relatedList; - - return !otherItems.includes(option.value); - }); - } + function getAvailableOptions(excludeIndex?: number): SelectOption[] { + const source = options ?? []; + const otherItems = + excludeIndex !== undefined + ? relatedList.filter((_, idx) => idx !== excludeIndex) + : relatedList; + return source.filter(({ value }) => + typeof value !== 'string' ? true : !otherItems.includes(value) + ); + }
425-439: Guard rowList access in on:change handlers to avoid NPE during slow loadsrowList can be undefined while users interact; accessing rowList.rows.find can crash.
- const selectedRow = rowList.rows.find( + const selectedRow = rowList?.rows?.find( (row) => row.$id === newItemValue );Do this in both handlers (Lines 429 and 457).
Also applies to: 453-466
🧹 Nitpick comments (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (4)
371-397: Keyed block keyed by array object causes excessive remounts; key by a stable signatureKeying by the array reference from getAvailableOptions() remounts on every recompute, risking flicker/state loss. Key by a stable string derived from values.
- {@const availableOptions = getAvailableOptions()} - {@const noAvailableOptions = availableOptions.length <= 0} - {#key availableOptions} + {@const availableOptions = getAvailableOptions()} + {@const noAvailableOptions = availableOptions.length <= 0} + {@const availableKey = availableOptions.map(({ value }) => value ?? '').join('|')} + {#key availableKey}
412-483: Same keyed-block issue on single-relation path; key by a stable signatureKeying by options (array identity) remounts too often.
- {#key options} + {@const optionsKey = options.map(({ value }) => value ?? '').join('|')} + {#key optionsKey}Also covered by the nullability and rowList guards above.
78-97: getRows(): ensure loadingRelationships resets on errorIf the request throws, loadingRelationships stays true → spinner stuck.
- async function getRows() { - loadingRelationships = true; - // already includes the `$id`, dw! - const displayNames = preferences.getDisplayNames(column.relatedTable); - const rows = await sdk - .forProject(page.params.region, page.params.project) - .tablesDB.listRows({ - databaseId, - tableId: column.relatedTable, - // limit `5` as `25` would look too much on sheet! - queries: [Query.select(displayNames), Query.limit(5)] - }); - - cachedRowsCopyList = rows; - loadingRelationships = false; - - return rows; - } + async function getRows() { + loadingRelationships = true; + try { + const displayNames = preferences.getDisplayNames(column.relatedTable); + const rows = await sdk + .forProject(page.params.region, page.params.project) + .tablesDB.listRows({ + databaseId, + tableId: column.relatedTable, + // limit `5` as `25` would look too much on sheet! + queries: [Query.select(displayNames), Query.limit(5)] + }); + cachedRowsCopyList = rows; + return rows; + } finally { + loadingRelationships = false; + } + }
99-104: Type the search parameter as nullableDefaulting a string parameter to null violates its declared type.
- async function searchRows(search: string = null) { + async function searchRows(search: string | null = null) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/create.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/create.svelte
⏰ 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 (5)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (1)
11-11: Tablet viewport import: LGTMConsistent with spreadsheet.svelte; keeps responsive logic unified across views.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (3)
7-7: Pagination Input import: LGTMInputSelect usage in the footer matches this import.
85-85: Tablet viewport import: LGTMNeeded for the new responsive thresholds.
1095-1101: Confirm API usage: verify prop support and UX copyPassing
confirmDeletionandconfirmDeletionLabellooks good, but please confirm the updated Confirm component exposes these props and renders the label as intended (and is localized if applicable). Ensure undefinedconfirmDeletionLabelgracefully omits the extra label.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (1)
90-91: Raising listRows limit to 5 — LGTMThis is a sensible UX tweak for related items in the sheet.
...ject-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte
Show resolved
Hide resolved
...e)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte
Show resolved
Hide resolved
2 participants
What does this PR do?
Adding array items and then selecting some values would reset the array list.
Test Plan
Manual.
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores