Misc fixes. #2310
Misc fixes. #2310
Conversation
ConsoleProject ID: Sites (2)
Note You can use Avatars API to generate QR code for any text or URLs. |
WalkthroughUpdates include dependency bumps in package.json. In the UI: confirm.svelte adds a new public prop to customize the confirmation checkbox label. deleteColumn.svelte now passes a dynamic confirmation label and removes a delete-choice UI. Filterable-columns logic in the table page is refactored to include/exclude Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/lib/components/confirm.svelte (1)
13-13: Configurable checkbox label: good enhancement.Prop name and binding are clear; keeps API backward compatible.
Minor: consider a more generic name (confirmLabel) to reuse the component beyond deletions without implying the action.
Also applies to: 50-50
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/+page.svelte (1)
74-74: System datetime labels switched to “$createdAt”/“$updatedAt”: consistent with keys.Matches the header rendering that prefers column.name for system keys. No issues spotted.
To avoid drift, consider a small map/const for system label overrides instead of inlining strings in multiple places.
Also applies to: 86-86
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (1)
64-65: Minor: defensively handle missing isAction.Until the mapping is fixed, guard to avoid TypeScript grief:
-return [...idColumn, ...columns.filter((column) => !column.isAction), ...systemColumns]; +return [...idColumn, ...columns.filter((c) => !(c as any)?.isAction), ...systemColumns];src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (1)
145-146: Label polish: use a true ellipsis and keep IDs compact.Purely cosmetic:
- label = `${values.join(' | ')} (...${row.$id.slice(-5)})`; + label = `${values.join(' | ')} (…${row.$id.slice(-5)})`;src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/sheetOptions.svelte (2)
166-171: Make “last divider” detection robust to future menu changes.index === menuItems.length - 2 assumes exactly one item after the final divider. Compute it from the tail instead.
- {@const isLastDivider = index === menuItems.length - 2} + {@const isLastDivider = menuItems.slice(index + 1).every((i) => !i.divider)} <div style:margin-inline="-1rem" style:padding-block-start="0.5rem" style:padding-block-end={isLastDivider ? '0.25rem' : '0.5rem'}>
188-194: Avoid relying on CSS nesting; write an explicit selector.If CSS nesting isn’t guaranteed by your tooling, the nested & rule may not apply.
- .action-menu-root { - margin-inline-start: calc(var(--space-2) * -1); - - & :global(:first-child) { - overflow: visible; - } - } + .action-menu-root { + margin-inline-start: calc(var(--space-2) * -1); + } + :global(.action-menu-root > :first-child) { + overflow: visible; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.json(2 hunks)src/lib/components/confirm.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/deleteColumn.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/edit.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/sheetOptions.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte(1 hunks)
🔇 Additional comments (8)
package.json (1)
29-29: Deps bump looks good; please verify pnpm version and UI impact
- pink-svelte@a7a9b5d peers on Svelte ^4.0.0 (matches our project) and isn’t duplicated in pnpm-lock.yaml
- Manually test Spreadsheet, Layout, Tooltip for any UI regressions
- Confirm local and CI are using pnpm 10.15.1 to prevent lockfile churn
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/edit.svelte (1)
171-171: UI spacing tweak LGTM.Increasing gap to xl improves readability in dense forms. No functional impact.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/spreadsheet.svelte (1)
574-575: Verify that updateRow strips system keys
- The implementation of updateRow wasn’t located—confirm it ignores or removes keys in PROHIBITED_ROW_KEYS (e.g.
$id,$createdAt,$updatedAt, etc.).- If it doesn’t, sanitize the payload client-side by filtering out those keys (e.g. via
Object.entries(row).filteragainst PROHIBITED_ROW_KEYS).src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte (2)
59-63: Confirm intent: system columns included only when “selected.”idColumn is included when NOT selected, but systemColumns are included only WHEN selected. If “selected” means hidden columns (as used in hide: selected.includes(...)), this is inverted relative to $id.
If the goal is “filters show visible columns,” flip the predicate:
- ].filter((col) => !!selected.includes(col.id)); + ].filter((col) => !selected.includes(col.id));
72-72: LGTM on using the helper for filterColumns.Nice centralization; once the isAction fix lands, this should behave as intended.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/rows/columns/types/relationship.svelte (2)
125-167: Signature simplification of generateOptions is clean.Dropping the editing param tightens the API; call sites updated accordingly.
181-182: Reactive options call looks correct.Good: guards undefined rows and threads column through the new signature.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/columns/deleteColumn.svelte (1)
92-92: Prop passing bug: use explicit prop syntax
{confirmDeletionLabel}inside the opening tag isn’t a valid Svelte prop and will fail to compile. Pass it as a named prop.Apply this diff:
- {confirmDeletionLabel}> + confirmDeletionLabel={confirmDeletionLabel}>Likely an incorrect or invalid review comment.
| function createFilterableColumns(columns: Column[], selected: string[] = []): Column[] { | ||
| const idColumn = [{ id: '$id', title: '$id', type: 'string' as ColumnType }].filter( | ||
| (col) => !selected.includes(col.id) | ||
| ); | ||
|
|
||
| const systemColumns = [ | ||
| { id: '$createdAt', title: '$createdAt', type: 'datetime' as ColumnType }, | ||
| { id: '$updatedAt', title: '$updatedAt', type: 'datetime' as ColumnType } | ||
| ].filter((col) => !!selected.includes(col.id)); | ||
|
|
||
| return [...idColumn, ...columns.filter((column) => !column.isAction), ...systemColumns]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Action columns are no longer filtered out (isAction lost in mapping).
createFilterableColumns relies on column.isAction, but createTableColumns doesn’t carry this flag over, so action columns will leak into Filters.
Apply one of the following:
- Preserve isAction in the mapping (preferred; update the type if needed):
function createTableColumns(columns: Columns[], selected: string[] = []): Column[] {
return columns.map((column) => ({
id: column.key,
title: column.key,
type: column.type as ColumnType,
hide: !!selected?.includes(column.key),
array: column?.array,
format: 'format' in column && column?.format === 'enum' ? column.format : null,
elements: 'elements' in column ? column.elements : null,
// ensure this is in Column type: isAction?: boolean
isAction: (column as any)?.isAction === true
}));
}- Or filter before mapping:
const raw = $table.columns.filter((c) => !c.isAction);
const freshColumns = createTableColumns(raw, selected);🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/+page.svelte
around lines 54-66, createFilterableColumns uses column.isAction but
createTableColumns drops that flag during mapping; preserve isAction when
mapping (and update the Column type to include isAction?: boolean) so action
columns remain marked and will be filtered out, or alternatively filter out
action columns from the source before mapping (e.g., $table.columns.filter(c =>
!c.isAction) then call createTableColumns) — choose the preserve-isAction
approach as preferred and ensure hide/other properties remain unchanged.
| const relatedColumn = $derived( | ||
| requiresTwoWayConfirm ? getAsRelationship(selectedColumns[0]) : undefined | ||
| ); | ||
|
|
||
| const confirmDeletionLabel = $derived( | ||
| !requiresTwoWayConfirm | ||
| ? 'I understand and confirm' | ||
| : `Delete relationship between ${relatedColumn.key} to ${relatedColumn.twoWayKey}` | ||
| ); |
There was a problem hiding this comment.
Fix two‑way label selection to avoid wrong column and runtime crash
requiresTwoWayConfirm can be true even when selectedColumns[0] isn’t the two‑way relationship (or is a string). Accessing relatedColumn.key in that case will be incorrect and can throw. Find the actual two‑way relationship column instead, and fix the label grammar (“between … and …”).
Apply this diff:
- const relatedColumn = $derived(
- requiresTwoWayConfirm ? getAsRelationship(selectedColumns[0]) : undefined
- );
-
- const confirmDeletionLabel = $derived(
- !requiresTwoWayConfirm
- ? 'I understand and confirm'
- : `Delete relationship between ${relatedColumn.key} to ${relatedColumn.twoWayKey}`
- );
+ const twoWayColumn = $derived(
+ selectedColumns.find(
+ (c): c is Models.ColumnRelationship =>
+ typeof c !== 'string' && isRelationship(c) && c.twoWay
+ )
+ );
+
+ const confirmDeletionLabel = $derived(
+ twoWayColumn
+ ? `Delete relationship between ${twoWayColumn.key} and ${twoWayColumn.twoWayKey}`
+ : 'I understand and confirm'
+ );Note: If you adopt this, getAsRelationship becomes unused and can be removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const relatedColumn = $derived( | |
| requiresTwoWayConfirm ? getAsRelationship(selectedColumns[0]) : undefined | |
| ); | |
| const confirmDeletionLabel = $derived( | |
| !requiresTwoWayConfirm | |
| ? 'I understand and confirm' | |
| : `Delete relationship between ${relatedColumn.key} to ${relatedColumn.twoWayKey}` | |
| ); | |
| const twoWayColumn = $derived( | |
| selectedColumns.find( | |
| (c): c is Models.ColumnRelationship => | |
| typeof c !== 'string' && isRelationship(c) && c.twoWay | |
| ) | |
| ); | |
| const confirmDeletionLabel = $derived( | |
| twoWayColumn | |
| ? `Delete relationship between ${twoWayColumn.key} and ${twoWayColumn.twoWayKey}` | |
| : 'I understand and confirm' | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
319-325: Fix reactive order and NaN risk in credits/total computationAs written,
totalAmountcomputes beforecreditsApplied(top-to-bottom reactive order in Svelte), andavailableCreditcan beundefined. This can yieldNaNand a stale first render. Compute credits first, coalesceavailableCredit, and use plan price consistently.Apply:
@@ - $: billingData = getBillingData(currentPlan, currentAggregation, $isSmallViewport); - - $: totalAmount = Math.max(currentPlan?.price - creditsApplied, 0); - - $: creditsApplied = Math.min( - currentAggregation?.amount ?? currentPlan?.price ?? 0, - availableCredit - ); + $: billingData = getBillingData(currentPlan, currentAggregation, $isSmallViewport); + // Compute credits before total; handle undefineds + $: planPrice = nextPlan?.price ?? currentPlan?.price ?? 0; + $: creditsApplied = Math.min( + currentAggregation?.amount ?? planPrice, + availableCredit ?? 0 + ); + $: totalAmount = Math.max(planPrice - creditsApplied, 0);
🧹 Nitpick comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
454-476: Drop redundantexpandable={false}on summary cells; avoid duplicate row id
ExpandableTable.Celllikely defaults to non-expandable; the explicitexpandable={false}is noise.- Both summary rows use
id="total-row". Even if mapped to a data-attribute, duplicate identifiers can confuse DOM/tests. Rename the credits row id.Apply:
- <ExpandableTable.Row {root} id="total-row" expandable={false}> + <ExpandableTable.Row {root} id="credits-row" expandable={false}> - <ExpandableTable.Cell {root} column="item" expandable={false}> + <ExpandableTable.Cell {root} column="item"> ... - <ExpandableTable.Cell {root} column="usage" expandable={false}> + <ExpandableTable.Cell {root} column="usage"> ... - <ExpandableTable.Cell {root} column="price" expandable={false}> + <ExpandableTable.Cell {root} column="price">And:
- <ExpandableTable.Row {root} id="total-row" expandable={false}> + <ExpandableTable.Row {root} id="total-row" expandable={false}> - <ExpandableTable.Cell {root} column="item" expandable={false}> + <ExpandableTable.Cell {root} column="item"> ... - <ExpandableTable.Cell {root} column="usage" expandable={false}> + <ExpandableTable.Cell {root} column="usage"> ... - <ExpandableTable.Cell {root} column="price" expandable={false}> + <ExpandableTable.Cell {root} column="price">Also applies to: 480-494
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(2 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ 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 (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
363-363: No changes needed:ExpandableTable.Cellsupports theexpandableprop
Verified innode_modules/@appwrite.io/pink-svelte/dist/expandable-table/cell/Cell.sveltethatexport let expandable = trueis declared.
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
New Features
Bug Fixes
Style
Chores