Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a confirmation AlertDialog for record deletion in the table tab, moves delete to a two-step flow, adds tooltips for PK/FK icons, updates preset row-count UI and constant, bumps server version and changelog, and updates several public site assets and roadmap entries. Changes
Sequence DiagramsequenceDiagram
participant User
participant DeleteBtn
participant AlertDialog
participant DeleteHandler
participant ForcedDeleteDialog
User->>DeleteBtn: Click delete button
DeleteBtn->>AlertDialog: Open confirmation dialog (title/description based on selection)
AlertDialog->>User: Show Cancel / Delete actions
User->>AlertDialog: Click Delete
AlertDialog->>DeleteBtn: Trigger handleConfirmDelete
DeleteBtn->>DeleteHandler: Execute deletion request
alt Deletion succeeds
DeleteHandler->>DeleteBtn: Return success
DeleteBtn->>AlertDialog: Close dialog, refresh UI
else Deletion fails (related records)
DeleteHandler->>DeleteBtn: Return error
DeleteBtn->>ForcedDeleteDialog: Open forced-delete dialog (fallback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
www/src/lib/content/changelog.ts (1)
19-55:⚠️ Potential issue | 🟡 MinorChangelog entry title and date were not updated for 1.3.31.
The version was bumped from 1.3.30 to 1.3.31, and a new improvement was added, but the
title(Line 21: "Add table actions menu") anddate(Line 20: "2026-02-15") still reflect the previous release. If 1.3.31 is a distinct release, consider updating the title to something like "Add confirmation dialog for delete record" and setting the date to today's date, or splitting this into a separate changelog entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/src/lib/content/changelog.ts` around lines 19 - 55, The changelog object for version "1.3.31" has stale metadata: update the title and date fields to match this release (e.g., change title from "Add table actions menu" to a current title like "Add confirmation dialog for delete record" or another accurate summary, and set date to today's date) so the entry for version "1.3.31" is consistent; locate the changelog entry by the version property "1.3.31" and modify the title and date fields accordingly in the object where features, bugsFixed, and improvements are defined.www/src/lib/content/roadmap.ts (1)
86-109:⚠️ Potential issue | 🟡 Minor"Data Management Improvements" parent status should be "in-progress".
Two of five items under this section are now marked as
"completed"(lines 91, 99), but the parent status on Line 87 remains"planned". This is inconsistent with the pattern used for other sections (e.g., "Core Views & Tabs" and "Multi-Database Support" are both"in-progress"when they have a mix of completed and planned items).Proposed fix
{ title: "Data Management Improvements", - status: "planned", + status: "in-progress", items: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/src/lib/content/roadmap.ts` around lines 86 - 109, The parent entry with title "Data Management Improvements" currently has status "planned" but contains a mix of completed and planned items; update its status to "in-progress" in the roadmap object in www/src/lib/content/roadmap.ts by changing the status field on the "Data Management Improvements" entry from "planned" to "in-progress" so it matches the item states and the pattern used by other mixed-status sections.packages/core/src/components/table-tab/header/delete-btn.tsx (1)
39-53:⚠️ Potential issue | 🔴 CriticalBug:
relatedRecordslocal variable shadows state and FK violations aren't handled correctly.Line 41 declares
let relatedRecords: RelatedRecord[] | undefinedwhich shadows the state variable from Line 33 and is never assigned. More critically, the error handling approach is broken: FK violations return successfully fromdeleteCells()withfkViolation: truein the result—they don't throw an exception. This means the catch block (Lines 48-52) never executes for FK violations, and theForceDeleteDialognever receives related record data.Additionally,
use-delete-cell.ts(lines 140-143) handles 409 responses but discards therelatedRecordswhen returning, compounding the problem.The correct pattern exists elsewhere in the codebase (sidebar-list-tables-menu.tsx): check
result.fkViolationon success and extractresult.relatedRecordsto pass to the dialog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/table-tab/header/delete-btn.tsx` around lines 39 - 53, handleConfirmDelete currently declares a useless local relatedRecords and relies on exceptions for FK violations; instead, remove the local relatedRecords variable and inspect the result returned by deleteCells for result.fkViolation and result.relatedRecords after the await; if fkViolation is true, call setPendingRowData(rowData), setRelatedRecords(result.relatedRecords || []), and setIsOpenFkDialog(true) otherwise proceed to setRowSelection({}) on success; also update the deletion hook (use-delete-cell.ts) to propagate relatedRecords in its successful response when a 409/FK case is detected so callers can read result.relatedRecords.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/components/table-tab/header/delete-btn.tsx`:
- Around line 39-53: handleConfirmDelete currently declares a useless local
relatedRecords and relies on exceptions for FK violations; instead, remove the
local relatedRecords variable and inspect the result returned by deleteCells for
result.fkViolation and result.relatedRecords after the await; if fkViolation is
true, call setPendingRowData(rowData), setRelatedRecords(result.relatedRecords
|| []), and setIsOpenFkDialog(true) otherwise proceed to setRowSelection({}) on
success; also update the deletion hook (use-delete-cell.ts) to propagate
relatedRecords in its successful response when a 409/FK case is detected so
callers can read result.relatedRecords.
In `@www/src/lib/content/changelog.ts`:
- Around line 19-55: The changelog object for version "1.3.31" has stale
metadata: update the title and date fields to match this release (e.g., change
title from "Add table actions menu" to a current title like "Add confirmation
dialog for delete record" or another accurate summary, and set date to today's
date) so the entry for version "1.3.31" is consistent; locate the changelog
entry by the version property "1.3.31" and modify the title and date fields
accordingly in the object where features, bugsFixed, and improvements are
defined.
In `@www/src/lib/content/roadmap.ts`:
- Around line 86-109: The parent entry with title "Data Management Improvements"
currently has status "planned" but contains a mix of completed and planned
items; update its status to "in-progress" in the roadmap object in
www/src/lib/content/roadmap.ts by changing the status field on the "Data
Management Improvements" entry from "planned" to "in-progress" so it matches the
item states and the pattern used by other mixed-status sections.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/components/table-tab/table-head-row.tsx (3)
114-127:⚠️ Potential issue | 🟡 Minor
handleConfirmDeletehas no error handling — dialog state leaks on failureIf
deleteColumnrejects, none of the cleanup (setDeleteDialogOpen(false),setColumnToDelete(null),setCascadeDelete(false)) runs, leaving the dialog stuck open with stale state.🛡️ Proposed fix
const handleConfirmDelete = useCallback(async () => { if (!columnToDelete || !activeTableName) return; - await deleteColumn({ - db: activeTableName, - tableName: activeTableName, - columnName: columnToDelete, - cascade: cascadeDelete, - }); - - setDeleteDialogOpen(false); - setColumnToDelete(null); - setCascadeDelete(false); + try { + await deleteColumn({ + db: activeTableName, + tableName: activeTableName, + columnName: columnToDelete, + cascade: cascadeDelete, + }); + } finally { + setDeleteDialogOpen(false); + setColumnToDelete(null); + setCascadeDelete(false); + } }, [columnToDelete, activeTableName, deleteColumn, cascadeDelete]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/table-tab/table-head-row.tsx` around lines 114 - 127, handleConfirmDelete currently calls deleteColumn without error handling, so if deleteColumn rejects the cleanup (setDeleteDialogOpen, setColumnToDelete, setCascadeDelete) never runs; wrap the await deleteColumn(...) call in try/catch and move the state cleanup into a finally block so the dialog always closes and state resets, and in the catch log or surface the error (e.g., via console.error or a toast) to inform the user. Ensure you update the useCallback body for handleConfirmDelete, keeping the same dependencies.
313-317:⚠️ Potential issue | 🟡 MinorWhitespace artifact in dialog description — closing
"is on a new lineJSX normalises the newline + indentation between
{columnToDelete}and"?into a space, rendering as"columnName "?(space before the closing quote).✏️ Proposed fix
- Are you sure you want to delete the column "{columnToDelete} - "? This action cannot be undone and will permanently remove all data in this - column. + Are you sure you want to delete the column "{columnToDelete}"? This action + cannot be undone and will permanently remove all data in this column.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/table-tab/table-head-row.tsx` around lines 313 - 317, The AlertDialogDescription contains a whitespace artifact because the closing quote and question mark are on the next line after the {columnToDelete} interpolation; update the JSX around AlertDialogDescription in table-head-row.tsx so the punctuation directly follows the {columnToDelete} expression (i.e., move the closing quote and ? to the same line/JSX node as {columnToDelete}) to remove the extra space; locate the AlertDialogDescription element that references columnToDelete and adjust the template so the expression and trailing punctuation are contiguous.
117-122:⚠️ Potential issue | 🟠 Major
dbis incorrectly set to the table name instead of the database identifier
activeTableNameis sourced fromRoute.useParams()as the$tableURL segment — it is the table name, not the database name. Passing it asdbcreates a mismatch with the function's contract, which expects a database identifier.The
deleteColumnhook actually retrieves the correct database fromuseDatabaseStore()and ignores thedbparameter passed by the component, but this is confusing and fragile: if the hook implementation changes to use the passeddbvalue, the deletion will fail with an incorrect database identifier.Fix: Remove the
dbparameter from thedeleteColumncall entirely, since the hook manages database selection via the store:Suggested fix
await deleteColumn({ - db: activeTableName, tableName: activeTableName, columnName: columnToDelete, cascade: cascadeDelete, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/table-tab/table-head-row.tsx` around lines 117 - 122, The call to deleteColumn incorrectly passes db: activeTableName (activeTableName comes from Route.useParams() and is a table name); remove the db property from the deleteColumn call so the hook uses the database from useDatabaseStore() as intended. Update the invocation in table-head-row.tsx (where deleteColumn is called with db, tableName: activeTableName, columnName: columnToDelete, cascade: cascadeDelete) to omit the db field and keep tableName, columnName and cascade intact; ensure no other code in this component relies on that db argument.
🧹 Nitpick comments (1)
www/public/robots.txt (1)
6-8: RedundantAllowdirectives underUser-agent: *
Allow: /already permits all paths for all crawlers. The explicitAllow: /roadmap,Allow: /docs, andAllow: /changeloglines are no-ops here and can be removed for clarity.🧹 Optional cleanup
User-agent: * Allow: / -Allow: /roadmap -Allow: /docs -Allow: /changelog🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/public/robots.txt` around lines 6 - 8, In the User-agent: * section of robots.txt remove the redundant explicit Allow directives for specific paths (the lines containing "Allow: /roadmap", "Allow: /docs", and "Allow: /changelog") because "Allow: /" already permits all paths; keep the single "Allow: /" directive (or explicit path allows only if you intend to restrict others) and update comments if needed for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@www/src/lib/content/changelog.ts`:
- Around line 19-20: The changelog entry currently shows version: "1.3.32" with
date "2026-02-18" but skips 1.3.31; either correct the version string in the
changelog object from "1.3.32" to "1.3.31" (update the line containing version:
"1.3.32") or, if 1.3.32 is intentional because 1.3.31 was released separately,
add a short comment/note near the entry clarifying that 1.3.31 was a separate
hotfix so the version jump is intentional.
---
Outside diff comments:
In `@packages/core/src/components/table-tab/table-head-row.tsx`:
- Around line 114-127: handleConfirmDelete currently calls deleteColumn without
error handling, so if deleteColumn rejects the cleanup (setDeleteDialogOpen,
setColumnToDelete, setCascadeDelete) never runs; wrap the await
deleteColumn(...) call in try/catch and move the state cleanup into a finally
block so the dialog always closes and state resets, and in the catch log or
surface the error (e.g., via console.error or a toast) to inform the user.
Ensure you update the useCallback body for handleConfirmDelete, keeping the same
dependencies.
- Around line 313-317: The AlertDialogDescription contains a whitespace artifact
because the closing quote and question mark are on the next line after the
{columnToDelete} interpolation; update the JSX around AlertDialogDescription in
table-head-row.tsx so the punctuation directly follows the {columnToDelete}
expression (i.e., move the closing quote and ? to the same line/JSX node as
{columnToDelete}) to remove the extra space; locate the AlertDialogDescription
element that references columnToDelete and adjust the template so the expression
and trailing punctuation are contiguous.
- Around line 117-122: The call to deleteColumn incorrectly passes db:
activeTableName (activeTableName comes from Route.useParams() and is a table
name); remove the db property from the deleteColumn call so the hook uses the
database from useDatabaseStore() as intended. Update the invocation in
table-head-row.tsx (where deleteColumn is called with db, tableName:
activeTableName, columnName: columnToDelete, cascade: cascadeDelete) to omit the
db field and keep tableName, columnName and cascade intact; ensure no other code
in this component relies on that db argument.
---
Nitpick comments:
In `@www/public/robots.txt`:
- Around line 6-8: In the User-agent: * section of robots.txt remove the
redundant explicit Allow directives for specific paths (the lines containing
"Allow: /roadmap", "Allow: /docs", and "Allow: /changelog") because "Allow: /"
already permits all paths; keep the single "Allow: /" directive (or explicit
path allows only if you intend to restrict others) and update comments if needed
for clarity.
www/src/lib/content/changelog.ts
Outdated
| version: "1.3.32", | ||
| date: "2026-02-18", |
There was a problem hiding this comment.
Version jumps from 1.3.30 → 1.3.32, skipping 1.3.31
Please confirm this is intentional (e.g., a hotfix was shipped separately at 1.3.31). If not, correct the version number for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@www/src/lib/content/changelog.ts` around lines 19 - 20, The changelog entry
currently shows version: "1.3.32" with date "2026-02-18" but skips 1.3.31;
either correct the version string in the changelog object from "1.3.32" to
"1.3.31" (update the line containing version: "1.3.32") or, if 1.3.32 is
intentional because 1.3.31 was released separately, add a short comment/note
near the entry clarifying that 1.3.31 was a separate hotfix so the version jump
is intentional.
1 participant
This PR was automatically updated from the
stagebranch.Triggered by push to stage: Added preset row count options to the table footer (resolves #38)
Commit: 0831490
Summary by CodeRabbit
New Features
Documentation