fix: tab session registry binding and fix-error prompt phrasing#1115
Merged
fix: tab session registry binding and fix-error prompt phrasing#1115
Conversation
|
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.
Summary
Two fixes that landed during AI Wave 2 test cleanup. One is a real user-facing bug discovered while diagnosing test failures; the other tightens an internal contract that was masking test-only crashes.
What changed
fix(coordinator): bind tab session registry when coordinator owns the fallback(8f5853a9)Production builds the
TabSessionRegistryonce inSessionStateFactory.createand hands it to bothQueryTabManagerANDMainContentCoordinator, sotabs.didSetkeeps the registry in sync. When the coordinator falls back to creating its own registry (e.g. tests, or any composition that builds the coordinator without going through the factory),QueryTabManagerhad no way to know about it.tabs.didSetearly-returned, noTabSessionwas registered, andrestoreLastFilterstripped theassertionFailureatMainContentCoordinator+FilterState.swift:311.Added
QueryTabManager.bindTabSessionRegistry(_:). The coordinator calls it after resolving the fallback registry, which back-fills sessions for any pre-existing tabs. The assertion stays — it now only fires on real registry-sync regressions, which is what it was always supposed to do.Also fixed the em-dash style violation in the assertion message.
fix(ai-chat): use display name and editor language for fix-error prompt phrasing(dec24036)AIPromptTemplates.fixErroralways built its prompt as"This <queryLanguageName> query failed". For SQL that produces "This SQL query failed" (correct). For MongoDB it produced "This MQL query failed" (the AI doesn't always recognize MQL). For Redis it produced "This Redis CLI query failed" (Redis ops are commands, not queries).queryInfo(for:)now switches on the plugin'seditorLanguageand usesdisplayNamefor non-SQL languages, plus"command"instead of"query"for bash:"SQL query"(unchanged)"<DisplayName> query"("MongoDB query")"<DisplayName> command"("Redis command")This is a user-facing improvement — Fix Error prompts now read naturally for non-SQL backends, which improves the AI response quality.
Test plan
xcodebuild buildgreenswiftlint lint --strictclean on every touched fileOpenTableTabTests/addsTabDirectlyWhenTabsEmptyNotSwitching,AIChatViewModelActionTests/fixErrorMongoDBConnection,AIChatViewModelActionTests/fixErrorRedisConnectionOut of scope
Two pre-existing baseline failures unrelated to this PR:
MainContentCoordinatorLazyLoadTests/Returns early when tab is already executingSessionStateFactoryTests/Connection-only payload without isNewTab creates empty tab managerAIChatViewModelMentionsTests/currentQueryResolvedalso fails but is asserting on the wrong API (expectsplainTextto contain attachment text beforeresolveTurnForWireruns; sibling tests confirm raw-then-expanded is the intended design). Either the test or the assertion needs updating, separate from this PR.