Accordions when many relation items in sheet #2352
Conversation
update: add accordion for relationship items on side sheet.
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
WalkthroughAdds a new optional exported prop Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/elements/forms/inputNumber.svelte (1)
8-8: Fix TS types for nullable numbers and error state.Defaults use
nullbut types arenumber/string. Align to avoid TS errors.- export let value: number = null; + export let value: number | null = null; @@ - export let min: number = null; - export let max: number = null; + export let min: number | null = null; + export let max: number | null = null; @@ - let error: string; + let error: string | null = null; @@ - $: if (value !== null && value !== undefined && !Number.isNaN(value)) { + $: if (value !== null && value !== undefined && !Number.isNaN(value)) { error = null; }Also applies to: 15-16, 21-21, 45-46
src/lib/elements/forms/inputDateTime.svelte (1)
7-7: Fix nullable typing and event type to avoid TS errors.
valueanderrorare assignednull, but typed asstring. Also,onChangeshould acceptEvent, notCustomEvent, when readingevent.target.Apply:
-export let value: string; +export let value: string | null; -let error: string; +let error: string | null = null; -function onChange(event: CustomEvent) { +function onChange(event: Event) { value = (event.target as HTMLInputElement).value; }Also applies to: 18-19, 38-44
🧹 Nitpick comments (13)
src/lib/elements/forms/inputPassword.svelte (1)
14-14: Improve autocomplete semantics for passwords.Use
"new-password"or"current-password"instead of generic on/off to help browsers.- export let autocomplete = false; + export let autocomplete: false | 'new-password' | 'current-password' = false; @@ - autocomplete={autocomplete ? 'on' : 'off'} + autocomplete={autocomplete || 'off'}Also applies to: 50-51
src/lib/elements/forms/inputText.svelte (2)
34-34: Remove redundant assignment to error.
erroris set tovalidationMessagetwice. Keep one.- error = inputNode.validationMessage; @@ - error = inputNode.validationMessage; + error = inputNode.validationMessage;(Delete one of the two identical lines.)
Also applies to: 40-40
5-5: Tighten prop types to match defaults.Avoid
string = undefined/nullwith strict TS.- export let label: string = undefined; + export let label: string | undefined = undefined; @@ - export let pattern: string = undefined; //TODO: implement pattern check + export let pattern: string | undefined = undefined; //TODO: implement pattern check @@ - export let error: string = null; + export let error: string | null = null;Also applies to: 10-10, 22-22
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (1)
469-475: Update the function signature in JSDoc or add type annotationsThe function signature has been enhanced to accept an optional
columnparameter of typeColumns, but consider adding JSDoc comments or more explicit type annotations to document the expected parameter types and behavior.Consider adding documentation:
+/** + * Opens a side sheet for managing relation-to-many relationships + * @param rows - The related row IDs or row objects to display + * @param column - Optional column object containing relationship metadata + */ function openSideSheetForRelationsToMany(rows: string | Models.Row[], column?: Columns) {src/lib/elements/forms/inputTextarea.svelte (2)
15-15: Prefer optional type overboolean = undefined.Use an optional to avoid an invalid default for a non-nullable boolean.
-export let spellcheck: boolean = undefined; +export let spellcheck?: boolean;
21-34: Alignerrortype with usage.
erroris set tonull; make it nullable to satisfy TS and avoid widening elsewhere.-let error: string; +let error: string | null = null;src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/string.svelte (2)
136-137: Icons and placeholders: good addition; minor consistency win available.Current textarea uses
getPlaceholder(), while text input hardcodes "Enter string". Consider usinggetPlaceholder()for both for consistency.- placeholder="Enter string" + placeholder={getPlaceholder()}Also applies to: 153-154
4-4: Import is fine; optional micro-optim.If
limitedis often true, consider lazy importing the icon to avoid loading it unnecessarily. Not blocking.src/lib/elements/forms/inputPhone.svelte (1)
16-16: Tighten types forminlengthanderror.Defaults assign
nullto non-nullable types.-export let minlength: number = null; +export let minlength: number | null = null; -let error: string; +let error: string | null = null;Also applies to: 22-47
src/lib/elements/forms/inputURL.svelte (1)
19-38: Nullableerrortype.
erroris set tonull; make the type nullable.-let error: string; +let error: string | null = null;src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte (3)
317-329: Consider defensive programming for display name extractionThe
getAccordionTitlefunction should handle edge cases more robustly:
- Validate that
preferences.getDisplayNamesreturns an array- Handle potential undefined/null values from the preferences store
- Consider truncating long display values to prevent UI overflow
function getAccordionTitle(row: Models.Row): string { - const names = preferences.getDisplayNames(row.$tableId).filter((name) => name !== '$id'); + const displayNames = preferences.getDisplayNames?.(row.$tableId) ?? []; + const names = Array.isArray(displayNames) + ? displayNames.filter((name) => name !== '$id') + : []; const values = names .map((name) => row?.[name]) - .filter((value) => value != null && typeof value === 'string' && value !== ''); + .filter((value) => value != null && typeof value === 'string' && value !== '') + .map((value) => value.length > 30 ? `${value.substring(0, 27)}...` : value); if (!values.length) { return row.$id; } return `${values.join(' | ')} (...${row.$id.slice(-5)})`; }
368-369: Duplicate CSS class applicationThe class
column-item-stackis applied to two nestedLayout.Stackcomponents on lines 367 and 368, which may cause unexpected styling behavior.- <Layout.Stack direction="column" gap="m" class="column-item-stack"> - <Layout.Stack direction="column" gap="xs" class="column-item-stack"> + <Layout.Stack direction="column" gap="xs" class="column-item-stack">
196-197: Remove unnecessary empty lineThere's an unnecessary empty line that can be removed for consistency.
const relatedIds = currentColumn.map((doc: string | Record<string, unknown>) => typeof doc === 'string' ? doc : doc.$id ); - return !symmetricDifference(workIds, relatedIds).length;
📜 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 (29)
package.json(1 hunks)src/lib/elements/forms/inputDate.svelte(3 hunks)src/lib/elements/forms/inputDateTime.svelte(3 hunks)src/lib/elements/forms/inputDomain.svelte(3 hunks)src/lib/elements/forms/inputEmail.svelte(3 hunks)src/lib/elements/forms/inputId.svelte(2 hunks)src/lib/elements/forms/inputNumber.svelte(3 hunks)src/lib/elements/forms/inputOTP.svelte(3 hunks)src/lib/elements/forms/inputPassword.svelte(3 hunks)src/lib/elements/forms/inputPhone.svelte(3 hunks)src/lib/elements/forms/inputProjectId.svelte(2 hunks)src/lib/elements/forms/inputSearch.svelte(3 hunks)src/lib/elements/forms/inputSecret.svelte(1 hunks)src/lib/elements/forms/inputSelect.svelte(2 hunks)src/lib/elements/forms/inputTags.svelte(3 hunks)src/lib/elements/forms/inputText.svelte(3 hunks)src/lib/elements/forms/inputTextarea.svelte(3 hunks)src/lib/elements/forms/inputTime.svelte(3 hunks)src/lib/elements/forms/inputURL.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/boolean.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/datetime.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/email.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/enum.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/integer.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/string.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/url.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T03:59:03.485Z
Learnt from: ItzNotABug
PR: appwrite/console#2290
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte:952-961
Timestamp: 2025-09-04T03:59:03.485Z
Learning: In the Appwrite console spreadsheet component, for relationship columns, the columnId corresponds to the tableId of the related table. When calling openSideSheetForRelationsToMany(tableId, rows), the columnId can be passed directly as the tableId parameter for relationship columns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte
🔇 Additional comments (33)
package.json (1)
27-29: Pin bump looks fine; confirm lockfile and API compatibility.Ensure pnpm-lock.yaml is updated and only one version of both packages is installed. Verify that
@appwrite.io/pink-svelte@d8326d0exposesleadingIconon allInput.*used here and that icons you import exist in@appwrite.io/pink-icons-svelte@d8326d0.src/lib/elements/forms/inputSecret.svelte (1)
2-2: LGTM on leadingIcon wiring.Type-only import and prop forwarding to
Input.Passwordlook correct.Also applies to: 9-9, 12-12
src/lib/elements/forms/inputTags.svelte (1)
3-3: LGTM on adding leadingIcon.Prop is correctly typed and forwarded to
Input.Tags.Also applies to: 13-13, 42-42
src/lib/elements/forms/inputEmail.svelte (1)
3-3: LGTM on leadingIcon support.Prop typing and forwarding look consistent with other inputs.
Also applies to: 16-16, 47-47
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/email.svelte (1)
3-4: LGTM on icon usage.Conditional
leadingIconrespectslimited, import order is fine.Also applies to: 24-24
src/lib/elements/forms/inputPassword.svelte (1)
3-3: LGTM on leadingIcon addition.Forwarding and typing are correct.
Also applies to: 16-16, 47-47
src/lib/elements/forms/inputText.svelte (1)
3-3: LGTM on leadingIcon wiring.Consistent with other components.
Also applies to: 20-20, 59-59
src/lib/elements/forms/inputNumber.svelte (1)
3-3: LGTM on leadingIcon forwarding.Matches the pattern in other inputs.
Also applies to: 19-19, 61-61
src/lib/elements/forms/inputDomain.svelte (2)
3-3: LGTM! Consistent leadingIcon prop implementation.The addition of the
ComponentTypeimport and theleadingIconprop follows the same pattern being applied consistently across all input components in this PR.Also applies to: 17-17
55-55: LGTM! Proper prop forwarding to underlying component.The
leadingIconprop is correctly forwarded to the underlyingInput.Textcomponent, maintaining backward compatibility while enabling the new icon functionality.src/lib/elements/forms/inputSelect.svelte (1)
22-22: LGTM! Consistent leadingIcon implementation.The
leadingIconprop addition follows the same pattern across all input components in this PR, with proper typing and forwarding to the underlyingInput.Selectcomponent.Also applies to: 60-60
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/enum.svelte (1)
4-4: LGTM! Conditional icon usage enhances UX.The conditional
leadingIcon={!limited ? IconList : undefined}appropriately shows the list icon only in non-limited contexts, providing visual consistency while avoiding icon clutter in constrained UI scenarios.Also applies to: 54-55
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (2)
9-9: LGTM! Proper icon import and semantic visual enhancement.The
IconRelationshipimport and its usage in the header provide clear visual context for relationship fields. The icon placement beside the label enhances the UI semantic clarity.Also applies to: 204-205
234-235: LGTM! Consistent leadingIcon usage across all InputSelect instances.All InputSelect components consistently use
leadingIcon={!limited ? IconRelationship : undefined}, maintaining proper conditional rendering and visual consistency throughout the relationship management interface.Also applies to: 251-252, 279-280, 307-308, 345-346
src/lib/elements/forms/inputTime.svelte (1)
3-3: LGTM! Standard leadingIcon implementation.The implementation correctly follows the established pattern: importing
ComponentType, adding the optionalleadingIconprop, and forwarding it to the underlyingInput.DateTimecomponent.Also applies to: 20-20, 54-54
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/boolean.svelte (1)
4-4: LGTM! Appropriate icon choice for boolean fields.The
IconToggleis semantically appropriate for boolean fields, and the conditional usageleadingIcon={!limited ? IconToggle : undefined}maintains consistency with other type components.Also applies to: 33-35
src/lib/elements/forms/inputSearch.svelte (1)
4-4: LGTM! leadingIcon prop added without disrupting existing functionality.The
leadingIconprop is properly implemented while preserving the existing search icon in the start slot. This allows for additional iconography while maintaining the component's search semantics.Also applies to: 14-14, 52-52
src/lib/elements/forms/inputOTP.svelte (1)
2-2: LGTM! Standard leadingIcon pattern implementation.The changes follow the established pattern perfectly: importing
ComponentType, adding the optionalleadingIconprop with proper typing, and forwarding it to the underlyingInput.Textcomponent.Also applies to: 18-18, 64-64
src/lib/elements/forms/inputDate.svelte (1)
3-3: LGTM! Clean implementation of leadingIcon support.The addition of the
leadingIconprop with proper TypeScript typing and forwarding to the underlyingInput.DateTimecomponent is well-implemented and follows the established pattern.Also applies to: 20-20, 54-54
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/integer.svelte (1)
5-5: LGTM! Appropriate icon choice for numeric fields.The conditional rendering of the hashtag icon when not in limited mode provides a nice visual enhancement for integer fields.
Also applies to: 34-35
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/datetime.svelte (1)
3-4: LGTM! Good visual indicator for date/time fields.The calendar icon appropriately indicates the date/time nature of the field when not in limited mode. The multi-line formatting also improves readability.
Also applies to: 20-28
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (2)
956-960: LGTM! Clean implementation of conditional inline editingThe logic correctly determines whether inline editing should be disabled for relation-to-many columns that have items, forcing users to use the side sheet for better UX.
972-975: Good fix! Correctly passes the column parameterThe updated call to
openSideSheetForRelationsToManynow correctly passes both the row data and the column metadata, which aligns with the function signature change and ensures the related table is properly identified.src/lib/elements/forms/inputDateTime.svelte (2)
16-16: Prop is correctly threaded through.Forwarding
leadingIconand exposing it as a public prop looks good.Also applies to: 58-58
58-58: No action required — Input.DateTime supportsleadingIcon.
Verified: node_modules/@appwrite.io/pink-svelte/dist/input/DateTime.svelte exportsleadingIconand dist/input/DateTime.svelte.d.ts includes the prop, so passing {leadingIcon} will be accepted.src/lib/elements/forms/inputProjectId.svelte (1)
7-7: Leading icon plumbing is correct.Prop definition and pass-through to
Input.Textare consistent with the pattern in this PR.Also applies to: 30-30
src/lib/elements/forms/inputTextarea.svelte (1)
19-19: LGTM onleadingIconsupport.Public prop + forwarding to
Input.Textarealooks good.Also applies to: 48-48
src/lib/elements/forms/inputPhone.svelte (1)
18-18: Leading icon pass-through looks correct.Matches the pattern used across inputs.
Also applies to: 60-60
src/lib/elements/forms/inputId.svelte (1)
8-8: Prop addition and forwarding are correct.No issues with
leadingIconintegration.Also applies to: 31-31
src/lib/elements/forms/inputURL.svelte (1)
17-17:leadingIconintegration looks good.Public prop + pass-through to
Input.Textis consistent.Also applies to: 51-51
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/url.svelte (2)
23-24: Usage looks correct.Passing
required={column.required}and gating the icon behind!limitedis appropriate.
3-4: Confirm: InputURL exports and forwardsleadingIcon.
It declaresexport let leadingIcon: ComponentType | undefined = undefined;and references{leadingIcon}in src/lib/elements/forms/inputURL.svelte, so no changes required.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte (1)
371-373: Good use of accordion for improved UXThe implementation of accordions for multiple related items with dynamic titles based on display preferences is a clean solution that improves the user experience when dealing with many relationship items.
...egion]-[project]/databases/database-[database]/table-[table]/rows/columns/types/email.svelte
Outdated
Show resolved
Hide resolved
...oject-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte
Outdated
Show resolved
Hide resolved
...e)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
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]/subNavigation.svelte (1)
55-58: Replace all Query.orderDesc('') — empty string is invalid / will errorAppwrite's Query.orderDesc expects a column name; passing '' can be rejected by the backend (docs + reported bug). (appwrite.io)
- Action: either remove the empty-order query or use an explicit attribute for deterministic results.
- Find occurrences: rg -n "Query.orderDesc(\s*''\s*)"
Option A — rely on server default:
- queries: [Query.orderDesc(''), Query.limit(100)] + queries: [Query.limit(100)]Option B — explicit order (adjust attribute to product needs):
- queries: [Query.orderDesc(''), Query.limit(100)] + queries: [Query.orderAsc('name'), Query.limit(100)]
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/subNavigation.svelte (2)
44-44: Coalesce to [] to avoid undefined and tighten TS types.
tables?.tablesis typed as possibly undefined; later you call.slice/.lengthonsortedTables. Default to[]and annotate the return type.- const sortedTables = $derived.by(() => tables?.tables); + const sortedTables = $derived.by((): Models.Table[] => tables?.tables ?? []);
89-114: Key the each block by table id to avoid DOM churn on reordering.Since you dropped client-side sorting and now rely on API order, reorders can happen; unkeyed each reuses DOM by index.
-{#each sortedTables as table, index} +{#each sortedTables as table, index (table.$id)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/subNavigation.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/email.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/editRelated.svelte(5 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/editRelated.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). (1)
- GitHub Check: e2e
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/email.svelte (3)
3-4: LGTM: icon import and input wiring look correct.
IconMailimport aligns with the Pink icons package and is used below;InputEmailimport unchanged. No issues.
22-22: Placeholder fixed (resolves earlier feedback).Switch to “[email protected]” is appropriate for email inputs.
23-24: Confirm InputEmail forwardsleadingIconto the underlying control.
src/lib/elements/forms/inputEmail.svelte exportsleadingIconbut I couldn't verify it's passed to the inner Input; ensure it's forwarded (e.g.<Input ... leadingIcon={leadingIcon} />or include it in a spread like{...$$restProps}).
| export let minlength: number = null; | ||
| export let maxlength: number = 15; | ||
| export let nullable = false; | ||
| export let leadingIcon: ComponentType | undefined = undefined; |
There was a problem hiding this comment.
is the = undefined here needed? Since props default to undefined, could this also be written as export let leadingIcon?: ComponentType;? 🤔
There was a problem hiding this comment.
optional type like leadingIcon?: ComponentType; can be used in objects, not as a standalone expression.
2 participants
What does this PR do?
leadingIconsupport for inputs from Pink2$id,displayNamesas titleTest Plan
Manual.
Accordion -
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Bug Fixes
Chores