Repository navigation
Gate fixes: modal scroll lock + focus restore (PR #100 findings) - #103
Merged
sgardoll merged 5 commits intoSep 28, 2026
Merged
Conversation
PR #100 review findings on the shared modal shell: F1 (Greptile P2): a repeated open of the already active modal — the walkthrough's blur and delayed Tab handlers both do this — saved "hidden" as the page's previous overflow and re-recorded the already-inert background. The eventual close restored "hidden" and left the background inert and aria-hidden, locking the page. A repeated open is now idempotent: it keeps the return target and the overflow saved before the first open, and setBackgroundInert records each child's pre-modal state once. F2 (Devin): openModal captured the return target after closing the replaced dialog, so a replacement opened from a control inside the previous dialog restored focus into the dialog it hid on close. The return target is now captured before the close and, when the active element lived inside the replaced dialog, falls back to whatever opened that dialog. Regression coverage: e2e/gate-modal-behavior.spec.js fails on base head 6dd8edb (close leaves overflow "hidden"; replacement close focuses a control inside the hidden walkthrough) and passes with this fix. F3 root cause (stu391 CI flake): the spec's signed-in fixture is an out-of-runs free user (usage_count 2, free limit 2), so the async identity resolution runs showPaywallExhausted, which closes an open walkthrough. When that landed after the spec's reopen click the tour was closed asynchronously and waitForWalkthroughOpen never settled (captured stack: resolveIdentity → updateUsageDisplay → showPaywallExhausted → closeModal). reopenWalkthrough now waits for the persisted usage decision before clicking. The app-side race (passive usage rendering closing a user-opened modal) is left untouched here; app.js is outside this change's ownership.
sgardoll
marked this pull request as ready for review
September 28, 2026 07:10
Contributor
|
This PR does not match any of the 1 configured review trigger rule. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
showPaywallExhausted closed any open walkthrough whenever the exhausted surface rendered, including the passive startup render that follows the async identity response. A user who opened the tour before the usage decision resolved lost it as the response landed. Gate the close on an explicit dismissWalkthrough option passed only by canRunPipeline, so an out-of-runs run attempt still surfaces the paywall in place of the tour. The STU-391 regression test gates the identity response to pin the race: the tour stays open for a passive resolution, while a run attempt still closes it and opens the pricing modal. The stu391 waitForUsageDecision helper is removed, restoring the flow that exercises the race itself.
…fixes Resolve the openModal conflict in favour of the base side. Base's version is a strict superset: it additionally handles modal.contains(target) so focus cannot be stranded inside a programmatically reopened dialog, and it gates the overflow save and setBackgroundInert behind `activeModal !== modal`. The PR's block called setBackgroundInert unconditionally, which on a repeated open re-recorded the already-inerted state and would have left the background permanently inert after close. With that gate in place, also revert the PR's setBackgroundInert guard. setBackgroundInert has exactly one call site, inside the `activeModal !== modal` branch, and restoreBackground() clears inertState whenever activeModal is cleared, so the map is always empty on entry and the `!inertState.has(child)` branch is unreachable. Trim the now-stale clause in showPaywallExhausted that justified the dismissWalkthrough opt-in via updateUsageDisplay: base's !hasActiveModal() gate means updateUsageDisplay can no longer reach the dismissing path.
Comment on lines
+5913
to
+5915
| if (options.dismissWalkthrough) { | ||
| const walkthroughModal = document.getElementById('walkthrough-modal') | ||
| if (walkthroughModal) closeModal(walkthroughModal, { restoreFocus: false }) |
Contributor
There was a problem hiding this comment.
🟡 Exhausted paywall stays hidden after tour
When quota resolves during an open tour, dismissWalkthrough never reaches the paywall because updateUsageDisplay skips active modals. Closing the tour does not refresh usage, leaving the exhausted paywall hidden.
Learn more
The usage display normally swaps the ready state for the exhausted paywall when a user reaches the run limit. updateUsageDisplay does not call showPaywallExhausted if any modal is active, even though this change makes the paywall renderer safe for an open walkthrough. It hides the paywall instead. closeWalkthroughModal only closes the dialog and does not rerun the usage display, so the exhausted surface remains hidden when the tour closes. The run gate still prevents a generation attempt, but the passive exhausted state is absent.
Example: A guest opens the tour with zero locally cached runs. Identity then reports two runs, the free-tier limit. The paywall remains hidden while the tour is open and after the guest closes it, although the new regression test expects it behind the tour.
Recommended fix: Adjust updateUsageDisplay to render exhausted usage while the walkthrough is open without dismissing it, or rerender on walkthrough close. Preserve the deferral for other active dialogs if swapping the stage beneath them is undesirable, and test the state both before and after closing the tour.
Was this helpful? React with 👍 or 👎 to provide feedback.
…s guard PR #103 added an opt-in `options.dismissWalkthrough` flag to showPaywallExhausted: always render the paywall, and only tear down an open tour when a caller passes the flag. The milestone base already carries a different fix (37ffe47): `updateUsageDisplay` guards its call with `!hasActiveModal()`, deferring the ENTIRE paywall while any dialog is open. The two are mutually exclusive, and the test written against the opt-in asserted a state base's guard makes unreachable — a passive exhausted identity resolution surfacing `#paywall-exhausted` beside a still-open tour. Base's mechanism wins: `showPaywallExhausted` returns to closing the walkthrough unconditionally, so app.js is now identical to the milestone base, and the opt-in and its comment are gone. The test now pins base's actual contract: a passive startup identity resolution for an exhausted free user defers the paywall entirely and leaves the open tour alone; once the tour is closed, the next usage decision does surface the paywall (deferral is a delay, not a suppression); and an active out-of-runs `runThinkingPipeline()` still surfaces the paywall, closes the walkthrough, and opens pricing. The decision's persisted metered count is used as the "the decision landed" signal, so the negative assertion cannot pass vacuously — removing `!hasActiveModal()` makes it fail.
sgardoll
merged commit Sep 28, 2026
a2a9e82
into
milestone/stu-implement-updated-ui-ux
3 checks passed
sgardoll
pushed a commit
that referenced
this pull request
Sep 28, 2026
…u445-exact-html-fidelity Re-sync STU-445 onto base f1c819d (merges #108 -> #104 -> #103). Single conflict, in index.html, comment-only: the base adds a five-line comment above <div class="pipeline-view"> documenting #104's initPipelinePanelOrder focus-order contract. Resolved by taking the base side's comment. The base's #104 CSS changes (dropping grid-template-areas in favour of JS-driven region ordering) auto-merge and are preserved. dist/ regenerated from the merged sources: app.js differs from both parents, so the previous bundle was pre-merge. index-CD_p5Yhb.js -> index-C31N1DOp.js. CSS is byte-identical (index-BQUorxWE.css), and a second build is a no-op. npm test 388/388 green (the base contributes 3 tests over the prior 385). No test changed, skipped, or weakened by this merge.
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.
Gate fixes for PR #100 review findings (base head
6dd8edb)Branch
sgardoll/gate-modal-fixes→milestone/stu-implement-updated-ui-ux.Files:
src/sharedControls.js,app.js(F3 gate only), newe2e/gate-modal-behavior.spec.js,e2e/stu391-tutorial-onboarding.spec.js. No index.html or other src changes.F1 — "Repeated modal opens lock scrolling" (Greptile P2, src/sharedControls.js:46-52)
Fix. A repeated open of the already active modal is now idempotent:
openModalkeeps the return target and the overflow saved before the first open, andsetBackgroundInertrecords each child's pre-modal state only once (the repeated call used to overwrite the saved state with the already-inert state, so the close never restored the background).Fail → pass evidence (
e2e/gate-modal-behavior.spec.jstest 1: open walkthrough → repeat open → close):The same test also asserts the background header is
inert === falseandaria-hidden === nullafter the final close (fails on base withinert === true,aria-hidden === "true").F2 — "Replacement dialog can restore focus into hidden content" (Devin, src/sharedControls.js:40-43)
Fix. The return target is captured before the replaced dialog closes. If the active element (or explicit trigger) lives inside the replaced dialog, the replacement falls back to whatever opened that dialog — so a replacement's close never focuses content the replacement hid.
Fail → pass evidence (test 2: focus a body trigger → open walkthrough → focus settles inside it →
openApiKeysModal()replaces it → close replacement):F3 —
e2e/stu391-tutorial-onboarding.spec.js:260CI flake (30s timeout inwaitForWalkthroughOpen)Root cause (refutes the F1 hypothesis; evidence below). The failing attempt is the first
reopenWalkthroughafterpage.goto("/")(CI stack:waitForWalkthroughOpen←reopenWalkthrough← spec:271). CI log for run 36351586609:The spec's
signedInContext()fixture is an out-of-runs free user (usage_count: 2,planLimits.free = 2). After the test clicks#wt-reopenand the tour opens, the async identity resolution runsupdateUsageDisplay()→showPaywallExhausted(), which unconditionallycloseModal(walkthroughModal)s the just-opened tour. The open is undone asynchronously andwaitForWalkthroughOpennever settles. Under CPU contention this reproduces reliably; a stack trace captured in the page pins it exactly:Reproduction harness (25 iterations, 12 CPU burners, same fixture), page state 400 ms after the click:
opens: 1(handler ran),closes: 0,openClass: false,aria-hidden: "true",transform: matrix(1,0,0,1,0,0)(openModal ran),hash: ""(no navigation). Repeated-open corruption is not involved.Fix (app-side, head
f3b5787;dismissWalkthroughgate).showPaywallExhaustednow dismisses an open walkthrough only when the caller passesdismissWalkthrough: true. The exhausted surface still renders for every caller — passive usage rendering included — but only the run-attempt path may take a tour away:canRunPipeline(app.js:5883) passes{ openModal: true, dismissWalkthrough: true }, so an out-of-runs run still surfaces the paywall in place of the tour.updateUsageDisplay(app.js:6219) callsshowPaywallExhausted(count, limit)with no options, so the startup identity resolution and the post-run reconciliation surface the exhausted state without closing a tour the user opened.reopenWalkthroughno longer waits for the usage decision (the earlier spec-sidewaitForUsageDecisionwait was removed inf3b5787): the click can land before or after the decision, the race is exercised, and the tour survives either order.Regression test (same spec). The identity response is gated until the tour is open, then answered with an exhausted free user (
usage_count = PLAN_LIMITS.free). Assertions: the passive resolution renders#paywall-exhaustedwhile the walkthrough stays.open; a subsequentrunThinkingPipeline()closes the walkthrough, keeps#paywall-exhaustedvisible, and opens#pricing-modal.CI on pushed head
f3b5787CI run 36393187510 — success on the first attempt, covering the full sequence:
npm test,npm run build,npm run test:ui(including the F3 regression test),npm run test:accessibility,npm run test:deploy-guard,npm run test:proxies, bundle leak scan.F1/F2 code is unchanged since
c4783ce, where the local full sequence passed:npm test354 passed; build ok;test:ui208 passed (206 baseline + 2 new); accessibility 7 shells; deploy-guard 9/9; proxies 30/30; no bundle leaks.Not implemented / left alone
{ timeout: 8000 }arguments in this spec (page.waitForFunction(fn, {timeout})— the object lands in theargslot, so the default 30s applies) are untouched: fixing the slot would shorten the effective timeout and is out of scope.app.jschange is the F3dismissWalkthroughgate; index.html, other src modules, and scripts are untouched.dist/andscreenshots/were restored after the UI suite rewrote them.Uncertainty / judgement calls
dismissWalkthroughis deliberately opt-in: any future caller that wants an open tour taken away must pass it; passive renders never do.PLAN_LIMITSfromsrc/plansSurface.js; if the fixture contract or that module moves, the test fails loudly.e2e/stu385-composer-settings.spec.js("a model chosen in the dialog is what the generation request carries",generatorModels === []on all 3 attempts). It passes locally; likely another owner's concern.