Skip to content

feat: connection groups with multi-selection and hierarchical management#140

Open
imhuytq wants to merge 24 commits intodatlechin:mainfrom
imhuytq:feat/connection-groups
Open

feat: connection groups with multi-selection and hierarchical management#140
imhuytq wants to merge 24 commits intodatlechin:mainfrom
imhuytq:feat/connection-groups

Conversation

@imhuytq
Copy link
Contributor

@imhuytq imhuytq commented Mar 2, 2026

Summary

Adds a hierarchical connection groups system to the welcome window, letting users organize database connections into named, color-coded folders with arbitrary nesting depth.

Core features

  • Hierarchical groups: create nested folders with color-coded icons to organize connections
  • NSOutlineView: replaces SwiftUI List with NSOutlineView for native tree view, expand/collapse, and drag-and-drop
  • Multi-selection: select multiple connections for bulk delete, bulk move to group, and multi-drag into groups
  • Drag-and-drop: reorder connections and groups within/across groups, move connections between groups (single and multi), with position-aware drop
  • Keyboard shortcuts: Backspace to delete selected items, Return to connect/toggle
  • 2-step delete confirmation: deleting a group with connections requires two confirmations to prevent accidental data loss
  • Expand/collapse persistence: group expanded state persists across app launches
  • Context menus: full context menu support for groups (new subgroup, edit, delete) and connections (connect, edit, duplicate, copy URL, move to group, delete)

Data model changes

  • ConnectionGroup: new model with parentGroupId for nesting, sortOrder, and color
  • DatabaseConnection.sortOrder: new field for stable ordering within groups
  • GroupStorage: CRUD, hierarchy helpers, expanded state persistence, descendant deletion with connection cleanup
  • Legacy migration: auto-assigns sequential sortOrder when upgrading from older versions without ordering

Storage behavior

  • Deleting a group deletes all connections inside (not ungroup)
  • ConnectionStorage.duplicateConnection preserves all fields and inserts right after the original
  • ConnectionFormView preserves sortOrder when editing existing connections

UI/UX details

  • Selection managed natively by NSOutlineView (no SwiftUI state overhead)
  • Scroll position and expanded state preserved across data reloads
  • Multi-group drag disabled (moving parent already moves children)
  • Empty state overlay allows context menu on empty list
  • Group form sheet loads groups once on appear (not on every recompute)

Summary by CodeRabbit

Release Notes

  • New Features

    • Connection groups: Organize connections into named, color-coded folders with nested subgroups, drag-and-drop reordering, and expand/collapse functionality
    • Group state persistence: Expanded/collapsed states are now automatically saved
  • Documentation

    • Added download guides in English and Vietnamese with Homebrew installation and direct download options for different platforms

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@imhuytq imhuytq force-pushed the feat/connection-groups branch from c3497e9 to e00e14e Compare March 2, 2026 11:34
@datlechin
Copy link
Owner

Run bị lỗi rồi anh ơi

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift
/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:239:8 Type 'DatabaseConnection' does not conform to protocol 'Hashable'

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:239:8 Type 'DatabaseConnection' does not conform to protocol 'Equatable'

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:256:9 Invalid redeclaration of 'groupId'

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:276:9 Invalid redeclaration of 'groupId'

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:313:1 Type 'DatabaseConnection' does not conform to protocol 'Decodable'

/Users/ngoquocdat/Projects/TablePro/TablePro/Models/DatabaseConnection.swift:313:1 Type 'DatabaseConnection' does not conform to protocol 'Encodable'

@imhuytq
Copy link
Contributor Author

imhuytq commented Mar 4, 2026

@datlechin please review

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a hierarchical “connection groups” feature to the welcome window, enabling nested folders for organizing database connections with expand/collapse persistence and drag-and-drop reordering.

Changes:

  • Replaces the SwiftUI List connection list with an NSOutlineView-backed tree view that supports nesting, drag/drop, and context menus.
  • Introduces a reusable group create/edit sheet and updates group pickers to support nested group selection.
  • Extends storage/models to support group hierarchy (parentGroupId), ordering (sortOrder), and persisted expanded state.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
TablePro/Views/WelcomeWindowView.swift Integrates group CRUD + expand state and swaps in the outline-based connection list.
TablePro/Views/WelcomeWindow/ConnectionOutlineView.swift New NSOutlineView wrapper implementing hierarchy rendering, drag/drop reorder, and context menus.
TablePro/Views/Connection/ConnectionGroupPicker.swift Updates the connection-form group picker to render nested groups and adds group management actions.
TablePro/Views/Connection/ConnectionGroupFormSheet.swift New create/edit group sheet with color and parent-group selection (cycle prevention).
TablePro/Views/Connection/ConnectionFormView.swift Minor update to reflect group support in form state (comment only in this diff).
TablePro/Resources/Localizable.xcstrings Adds/removes localization keys used by new group UI.
TablePro/Models/DatabaseConnection.swift Adds sortOrder to support stable ordering/reordering.
TablePro/Models/ConnectionGroup.swift Adds hierarchy + ordering fields and migration-aware decoding.
TablePro/Core/Storage/GroupStorage.swift Adds hierarchy helpers, expanded-state persistence, and descendant deletion behavior.
TablePro/Core/Storage/ConnectionStorage.swift Persists sortOrder with migration fallback for pre-existing connections.
CHANGELOG.md Documents the new “Connection groups” feature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@datlechin
Copy link
Owner

@imhuytq fix conflict lun nha a :d

@imhuytq
Copy link
Contributor Author

imhuytq commented Mar 6, 2026

@datlechin review giúp với

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

datlechin and others added 2 commits March 6, 2026 13:28
- GroupStorage.addGroup returns Bool; duplicate check scoped to siblings
- ConnectionGroupPicker delete now requires confirmation dialog
- acceptGroupDrop adjusts childIndex for mixed group/connection children
- isDragging set only after validation passes in drag source
- totalConnectionCount guards against circular parentGroupId cycles
- duplicateConnection returns placed copy with correct sortOrder
- 2-step delete step2 only shown for group-resident connections
- Remove duplicated collectDescendantIds; reuse GroupStorage method
- Remove unnecessary comments in WelcomeWindowView
@imhuytq imhuytq changed the title feat: add connection groups feat: connection groups with multi-selection and hierarchical management Mar 6, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@datlechin datlechin force-pushed the feat/connection-groups branch from 5620545 to 864f32a Compare March 6, 2026 08:14
@datlechin
Copy link
Owner

Screenshot 2026-03-06 at 19 45 01 Em chạy lên thấy màu active hình như bị nhạt hơn so với bình thường

# Conflicts:
#	TablePro/Core/Storage/ConnectionStorage.swift
#	TablePro/Models/DatabaseConnection.swift
#	TablePro/Resources/Localizable.xcstrings
#	TablePro/Views/Connection/ConnectionGroupPicker.swift
#	TablePro/Views/Connection/WelcomeWindowView.swift
@github-actions
Copy link
Contributor

Thank you for your contribution! Before we can merge this PR, you need to sign our Contributor License Agreement.

To sign, please comment below with:

I have read the CLA Document and I hereby sign the CLA.


I have read the CLA Document and I hereby sign the CLA.


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@coderabbitai
Copy link

coderabbitai bot commented Mar 18, 2026

📝 Walkthrough

Walkthrough

This PR introduces a connection groups feature enabling hierarchical organization of database connections into color-coded folders with sorting, persist expanded/collapsed states, and connection ordering. Data models are extended with parent-child relationships and sort orders across storage and view layers. Documentation updated with download instructions.

Changes

Cohort / File(s) Summary
Data Model Expansion
TablePro/Models/Connection/ConnectionGroup.swift, TablePro/Models/Connection/DatabaseConnection.swift
Added parentGroupId and sortOrder fields to ConnectionGroup; added sortOrder to DatabaseConnection. Updated ConnectionGroup initializer with new parameters and custom Codable migration layer with default values (.blue color, sortOrder 0).
Storage Layer - Groups
TablePro/Core/Storage/GroupStorage.swift
Introduced expanded-groups persistence with load/save methods. Added sortOrder migration when all groups have sortOrder = 0. Enhanced addGroup() with duplicate name checking and boolean return. Extended deleteGroup() to cascade delete descendants and related connection credentials. Added helpers: childGroups(of:), nextSortOrder(parentId:), loadExpandedGroupIds(), saveExpandedGroupIds(), collectDescendantIds(), and connectionCount().
Storage Layer - Connections
TablePro/Core/Storage/ConnectionStorage.swift
Added sortOrder field to StoredConnection with Codable support. Implemented migrateSortOrderIfNeeded() for legacy data. Enhanced duplicate logic to assign computed sortOrder and adjust siblings. Updated load path to invoke migration and return mutable array.
View Layer
TablePro/Views/Connection/ConnectionFormView.swift
Updated documentation comment. Preserved original connection's sortOrder when editing existing connections before saving.
Documentation
CHANGELOG.md
Added feature description for connection groups covering organization, color-coding, nesting, drag-and-drop reordering, state persistence, and multi-selection.
Download Documentation
docs/download.mdx, docs/vi/download.mdx
New download pages with Homebrew installation instructions, platform-specific download links (Apple Silicon/Intel), version guidance, post-installation steps including macOS quarantine handling, and links to related resources. Vietnamese and English versions included.
Documentation Navigation
docs/docs.json
Updated navbar primary Download button from external GitHub URL to internal /download path. Added download page entries to English and Vietnamese site navigation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Whiskers twitching with glee,
Groups now nest like trees,
Drag and drop with care,
Sort orders floating through the air,
Hierarchies bloom in colors bright—
Connections organized just right! 🌈

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main feature additions: connection groups, multi-selection, and hierarchical management, which are central to this comprehensive changeset.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
docs/docs.json (1)

220-226: Consider adding Chinese download page for consistency.

The English (line 29) and Vietnamese (line 124) Getting Started sections now include download, but the Chinese section does not have zh/download. If Chinese documentation should maintain parity, consider adding it here or tracking it for future work.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/docs.json` around lines 220 - 226, The Chinese "快速开始" section in
docs.json is missing the zh/download entry; update the pages array for the group
"快速开始" to include "zh/download" alongside "zh/index", "zh/quickstart",
"zh/installation", and "zh/changelog" so the Chinese docs mirror the English and
Vietnamese Getting Started sections.
docs/download.mdx (1)

35-35: Apple logo character may not render universally.

The Apple logo character (``) in "Apple menu ()" relies on Apple-specific fonts and may display as a missing glyph on non-Apple devices or certain browsers. Consider using "Apple menu" without the symbol, or add fallback text like "Apple menu (Apple icon)".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/download.mdx` at line 35, Replace the Apple logo glyph in the string
"Click the **Apple menu** () > **About This Mac**:" with either plain text or a
fallback label; specifically update the phrase "Apple menu ()" to "Apple menu"
or "Apple menu (Apple icon)" so the documentation doesn't rely on an
Apple-specific glyph that may not render on all devices.
docs/vi/download.mdx (1)

35-35: Same Apple logo rendering concern applies here.

As noted in the English version, the Apple logo character (``) may not render on non-Apple devices.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/vi/download.mdx` at line 35, The Apple logo character in the sentence
"Nhấp vào **menu Apple** () > **Giới thiệu về máy Mac này**" may not render on
non‑Apple devices; update that fragment (the "menu Apple" line) to use a
visible, accessible fallback instead of relying on the single logo glyph—either
replace the empty parentheses with the word "Apple" (e.g., "menu Apple (Apple
logo)"), or replace the glyph with an inline image/SVG that includes appropriate
alt text, matching the approach used in the English doc to ensure correct
rendering and accessibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 239: The changelog entry "Connection groups: organize connections into
named, color-coded folders with support for nested subgroups, drag-and-drop
reordering, expand/collapse state persistence, multi-selection (bulk delete,
bulk move to group), and context menus for group and connection management"
should be removed from the "## [0.14.1]" section and added under "##
[Unreleased]" inside the "### Added" subsection; locate that exact entry text in
CHANGELOG.md, cut it from the 0.14.1 section and paste it as a new bullet under
the Unreleased -> Added heading so the feature is tracked as unreleased.

In `@TablePro/Core/Storage/GroupStorage.swift`:
- Around line 100-115: When removing connections in the Group deletion loop,
ensure you both clean TOTP secrets and record deletions with the sync tracker:
for each conn where you currently call storage.deletePassword(for:),
storage.deleteSSHPassword(for:) and storage.deleteKeyPassphrase(for:), also call
storage.deleteTOTPSecret(for: conn.id) and notify the sync system (e.g.
SyncChangeTracker.shared.recordDeletion(for: conn.id) or the appropriate method
on SyncChangeTracker) so a tombstone/change is recorded before calling
storage.saveConnections(remaining).
- Around line 177-183: The recursive helper collectDescendantIds(of:in:) lacks
cycle protection; modify it to track visited nodes (e.g., add an inout Set<UUID>
visited parameter or create a private helper
collectDescendantIds(of:in:visited:)) and on entry check/mark the current
groupId in visited so you skip already-seen IDs before filtering children or
recursing; ensure you still accumulate child IDs into result but avoid recursing
into a child whose id is in visited to prevent infinite recursion on malformed
cycles.

In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 1056-1059: The code unconditionally copies
savedConnections[index].sortOrder into updated.sortOrder which breaks ordering
when the user changed connectionToSave.groupId; instead, check if
connectionToSave.groupId == savedConnections[index].groupId and only preserve
updated.sortOrder in that case, otherwise assign a new sortOrder appropriate for
the target group (e.g., compute max sortOrder among savedConnections with the
new groupId and set updated.sortOrder = max + 1 or nil as your model requires)
before replacing savedConnections[index]; update the logic around
connectionToSave, updated.sortOrder, and savedConnections to implement this
conditional behavior.

---

Nitpick comments:
In `@docs/docs.json`:
- Around line 220-226: The Chinese "快速开始" section in docs.json is missing the
zh/download entry; update the pages array for the group "快速开始" to include
"zh/download" alongside "zh/index", "zh/quickstart", "zh/installation", and
"zh/changelog" so the Chinese docs mirror the English and Vietnamese Getting
Started sections.

In `@docs/download.mdx`:
- Line 35: Replace the Apple logo glyph in the string "Click the **Apple menu**
() > **About This Mac**:" with either plain text or a fallback label;
specifically update the phrase "Apple menu ()" to "Apple menu" or "Apple menu
(Apple icon)" so the documentation doesn't rely on an Apple-specific glyph that
may not render on all devices.

In `@docs/vi/download.mdx`:
- Line 35: The Apple logo character in the sentence "Nhấp vào **menu Apple** ()
> **Giới thiệu về máy Mac này**" may not render on non‑Apple devices; update
that fragment (the "menu Apple" line) to use a visible, accessible fallback
instead of relying on the single logo glyph—either replace the empty parentheses
with the word "Apple" (e.g., "menu Apple (Apple logo)"), or replace the glyph
with an inline image/SVG that includes appropriate alt text, matching the
approach used in the English doc to ensure correct rendering and accessibility.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c79d7f7a-9262-4047-808d-0c6c0455f6c5

📥 Commits

Reviewing files that changed from the base of the PR and between 10cd406 and 6022d60.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • TablePro/Core/Storage/ConnectionStorage.swift
  • TablePro/Core/Storage/GroupStorage.swift
  • TablePro/Models/Connection/ConnectionGroup.swift
  • TablePro/Models/Connection/DatabaseConnection.swift
  • TablePro/Views/Connection/ConnectionFormView.swift
  • docs/docs.json
  • docs/download.mdx
  • docs/vi/download.mdx


### Added

- Connection groups: organize connections into named, color-coded folders with support for nested subgroups, drag-and-drop reordering, expand/collapse state persistence, multi-selection (bulk delete, bulk move to group), and context menus for group and connection management
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Move this entry to [Unreleased] instead of [0.14.1].

Line 239 documents a new feature in a historical release section. This should be listed under ## [Unreleased] (### Added) until release.

📘 Suggested changelog placement
 ## [Unreleased]
+
+### Added
+
+- Connection groups: organize connections into named, color-coded folders with support for nested subgroups, drag-and-drop reordering, expand/collapse state persistence, multi-selection (bulk delete, bulk move to group), and context menus for group and connection management

 ## [0.14.1] - 2026-03-06

 ### Added

-- Connection groups: organize connections into named, color-coded folders with support for nested subgroups, drag-and-drop reordering, expand/collapse state persistence, multi-selection (bulk delete, bulk move to group), and context menus for group and connection management
 - Add database and schema switching for PostgreSQL connections via ⌘K

As per coding guidelines: "Update CHANGELOG.md under [Unreleased] section (Added/Fixed/Changed) for new features and notable changes."

📝 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.

Suggested change
- Connection groups: organize connections into named, color-coded folders with support for nested subgroups, drag-and-drop reordering, expand/collapse state persistence, multi-selection (bulk delete, bulk move to group), and context menus for group and connection management
## [Unreleased]
### Added
- Connection groups: organize connections into named, color-coded folders with support for nested subgroups, drag-and-drop reordering, expand/collapse state persistence, multi-selection (bulk delete, bulk move to group), and context menus for group and connection management
## [0.14.1] - 2026-03-06
### Added
- Add database and schema switching for PostgreSQL connections via ⌘K
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 239, The changelog entry "Connection groups: organize
connections into named, color-coded folders with support for nested subgroups,
drag-and-drop reordering, expand/collapse state persistence, multi-selection
(bulk delete, bulk move to group), and context menus for group and connection
management" should be removed from the "## [0.14.1]" section and added under "##
[Unreleased]" inside the "### Added" subsection; locate that exact entry text in
CHANGELOG.md, cut it from the 0.14.1 section and paste it as a new bullet under
the Unreleased -> Added heading so the feature is tracked as unreleased.

Comment on lines +100 to +115
// Delete connections that belonged to deleted groups
let storage = ConnectionStorage.shared
let connections = storage.loadConnections()
var remaining: [DatabaseConnection] = []
for conn in connections {
if let gid = conn.groupId, allDeletedIds.contains(gid) {
// Clean up keychain entries
storage.deletePassword(for: conn.id)
storage.deleteSSHPassword(for: conn.id)
storage.deleteKeyPassphrase(for: conn.id)
} else {
remaining.append(conn)
}
}
storage.saveConnections(remaining)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Group deletion misses connection tombstones and TOTP keychain cleanup.

In Lines 105–114, deleted connections are removed from persisted data, but:

  • SyncChangeTracker is not notified for each deleted connection.
  • deleteTOTPSecret(for:) is not called.

This can leave stale synced records and orphaned secrets.

🛠️ Suggested fix
         for conn in connections {
             if let gid = conn.groupId, allDeletedIds.contains(gid) {
+                SyncChangeTracker.shared.markDeleted(.connection, id: conn.id.uuidString)
                 // Clean up keychain entries
                 storage.deletePassword(for: conn.id)
                 storage.deleteSSHPassword(for: conn.id)
                 storage.deleteKeyPassphrase(for: conn.id)
+                storage.deleteTOTPSecret(for: conn.id)
             } else {
                 remaining.append(conn)
             }
         }
📝 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.

Suggested change
// Delete connections that belonged to deleted groups
let storage = ConnectionStorage.shared
let connections = storage.loadConnections()
var remaining: [DatabaseConnection] = []
for conn in connections {
if let gid = conn.groupId, allDeletedIds.contains(gid) {
// Clean up keychain entries
storage.deletePassword(for: conn.id)
storage.deleteSSHPassword(for: conn.id)
storage.deleteKeyPassphrase(for: conn.id)
} else {
remaining.append(conn)
}
}
storage.saveConnections(remaining)
}
// Delete connections that belonged to deleted groups
let storage = ConnectionStorage.shared
let connections = storage.loadConnections()
var remaining: [DatabaseConnection] = []
for conn in connections {
if let gid = conn.groupId, allDeletedIds.contains(gid) {
SyncChangeTracker.shared.markDeleted(.connection, id: conn.id.uuidString)
// Clean up keychain entries
storage.deletePassword(for: conn.id)
storage.deleteSSHPassword(for: conn.id)
storage.deleteKeyPassphrase(for: conn.id)
storage.deleteTOTPSecret(for: conn.id)
} else {
remaining.append(conn)
}
}
storage.saveConnections(remaining)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Core/Storage/GroupStorage.swift` around lines 100 - 115, When
removing connections in the Group deletion loop, ensure you both clean TOTP
secrets and record deletions with the sync tracker: for each conn where you
currently call storage.deletePassword(for:), storage.deleteSSHPassword(for:) and
storage.deleteKeyPassphrase(for:), also call storage.deleteTOTPSecret(for:
conn.id) and notify the sync system (e.g.
SyncChangeTracker.shared.recordDeletion(for: conn.id) or the appropriate method
on SyncChangeTracker) so a tombstone/change is recorded before calling
storage.saveConnections(remaining).

Comment on lines +177 to +183
func collectDescendantIds(of groupId: UUID, in groups: [ConnectionGroup]) -> Set<UUID> {
var result = Set<UUID>()
let children = groups.filter { $0.parentGroupId == groupId }
for child in children {
result.insert(child.id)
result.formUnion(collectDescendantIds(of: child.id, in: groups))
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Descendant traversal needs cycle protection.

collectDescendantIds (Lines 177–183) recursively follows parent links with no visited-set guard. A malformed cycle can recurse indefinitely and crash delete/count flows.

🛡️ Suggested fix
-    func collectDescendantIds(of groupId: UUID, in groups: [ConnectionGroup]) -> Set<UUID> {
-        var result = Set<UUID>()
-        let children = groups.filter { $0.parentGroupId == groupId }
-        for child in children {
-            result.insert(child.id)
-            result.formUnion(collectDescendantIds(of: child.id, in: groups))
-        }
-        return result
-    }
+    private func collectDescendantIds(of groupId: UUID, in groups: [ConnectionGroup]) -> Set<UUID> {
+        var visited: Set<UUID> = []
+        return collectDescendantIds(of: groupId, in: groups, visited: &visited)
+    }
+
+    private func collectDescendantIds(
+        of groupId: UUID,
+        in groups: [ConnectionGroup],
+        visited: inout Set<UUID>
+    ) -> Set<UUID> {
+        guard visited.insert(groupId).inserted else { return [] }
+        var result = Set<UUID>()
+        let children = groups.filter { $0.parentGroupId == groupId }
+        for child in children {
+            result.insert(child.id)
+            result.formUnion(collectDescendantIds(of: child.id, in: groups, visited: &visited))
+        }
+        return result
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Core/Storage/GroupStorage.swift` around lines 177 - 183, The
recursive helper collectDescendantIds(of:in:) lacks cycle protection; modify it
to track visited nodes (e.g., add an inout Set<UUID> visited parameter or create
a private helper collectDescendantIds(of:in:visited:)) and on entry check/mark
the current groupId in visited so you skip already-seen IDs before filtering
children or recursing; ensure you still accumulate child IDs into result but
avoid recursing into a child whose id is in visited to prevent infinite
recursion on malformed cycles.

Comment on lines +1056 to +1059
// Preserve sortOrder from existing connection
var updated = connectionToSave
updated.sortOrder = savedConnections[index].sortOrder
savedConnections[index] = updated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preserving sortOrder unconditionally breaks cross-group edits.

At Line 1058, updated.sortOrder is always copied from the old record. If the user changed groupId, this reuses an order from a different sibling set and can cause ordering collisions.

🔧 Suggested fix
             if let index = savedConnections.firstIndex(where: { $0.id == connectionToSave.id }) {
-                // Preserve sortOrder from existing connection
                 var updated = connectionToSave
-                updated.sortOrder = savedConnections[index].sortOrder
+                let previous = savedConnections[index]
+                if previous.groupId == updated.groupId {
+                    updated.sortOrder = previous.sortOrder
+                } else {
+                    let nextSortOrder = (
+                        savedConnections
+                            .filter { $0.id != updated.id && $0.groupId == updated.groupId }
+                            .map(\.sortOrder)
+                            .max() ?? -1
+                    ) + 1
+                    updated.sortOrder = nextSortOrder
+                }
                 savedConnections[index] = updated
                 storage.saveConnections(savedConnections)
                 SyncChangeTracker.shared.markDirty(.connection, id: connectionToSave.id.uuidString)
             }
📝 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.

Suggested change
// Preserve sortOrder from existing connection
var updated = connectionToSave
updated.sortOrder = savedConnections[index].sortOrder
savedConnections[index] = updated
var updated = connectionToSave
let previous = savedConnections[index]
if previous.groupId == updated.groupId {
updated.sortOrder = previous.sortOrder
} else {
let nextSortOrder = (
savedConnections
.filter { $0.id != updated.id && $0.groupId == updated.groupId }
.map(\.sortOrder)
.max() ?? -1
) + 1
updated.sortOrder = nextSortOrder
}
savedConnections[index] = updated
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Views/Connection/ConnectionFormView.swift` around lines 1056 - 1059,
The code unconditionally copies savedConnections[index].sortOrder into
updated.sortOrder which breaks ordering when the user changed
connectionToSave.groupId; instead, check if connectionToSave.groupId ==
savedConnections[index].groupId and only preserve updated.sortOrder in that
case, otherwise assign a new sortOrder appropriate for the target group (e.g.,
compute max sortOrder among savedConnections with the new groupId and set
updated.sortOrder = max + 1 or nil as your model requires) before replacing
savedConnections[index]; update the logic around connectionToSave,
updated.sortOrder, and savedConnections to implement this conditional behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants