Repository navigation
fix(engine): apply the configured engine at boot, wait for it before the first send, sync the mobile chips - #127
Conversation
…the first send, sync the mobile chips Three ways the UI and the chat disagree about the engine. #103 and #105 proposed two of them and were withdrawn because they did not cure the reported 402; verifying them in a browser found the cause they both missed. - checkVersion() called loadTerminalCapability() before it stored the default engine, and that helper lives in the last script block. A reply landing before the parser reached it threw a ReferenceError into checkVersion's own catch{}, skipping the configured engine, the chat defaults and loadBots(). Measured on a clean main with defaultEngine=subscription and a late xterm.js: curEngine stayed "api". The call is now optional; its other callers load the capability lazily. - send() read curEngine on its 'api' placeholder before /api/version had landed. It now awaits engineDefaultReady(), bounded by VERSION_WAIT_MS so a dead /api/version cannot freeze the composer. First chat frame: engine "api" after 8 ms -> "subscription" after 2722 ms. - syncMobSheetFromDesktop() had no engine branch, so cur stayed '' and the toggle removed the shipped `on` from both engine chips. test/render/engine-default.test.mjs: 10 tests, each watched failing for the right reason first. CLAUDE.md test counts corrected (101 = 20 + 81; the 89/19/70 were already stale). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ignores the GitHub lookup The first commit made send() wait for /api/version. An independent review (codex, agy) found two defects in that wait, both reproduced before this change: - A click on an engine chip made before /api/version answered was overwritten by resolveEngineForView() and the message then left on the overwritten engine: the user chose Subscription, the turn ran on API. _engineUserPicked (set by setOpt/setMobOpt, cleared by resolveEngineForView) now exempts the pick from both the wait and checkVersion()'s resolve. - _versionCheckDone was the promise of the whole checkVersion(), which ends by awaiting api.github.com. With packets to GitHub dropped, every send() paid the full VERSION_WAIT_MS. The lookup moved to _checkLatestRelease(), not awaited. Also from the review: the default engine is stored right after applyTmuxCapability(), ahead of the notification, terminal and loadBots() calls that can throw. Measured in a browser: user picks Subscription while /api/version takes 2.5 s -> frame on "subscription" after 7 ms; GitHub unreachable -> first send after 6 ms. test/render/engine-default.test.mjs: 18 tests, the new ones watched failing first. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f0924a0f
ℹ️ 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".
| } | ||
|
|
||
| async function send() { | ||
| await engineDefaultReady(); // a settled promise costs one microtask once the page has been open a moment |
There was a problem hiding this comment.
When /api/version is still pending, this await can last up to three seconds while leaving the composer interactive. If the user presses Send in tab A and then switches to tab B, switchTab() saves A's composer and restores B's; after the await, send() reads the now-current inEl, attachments, activeTabId, session, and project. It will therefore silently abandon A's send or send B's existing draft to B without the user submitting it there. Snapshot or otherwise bind the pending operation to the originating tab before waiting.
Useful? React with 👍 / 👎.
1 participant
Three ways the UI and the chat disagree about the engine. Two were proposed earlier (#103, #105)
and withdrawn by their author because they did not cure the reported
402 api_key allotment exhausted while Subscription is selected. Verifying them in a real browserfound why: neither touched the cause, which is item 1 and is in
main.1. The configured default engine is silently not applied (the cause)
checkVersion()calledloadTerminalCapability()before it stored the default engine, and thathelper is declared in the LAST script block. When the
/api/versionreply lands before the parser hasreached that block, the call throws
ReferenceError, whichcheckVersion's owncatch {}swallows.Everything after it was skipped: the default engine, the editor label, the #58 chat defaults and
loadBots().The default is now stored right after
applyTmuxCapability(), ahead of every call that can throw, andthe late-block call is optional (its other callers load it lazily). Moving the function to the early block
was rejected: it owns
_terminalAvailable/_termCapabilityReason, which arelets in the late block.Same family as
isEnginePaneId(test/render/script-scope.test.mjs).2. The first message can leave on the
'api'placeholder (#103)curEnginestarts as'api'and only becomes the configured default once/api/versionhas beenhandled.
send()now awaitsengineDefaultReady()first. Not "for the life of the chat", as #103claimed:
server.jsre-persistsrun_engineon every turn, so the damage is the one turn sent in thatwindow. The wait has three rules, the last two found by review (below):
VERSION_WAIT_MS, 3 s) and never throws: a dead/api/versionmust not freeze the composer._engineUserPicked, set bysetOpt/setMobOpt, cleared byresolveEngineForView()); their pick is not a placeholder._checkLatestRelease(), not awaited.3. The mobile sheet blanks the engine chips (#105)
The API chip ships as
class="mob-chip on", andsyncMobSheetFromDesktop()compared every chip againsta
curthat stayed''for the engine group, so opening the sheet removed the highlight from both chips.Independent review
Before merging I had the diff reviewed by two advisors from other providers (codex, agy). They found two
real defects in my first version of item 2, both reproduced first, then fixed with the tests written first:
/api/versionanswered was overwritten byresolveEngineForView()and then sent: pick Subscription, default is API, message leaves on
api. A regression my changeintroduced into a window that used to be harmless.
_versionCheckDonewas the promise of the wholecheckVersion(), which ends by awaitingapi.github.com; with packets to GitHub dropped, everysend()paid the full bound.Measured (real browser, patched vs clean
main, same server config)mainsubscription, terminal script block late (2.5 s)curEngine: "api""subscription"/api/versiontakes 2.5 sengine: "api"after 8 ms"subscription"after 2467 ms/api/versionnever answersapi,/api/versiontakes 2.5 s"subscription"after 7 ms, still selected afterwardsapi.github.comunreachable, first sendcurEngine = "subscription"api:off subscription:offapi:off subscription:ONTests
test/render/engine-default.test.mjs— 18 tests. 15 were watched failing for the right reason before the change(including the reviewer's scenario end to end on the real
setOpt→checkVersion→send); the other 3 areguards that pass before and after (other chip groups untouched, a model/mode click is not an engine pick, the
terminal capability is still preloaded once its block exists).
Render suite 111/111; the full
npm testchain was run file by file, 81 of 82 commands clean.The one that was not is
test/global-workspace.test.js. Its fixtures are time-dependent (in_progresstasksreverted to
todowhile seeding is slow) and it fails the same two assertions on a cleanmaintoo(1 of 3 standalone runs). The limits of that evidence: standalone it passed 5 of 5 on this branch, but inside the
long
npm testchain it failed 4 of 5 runs here, and I did not run the chain onmainfor comparison.CI (Node 20 and 24) passed on the PR, and on
mainafter the merge.test/auth-boundary.test.js(
revoked socket stayed open) is intermittent the same way (main1 of 7, branch 3 of 7). Neither readsindex.htmlorCLAUDE.md; neither is touched here.Also:
CLAUDE.mdtest counts corrected (101 = 20 + 81; the 89/19/70 were already stale), and a note thata literal script tag in an
index.htmlcomment breaksscript-scope.test.mjs's block count (it did, once, while writing this).Not a release; intended for the next one.