fix(hig): finish the unreleased surfaces so Escape, search and column reorder behave natively - #2591
Merged
Merged
Conversation
… reorder behave natively Claude-Session: https://claude.ai/code/session_01Gy6Q4tzwG3bL9h15SMqep1
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
A UI/UX audit of everything added since 0.69.0, and the fixes it produced. The editor tab strip, the connections-strip cell and the statement-model split behind the Mongo shell came out clean. The findings concentrated in Copy To, column reorder, and plugin install progress.
Copy To and Duplicate Database (#2487)
Escape was inert on the Review and Result steps. Only
.configuringand.copyingcarried a.cancelAction, so a user who read the script and changed their mind had to press Back then Cancel, and Escape did nothing on either step. The HIG asks every sheet to be dismissible with Escape, and the app's ownSQLReviewSheetalready is. Review now carries a real Cancel, and.onExitCommandcovers the result step, whose only button owns Return. A copy in flight is deliberately excluded: the sheet is the only thing reporting what the run is doing.Both exits go through
close(), which cancels the session first.review()holds a task that reads both databases and promotes its weakselfto a strong one before the first await, so dismissing without cancelling left the session and its metadata reads alive with nothing to show them to.backToConfiguring()already cancelled; the exits have to agree.The object search was a text field wearing a magnifying glass, and Escape in it destroyed the configured copy. The app already ships
NativeSearchField, anNSSearchFieldwrapper used in Settings, the welcome window and the type chooser. The hand-rolled version had no clear button, no recessed search shape, and no Escape-clears-first, so Escape after typing a filter went straight past it to the sheet's Cancel and discarded the target, the content choice and every tick.NativeSearchFieldtakes the key itself while it holds text.Also: Select All / Deselect All replace All / None, dimmed when they would do nothing; a count under the list reports the whole selection rather than the part the search is showing; the footer's error is selectable and reachable in full through a help tag; and the settings column scrolls, because duplicating a database adds every option that engine's
CREATE DATABASEtakes to a sheet whose height is fixed.One command, one name. The Database menu said Copy Objects To… while the sidebar and the sheet said Copy To….
The endpoint picker had no search. Connections, then databases, then schemas, as a plain list 320 points wide. A server answering with hundreds of databases is not something scrolling finds. Each level now has its own search field, which also reaches Compare & Sync, where the picker is shared.
Column reorder (#2479)
Reordering was drag-only: no menu command, no keyboard, nothing VoiceOver could perform. The editor tab strip closed exactly this gap two commits earlier, in its own words: "a gesture with no menu equivalent cannot be found by a user who does not already expect it, and cannot be performed at all by VoiceOver." A column row's contextual menu now carries Move Column Up and Move Column Down, resolved through the same
ColumnReorderPolicythe drag is offered on, so the two can never disagree.They are built by
StructureGridDelegate, not by the row view, because a column row raises two different menus.KeyHandlingTableView.rightMouseDownintercepts a click inside the selection and answers fromDataGridRowView.contextMenu(for:); a click outside it falls through toStructureRowViewWithMenu.menu(for:). Items added only to the override would have been missing from select-then-right-click, which is the ordinary path and the one keyboard and assistive access take. One builder serves both, and the UI test now exercises both.The reason a reorder was withheld lived only in a help tag on the row-number gutter. The row still lifted, the no-drop cursor appeared, the drop did nothing, and the explanation needed a two-second hover over a narrow gutter. It is now spelled out as a dimmed item under the two dimmed commands.
The index arithmetic moved out of the AppKit subclass into
ColumnMove, a pure function, becausedesiredOrderspeaks NSTableView's drop index and moving down by one lands two rows on.Plugin install progress
All five
installMissingPlugincall sites discarded the fraction it publishes, so Install closed the alert and the app looked hung for the length of a network download.PluginInstallStatusRowin the connection form was built to draw a bar and reaches the fraction only throughPluginInstallTracker, which nothing on this path ever fed, so it always fell back to its indeterminate spinner.installMissingPluginnow publishes to the tracker, which fixes the connection form by itself. The two paths with no progress surface of their own (a Finder open, and the install alert) getPluginInstallProgressPresenter: a sheet on the window the user was working in, or a panel when there is none, which is the Finder-open case. A panel rather than anNSAlertbecauseAlertHelperruns an alert application-modal when no window qualifies, and a modal run loop cannot be updated from theawaitdriving it.Measured, not assumed: the installer publishes
0before the transfer and nothing again until the whole file is down, so a determinate bar drawn from it would sit at 0% for exactly the wait it exists to explain. The obvious fix,download(from:delegate:)with aURLSessionDownloadDelegate, was written and then measured against a 25.7 MB download with a knownContent-Length: zerodidWriteDatacallbacks, because the async form routes onlyURLSessionTaskDelegatemessages to a per-task delegate. That code was removed rather than shipped inert. Both the panel andPluginInstallStatusRowstay indeterminate until a fraction actually moves, which also retires a pre-existing 0% bar in the connection form.Editor tab strip (#2438)
EditorTabStrip.tooltipwas dead: the AppKit interaction view supplies the real tooltip fromEditorTabStripInteraction.tooltip. Removed, and the doc comment abovecanKeepOpenmoved topositionDescription, which is what it describes.Deliberately not shipped: the copy failure-policy picker
ObjectCopySession.errorHandlingis pinned to.stopAndRollbackwhileObjectCopyPlannerandObjectCopyRunnerread all threeImportErrorHandlingcases, and Compare & Sync exposes the identical picker over the same machinery. Exposing it here was in this branch and has been taken back out, because two review passes established that the runner cannot honour any honest description of the three choices:runStructureruns every cleanup group before any creation group under one transaction. Committing that phase on a stop, which is what.stopAndCommitwould ask for, can leaveDROP Bcommitted whenCREATE Afailed andB's creation was never attempted. The DDL loop also represents Stop as bothcancelledandstopped, so Stop itself would commit the destructive half.clearsInsideDataTransactionis false under.skipAndContinue, so target tables are emptied in an earlier phase. A per-table transaction around the row stream cannot restore them, and a failed clear leaves the table indataStepsso the runner appends into an uncleared table.ObjectCopyRowCopierkeeps its inserted count in a local, so a source-stream failure after committed batches reports zero rows copied. A retry duplicates them.Making the picker safe means an object-atomic transaction model in
ObjectCopyRunnerplus a tested policy matrix over structure replacement, data-only replacement, structure-and-data replacement, engines without transactions, failure and cancellation. That is a runner change, not a UI polish change, so the capability stays dark and this is reported rather than shipped half-right.Verification
verify.sh buildPASSverify.sh testPASS, 63 cases, overObjectCopySessionTests,ObjectCopyPlanTests,ObjectCopyRowCopierTests,ColumnReorderPolicyTests,ColumnMoveTests,PluginInstallTrackerStagedTestsverify.sh uitest StructureColumnMoveUITests CopyObjectsUITestsPASS, 8 casesverify.sh lint TablePro TableProTests TableProUITests0 violations. The oneagent docsfinding (AXCell,CLAUDE.md:220) is pre-existing and untouched by this branch.docs/scripts/check-writing-style.shandcheck-docs-against-source.pyboth passverify.sh pluginsfails locally on oracle-nio's@TaskLocalmacro (unknown attribute 'usableFromInlinenonisolated'), a known local-toolchain issue. This branch touches no plugin.reviewandadversarial-review. Every finding was verified against source before acting; three were fixed, and the fourth is why the picker was withdrawn.New tests:
ColumnMoveTestspins the drop-index arithmetic againstdesiredOrderrather than against the number it happens to produce;ObjectCopySessionTestsgains the two list buttons and the count's independence from the search;StructureColumnMoveUITestsproves both commands reach a column row's menu on both AppKit paths;CopyObjectsUITestsproves Escape in the search field clears it and the second Escape closes the sheet.CopyObjectsUITestsalso gained a scoped context-menu query. Database > Copy To… now carries the same title as the sidebar item, on purpose, and a closed menu bar submenu is still in the accessibility tree, so an app-rootedmenuItems[title]matched two elements and refused to click either.Not covered by automation
docs/features/copy-objects.mdxstill has no screenshot, the only feature page without one. Capturing it needs a driven Debug build and a light/dark pair; the attempt here kept colliding with the running app, and committing a placeholder would put a fake screenshot in the docs repo. Left as it was rather than made worse.