feat(sidebar): add favorite tables with iCloud sync and sidebar improvements

[J2TeamNNL]

Summary

• Favorite tables. Each sidebar table row has a star button. Favorites are scoped to the connection, database, and schema, pinned to the top of their section, shown in a dedicated Tables group in the Favorites tab, and synced via iCloud behind a new Table Favorites toggle (a FavoriteTable CloudKit record). The filled star is always visible as a status indicator; the empty star reveals on hover (the Apple Music pattern).
• Create from the footer. A plus button in the Tables sidebar footer opens a New Table / New View menu, so you no longer need the right-click menu. It is disabled while safe mode blocks writes. "Create New Table…" was removed from the right-click menu.
• Context menu. "Show ER Diagram" is available on table rows. The Maintenance submenu is hidden when there are no operations or the target is read-only, instead of showing an empty disabled menu.
• Window. The minimum width adjusts to the visible panes, so opening the inspector on a small window no longer clips content.

Internals / refactors

• Type-safe favorites selection (FavoriteSelection) replaces the previous string-prefix routing in the Favorites tab; dispatch switches on the enum and FavoriteNode.content.
• The Favorites tree uses a recursive row view instead of AnyView, keeping per-folder expansion state.
• The sidebar footer inherits the native sidebar vibrancy with a static divider (no custom material).
• Schema picker extracted to SchemaPickerControl; accessibility labels/actions added to favorite rows.

Tests

• FavoriteTablesStorage: dirty/tombstone tracking, connection/database/schema scoping, toggle idempotence.
• SyncRecordMapper: favorite record round-trip (including database).
• FavoriteSelection: raw-value round-trip and invalid/legacy decoding.

Manual QA

The Favorites > Queries tree (select / right-click / double-click / Delete / folder rename / expand-collapse persistence / linked files) went through the selection-model and row-view refactor and is not unit-testable. Please verify manually.

Checklist

• Tests added or updated
• CHANGELOG.md updated under [Unreleased]
• Docs updated in docs/ (favorites.mdx, table-operations.mdx, icloud-sync.mdx)
• User-facing strings localized
• Changed files pass swiftlint lint --strict

[github-actions]

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.

---

1 out of 2 committers have signed the CLA.
✅ (J2TeamNNL)[https://github.com/J2TeamNNL]
❌ @claude
_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[J2TeamNNL]

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

[datlechin]

Code review

Three features in one PR: table favorites with iCloud sync, in-memory recent tables, and sidebar UX (plus button, dynamic window min, conditional Maintenance menu). The split is reasonable and the test coverage on the value types (FavoriteTablesStorage, RecentTablesStore, SidebarContextMenuLogic) is good. The sync wiring and the design choices around favorites need work before merging.

Blockers

1. Sync push/clear is not gated by a sync settings toggle. Every existing record type wraps its push, clear, and tombstone-removal in if settings.syncConnections / if settings.syncSettings / etc. tableFavorite is the only type that runs unconditionally:

collectDirtyTableFavorites(into: &recordsToSave, deletions: &recordIDsToDelete, zoneID: zoneID)
...
changeTracker.clearAllDirty(.tableFavorite)
for tombstone in metadataStorage.tombstones(for: .tableFavorite) {
    metadataStorage.removeTombstone(type: .tableFavorite, id: tombstone.id)
}

Either add settings.syncTableFavorites (with a toggle in SyncSection and a check in the if-let chain), or attach favorites to an existing toggle (syncSettings is the closest match) and gate consistently. Documenting "no toggle" doesn't make the inconsistency safe — if Settings_* syncing is later turned off mid-session, table favorites will still push.

2. "View ER Diagram" silently removed from the sidebar right-click menu. The CHANGELOG only mentions removing "Create New Table…", but the diff also drops:

-Button(String(localized: "View ER Diagram")) {
-    coordinator?.showERDiagram()
-}

The Favorites tab still has it, but right-click on any sidebar table no longer does. Either restore it (it's the documented entry point for many users) or add a CHANGELOG Removed entry justifying it. Right now it reads as an accidental regression.

3. Localizable.xcstrings carries unrelated stale flags. The PR legitimately marks "Create New Table..." and "Syncs connections, settings, and SSH profiles across your Macs via iCloud." as stale because it replaces both usages. But it also marks seven unrelated entries stale that this PR doesn't touch:

• %lld of %lld, Format JSON, Limit, Next Page (⌘]), Offset, Pagination Settings, Previous Page (⌘[)

These look like collateral from running the contributor's local Xcode against a different working tree. Revert those stale flags — they'll corrupt the catalog and silently kill translations elsewhere.

4. UI test bar moved, then under-met. This PR edits CLAUDE.md to add a requirement that UI/user-flow changes include TableProUITests automation, then ships only testApplicationLaunchesMainWindow (a launch test). The actual user-flow this PR adds — click star → row pins to top, Favorites tab Tables section, Recent section appears after open — has no UI test. Either remove the CLAUDE.md change (it should be a separate doc PR with a wider discussion) or add UI tests that actually exercise the new flow. Editing the rule and meeting the lowered bar in the same PR is a bad pattern.

5. Merge conflicts. Three files now conflict with main:

• CHANGELOG.md (mechanical; main has new entries in the same section)
• TablePro/Core/Services/Infrastructure/MainSplitViewController.swift (main refactored defaultTitle resolution into a static method; PR added window-min-size logic in the same file)
• TablePro/Resources/Localizable.xcstrings (string catalog; partially mechanical, partially overlapping with Centralize keyboard shortcuts following macOS HIG patterns #3 above)

Rebase and reapply before re-review.

Design concerns

6. Favorites are global by table name with no qualifier. FavoriteTablesStorage stores Set<String> keyed only on table.name. Consequences:

• Favoriting users in connection A also marks users in every other connection.
• public.users and app.users collide — schema is dropped on the storage side and only rediscovered at render time via tables.first { $0.name == favName }.
• After iCloud sync, a Mac with a totally different connection set will see "ghost" favorites that match by name.

The CLAUDE.md storage table addition documents this as the design, but it doesn't match how DataGrip, TablePlus, or Postico scope favorites (per-connection or per-(connection,schema)). At minimum, key by (connectionId, schema?, name) — the sync ID can hash all three. If you do want global-by-name as a deliberate choice, the docs should explain why, not just that, and the iCloud sync description should warn users.

7. FavoriteTablesStorage cache has no isolation. private var cache: Set<String>? is read/written from loadFavorites, addFavorite, removeFavorite, toggle, and removeFavoriteWithoutSync with no actor, lock, or @MainActor. The class is final class (no Sendable or @unchecked Sendable). UI mutations happen on main, but the sync-apply path (applyRemoteTableFavorite → addFavoriteWithoutSync) and the dirty/tombstone collectors run from SyncCoordinator queues. Mark the class @MainActor, or wrap mutations behind a lock — the current shape is a race waiting to happen the moment a sync arrives during a star toggle.

8. RecentTablesStore.push is keyed by (connectionID, activeDatabaseName) — activeDatabaseName falls back to connection.database. For SQLite (single-file databases) and several no-database drivers this can be "". All recents for those connections then collide under the same ("", connID) bucket — which happens to be correct here, but it's load-bearing implicit behavior. Either document the empty-string convention in RecentTablesStore or use Optional<String> as the key component.

9. recomputeWindowMinSize runs from splitViewDidResizeSubviews and calls window.setFrame(_:display:animate:) when the frame is smaller than the new min. splitViewDidResizeSubviews fires during user drags. Resetting the frame mid-drag while the inspector toggles could fight the user. The guard window.minSize != newMinSize skips work when nothing changes, which prevents most loops, but the frame-bump can still happen on every drag if the user has the window resized very small. Worth a manual test: drag the window narrower than the inspector min while the inspector is open. If you don't want to debounce, at least pass animate: false from splitViewDidResizeSubviews to avoid mid-drag animations.

10. CLAUDE.md changes in a feature PR. The PR rewords mandatory rule #4 and principle #7, and adds a CLAUDE.md storage-table row. These are project-convention changes that affect every future PR — they belong in a doc-only PR where reviewers can weigh in without the feature noise. The new CLAUDE.md storage row also documents a design (#6) that I'd argue against above.

Lower-priority observations

• .gitignore drive-bys. Local.xcconfig and .docs/ are added without context. If they're personal, they shouldn't land. If they're intentional (a planned local-override pattern), call it out in the PR.
• favoriteTable(forNodeId:) matches the first table by name. If two schemas have a users table and both end up in availableFavoriteTables, selecting one nondeterministically picks the first. Tie the node id to schema.name like RecentTablesStore.Entry.id does.
• Star button inside List row likely propagates the row click. The .buttonStyle(.plain) keeps the button from drawing default styling, but in a List(selection:) the click can still bubble to row selection. Worth manually verifying: clicking the star should toggle favorite only, not also select the row.
• accessibilityLabel ordering: else if isFavorite means a pending-delete favorite is announced without "favorite". Probably fine (pending state is more important), but worth a test for pending delete + favorite to lock the intent.
• Section(isExpanded: $viewModel.isRecentsExpanded) adds state but isRecentsExpanded defaults to true and is never persisted. Quitting and reopening will reset it. If that's intentional (matching the in-memory store) it's fine; if not, persist alongside other sidebar disclosure state.
• SyncCoordinator.collectDirtyTableFavorites reads favorites.loadFavorites() then filters with dirtyIds.contains(...). This means a favorite that was added and later removed in the same session may still be in dirtyIds (only the tombstone path clears dirty for the removed name) — verify the toggle/remove path doesn't leave stale dirty entries by adding a test that toggles a favorite on, then off, then pushes.
• CHANGELOG entry for window min-width should call out that small windows will jump wider when the inspector is opened. Today the message reads as a fix, but for users with tiny windows it's a behavior change.

Tests

The new unit tests are well-scoped. Gaps worth closing:

• Sync conditional behavior (issue feat: Query history #1) — once gated by a setting, add a test for "sync off → no push, sync on → push".
• applyRemoteTableFavorite tombstone path — confirm that an inbound record matching an existing tombstone is rejected.
• RecentTablesStore schema-distinct test exists but no equivalent for FavoriteTablesStorage — currently impossible because storage is name-only (issue Fix/bug #6).

Verification status

PR description has [ ] No SwiftLint/SwiftFormat violations unchecked. The diff doesn't obviously violate limits, but SidebarView.swift and FavoritesTabView.swift grew meaningfully — run swiftlint lint --strict and check file_length headroom before merging. Also no Xcode build evidence ("relying on CI" implied by the test plan); given the merge conflicts, please rebase, build locally, and rerun the test plan before re-requesting review.

Verdict

Not mergeable as-is. The sync gating (#1), the silent menu removal (#2), the unrelated stale strings (#3), and the merge conflicts (#5) are hard blockers. The favorites scoping (#6) and the storage thread-safety (#7) are design issues I'd push back on before this lands — they're the kind of thing that's easy to fix now and very hard to fix once user data is in the wild and syncing. Happy to walk through any of these in more depth.

[datlechin]

Review

Overview

Substantial sidebar feature work (+1,230 / −82 across 35 files). Adds three user-facing capabilities plus supporting work:

• Favorite tables — star toggle on every sidebar row, persisted in UserDefaults (FavoriteTablesStorage), pinned in a new "Tables" section of the Favorites tab, syncs through iCloud (SyncRecordType.tableFavorite).
• Recent tables — in-memory RecentTablesStore, last 10 per (connection, database), shown in a new "Recent" section at the top of the Tables sidebar.
• Create-table "+" button in the sidebar header (replaces the context-menu item) plus removal of "View ER Diagram" from the sidebar context menu.
• Dynamic window minimum size in MainSplitViewController based on which panes are visible.
• Maintenance submenu hidden when no ops available or target is read-only.
• New TableProUITests target.

---

Correctness

🔴 Duplicate List tags between Recent and main sections

SidebarView.swift tags both the recent-section row and the normal-section row with the same TableInfo (recents are looked up out of tables, so it's the literal same value). SwiftUI List(selection:) keys selection by tag; duplicate tags cause ambiguous highlight behavior. Wrap the recent entry in a distinct tag (e.g., "recent:\(info.schema ?? "").\(info.name)").

🔴 Node-ID collision in FavoritesTabView drops schema

tableNodeId(_ name:) builds "table:\(name)" — schema is discarded. If a user favorites public.users and app.users on the same connection, both rows share the same ID and favoriteTable(forNodeId:) returns whichever first finds. Mirror what RecentTablesStore.Entry.id does and include the schema.

🟡 NotificationCenter.post while holding NSLock in FavoriteTablesStorage._persist

_persist is invoked under lock.lock() and posts .favoriteTablesDidChange synchronously. Today's observers dispatch async on the main queue so it's safe in practice, but NSLock is non-recursive — any future synchronous observer that calls back into the storage on the same thread will deadlock. Drop the lock before posting (or post via DispatchQueue.main.async).

🟡 toggle(name:schema:connectionId:) is not atomic

Reads under the lock, releases, then calls addFavorite/removeFavorite. Two concurrent toggles can race. Single-process + mostly-@MainActor callers make this unlikely to bite, but the function shouldn't pretend to be atomic.

🟡 Orphaned favorites when a connection is deleted

FavoriteTablesStorage has no observer for connection deletion. Entries persist forever; the UI filters them out via availableFavoriteTables, but they still ride through every iCloud sync, and the user can't clean them up. Hook into the connection-deleted event and emit tombstones.

🟡 recomputeWindowMinSize re-entrancy from splitViewDidResizeSubviews

splitViewDidResizeSubviews → recomputeWindowMinSize → window.setFrame(_:display:animate:) re-fires splitViewDidResizeSubviews. The window.minSize != newMinSize guard prevents infinite recursion only if min doesn't change on the second tick. setFrame(_, animate: window.isVisible) during a live resize will also cause a visible jump. Worth eyeballing at runtime.

🟡 UI test asserts a width (950) that isn't the documented minimum

testMainWindowRespectsMinimumSize asserts frame.width >= 950 with the comment "must be at least the base minimum", but baseWindowMinWidth = 720. The 950 is the default launch width, not the minimum. Either rename the test (…RespectsDefaultLaunchSize) or assert against 720 and add a separate test that actually tries to shrink below the min.

---

Code Quality / Style

Project-rule violations

• File-header comments in RecentTablesStore.swift and RecentTablesStoreTests.swift. CLAUDE.md's "No comments" rule applies; other Core/Storage/ files don't carry them.
• DocC comments on SidebarContextMenuLogic.maintenanceGroupEnabled — internal helper, not public API. Drop them; the function name carries the intent.
• Long line in SidebarView.swift (isFavorite: favoriteTables.contains(FavoriteTablesStorage.FavoriteEntry(...))) is ~190 chars and trips the 180-char SwiftLint warning. Hoist into a private func isFavorite(_ table: TableInfo) -> Bool; it's used in two places, both gnarly.

Minor

• SyncRecordMapper.toCKRecord(favoriteEntry:) writes a favoriteTableId field that favoriteEntry(from:) never reads. Either consume + validate on decode, or drop the field.
• applyRemoteTableFavorite snapshots favorites before/after just to detect change (two full Set copies per remote apply). Have addFavoriteWithoutSync return Bool (it already early-returns on insert(...).inserted == false).
• RecentTablesStore.entryId(name:schema:) duplicates Entry.id's logic — call Entry(...).id or extract a static helper to avoid drift.
• SidebarContainerViewController.updateSidebarState(_:windowState:coordinator:) defaults coordinator to nil, but all in-tree callers pass it. Drop the default to force explicitness.

---

Sync / iCloud

• Push/pull paths, tombstones, conflict-resolution view, and "mark everything dirty" all updated consistently.
• New record type is CK-level, not PluginKit ABI — no currentPluginKitVersion bump needed.
• addFavorite correctly persists under the lock then calls syncTracker.markDirty after unlock (matches the CLAUDE.md "persist first, then notify" invariant).

---

Test Coverage

Good unit coverage on storage + mapper + sidebar logic. Gaps:

• No end-to-end test for SyncCoordinator.applyRemoteTableFavorite (tombstone short-circuit, change-detect return).
• RecentTablesStore.clearAll() is uncovered.
• No regression test for the duplicate-tag selection or schema-collision node-id issues (both worth pinning once fixed).

---

Security

No new input boundaries; favorites round-trip through Codable Set<FavoriteEntry> with UUID + String fields. syncId uses SHA-256 as a stable record ID — fine, not a secret.

---

Recommendation

Request changes. The two List tag/ID collisions (Recent ↔ main, and same-name-different-schema in the Favorites tab) are real correctness bugs that will surface in normal use. The lock-while-posting and connection-deletion cleanup are smaller but worth addressing. Style/comments/UI-test-assertion items are easy follow-ups in the same pass.