Revamp designs for Suggestions UI #2486
Conversation
update: allow passing a subtitle snippet. update: allow setting icons to secondary and primary actions. remove: confirm on indexes. remove: empty column on indexes when on a small viewport.
|
This PR was not deployed automatically as @ItzNotABug does not have access to the Railway project. In order to get automatic PR deploys, please add @ItzNotABug to your workspace on Railway. |
ConsoleProject ID: Sites (2)
Tip Appwrite has a Discord community with over 16 000 members. |
|
Warning Rate limit exceeded@ItzNotABug has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR adds an AI-driven column-suggestions feature (new modal, store fields, suggestion flow), a card-based empty-state UX (new card component and AI icon), and refactors EmptySheet to use Snippet-based actions with dynamic overlay/column sizing. It exposes many new public props (global Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/relationship.svelte (1)
188-207: Asymmetry confirmed: Primary key should also be readonly during editing.The verification reveals the asymmetry is real but indicates a design inconsistency:
- twoWayKey (line 207):
readonly={editing}is correct because the API call doesn't include anewTwoWayKeyparameter—it cannot be modified at all.- Primary key (line 195): The API call sends
newKey: data.key !== originalKey ? data.key : undefined(line 53), allowing the key to be renamed during updates.However, the helper text at lines 205-206 states: "Once created, column key cannot be adjusted to maintain data integrity." This applies to both keys conceptually, but only the twoWayKey is prevented from editing.
Add
readonly={editing}to the primary key input (line 195) to prevent modification after relationship creation, making the behavior consistent with the helper message and the twoWayKey logic.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (1)
1110-1136: Duplicate id="key" in repeated inputs; generate unique IDs per column.Repeated static IDs harm a11y and label association.
- <InputText - id="key" + <InputText + id={`key-${column.id}`} autofocus @@ - <InputText - id="key" + <InputText + id={`key-${id}`} label="Key"Also applies to: 1153-1160
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (2)
89-91: Null-safety for hasColumns to avoid runtime on undefined columnsDirect
.lengthon possibly undefined can throw. Mirror yourhasValidColumnsoptional chaining.- $: hasColumns = !!$table.columns.length; + $: hasColumns = !!$table?.columns?.length;
73-79: Inverted filter for system columns in Filters optionsYou exclude
$idwhen selected but include system columns only when selected. Likely both should exclude when selected.- ].filter((col) => !!selected.includes(col.id)); + ].filter((col) => !selected.includes(col.id));src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (1)
95-104: Overlay offsets not updated on scroll; add listenersResizeObserver won’t catch scroll. Update offsets/heights on scroll/viewport resize to keep the overlay aligned.
onMount(async () => { if (spreadsheetContainer) { resizeObserver = new ResizeObserver(debouncedUpdateOverlayHeight); resizeObserver.observe(spreadsheetContainer); overlayOffsetHandler = new ResizeObserver(updateOverlayLeftOffset); overlayOffsetHandler.observe(spreadsheetContainer); } + + const onScrollOrResize = () => { + updateOverlayLeftOffset(); + debouncedUpdateOverlayHeight(); + }; + window.addEventListener('scroll', onScrollOrResize, { passive: true }); + window.addEventListener('resize', onScrollOrResize, { passive: true }); }); onDestroy(() => { if (resizeObserver) { resizeObserver.disconnect(); } if (overlayOffsetHandler) { overlayOffsetHandler.disconnect(); } + window.removeEventListener('scroll', onScrollOrResize); + window.removeEventListener('resize', onScrollOrResize); });Also applies to: 105-113
🧹 Nitpick comments (15)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/url.svelte (1)
59-69: Consider simplifying the reactive pattern.The
createConservativehelper adds indirection by creating stores that mirrordata.requiredanddata.array, syncing them vialisten(data), and then using those stores in a reactive statement. Since the UI binds directly todata.requiredanddata.array(lines 84, 92), this could be simplified to:-const { - stores: { required, array }, - listen -} = createConservative<Partial<Models.ColumnUrl>>({ - required: false, - array: false, - ...data -}); -$: listen(data); - -$: handleDefaultState($required || $array); +$: handleDefaultState(data.required || data.array);If
createConservativeprovides specific optimizations (e.g., preventing unnecessary re-renders), this pattern is acceptable, but the added complexity warrants verification.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/datetime.svelte (1)
63-74: Consider using stores consistently for reactivity.The
createConservativehelper creates reactive stores forrequiredandarray, which are used in thehandleDefaultStateeffect (line 74). However, the disabled expressions (lines 81, 89, 97) accessdata.requiredanddata.arraydirectly. This creates two sources of truth that might drift out of sync.If intentional for performance or reactivity reasons, this is fine. Otherwise, consider using either the stores consistently throughout or direct data access everywhere.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/createColumn.svelte (1)
235-249: Consider using CSS variables instead of hardcoded values.The custom styling uses hardcoded spacing values. For consistency with the design system, consider using existing CSS variable names if they're semantically appropriate.
.custom-inline-alert { & :global(article) { - padding: var(--space-4, 8px); + padding: var(--space-xs); }This assumes
--space-xsmaps to 8px in your design system. Ifvar(--space-4, 8px)is the correct pattern, then this change isn't needed.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/icon/aiForButton.svelte (1)
10-21: Consider using CSS variables for the icon color.The fill color is hardcoded to
#97979B. For better theming support and consistency with the design system, consider using a CSS variable.<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 20 20" fill="none"> <path fill-rule="evenodd" clip-rule="evenodd" d="M5.00049 2C5.55277 2 6.00049 2.44772 6.00049 3V4H7.00049C7.55277 4 8.00049 4.44772 8.00049 5C8.00049 5.55228 7.55277 6 7.00049 6H6.00049V7C6.00049 7.55228 5.55277 8 5.00049 8C4.4482 8 4.00049 7.55228 4.00049 7V6H3.00049C2.4482 6 2.00049 5.55228 2.00049 5C2.00049 4.44772 2.4482 4 3.00049 4H4.00049V3C4.00049 2.44772 4.4482 2 5.00049 2ZM5.00049 12C5.55277 12 6.00049 12.4477 6.00049 13V14H7.00049C7.55277 14 8.00049 14.4477 8.00049 15C8.00049 15.5523 7.55277 16 7.00049 16H6.00049V17C6.00049 17.5523 5.55277 18 5.00049 18C4.4482 18 4.00049 17.5523 4.00049 17V16H3.00049C2.4482 16 2.00049 15.5523 2.00049 15C2.00049 14.4477 2.4482 14 3.00049 14H4.00049V13C4.00049 12.4477 4.4482 12 5.00049 12Z" - fill="#97979B" /> + fill="var(--fgcolor-neutral-tertiary)" /> <path fill-rule="evenodd" clip-rule="evenodd" d="M12.0004 2C12.4542 2 12.851 2.30548 12.9671 2.74411L14.1464 7.19893L17.5002 9.13381C17.8097 9.3124 18.0004 9.64262 18.0004 10C18.0004 10.3574 17.8097 10.6876 17.5002 10.8662L14.1464 12.8011L12.9671 17.2559C12.851 17.6945 12.4542 18 12.0004 18C11.5467 18 11.1498 17.6945 11.0337 17.2559L9.85451 12.8011L6.50076 10.8662C6.19121 10.6876 6.00049 10.3574 6.00049 10C6.00049 9.64262 6.19121 9.31241 6.50076 9.13382L9.85451 7.19893L11.0337 2.74411C11.1498 2.30548 11.5467 2 12.0004 2Z" - fill="#97979B" /> + fill="var(--fgcolor-neutral-tertiary)" /> </svg>Verify that
--fgcolor-neutral-tertiary(or the appropriate variable from your design system) resolves to the desired color.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/columns.svelte (1)
30-53: Ensure route change settles before reading page data.After await goto(...), consider await tick() before using page.data (for name fallback) to avoid transient stale values post-navigation.
- if (!isOnRowsPage) { - await goto( + if (!isOnRowsPage) { + await goto( resolve( '/(console)/project-[region]-[project]/databases/database-[database]/table-[table]', { region: page.params.region, project: page.params.project, database: page.params.database, table: page.params.table } ) - ); + ); + await tick(); }src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheetCards.svelte (1)
23-31: Prevent clicks when disabled; avoid firing onClick for disabled cards.Currently onClick fires regardless of disabled. Guard it to match visual state and avoid accidental actions.
- on:click={() => onClick?.()}> + on:click={(e) => { + if (disabled) { + e.preventDefault(); + e.stopPropagation(); + return; + } + onClick?.(); + }}>src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (4)
1012-1013: Throttle window scroll handler to reduce layout thrash.recalcAll is heavy; reuse recalcAllThrottled for window scroll.
-<svelte:window on:resize={recalcAll} on:scroll={recalcAll} on:click={handleGlobalClick} /> +<svelte:window on:resize={recalcAll} on:scroll={recalcAllThrottled} on:click={handleGlobalClick} />
591-595: Also clear selectedColumnName on deselect to avoid stale labels.Prevents stale badge text if UI reopens quickly after a deselect.
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
542-553: Use rAF instead of setTimeout(0) for mobile overlay updates.rAF aligns DOM writes with paint for smoother visuals.
- async function updateOverlaysForMobile(value: boolean) { + async function updateOverlaysForMobile(value: boolean) { if ($isSmallViewport) { - setTimeout(() => { + requestAnimationFrame(() => { [rangeOverlayEl, fadeBottomOverlayEl].forEach((el) => { if (el) { el.style.opacity = value ? '0' : '1'; } }); - }, 0); + }); } }
472-483: Align min with known backend cap to fill placeholders.You request min: 6 while placeholders are 7 and backend max is 7. Consider min: 7 to fully populate initial slots. Based on learnings.
- min: 6 + min: 7src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (1)
246-253: Add analytics for “Generate random data” to match Clear filters/Import CSVFor parity and funnel tracking, fire a click/submit event when opening the random‑data modal.
Example:
onClick={() => { - $randomDataModalState.show = true; + $randomDataModalState.show = true; + trackEvent(Click.DatabaseRandomDataOpen); }} />If
Click.DatabaseRandomDataOpendoesn’t exist, align with your analytics enum naming.Also applies to: 277-284
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (4)
130-161: Hard-coded 1387px width is brittle; compute filler widthWhen no custom columns,
actions: 1387will break across viewports. Compute from container width and other fixed columns.- let columnWidths = { + const viewportWidth = + spreadsheetContainer?.clientWidth || + (typeof window !== 'undefined' ? window.innerWidth : 0); + + let columnWidths = { id: fixedWidths.id, createdAt: fixedWidths.id, updatedAt: fixedWidths.id, custom: minColumnWidth, - actions: hasCustomColumns ? fixedWidths.actions : 1387 + actions: hasCustomColumns + ? fixedWidths.actions + : Math.max( + minColumnWidth, + viewportWidth - (fixedWidths.id + 2 * minColumnWidth) // $id + created/updated + ) };Also applies to: 200-209
324-332:dynamicOverlayHeightis written but not used; apply or removeYou set
style:--dynamic-overlay-heightbut the CSS never reads it. Either apply it or drop the state to reduce complexity.Option A — apply:
.spreadsheet-fade-bottom { + height: var(--dynamic-overlay-height, 60.5vh); right: 0; bottom: 0; position: fixed;Option B — remove the state and related code if bottom anchoring is sufficient.
Also applies to: 414-435
284-305: Remove unreachable nestedcolumn.isActionbranchInside the non‑action branch,
column.isActionis always false. Drop the inner check for clarity.- {#if column.isAction} - <Button.Button icon variant="extra-compact"> - <Icon icon={IconPlus} color="--fgcolor-neutral-primary" /> - </Button.Button> - {:else if column.id === 'actions' || column.id === 'empty'} + {#if column.id === 'actions' || column.id === 'empty'} {column.title} {:else} <Layout.Stack
86-89: Avoid magic number 89px for collapsed tabs
89pxis fragile. Source this from a CSS var or compute from the header element height.Example:
- if (!$expandTabs) { - dynamicOverlayHeight = `calc(${dynamicHeight}px - 89px)`; - } + if (!$expandTabs && headerElement) { + const headerExtra = headerElement.offsetHeight ?? 0; + dynamicOverlayHeight = `calc(${dynamicHeight}px - ${headerExtra}px)`; + }
📜 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 (32)
package.json(1 hunks)src/lib/components/card.svelte(2 hunks)src/lib/elements/forms/inputLine.svelte(4 hunks)src/lib/elements/forms/inputPoint.svelte(3 hunks)src/lib/elements/forms/inputPolygon.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/columns.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(32 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/icon/aiForButton.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/indexes.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/input.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+layout.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/boolean.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/datetime.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/email.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/enum.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/float.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/integer.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/ip.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/point.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/relationship.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/string.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/url.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/createColumn.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte(9 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheetCards.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/columns.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/createColumn.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+layout.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte
📚 Learning: 2025-10-04T11:46:32.504Z
Learnt from: ItzNotABug
PR: appwrite/console#2442
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/float.svelte:88-88
Timestamp: 2025-10-04T11:46:32.504Z
Learning: In the Appwrite Console codebase, for float/double column inputs in database table column configuration files (like float.svelte), use step={0.1} for InputNumber components, not step="any". This is the established pattern for float/double precision inputs.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/float.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: build
- GitHub Check: e2e
🔇 Additional comments (55)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/ip.svelte (1)
42-42: LGTM! Clean implementation of the disabled prop.The disabled prop is correctly exported and consistently propagated to all three form controls (InputText and both Selector.Checkbox components). The logic properly extends existing disabled conditions without disruption, maintaining the component's existing behavior while enabling external control of interactivity.
Also applies to: 77-77, 85-85, 93-93
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/boolean.svelte (1)
42-102: LGTM! Disabled prop correctly propagated.The disabled prop is properly introduced and threaded through to all relevant UI controls (default value selector, required checkbox, and array checkbox). The logic correctly combines the new prop with existing disabled conditions using OR operations, ensuring that when
disabledis true, all controls are appropriately disabled without breaking existing behavior.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (1)
863-863: LGTM! Hover effect added appropriately.The
hoverEffectprop is correctly added to data rows while being intentionally excluded from loading rows (lines 842-855). This follows the existing pattern where interactive features likeshowSelectOnHoverare only applied to actual data rows, not placeholder loading rows.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/point.svelte (2)
46-46: LGTM! Disabled state integration is well-implemented.The addition of the
disabledprop and its propagation to form controls follows the standard pattern. Explicit defaults fordata,editing, anddisabledin the props destructuring improve clarity and prevent undefined values. The disabled state is correctly passed to all relevant UI controls (Required checkbox, Default value checkbox, and InputPoint component) without disrupting existing logic.Also applies to: 50-57, 107-107, 118-118, 139-139
47-47: Line marked as changed but appears unchanged.Line 47 is annotated as modified but shows no visible difference. This may be a whitespace change, formatting adjustment, or annotation inconsistency.
src/lib/elements/forms/inputPoint.svelte (1)
13-13: LGTM! Clean implementation of disabled prop.The
disabledprop is properly defined, destructured, and propagated to both the number inputs and delete button. The delete button correctly combines all disabling conditions.Also applies to: 26-26, 43-43, 51-51
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/line.svelte (1)
46-46: LGTM! Consistent disabled prop implementation.The
disabledprop is properly integrated throughout the component:
- Correctly defined with a sensible default (
false)- Properly propagated to both checkboxes and the
InputLinecomponent- The destructuring pattern is clean and follows Svelte conventions
Also applies to: 50-54, 114-114, 125-125, 146-146
src/lib/elements/forms/inputPolygon.svelte (1)
20-20: LGTM! Proper disabled prop propagation.The
disabledprop is correctly added and forwarded to allInputLineinstances, enabling unified disable behavior across polygon line inputs.Also applies to: 30-32, 39-39
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/polygon.svelte (1)
46-46: LGTM! Well-integrated disabled state.The
disabledprop is properly implemented and propagated throughout:
- Correctly defined with a sensible default (
false)- Properly forwarded to both checkboxes and the
InputPolygoncomponent- Follows the same pattern established across other column editors
Also applies to: 50-57, 128-128, 140-140, 161-161
src/lib/elements/forms/inputLine.svelte (1)
17-17: LGTM! Complete disabled prop integration.The
disabledprop is correctly implemented:
- Properly propagated to all
InputPointinstances- Add coordinate button correctly combines both disabling conditions (
nullable || disabled)- Completes the cascading disable behavior through the input component hierarchy
Also applies to: 28-30, 45-45, 58-64
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/enum.svelte (4)
42-45: LGTM! Import additions are properly used.The new imports (
createConservativeandIconInfo) are correctly utilized in the component.
48-48: LGTM! Disabled prop follows Svelte conventions.The new
disabledprop is correctly declared and enables external control of the form's disabled state.
90-102: Verify: Should the Elements input also respect the disabled state?The
InputTagscomponent for "Elements" doesn't receive thedisabledprop, while all other form inputs (default value, required, array) do. Consider whether this is intentional or if the Elements input should also be disabled when the parent component'sdisabledprop is true for consistency.
107-107: LGTM! Disabled logic is correctly implemented.The disabled conditions for each form input properly combine business rules with the component-level
disabledprop:
- Default value disabled when array/required (no default needed) or form disabled
- Required disabled when array (mutually exclusive) or form disabled
- Array disabled when required (mutually exclusive), editing (immutable), or form disabled
Also applies to: 117-117, 125-125
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/float.svelte (4)
45-46: LGTM! New imports align with UI revamp.The new imports for
createConservative,Layout, andSelectorare properly utilized throughout the component and align with the pink-svelte UI library adoption mentioned in the AI summary.
49-49: LGTM! Disabled prop correctly added to public API.The new
disabledprop with a default value offalseis correctly exposed and aligns with the pattern being applied across all column editors in this PR.
82-111: LGTM! Disabled state correctly propagated to all InputNumber components.The disabled prop is properly threaded through all InputNumber components with appropriate logic:
- Min/Max inputs: simple disabled prop
- Default value input: correctly combines
data.required || data.array || disabledto prevent defaults on required or array fieldsThe
step={0.1}is correctly maintained as per established pattern.Based on learnings
113-127: LGTM! Disabled state correctly propagated to checkboxes with proper business rules.The disabled prop is appropriately integrated into both checkboxes while maintaining existing business rules:
- Required checkbox:
disabled={data.array || disabled}- prevents required when array is true- Array checkbox:
disabled={data.required || editing || disabled}- prevents array when required is true or in edit modeThe mutual exclusion between required and array fields is correctly preserved.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/email.svelte (4)
41-42: LGTM: New imports support disabled state management.The
Selectorcomponent from the external UI library andcreateConservativehelper are appropriately imported to support the new disabled prop functionality and reactive state tracking.
45-45: LGTM: Disabled prop correctly added to public interface.The
disabledprop with a default value offalseprovides parent components control over the form's interactive state.
59-70: LGTM: Reactive state management correctly handles default value logic.The
createConservativehelper appropriately tracksrequiredandarraystate changes to triggerhandleDefaultState, which correctly clears the default value when either condition is true (required fields must be provided, array fields default to empty arrays).
72-94: LGTM: Disabled logic correctly implements mutual exclusion and prop threading.The disabled state is properly threaded through all form inputs with correct business logic:
- InputEmail: Disabled when a default value doesn't apply (required or array fields)
- Required/Array checkboxes: Mutual exclusion prevents both being true simultaneously
- Array checkbox: Additionally disabled during editing (array type can't change post-creation)
- All inputs respect the parent's
disabledpropsrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/string.svelte (2)
58-58: LGTM! Disabled prop correctly propagated through form controls.The new
disabledprop is properly integrated with existing disabled logic across all form inputs and checkboxes. The OR combination ensures the component respects both the global disabled state and field-specific constraints.Also applies to: 92-92, 107-107, 115-115, 123-123, 137-137
129-130: LGTM! Container styling properly updated for disabled state.The cursor and CSS class bindings on the popover holder correctly incorporate the new
disabledprop, ensuring appropriate visual feedback when the encrypt controls are disabled.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/integer.svelte (4)
45-46: LGTM! Imports are properly utilized.The new imports are correctly integrated:
createConservativeis used for reactive store management (lines 69-77), and the UI components are used for layout and form controls throughout the template.
49-49: LGTM! Disabled prop properly introduced.The new
disabledprop is correctly typed and defaulted tofalse, consistent with the broader pattern of centralized disablement across column editors mentioned in the AI summary.
82-98: LGTM! Disabled state properly propagated to min/max inputs.The
disabledprop is correctly passed to bothInputNumbercomponents, ensuring consistent behavior when the component is in a disabled state.
110-124: LGTM! Checkbox disabled states properly configured.Both checkboxes correctly combine business rules with the component-level
disabledprop:
- Required checkbox (line 115): Disabled when
array=trueor the component is disabled- Array checkbox (line 123): Disabled when
required=true, in editing mode, or the component is disabledThe logic properly prevents conflicting states while respecting the disabled prop.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/url.svelte (3)
41-42: LGTM! Imports are used correctly.Both imports are actively utilized:
Selectorfor the checkbox components (lines 80, 88)createConservativefor reactive store management (lines 59-66)
45-46: LGTM! Disabled prop correctly added.The
disabledprop with a sensible default aligns with the broader pattern of disabled state propagation across column editors mentioned in the PR summary. The property is properly exported and consistently propagated to child components.
77-93: LGTM! Disabled logic is correctly implemented.The
disabledprop is appropriately propagated to all interactive elements with sound logic:
- InputURL (line 77): Disabled when
required(no default for required fields),array(constraint), or globally disabled- Required checkbox (line 85): Disabled when
array(mutual exclusivity) or globally disabled- Array checkbox (line 93): Disabled when
required(mutual exclusivity),editing(immutability after creation), or globally disabledThe mutual exclusivity between
requiredandarray, combined with immutability of the array property during editing, is correctly enforced.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/datetime.svelte (3)
45-46: LGTM! Clean import additions.The new imports are properly utilized:
Selectorfor the checkbox components andcreateConservativefor reactive state management.
77-98: LGTM! Disabled logic properly implements business rules.The disabled prop is correctly threaded through all UI controls with appropriate business logic:
- Default value disabled when required, array, or globally disabled
- Required checkbox disabled when array or globally disabled
- Array checkbox disabled when required, editing, or globally disabled
The implementation correctly prevents invalid state combinations.
49-49: No action required—disabled prop is correctly implemented.The
disabledprop is properly declared with a sensible default and consistently applied throughout the component. All column editor components follow the same pattern (integer, float, string, boolean, etc.), suggesting this is an established convention in the codebase. The prop correctly controls disabled state on form elements.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/relationship.svelte (2)
72-72: LGTM! Disabled prop correctly added.The new
disabledprop follows the established pattern and allows parent components to control the disabled state of all interactive elements within the relationship column editor.
162-217: Pattern verified—no changes needed. The disabled state strategy is correct and consistent with other column editors.The composite
disabled={editing || disabled}applied to structural fields (way, related table, relationship type) matches the established pattern used for immutable fields across all column editors. Other editors similarly lock the array checkbox withdisabled={data.required || editing || disabled}to prevent structural changes once created. Keys remaining editable with simple{disabled}aligns with how other editors keep value fields editable during edit mode. The approach correctly distinguishes immutable structural properties from editable value properties.package.json (1)
27-29: LGTM! Dependency updates support new UI components.The updated pink-icons-svelte and pink-svelte dependencies align with the new AI suggestions UI, icon components, and form enhancements introduced in this PR.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/indexes.svelte (2)
11-11: LGTM! Simplified imports after removing confirmation flow.The removal of the
Confirmcomponent import aligns with the simplified dismiss flow where users can directly cancel without an additional confirmation step.
355-355: LGTM! Direct dismissal improves UX.The Cancel button now directly invokes
dismissIndexes(), streamlining the user experience by removing an unnecessary confirmation step. The implementation is consistent with the mobile cancel flow at Line 384.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/createColumn.svelte (2)
41-42: Verify that the feature is production-ready.The feature flag is hardcoded to
truewith a comment stating "flow isn't complete yet!" This could expose incomplete functionality to users.Consider using an environment variable or feature flag service to control this:
-// flow isn't complete yet! -const isSuggestionsFeatureEnabled = true; +const isSuggestionsFeatureEnabled = import.meta.env.VITE_ENABLE_AI_SUGGESTIONS === 'true';Or if the flow is actually complete, remove the comment to avoid confusion.
189-203: LGTM! AI suggestion integration is well-structured.The inline alert provides a clear call-to-action for users to leverage AI-powered column suggestions. The modal state management correctly hides the create column sheet while showing the suggestions modal.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte (2)
293-345: LGTM! Enhanced empty states with AI suggestions.The empty state updates provide users with clear options for both AI-assisted and manual workflows. The dual-action cards (AI suggestions and manual creation) improve discoverability and offer flexibility based on user preference.
The conditional logic properly handles two scenarios:
- No indexes but columns exist → suggest indexes or create manually
- No columns → create columns or use AI suggestions
69-76: Review comment mischaracterizes the width implementation pattern.The column widths are not actually "fixed" as described. The
getColumnWidthfunction (lines 117-122) retrieves saved user preferences and returns the user's resized value if available, otherwise returns the default. Sinceresizable: trueis set on each column, users can resize and persist custom widths via the preferences system. This is a flexible, persistence-based approach—not a transition to truly fixed pixel values. The same pattern is used consistently across other pages (spreadsheet.svelte, columns/+page.svelte), making it an intentional architectural choice.Likely an incorrect or invalid review comment.
src/lib/components/card.svelte (2)
23-31: LGTM! Enhanced prop flexibility.The prop type refinements improve the component API:
isButtonis now a flexible boolean instead of a literalexternalprop enables controlled new-tab behavior for linksAnchorProps.isButtonis now optional, providing more flexibilityThese changes maintain backwards compatibility while expanding the API surface for more use cases.
50-62: LGTM! Clean conditional spread for external links.The use of a conditional spread operator
{...external ? { target: '_blank' } : {}}is an elegant pattern for conditionally applying the target attribute. This ensures links only open in new tabs when explicitly requested via theexternalprop.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts (2)
6-6: LGTM! Optionalforcefield added to suggestion type.The
force?: booleanfield extendsTableColumnSuggestionswithout breaking existing code. The field likely controls whether to force regeneration of suggestions or bypass certain logic in the AI-driven workflow.
52-52: LGTM! New modal state store.The
showColumnsSuggestionsModalwritable store provides clean state management for the column suggestions modal visibility. This follows the same pattern asshowIndexesSuggestionsat Line 50.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte (2)
10-23: LGTM! Clean API extension for mobile footer actions.The new props enhance the component's flexibility:
mobileFooterChildren: Optional snippet for custom mobile footer contentonChildrenClick: Optional callback for click handlingBoth are properly typed as optional and maintain backwards compatibility.
63-65: LGTM! Footer snippet integration.The footer snippet correctly passes the toggle function to
mobileFooterChildren, maintaining consistency with the pattern used elsewhere in the component. The optional chaining ensures the snippet is only rendered when provided.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/input.svelte (2)
10-15: Prop addition LGTM.Public prop isModal with default false is clear and typed.
30-36: Modal-aware copy and switch visibility LGTM.Subtitle logic and hiding the switch in modal mode match the intended UX.
Also applies to: 53-61
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/columns.svelte (2)
11-16: Bindable show prop LGTM.Using $bindable for show enables clean two-way binding from layout.
55-62: Modal content and actions LGTM.Input respects modal mode; Cancel fully resets suggestion state.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+layout.svelte (2)
68-73: Imports are correctly configured; no tree-shaking issues.The index file properly re-exports all symbols from store.ts via
export * from './store', includingshowColumnsSuggestionsModal. The import statement in +layout.svelte correctly sources from the../(suggestions)aggregator, which chains to the store exports. No mismatches exist.
490-491: Remove this review comment — code is correctly implemented.The binding is properly configured. The
showprop in ColumnsSuggestions is marked with$bindable(), andshowColumnsSuggestionsModalis a writable store. Two-way binding directly to a $-prefixed store value in component props is supported in Svelte 5 when the child prop is declared bindable and the store is writable. Both conditions are met here, so no refactoring is needed.Likely an incorrect or invalid review comment.
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
...console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte
Outdated
Show resolved
Hide resolved
...console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte
Outdated
Show resolved
Hide resolved
...roject-[region]-[project]/databases/database-[database]/table-[table]/columns/integer.svelte
Show resolved
Hide resolved
...project-[region]-[project]/databases/database-[database]/table-[table]/columns/string.svelte
Show resolved
Hide resolved
...ject-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte
Show resolved
Hide resolved
update: actions on indexes empty state.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte (1)
337-343: Gate "Suggest columns" card to Cloud/feature availability.Similar to the indexes card, this "Suggest columns" card is visible regardless of cloud availability or feature enablement, potentially misleading users in non-cloud environments.
Apply the same gating pattern as recommended for the suggest indexes card:
+ {#if isCloud && $tableColumnSuggestions.enabled} <EmptySheetCards icon={IconAI} title="Suggest columns" subtitle="Use AI to generate columns" onClick={() => { $showColumnsSuggestionsModal = true; - }} /> + }} /> + {/if}src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (1)
269-276: Gate "Suggest columns" card to Cloud/feature availability.The TODO comment and past review both flag this: the card is always rendered even when suggestions are disabled or unavailable. Users noted this for internal design evaluation but it remains unaddressed.
Wrap the card in a conditional:
+ {#if isCloud && $tableColumnSuggestions.enabled} <EmptySheetCards icon={IconAI} title="Suggest columns" subtitle="Use AI to generate columns" onClick={() => { $showColumnsSuggestionsModal = true; - }} /> + }} /> + {/if}src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (2)
280-280: Use standard scroll behavior value.
ScrollToOptionsaccepts'auto'or'smooth';'instant'is non-standard and browsers may ignore it.Apply this diff:
directAccessScroller.scrollTo({ left: Math.max(0, scrollLeft), - behavior: 'instant' + behavior: 'auto' });
679-695: Fix type annotation for columnSchema.Line 683 uses
= null(value assignment) instead of| null(type union), which doesn't makecolumnSchemanullable in TypeScript. While the guard at 693-695 prevents the crash mentioned in past reviews, the type is still incorrect.Apply this diff:
function deleteColumn(columnId: string) { if (!columnId) return; let columnIndex = -1; - let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null; for (let index = 0; index < customColumns.length; index++) {
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (1)
46-50: Remove unused userColumns prop.The
userColumnsprop is accepted but never used. Line 403 has it commented out, and line 419 includes it only to silence linting. This adds unnecessary API surface.Remove the prop or document its intended future use:
- let { - userColumns = [] - }: { - userColumns?: Column[]; - } = $props();And remove line 419:
- userColumns; /* silences lint check, variable not read */Also applies to: 403-403, 419-419
📜 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]/(suggestions)/columns.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(32 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/columns.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.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 (9)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte (1)
69-76: LGTM: Column width configuration.The minimum width constraints for the key and type columns are appropriate and improve readability.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (3)
238-252: LGTM: EmptySheetCards integration for rows.The action cards provide clear options for creating rows and generating test data, improving the empty state UX.
216-231: Approve refactored filtered rows empty state.The actions snippet pattern simplifies the component structure and maintains functionality.
208-208: Store initialization confirmed—no issues found.The
tableColumnSuggestionsstore initializesforceasfalse(notundefined/null), making the condition!== truecorrectly allow grid rendering in the initial state while respecting explicitforce: truetoggles.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (5)
788-796: LGTM: Column key validation.The guard prevents creating columns with invalid/empty keys and provides clear user feedback.
679-724: Approve delete/undo workflow with 10-second timer.The implementation correctly handles column deletion with a timed undo option, stores the index for restoration, and cleans up state appropriately.
285-309: Approve column highlight positioning.The dynamic CSS variable approach for positioning the highlight overlay is clean and integrates well with the spreadsheet layout.
627-677: LGTM: Column visibility and scroll-into-view logic.The visibility checks account for sticky columns and viewport bounds correctly, and the smooth scrolling centers the column appropriately.
1381-1428: Approve changeColumnTypePopover snippet extraction.The snippet pattern reduces duplication and makes the column type selector reusable across header and editor contexts.
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Outdated
Show resolved
Hide resolved
.../project-[region]-[project]/databases/database-[database]/table-[table]/indexes/+page.svelte
Outdated
Show resolved
Hide resolved
fix: overlay on mobile's side sheet.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (3)
796-804: Consider providing more specific validation feedback.The validation correctly prevents creation when column keys are empty, but the generic warning doesn't help users identify which specific columns are invalid. Consider listing the problematic column positions or keys in the error message.
Example improvement:
const isAnyEmpty = customColumns.some((col) => !col.key); if (isAnyEmpty) { creatingColumns = false; + const invalidColumns = customColumns + .map((col, idx) => (!col.key ? `column ${idx + 1}` : null)) + .filter(Boolean) + .join(', '); addNotification({ type: 'warning', - message: 'Some columns have invalid keys' + message: `Invalid keys found in: ${invalidColumns}` }); return; }
1047-1077: Remove or document the commented countdown progress code.This 31-line commented block (plus related commented sections at lines 1411-1412 and 1908-1932) should either be removed if abandoned, or preserved with a TODO comment explaining why it's temporarily disabled and when it will be re-enabled.
If this is work-in-progress:
<!-- TODO: Restore countdown progress UI after design review (ticket #XXXX) --> <!--{#snippet countdownProgress()}--> ...If abandoned, remove all related commented blocks.
1877-1880: Remove or document the commented CSS property.The commented
margin-block-startproperty should either be removed if no longer needed, or preserved with a comment explaining why it's disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
🔇 Additional comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (1)
1695-1701: Well done addressing the experimental CSS property!The
@supportswrapper forinterpolate-sizeproperly implements feature detection as recommended in past reviews. This ensures graceful degradation in unsupported browsers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (7)
46-50: Remove unuseduserColumnsprop or implement its usage.This issue remains unresolved from previous reviews. The
userColumnsprop is declared but never used—line 412 shows commented-out usage, and line 428 contains a linter workaround. This is dead code.Apply this diff to remove the unused prop:
- let { - userColumns = [] - }: { - userColumns?: Column[]; - } = $props(); -And remove the workaround:
onMount(async () => { - userColumns; /* silences lint check, variable not read */ - if (spreadsheetContainer) {If this prop is planned for future use, replace the workaround with a TODO comment explaining the intent.
Also applies to: 412-412, 428-428
284-287: Use a standard scroll behavior value.This issue remains unresolved. The
behavior: 'instant'value on line 286 is not part of the ScrollToOptions standard—only'auto'and'smooth'are supported. Browsers may ignore this property.Apply this diff:
directAccessScroller.scrollTo({ left: Math.max(0, scrollLeft), - behavior: 'instant' + behavior: 'auto' });
693-693: Fix type annotation to match null initialization.Line 693 declares
columnSchemawith typeSuggestedColumnSchemabut initializes it tonull. This creates a type mismatch.Apply this diff:
let columnIndex = -1; - let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null;
601-605: UncommentselectedColumnNamereset for complete state cleanup.This issue remains unresolved. The
selectedColumnNameis set on lines 723 and 764 but isn't reset inresetSelectedColumn()due to being commented out on line 604, creating inconsistent state management.Apply this diff:
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
1197-1209: Fix Svelte event bindings: useon:focusinandon:focusout.Lines 1199 and 1205 use plain
onfocusinandonfocusoutattributes which won't bind correctly in Svelte. Use Svelte's event directive syntax.Apply this diff:
<div class="cell-editor" - onfocusin={() => { + on:focusin={() => { isInlineEditing = true; showHeadTooltip = false; resetSelectedColumn(); handlePreviousColumnsBorder(column.id); }} - onfocusout={() => { + on:focusout={() => { showHeadTooltip = true; isInlineEditing = false; handlePreviousColumnsBorder(column.id, false); }}>
1310-1335: Fix Svelte event bindings: useon:mouseenter,on:mouseleave, andon:click.Lines 1313, 1326, and 1327 use plain HTML event attributes (
onmouseenter,onmouseleave,onclick) which won't bind correctly in Svelte. This breaks column hover and selection functionality.Apply this diff:
<Spreadsheet.Cell {root} column={column.id} isEditable={false}> <button + type="button" class="column-selector-button" aria-label="Select column" - onmouseenter={() => { + on:mouseenter={() => { if ( isColumnInteractable && !selectedColumnId && !isInlineEditing && !$isTabletViewport && !$isSmallViewport && !$tableColumnSuggestions.thinking && !creatingColumns ) { hoveredColumnId = column.id; } }} - onmouseleave={() => (hoveredColumnId = null)} - onclick={() => { + on:mouseleave={() => (hoveredColumnId = null)} + on:click={() => { if (isColumnInteractable) { if (!$isTabletViewport) { selectColumnWithId(column); } else if ($isSmallViewport) { triggerColumnId = column.id; } } }}></button> </Spreadsheet.Cell>
1385-1448: Render the Delete/Undo bar only when a column is selected or undo is active.This issue from previous reviews remains partially unresolved. The Delete/Undo bar (lines 1385-1448) currently renders whenever
!creatingColumns, but should only appear whenhasSelectionis true to avoid showing an empty badge or confusing UI states.Apply this diff:
- {#if !creatingColumns} - <div class="floating-action-wrapper expanded" class:selection={hasSelection}> + {#if !creatingColumns && hasSelection} + <div class="floating-action-wrapper expanded" class:selection={true}> <FloatingActionBar>This ensures the Delete/Undo bar only appears when there's an active selection or undo state, while the Review/Edit bar below (lines 1451-1500) remains always visible.
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte (1)
53-53: Consider whether the grid wrapper is necessary.The
display="grid"wrapper around the Tooltip doesn't appear to serve a clear layout purpose for a single child. Unless this is for a specific alignment requirement, it could be simplified.If no layout need exists, consider removing the wrapper:
-<div style:display="grid"> - <Tooltip maxWidth="225px" portal disabled={!headerTooltipText || showing} delay={100}> - <button - onclick={() => enabled && onChildrenClick?.()} - style:cursor={enabled ? 'pointer' : undefined}> - {@render children(toggle)} - </button> +<Tooltip maxWidth="225px" portal disabled={!headerTooltipText || showing} delay={100}> + <button + onclick={() => enabled && onChildrenClick?.()} + style:cursor={enabled ? 'pointer' : undefined}> + {@render children(toggle)} + </button> - <svelte:fragment slot="tooltip"> - {headerTooltipText} - </svelte:fragment> - </Tooltip> -</div> + <svelte:fragment slot="tooltip"> + {headerTooltipText} + </svelte:fragment> +</Tooltip>
📜 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 (3)
package.json(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
🔇 Additional comments (11)
package.json (1)
27-27: Dependency pinning via commit hashes—verify this is the intended pattern.The updates to
@appwrite.io/pink-icons-svelteand@appwrite.io/pink-svelteusepkg.vcURLs pinned to specific commit hashes (634a501) instead of stable version numbers. While this approach may be intentional for pre-release or development workflows, it introduces maintainability and reproducibility risks:
- Commits can be deleted or rewritten, breaking builds.
- URL service availability or API changes could affect resolution.
- Hard to audit or upgrade without explicit commit knowledge.
Other pink-related dependencies in this file (e.g.,
@appwrite.io/pink-iconson line 26) use stable semver versions, which creates an inconsistency.Please confirm:
- Is this commit-hash pinning pattern documented or standard practice in the codebase?
- Are these commits stable and maintained in a way that guarantees long-term availability?
- Should these be updated to use stable semver versions or release tags instead?
Also applies to: 29-29
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte (2)
83-85: LGTM: Footer snippet integration is clean.The optional footer children are rendered correctly with the toggle callback, consistent with how
tooltipChildrenis handled elsewhere in the component.
37-41: No auto-reopen issue:triggerOpen()resets itself on invocation.The implementation in
empty.svelte(lines 1152-1156) showstriggerOpen()resetstriggerColumnId = nullas a side effect before returningtrue. This means the function returnstrueonly once; subsequent calls returnundefined(falsy) since the condition no longer holds. The sheet opens once and won't auto-reopen on subsequent effect runs.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (8)
798-806: LGTM: Column key validation.The validation logic correctly checks for empty column keys before attempting column creation and provides clear user feedback.
816-818: LGTM: Conditional encrypt property handling.Using the
inoperator to conditionally include theencryptproperty only when present is the correct approach for optional properties.
291-325: LGTM: Column highlight positioning logic.The
updateColumnHighlight()function correctly calculates overlay positioning based on the active column and updates CSS custom properties for visual feedback.
689-767: LGTM: Delete/undo workflow logic.The delete and undo functions implement a solid UX pattern with appropriate state management, timer handling, and DOM recalculation. The 10-second undo window is reasonable.
Note: The type annotation issue on line 693 is flagged separately.
962-1040: LGTM: Visual state management effects.The
$effectblocks correctly manage border visibility and resizer display for selection and hover states. The cleanup logic before applying new classes is good practice, and the intentional disabling of auto-scroll on hover (lines 1026-1028) appears deliberate.
1104-1117: LGTM: Overlay elements and styling.The new
column-highlight-overlayandsnowFadeBottomOverlayElelements with their associated CSS provide good visual feedback for selection states and mobile UX. The fade transition and responsive opacity handling are well-implemented.Also applies to: 1349-1353, 1802-1828
1504-1551: LGTM: Column type popover snippet.The
changeColumnTypePopoversnippet correctly implements the column type selector with appropriate conditional rendering, animations, and state updates. The use of Svelte 5 snippet syntax is appropriate.
1605-1776: LGTM: CSS styling additions.The new CSS styles appropriately support the selection, hover, and undo features with:
- Proper z-index layering
- Smooth animations with cubic-bezier easing
- Responsive breakpoints
- Progressive enhancement with
@supportsfor experimental featuresAlso applies to: 1802-1828
...nsole)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (6)
46-50: Remove unused userColumns prop and linter workaround (dead code).Prop is never read; onMount reference only silences the linter. Either wire it or remove it.
- let { - userColumns = [] - }: { - userColumns?: Column[]; - } = $props(); + // (Removed) userColumns prop: add back when there is a real use @@ - onMount(async () => { - userColumns; /* silences lint check, variable not read */ - + onMount(async () => { if (spreadsheetContainer) {Also applies to: 427-429
1379-1442: Show Delete/Undo bar only when there’s a selection or undo is active.Currently renders with empty badge/text when nothing is selected; confusing UX. Gate on hasSelection.
- {#if !creatingColumns} + {#if !creatingColumns && hasSelection} <div class="floating-action-wrapper expanded" class:selection={hasSelection}> <FloatingActionBar>Optional: keep the review/edit bar always visible as-is.
284-287: Use a standard scroll behavior (instant is non-standard).ScrollToOptions supports 'auto' or 'smooth'; 'instant' may be ignored.
directAccessScroller.scrollTo({ left: Math.max(0, scrollLeft), - behavior: 'instant' + behavior: 'auto' });
1190-1202: Fix Svelte event binding: use on:focusin / on:focusout.Plain onfocusin/onfocusout won’t bind in Svelte; handlers don’t run.
- <div - class="cell-editor" - onfocusin={() => { + <div + class="cell-editor" + on:focusin={() => { isInlineEditing = true; showHeadTooltip = false; resetSelectedColumn(); handlePreviousColumnsBorder(column.id); }} - onfocusout={() => { + on:focusout={() => { showHeadTooltip = true; isInlineEditing = false; handlePreviousColumnsBorder(column.id, false); }}>
601-605: Reset selectedColumnName for consistent state cleanup.Leaving it non-null causes stale badge text.
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
692-694: Type correctness: columnSchema can be null.It’s initialized to null but typed non-null; fix union type to avoid TS error.
- let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null;
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (2)
1408-1416: Use resetSelectedColumn() for Cancel to clear all related state.Directly nulling just selectedColumnId leaves selectedColumnName stale if not reset elsewhere.
- <Button.Button + <Button.Button size="xs" variant="text" on:click={() => (selectedColumnId = null)}> Cancel </Button.Button> + <!-- Prefer unified cleanup --> + <!-- on:click={resetSelectedColumn} -->
510-524: Clear scheduled timeouts on destroy to avoid late UI mutations.Multiple setTimeouts are created (animations, undo timers, overlays) and aren’t cleaned up, risking updates after unmount. Track and clear them in onDestroy.
Add tracker and cleanup:
@@ - // for deleting a column + undo + // for deleting a column + undo let undoTimer: ReturnType<typeof setTimeout> | null = $state(null); + const timeouts: ReturnType<typeof setTimeout>[] = []; @@ onDestroy(() => { + timeouts.forEach(clearTimeout); resizeObserver?.disconnect(); hScroller?.removeEventListener('scroll', recalcAllThrottled); if (scrollAnimationFrame) { cancelAnimationFrame(scrollAnimationFrame); }Adopt when scheduling (example):
- setTimeout(() => { + const t = setTimeout(() => { if (index < customColumns.length) { // replace existing placeholder customColumns[index] = { ...column, isPlaceholder: false }; } else { // new column customColumns.push({ ...column, isPlaceholder: false }); } requestAnimationFrame(() => updateOverlayBounds()); - }, index * 150); + }, index * 150); + timeouts.push(t);Repeat for other setTimeout calls (mapping completion, mobile overlay, etc.).
Also applies to: 527-538, 551-561, 719-724, 730-734, 1063-1072
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
🔇 Additional comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (2)
61-64: Placeholders count matches backend cap (7). Good.The initial 7 placeholders align with the suggestions API’s max of 7 columns. Based on learnings
1084-1085: CSS var aliasing: set the custom prop to var(--token), not the token name.Setting --non-overlay-icon-color to the literal “--fgcolor-neutral-weak” won’t resolve to a color. Make it an alias via var().
- style:--non-overlay-icon-color="--fgcolor-neutral-weak" + style:--non-overlay-icon-color="var(--fgcolor-neutral-weak)"No further code changes needed since Icon color prop uses the var name.
Also applies to: 1133-1135
⛔ Skipped due to learnings
Learnt from: ItzNotABug PR: appwrite/console#2373 File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:629-631 Timestamp: 2025-09-26T06:48:57.938Z Learning: In the Appwrite console codebase using appwrite.io/pink-svelte, the Icon component automatically handles CSS variable names passed to its color prop by internally wrapping them with var(). Therefore, passing '--some-css-variable' as a string to the Icon color prop works correctly without needing to manually wrap it with var().
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (6)
46-50: Remove unuseduserColumnsprop or implement its usage.The
userColumnsprop remains unused despite being flagged in previous reviews. Line 413 shows commented-out usage and line 429 contains a linter workaround.
694-694: Fix TypeScript type error:columnSchemacannot be initialized tonull.The type
SuggestedColumnSchemadoesn't includenull, so the assignment fails. This was flagged in a previous review.Apply:
- let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null;
1191-1203: Fix Svelte event bindings: useon:focusinandon:focusout.Plain
onfocusinandonfocusoutattributes won't bind in Svelte. Inline editing state management and border styling will be broken. This was flagged in a previous review but remains unfixed.Apply:
<div class="cell-editor" - onfocusin={() => { + on:focusin={() => { isInlineEditing = true; showHeadTooltip = false; resetSelectedColumn(); handlePreviousColumnsBorder(column.id); }} - onfocusout={() => { + on:focusout={() => { showHeadTooltip = true; isInlineEditing = false; handlePreviousColumnsBorder(column.id, false); }}>
1323-1323: Fix Svelte event binding: useon:clickand set button type.The
onclickattribute won't bind in Svelte. Column selection won't work. This was flagged in previous reviews but remains unfixed.Apply:
}} - onclick={() => { + type="button" + on:click={() => { if (isColumnInteractable) { if (!$isTabletViewport) { selectColumnWithId(column);
602-606: UncommentselectedColumnNamereset for complete state cleanup.The
selectedColumnNameisn't reset inresetSelectedColumn(), causing inconsistent state management. This was flagged in a previous review.Apply:
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
285-288: Use standard scroll behavior value.The
behavior: 'instant'option isn't standard in ScrollToOptions. Use'auto'for immediate scroll or'smooth'for animation. This was flagged in a previous review.Apply:
directAccessScroller.scrollTo({ left: Math.max(0, scrollLeft), - behavior: 'instant' + behavior: 'auto' });
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (1)
817-818: Simplify redundant boolean logic.The expression
column.required || falseis redundant. Whencolumn.requiredis already a boolean or undefined, use nullish coalescing for clarity.Apply:
const baseParams = { databaseId: page.params.database, tableId: page.params.table, key: column.key, - required: column.required || false, + required: column.required ?? false, encrypt: 'encrypt' in column ? column.encrypt : undefined };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte (2)
53-53: Consider simplifying the wrapper display.The
display="grid"wrapper appears unnecessary for a single child. Unless there's a specific layout requirement, a simpler display mode would suffice.-<div style:display="grid"> +<div>
55-59: Past enabled check issue resolved.The desktop button now correctly checks
enabledbefore invokingonChildrenClick?.()(line 56), which resolves the inconsistency flagged in the previous review. The cursor styling (line 57) also appropriately reflects the enabled state.For better accessibility and to fully prevent interaction when disabled, consider adding a
disabledattribute andaria-disabled:<button + disabled={!enabled} + aria-disabled={!enabled} onclick={() => enabled && onChildrenClick?.()} - style:cursor={enabled ? 'pointer' : undefined}> + style:cursor={enabled ? 'pointer' : 'not-allowed'}> {@render children(toggle)} </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte(4 hunks)
🔇 Additional comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte (1)
83-85: LGTM!The mobile footer snippet correctly uses optional chaining and passes a consistent toggle function to close the sheet.
...nsole)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte
Show resolved
Hide resolved
update: allow userColumns in the suggestions sheet.
update: scroll correctly to custom column on mobile.
update: hide tooltip [temporarily].
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (2)
749-753: UncommentselectedColumnNamereset for state consistency.The
selectedColumnNameis assigned in multiple places (lines 871, 912, 1098) but isn't reset inresetSelectedColumn()due to the commented line 752. This creates inconsistent state management whereselectedColumnIdis cleared butselectedColumnNamepersists.Apply this diff:
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
837-882: Fix TypeScript type forcolumnSchema.Line 841 declares
columnSchemaasSuggestedColumnSchema = null, which assignsnullto a non-nullable type. This should be typed as nullable.Apply this diff:
function deleteColumn(columnId: string) { if (!columnId) return; let columnIndex = -1; - let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts
🪛 GitHub Actions: Tests
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
[error] 1696-1696: svelte-check: 'customTooltip' is declared but its value is never read. (ts)
[error] svelte-check reported 1 error and 0 warnings in 1 file.
⏰ 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 (10)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/store.ts (3)
5-57: LGTM: Type extensions and store initialization.The new fields (
force,array,encrypt,elements,isPlaceholder) are properly typed with appropriate optional markers, and the stores are correctly initialized.
141-160: Verify:arrayfield hardcoded tofalse.Line 146 always sets
array: falsefor all suggested columns. If the column suggestions API can return array-type columns or if this will be a future enhancement, consider making this field configurable via theColumnInputtype or deriving it from the input data.If array columns are intentionally not supported in suggestions, this is fine—but worth confirming.
59-124: LGTM: Mock data field renaming.The mock suggestion field names have been updated to more generic terms (e.g.,
publishedYear→year,genre→category). This improves the mock data's reusability.src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (7)
48-95: LGTM:userColumnsprop is now properly utilized.The
userColumnsprop that was previously flagged as unused is now actively used in thestaticUserColumnsderived state (line 68+), where it's mapped with width/ordering logic. The previous concern no longer applies.
106-133: LGTM: State management for column interactions.The new state variables for selection, hover, tooltips, and deletion/undo are properly initialized. The 10-second undo timer limit is reasonable for UX.
176-394: LGTM: Overlay bounds calculation handles user columns.The overlay positioning logic correctly accounts for both user-defined columns and custom suggested columns, with appropriate fallbacks when elements aren't found (e.g., lines 218-222).
396-496: LGTM: Column highlight and scroll handling.The
updateColumnHighlightfunction properly calculates overlay positioning with viewport-aware tooltip placement. The scroll handler correctly resets selection/hover states when columns scroll out of view.
942-1077: LGTM: Column creation with validation and encrypt support.The column creation flow properly validates keys before submission (lines 946-954) and conditionally includes the
encryptproperty only when present (line 965), preventing undefined values in the payload.
1249-1249: Event handlers are valid for Svelte 5.The code uses vanilla event attributes (
onmousemove,onfocusin,onfocusout,onmouseenter,onclick) which are valid in Svelte 5.25.3. These past concerns from previous reviews are no longer applicable as Svelte 5 supports bothoneventandon:eventsyntaxes.Also applies to: 1383-1395, 1504-1518, 1519-1527
1946-1952: LGTM:interpolate-sizeproperly feature-detected.The experimental
interpolate-sizeproperty is now correctly wrapped in an@supportsquery (lines 1949-1951), addressing the previous browser compatibility concern.
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (5)
1250-1251: Fix Svelte event bindings (use on:*) and set button type.Plain DOM attributes won’t bind; selection/hover and editor focus handlers won’t fire. Also add type="button" to avoid unintended submits.
- style:--non-overlay-icon-color="--fgcolor-neutral-weak" - onmousemove={columnHoverMouseTracker}> + style:--non-overlay-icon-color="var(--fgcolor-neutral-weak)" + on:mousemove={columnHoverMouseTracker}> @@ - onfocusin={() => { + on:focusin={() => { isInlineEditing = true; showHeadTooltip = false; resetSelectedColumn(); handlePreviousColumnsBorder(column.id); }} - onfocusout={() => { + on:focusout={() => { showHeadTooltip = true; isInlineEditing = false; handlePreviousColumnsBorder( column.id, false ); }}> @@ - <button + <button + type="button" class="column-selector-button" aria-label="Select column" data-column-hover={column.id} - onmouseenter={() => { + on:mouseenter={() => { if ( isColumnInteractable && !selectedColumnId && !isInlineEditing && !$isTabletViewport && !$isSmallViewport && !$tableColumnSuggestions.thinking && !creatingColumns && hoveredColumnId !== column.id ) { hoveredColumnId = column.id; tooltipTopPosition = 35 + Math.random() * 20; } }} - onclick={() => { + on:click={() => { if (isColumnInteractable) { if (!$isTabletViewport) { selectColumnWithId(column); } else if ($isSmallViewport) { triggerColumnId = column.id; } } }}>Also applies to: 1385-1398, 1503-1530
389-392: Use a standard scroll behavior.ScrollToOptions doesn’t support 'instant'. Replace with 'auto' (or 'smooth' if desired).
- directAccessScroller.scrollTo({ - left: Math.max(0, scrollLeft), - behavior: 'instant' - }); + directAccessScroller.scrollTo({ + left: Math.max(0, scrollLeft), + behavior: 'auto' + });
749-753: Reset selectedColumnName for consistent deselect state.Avoid stale badge/labels after deselect.
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
1579-1642: Gate Delete/Undo bar by selection or undo.Prevents empty badge and confusing “is selected” copy with nothing selected.
-{#if !creatingColumns} - <div class="floating-action-wrapper expanded" class:selection={hasSelection}> +{#if !creatingColumns && hasSelection} + <div class="floating-action-wrapper expanded" class:selection={true}> <FloatingActionBar>
1700-1710: Fix pipeline: remove unused customTooltip snippet (or re-use it).svelte-check error: “customTooltip is declared but its value is never read.” Remove the snippet or restore its usage; removal is simplest.
-{#snippet customTooltip({ text, show })} - <div - transition:fadeSlide - class="custom-tooltip" - style:top={`${tooltipTopPosition}%`} - class:show-tooltip={show && !$isTabletViewport}> - <Typography.Text> - {text} - </Typography.Text> - </div> -{/snippet} +<!-- removed unused customTooltip snippet to fix svelte-check -->
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (3)
233-251: Clamp overlay width to non-negative to avoid invalid CSS.Width can go negative if left overshoots actionsLeft. Clamp to 0.
- const width = actionsLeft - left; + const width = Math.max(0, actionsLeft - left); @@ - const width = right - left; + const width = Math.max(0, right - left);Also applies to: 343-349
419-467: Cast hovered boolean explicitly.isHovered holds a string; cast to boolean for clarity. No runtime change.
- const isHovered = !selectedColumnId && hoveredColumnId; + const isHovered = !selectedColumnId && Boolean(hoveredColumnId);
699-709: Use rAF instead of setTimeout(0) for mobile overlay opacity flip.Minor polish for visual sync with paint.
- if ($isSmallViewport) { - setTimeout(() => { + if ($isSmallViewport) { + requestAnimationFrame(() => { [rangeOverlayEl, fadeBottomOverlayEl, snowFadeBottomOverlayEl].forEach((el) => { if (el) { el.style.opacity = value ? '0' : '1'; } }); - }, 0); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(36 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
🪛 GitHub Actions: Tests
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
[error] 1700-1700: customTooltip is declared but its value is never read. (ts)
⏰ 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 (4)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (4)
106-108: Placeholder count matches backend max (7).Good alignment with suggestColumns limit; keeps UI skeleton consistent. Based on learnings
498-518: Nit: ensure suggested columns are non-resizable consistently.You already set resizable: false; keep consistent across mapped arrays. LGTM otherwise.
Also applies to: 560-562
576-587: Preferences load on mount: good.columnsOrder/columnsWidth hydration before render is correct; avoids layout jumps.
132-134: Store resets and tracking look correct.Undo timer constant, store resets, and analytics events are coherent.
Also applies to: 688-697, 1063-1064
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
| let columnSchema: SuggestedColumnSchema = null; | ||
| for (let index = 0; index < customColumns.length; index++) { |
There was a problem hiding this comment.
Type fix: allow null for columnSchema.
TS: “Type 'null' is not assignable to type 'SuggestedColumnSchema'”. Make it nullable; subsequent guard already narrows.
- let columnSchema: SuggestedColumnSchema = null;
+ let columnSchema: SuggestedColumnSchema | null = null;
@@
- if (columnIndex === -1 || !columnSchema) {
+ if (columnIndex === -1 || !columnSchema) {
return;
}
@@
- columnBeingDeleted = { ...columnSchema, deletedIndex: columnIndex };
+ columnBeingDeleted = { ...columnSchema, deletedIndex: columnIndex };Also applies to: 859-860, 851-853
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
around lines 841-843 (and likewise lines 851-853 and 859-860), the variable
columnSchema is initialized to null but typed as SuggestedColumnSchema, causing
a TS error; change the type declarations to allow null (SuggestedColumnSchema |
null) for each occurrence so the subsequent runtime guard can narrow the type
without error.
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (8)
749-753: UncommentselectedColumnNamereset for complete state cleanup.
selectedColumnNameis set in multiple places (lines 871, 912, 1102) but isn't reset here, creating inconsistent state management.Apply this diff:
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
389-392: Use standard scroll behavior value.
behavior: 'instant'is not standard in ScrollToOptions (only 'auto' and 'smooth' are). Browsers may ignore this non-standard value.Apply this diff:
directAccessScroller.scrollTo({ left: Math.max(0, scrollLeft), - behavior: 'instant' + behavior: 'auto' });
524-549: Don't override persisted user column widths.
staticUserColumnsalready have their widths set fromgetUserColumnWidth(lines 79-81), which respects saved preferences. But line 543 replaces them withdistributedWidth, discarding user preferences.Apply this diff to preserve persisted widths and only distribute to suggested columns:
- const equalWidthColumns = [...staticUserColumns, ...customSuggestedColumns]; + const customCount = customSuggestedColumns.length; + + // calculate total width used by user columns (with their saved widths) + const userTotalWidth = staticUserColumns.reduce((sum, col) => { + const w = col.width; + return sum + (typeof w === 'number' ? w : (w && 'min' in w ? w.min : minColumnWidth)); + }, 0); const totalBaseWidth = fixedWidths.id + fixedWidths.actions + fixedWidths.selection + - equalWidthColumns.length * minColumnWidth; + userTotalWidth + + customCount * minColumnWidth; const viewportWidth = spreadsheetContainer?.clientWidth || (typeof window !== 'undefined' ? window.innerWidth : totalBaseWidth); const excessSpace = Math.max(0, viewportWidth - totalBaseWidth); const extraPerColumn = - equalWidthColumns.length > 0 ? excessSpace / equalWidthColumns.length : 0; + customCount > 0 ? excessSpace / customCount : 0; const distributedWidth = minColumnWidth + extraPerColumn; - const userColumnsWithWidth = staticUserColumns.map((col) => ({ - ...col, - width: distributedWidth - })); + const userColumnsWithWidth = staticUserColumns; // keep their saved widths const finalCustomColumns = customSuggestedColumns.map((col) => ({ ...col, width: { min: distributedWidth } }));
1252-1252: Wrap CSS variable reference withvar().When setting a CSS custom property to reference another CSS variable, you must use
var(). Without it, the value is treated as a literal string.Apply this diff:
- style:--non-overlay-icon-color="--fgcolor-neutral-weak" + style:--non-overlay-icon-color="var(--fgcolor-neutral-weak)"
837-882: Fix type error:columnSchemamust allownull.Line 841 initializes
columnSchematonullbut declares it asSuggestedColumnSchema, causing a TypeScript error. The guard at line 851 already handles the null case correctly.Apply this diff:
function deleteColumn(columnId: string) { if (!columnId) return; let columnIndex = -1; - let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null; for (let index = 0; index < customColumns.length; index++) {
1253-1253: Fix Svelte event binding: useon:mousemove.The
onmousemoveattribute won't bind in Svelte. Column hover tracking will fail.Apply this diff:
style:--overlay-icon-color="#fd366e99" style:--non-overlay-icon-color="var(--fgcolor-neutral-weak)" - onmousemove={columnHoverMouseTracker}> + on:mousemove={columnHoverMouseTracker}>
1582-1646: Only render delete/undo bar when there's an active selection.The delete/undo floating bar (lines 1582-1646) currently renders whenever
!creatingColumns, but should only appear whenhasSelectionis true (column selected or undo active). This can show an empty badge and confusing UI when nothing is selected.Apply this diff:
- {#if !creatingColumns} + {#if !creatingColumns && hasSelection} <div class="floating-action-wrapper expanded" class:selection={hasSelection}>Based on learnings.
1505-1532: Fix Svelte event bindings: useon:mouseenterandon:click(and add button type).Lines 1509 and 1524 use
onmouseenterandonclickwhich won't bind in Svelte. Column hover and selection won't work. Also addtype="button"to prevent form submission.Apply this diff:
<Spreadsheet.Cell {root} column={column.id} isEditable={false}> <button + type="button" class="column-selector-button" aria-label="Select column" data-column-hover={column.id} - onmouseenter={() => { + on:mouseenter={() => { if ( isColumnInteractable && !selectedColumnId && !isInlineEditing && !$isTabletViewport && !$isSmallViewport && !$tableColumnSuggestions.thinking && !creatingColumns && hoveredColumnId !== column.id ) { hoveredColumnId = column.id; tooltipTopPosition = 35 + Math.random() * 20; } }} - onclick={() => { + on:click={() => { if (isColumnInteractable) { if (!$isTabletViewport) { selectColumnWithId(column); } else if ($isSmallViewport) { triggerColumnId = column.id; } } }}></button> </Spreadsheet.Cell>
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (1)
1703-1713: Remove unusedcustomTooltipsnippet or restore its usage.The
customTooltipsnippet is defined but never used—its only call site is commented out at line 1282. Remove this dead code or uncomment line 1282 if the tooltip is intended to be shown.To remove:
-{#snippet customTooltip({ text, show })} - <div - transition:fadeSlide - class="custom-tooltip" - style:top={`${tooltipTopPosition}%`} - class:show-tooltip={show && !$isTabletViewport}> - <Typography.Text> - {text} - </Typography.Text> - </div> -{/snippet} -Or uncomment line 1282 to restore usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
📚 Learning: 2025-09-26T06:48:57.938Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:629-631
Timestamp: 2025-09-26T06:48:57.938Z
Learning: In the Appwrite console codebase using appwrite.io/pink-svelte, the Icon component automatically handles CSS variable names passed to its color prop by internally wrapping them with var(). Therefore, passing '--some-css-variable' as a string to the Icon color prop works correctly without needing to manually wrap it with var().
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
...console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte (5)
520-571: Preserve user column widths instead of overriding with distributedWidth.Lines 541-544 override the carefully preserved widths from
staticUserColumns(which usesgetUserColumnWidthto respect stored preferences) by mapping every column todistributedWidth. This discards user column resize preferences.Apply this diff to preserve user widths and only distribute excess space to suggested columns:
- const equalWidthColumns = [...staticUserColumns, ...customSuggestedColumns]; + const customCount = customSuggestedColumns.length; const totalBaseWidth = fixedWidths.id + fixedWidths.actions + fixedWidths.selection + - equalWidthColumns.length * minColumnWidth; + staticUserColumns.reduce((sum, col) => { + const w = col.width; + return sum + (typeof w === 'number' ? w : (w && 'min' in w ? w.min : minColumnWidth)); + }, 0) + + customCount * minColumnWidth; const viewportWidth = spreadsheetContainer?.clientWidth || (typeof window !== 'undefined' ? window.innerWidth : totalBaseWidth); const excessSpace = Math.max(0, viewportWidth - totalBaseWidth); - const extraPerColumn = - equalWidthColumns.length > 0 ? excessSpace / equalWidthColumns.length : 0; + const extraPerColumn = customCount > 0 ? excessSpace / customCount : 0; const distributedWidth = minColumnWidth + extraPerColumn; - const userColumnsWithWidth = staticUserColumns.map((col) => ({ - ...col, - width: distributedWidth - })); + const userColumnsWithWidth = staticUserColumns; // preserve stored widths const finalCustomColumns = customSuggestedColumns.map((col) => ({ ...col, width: { min: distributedWidth } }));
1253-1253: Fix Svelte event bindings: useon:directive syntax.Multiple event handlers use plain DOM attributes (
onmousemove,onfocusin,onfocusout,onmouseenter,onclick) which don't bind correctly in Svelte. These will break hover tracking, focus handling, and click interactions.Apply this diff:
style:--overlay-icon-color="#fd366e99" style:--non-overlay-icon-color="--fgcolor-neutral-weak" - onmousemove={columnHoverMouseTracker}> + on:mousemove={columnHoverMouseTracker}><div class="cell-editor" - onfocusin={() => { + on:focusin={() => { isInlineEditing = true; showHeadTooltip = false; resetSelectedColumn(); handlePreviousColumnsBorder(column.id); }} - onfocusout={() => { + on:focusout={() => { showHeadTooltip = true; isInlineEditing = false;<button + type="button" class="column-selector-button" aria-label="Select column" data-column-hover={column.id} - onmouseenter={() => { + on:mouseenter={() => {- onclick={() => { + on:click={() => { if (isColumnInteractable) {Also applies to: 1388-1388, 1394-1394, 1509-1509, 1524-1524
837-882: Fix TypeScript error: allow null for columnSchema.Line 841 assigns
nullto a variable typed asSuggestedColumnSchema, causing a type error. The variable must be nullable since it's initialized tonulland guarded at line 851.Apply this diff:
function deleteColumn(columnId: string) { if (!columnId) return; let columnIndex = -1; - let columnSchema: SuggestedColumnSchema = null; + let columnSchema: SuggestedColumnSchema | null = null; for (let index = 0; index < customColumns.length; index++) {
749-753: UncommentselectedColumnNamereset for complete state cleanup.Line 752 leaves
selectedColumnNamecommented out, but this variable is set in multiple places (lines 871, 912, 1102). This creates inconsistent state management.Apply this diff:
function resetSelectedColumn() { selectedColumnId = null; previousColumnId = null; - /*selectedColumnName = null;*/ + selectedColumnName = null; }
1252-1252: Wrap CSS variable reference withvar().Line 1252 forwards a CSS custom property without using
var(), resulting in the literal string"--fgcolor-neutral-weak"instead of the variable's value.Apply this diff:
style:--overlay-icon-color="#fd366e99" - style:--non-overlay-icon-color="--fgcolor-neutral-weak" + style:--non-overlay-icon-color="var(--fgcolor-neutral-weak)" on:mousemove={columnHoverMouseTracker}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte(37 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
PR: appwrite/console#2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
📚 Learning: 2025-09-26T06:48:57.938Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:629-631
Timestamp: 2025-09-26T06:48:57.938Z
Learning: In the Appwrite console codebase using appwrite.io/pink-svelte, the Icon component automatically handles CSS variable names passed to its color prop by internally wrapping them with var(). Therefore, passing '--some-css-variable' as a string to the Icon color prop works correctly without needing to manually wrap it with var().
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
🪛 GitHub Actions: Tests
src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte
[warning] 1796-1796: Unused CSS selector ".custom-tooltip". See https://svelte.dev/e/css_unused_selector (svelte)
[warning] 1796-1796: Unused CSS selector ".custom-tooltip.show-tooltip". See https://svelte.dev/e/css_unused_selector (svelte)
[error] 121-121: 'tooltipTopPosition' is declared but its value is never read. (ts)
[error] 1113-1113: 'fadeSlide' is declared but its value is never read. (ts)
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