Skip to content

refactor: WelcomeViewModel, ERDiagram, ResultTabBar#892

Merged
datlechin merged 4 commits into
mainfrom
refactor/hig-audit-round-2
Apr 26, 2026
Merged

refactor: WelcomeViewModel, ERDiagram, ResultTabBar#892
datlechin merged 4 commits into
mainfrom
refactor/hig-audit-round-2

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Second round of macOS HIG audit fixes, addressing MVVM layer violations, polling patterns, and remaining accessibility gaps.

WelcomeViewModel MVVM cleanup

  • Extracted pendingConnectionShareURL and pendingDeeplinkImport from AppDelegate into PendingActionStore singleton — ViewModel no longer casts NSApp.delegate
  • Replaced NSOpenPanel in ViewModel with .fileImporter modifier in View
  • Replaced NSAlert in ViewModel with .alert modifier in View
  • Moved window focus management to AppDelegate via .focusConnectionFormWindowRequested notification

ERDiagramViewModel reactive connection wait

  • Replaced 500ms x 20 polling loop with notification-based wait using .databaseDidConnect
  • Uses OSAllocatedUnfairLock for safe single-resume of CheckedContinuation
  • 10-second timeout preserved as fallback

Quick UI fixes

  • ResultTabBar: .onTapGesture replaced with Button for keyboard/VoiceOver accessibility
  • HistoryPanelView: .listStyle(.plain) changed to .listStyle(.sidebar) per macOS HIG

Test plan

  • Welcome window: create new connection, import connections from file, import from app — all sheets present correctly
  • Welcome window: open a .tablepro share file — import sheet appears
  • Welcome window: receive deep link import — sheet appears
  • Welcome window: click "New Connection" when form window exists — it focuses correctly
  • ER diagram: open on a connection that's still connecting — diagram loads after connection completes (no polling)
  • Result tab bar: Tab + Return selects result tabs; close/pin/context menu still work
  • History panel: sidebar-style list with inset selection appearance

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d5d6689a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +150 to +153
let observer = NotificationCenter.default.addObserver(
forName: .databaseDidConnect, object: nil, queue: .main
) { _ in
resumeOnce()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Scope connection notification before resuming load

waitForConnection() resumes on any .databaseDidConnect event because the observer is registered with object: nil and the callback does not verify which connection actually finished. In this codebase those notifications are emitted globally (DatabaseManager+Sessions.swift and DatabaseManager+Health.swift), so a different session reconnecting can wake this continuation and make loadDiagram() fail with "No database connection" for the requested connectionId even though it is still connecting.

Useful? React with 👍 / 👎.

Comment on lines +90 to +91
if DatabaseManager.shared.driver(for: connectionId) == nil {
await waitForConnection()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-check driver after registering connect observer

This pre-check waits only once before subscribing, and waitForConnection() does not perform an immediate post-subscription driver check. If the target connection is established in the small window between this driver(for:) == nil test and observer registration, its connect notification is missed and the code unnecessarily waits for the full 10-second timeout before loading the diagram.

Useful? React with 👍 / 👎.

@datlechin datlechin changed the title refactor: HIG audit round 2 — WelcomeViewModel, ERDiagram, ResultTabBar refactor: WelcomeViewModel, ERDiagram, ResultTabBar Apr 26, 2026
@datlechin datlechin merged commit 52ffebc into main Apr 26, 2026
2 checks passed
@datlechin datlechin deleted the refactor/hig-audit-round-2 branch April 26, 2026 19:20
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.

1 participant