From 5e40fb6f656cadae4e3484fa93ff4232fe78c9dd Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 9 Aug 2026 11:27:29 +0200 Subject: [PATCH] fix(chatgpt-review): retry finding the send button instead of racing its render Follow-up to PR #656: the user correctly pushed back on my initial diagnosis that live-testing failures against big/long conversations were purely a self-inflicted dangling-CDP-connection artifact. Testing properly (isolating "pre-existing" from "big page", and using the real production selector- fallback discipline instead of a hardcoded selector index) found the actual production code works fine on big pages -- but surfaced a real, separate, previously-undetected bug: insertNatively() resolves as soon as its in-page evaluate() call returns, which is BEFORE React has necessarily re-rendered in response to it. Confirmed live against the real ChatGPT composer: the send button's own selector showed 0 matches on the very next synchronous DOM query, and only 1 match after yielding a render tick (requestAnimationFrame). fillAndSend's single, immediate, no-retry check for the send button was silently falling through to composer.press('Enter') on this timing race, not because the button was missing. Extracted a shared submit(page, composer) helper (used by both fillAndSend and the stall-recovery nudge) that retries finding the send button briefly (up to 5 x 100ms) before falling back to Enter, removing the race instead of relying on incidental Playwright-call latency to happen to mask it. Verified: 59/59 unit tests (2 new, directly reproducing the race and the still-correct Enter fallback when the button genuinely never appears), and live reproduction of the exact race against the real, currently-open, long-running conversation this was originally investigated on. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- skills/chatgpt-review/scripts/lib/browser.mjs | 25 +++++++++--- skills/chatgpt-review/tests/browser.test.mjs | 39 +++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/skills/chatgpt-review/scripts/lib/browser.mjs b/skills/chatgpt-review/scripts/lib/browser.mjs index 92e65128..76a818e5 100644 --- a/skills/chatgpt-review/scripts/lib/browser.mjs +++ b/skills/chatgpt-review/scripts/lib/browser.mjs @@ -191,6 +191,24 @@ export class ChatGptBrowser { } catch { return false; } } + // insertNatively resolves as soon as the in-page evaluate() call returns, which is + // BEFORE React has necessarily re-rendered in response to it — confirmed live: the send + // button's own selector showed 0 matches on the very next synchronous DOM query, and 1 + // match only after yielding a render tick (requestAnimationFrame). Checking for it exactly + // once, immediately, made this silently fall through to the composer.press('Enter') + // fallback on what is actually a timing race, not a missing/dead selector — a few short + // retries (bounded, cheap) removes the race instead of relying on it happening to be + // masked by whatever incidental latency the caller's own prior awaits introduced. + async submit(page, composer) { + let send = null; + for (let attempt = 0; attempt < 5 && !send; attempt += 1) { + send = await firstVisible(page, SELECTORS.send); + if (!send) await this.sleep(100); + } + if (send) await send.click(); + else await composer.press('Enter'); + } + async fillAndSend(page, prompt, deadline = this.now() + 242_000) { const composer = await firstVisible(page, SELECTORS.composer); if (!composer) throw new ReviewError('ui_incompatible', 'ChatGPT composer disappeared before submission'); @@ -209,9 +227,7 @@ export class ChatGptBrowser { } if (lastError) throw lastError; } - const send = await firstVisible(page, SELECTORS.send); - if (send) await send.click(); - else await composer.press('Enter'); + await this.submit(page, composer); } async waitForPermanentConversationUrl(page, deadline = this.now() + 15_000) { @@ -375,8 +391,7 @@ export class ChatGptBrowser { await this.sleep(1000); const composer = await firstVisible(page, SELECTORS.composer); if (composer && (await this.insertNatively(composer, RECOVERY_NUDGE))) { - const send = await firstVisible(page, SELECTORS.send); - if (send) await send.click(); else await composer.press('Enter'); + await this.submit(page, composer); } lastText = ''; stableSince = null; diff --git a/skills/chatgpt-review/tests/browser.test.mjs b/skills/chatgpt-review/tests/browser.test.mjs index a7ae3566..51eed629 100644 --- a/skills/chatgpt-review/tests/browser.test.mjs +++ b/skills/chatgpt-review/tests/browser.test.mjs @@ -360,6 +360,45 @@ test('native insertion is preferred when the composer supports it; Playwright .f assert.equal(composer.value, undefined); }); +test('a send button that only mounts a moment after native insertion is still found and clicked, not skipped for the Enter fallback', async () => { + // insertNatively resolves as soon as the in-page evaluate() call returns, BEFORE React + // has necessarily re-rendered in response to it — confirmed live against the production + // ChatGPT composer: the real send-button selector showed 0 matches on the very next + // synchronous DOM query and only 1 match after yielding a render tick + // (requestAnimationFrame). A single, immediate, no-retry check for the send button would + // silently fall through to composer.press('Enter') on this timing race every time, not + // because the button is actually missing. + let checkCount = 0; + let clicked = false; + let pressed = false; + const composer = new Element({ evaluate: () => {} }); + composer.press = async (value) => { pressed = value; }; + const page = readyPage({ + [SELECTORS.composer[0]]: [composer], + // The send selector "exists" only from the 3rd check onward — exactly like the real + // send button only appearing after React's next render tick, not on the very first, + // immediate, synchronous check right after insertion. + [SELECTORS.send[0]]: () => { + checkCount += 1; + if (checkCount < 3) return []; + return [new Element({ onClick: () => { clicked = true; } })]; + }, + }); + await driverWith(page).fillAndSend(page, 'prompt text'); + assert.ok(checkCount >= 3, `expected at least 3 checks before the button appeared, got ${checkCount}`); + assert.equal(clicked, true, 'expected the send button to be found and clicked once it mounted'); + assert.equal(pressed, false, 'must not fall back to Enter when the button eventually appears'); +}); + +test('submit() still falls back to Enter if the send button genuinely never appears', async () => { + let pressed = false; + const composer = new Element(); + composer.press = async (value) => { pressed = value; }; + const page = readyPage({ [SELECTORS.composer[0]]: [composer] }); // no [SELECTORS.send] at all + await driverWith(page).submit(page, composer); + assert.equal(pressed, 'Enter'); +}); + test('the whole review() call is bounded by its own timeoutMs even when a setup phase needs its own retry', async () => { // Before the unified-deadline fix, assertReady/upload/fillAndSend/waitForPermanentConversationUrl // each had their own independent, ADDITIONAL worst-case allowance on top of timeoutMs, so