feat: multi-select, bulk actions and reorder for welcome window#449
Merged
feat: multi-select, bulk actions and reorder for welcome window#449
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReplace single-selection with multi-selection in the Welcome window; add bulk connect/delete, deferred "Move to New Group", reorder groups and connections within the Welcome window; add batch delete API to storage; update localization, keyboard shortcut docs, CHANGELOG, and Xcode plug‑in build entries. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as WelcomeWindowView
participant Storage as ConnectionStorage
participant Tracker as SyncChangeTracker
participant Defaults as UserDefaults
participant Keychain as Keychain
User->>UI: Multi-select connections
User->>UI: Trigger delete (Cmd+Delete / context menu)
UI->>UI: Show count-based confirmation
User->>UI: Confirm
UI->>Storage: deleteConnections([selected])
loop per connection
Storage->>Tracker: mark connection ID deleted
Storage->>Defaults: load & save filtered connections list
Storage->>Keychain: delete password/SSH/passphrase/TOTP/plugin fields
end
Storage-->>UI: deletion complete
UI->>UI: clear selection & refresh view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
TablePro/Core/Storage/ConnectionStorage.swift (1)
111-111: Consider early return for empty input.If
connectionsToDeleteis empty, the method still performs a load/save cycle. An early return would avoid unnecessary I/O.🔧 Optional: Add guard for empty array
func deleteConnections(_ connectionsToDelete: [DatabaseConnection]) { + guard !connectionsToDelete.isEmpty else { return } for conn in connectionsToDelete {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/ConnectionStorage.swift` at line 111, The deleteConnections(_ connectionsToDelete: [DatabaseConnection]) method should return immediately when connectionsToDelete.isEmpty to avoid unnecessary I/O; add a guard at the top of deleteConnections checking for an empty array and exit early before performing the load/save cycle (the existing load/save calls, e.g., loadConnections()/saveConnections() or similar methods used in this function).TablePro/Resources/Localizable.xcstrings (1)
3924-3925: Populate explicit localization payloads for newly added keys.The new entries appear as empty objects in these hunks. Add at least source-language
localizations(and existing supported locales) so these strings are tracked/translated explicitly instead of silently falling back.Also applies to: 6532-6533, 9309-9310, 12284-12285, 17590-17591, 18133-18134, 23591-23592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Resources/Localizable.xcstrings` around lines 3924 - 3925, The new localization entries (e.g. the key "Are you sure you want to delete %lld connections? This cannot be undone.") were added as empty objects; update each of these keys to include explicit localization payloads by adding a source-language "localizations" dictionary and entries for existing supported locales (matching the project’s other keys), e.g., include the source text under the source locale and provide placeholder translations for other locales so the strings are tracked for translation; repeat this for the other new keys mentioned in the review so none remain empty objects.TablePro/Views/Connection/WelcomeWindowView.swift (2)
947-968: Consider adding defensive bounds check foractiveGroupIndicesaccess.The guard at lines 949-950 validates against
active.count, while line 956 accessesactiveGroupIndices[$0]. AlthoughactiveGroupsis derived from filteringgroups(socompactMapshould always succeed), the index access could fail if there's ever a mismatch.A defensive check would make this safer:
🛡️ Optional defensive fix
let activeGroupIndices = active.compactMap { activeGroup in groups.firstIndex(where: { $0.id == activeGroup.id }) } +guard activeGroupIndices.count == active.count else { return } let globalSource = IndexSet(source.map { activeGroupIndices[$0] })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/WelcomeWindowView.swift` around lines 947 - 968, The moveGroups(from:to:) function currently maps activeGroups to activeGroupIndices and then indexes into that array using source indices, which can crash if there's a mismatch; update this method to defensively validate that every index in source exists in activeGroupIndices before accessing (e.g., compute global indices with a compactMap that skips invalid mappings and/or guard that source.max() < activeGroupIndices.count), and if any mapping fails bail out early; keep use of groups.move(fromOffsets:toOffset:) and groupStorage.saveGroups(groups) unchanged but ensure globalSource and globalDestination are computed only from validated indices (referencing moveGroups(from:to:), activeGroups, activeGroupIndices, groups.move, groupStorage.saveGroups).
777-781: Consider guarding against redundant close calls during multi-connect.Each
connectToDatabasecall executesNSApplication.shared.closeWindows(withId: "welcome"). After the first connection, the welcome window is already closed, making subsequent close calls redundant. Additionally, if an intermediate connection fails,handleConnectionFailurereopens the welcome window while remaining connections are still being processed.Consider closing the welcome window once before the loop or tracking whether it's already closed:
♻️ Suggested refactor
private func connectSelectedConnections() { + NSApplication.shared.closeWindows(withId: "welcome") for connection in selectedConnections { - connectToDatabase(connection) + connectToDatabaseWithoutClosingWelcome(connection) } }Alternatively, extract the window management into a dedicated multi-connect flow that defers error handling until all connections are attempted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/WelcomeWindowView.swift` around lines 777 - 781, connectSelectedConnections currently calls connectToDatabase for each selectedConnections which causes NSApplication.shared.closeWindows(withId: "welcome") to be invoked multiple times and allows handleConnectionFailure to reopen the welcome window while other connects are still running; fix by moving the welcome-window close out of the per-connection path and into the multi-connect flow (call NSApplication.shared.closeWindows(withId: "welcome") once before the for-loop in connectSelectedConnections or set a boolean flag like isWelcomeClosed that connectToDatabase checks), and ensure handleConnectionFailure does not unconditionally reopen the welcome window while there are still pending connection attempts (defer reopening until all attempts complete or check the flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Core/Storage/ConnectionStorage.swift`:
- Line 111: The deleteConnections(_ connectionsToDelete: [DatabaseConnection])
method should return immediately when connectionsToDelete.isEmpty to avoid
unnecessary I/O; add a guard at the top of deleteConnections checking for an
empty array and exit early before performing the load/save cycle (the existing
load/save calls, e.g., loadConnections()/saveConnections() or similar methods
used in this function).
In `@TablePro/Resources/Localizable.xcstrings`:
- Around line 3924-3925: The new localization entries (e.g. the key "Are you
sure you want to delete %lld connections? This cannot be undone.") were added as
empty objects; update each of these keys to include explicit localization
payloads by adding a source-language "localizations" dictionary and entries for
existing supported locales (matching the project’s other keys), e.g., include
the source text under the source locale and provide placeholder translations for
other locales so the strings are tracked for translation; repeat this for the
other new keys mentioned in the review so none remain empty objects.
In `@TablePro/Views/Connection/WelcomeWindowView.swift`:
- Around line 947-968: The moveGroups(from:to:) function currently maps
activeGroups to activeGroupIndices and then indexes into that array using source
indices, which can crash if there's a mismatch; update this method to
defensively validate that every index in source exists in activeGroupIndices
before accessing (e.g., compute global indices with a compactMap that skips
invalid mappings and/or guard that source.max() < activeGroupIndices.count), and
if any mapping fails bail out early; keep use of
groups.move(fromOffsets:toOffset:) and groupStorage.saveGroups(groups) unchanged
but ensure globalSource and globalDestination are computed only from validated
indices (referencing moveGroups(from:to:), activeGroups, activeGroupIndices,
groups.move, groupStorage.saveGroups).
- Around line 777-781: connectSelectedConnections currently calls
connectToDatabase for each selectedConnections which causes
NSApplication.shared.closeWindows(withId: "welcome") to be invoked multiple
times and allows handleConnectionFailure to reopen the welcome window while
other connects are still running; fix by moving the welcome-window close out of
the per-connection path and into the multi-connect flow (call
NSApplication.shared.closeWindows(withId: "welcome") once before the for-loop in
connectSelectedConnections or set a boolean flag like isWelcomeClosed that
connectToDatabase checks), and ensure handleConnectionFailure does not
unconditionally reopen the welcome window while there are still pending
connection attempts (defer reopening until all attempts complete or check the
flag).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b64428-b8a1-491d-88cb-0b4f0ac601dc
📒 Files selected for processing (6)
CHANGELOG.mdTablePro.xcodeproj/project.pbxprojTablePro/Core/Storage/ConnectionStorage.swiftTablePro/Resources/Localizable.xcstringsTablePro/Views/Connection/WelcomeWindowView.swiftdocs/features/keyboard-shortcuts.mdx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
TablePro/Resources/Localizable.xcstrings (1)
3924-3926:⚠️ Potential issue | 🟡 MinorAdd plural variations for the confirmation message.
This count-based string uses
%lldbut lacks plural variations, unlike the properly pluralized"Connect %lld Connections"and"Delete %lld Connections"entries added in this PR. Without variations, the text will display "1 connections" (grammatically incorrect) for singular counts.Proposed fix to add plural variations
"Are you sure you want to delete %lld connections? This cannot be undone." : { - + "localizations" : { + "en" : { + "variations" : { + "plural" : { + "one" : { + "stringUnit" : { + "state" : "translated", + "value" : "Are you sure you want to delete %lld connection? This cannot be undone." + } + }, + "other" : { + "stringUnit" : { + "state" : "translated", + "value" : "Are you sure you want to delete %lld connections? This cannot be undone." + } + } + } + } + } + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Resources/Localizable.xcstrings` around lines 3924 - 3926, Replace the single-count entry for "Are you sure you want to delete %lld connections? This cannot be undone." with pluralized variations to match the other pluralized keys (e.g., provide singular and plural forms consistent with "Connect %lld Connections" / "Delete %lld Connections"); add a dictionary with the appropriate NSLocalizedString plural keys (one/other or the project's existing plural key format) so the message renders "1 connection" for singular and "%lld connections" for other counts, keeping the same placeholder (%lld) and exact message text for each variation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@TablePro/Resources/Localizable.xcstrings`:
- Around line 3924-3926: Replace the single-count entry for "Are you sure you
want to delete %lld connections? This cannot be undone." with pluralized
variations to match the other pluralized keys (e.g., provide singular and plural
forms consistent with "Connect %lld Connections" / "Delete %lld Connections");
add a dictionary with the appropriate NSLocalizedString plural keys (one/other
or the project's existing plural key format) so the message renders "1
connection" for singular and "%lld connections" for other counts, keeping the
same placeholder (%lld) and exact message text for each variation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 343bd986-0891-46cf-870f-466e4eec5048
📒 Files selected for processing (1)
TablePro/Resources/Localizable.xcstrings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
moveUngroupedConnectionscaused by mismatch betweenungroupedConnections(includes orphaned groupIds) and the move handler (only matched nil groupIds)Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation