Skip to content

STU-385: Move generation options into the composer settings dialog - #97

Merged
sgardoll merged 4 commits into
milestone/stu-implement-updated-ui-uxfrom
sgardoll/stu-385-move-generation-options-into-the-composer-settings
Sep 23, 2026
Merged

sgardoll merged 4 commits into
milestone/stu-implement-updated-ui-uxfrom
sgardoll/stu-385-move-generation-options-into-the-composer-settings

Conversation

@sgardoll

@sgardoll sgardoll commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

STU-385 — Move generation options into the composer settings dialog

Linear: https://linear.app/stuart-gardoll/issue/STU-385/move-generation-options-into-the-composer-settings-dialog

What changed

The composer's gear now opens a generation settings dialog that presents the existing model selection and the supported generation option (image input), bound to the live MODEL_LABELS / FREE_MODEL / PRO_MODELS and tier state (free / paid / unresolved, including checking/failed resolution). Provider-key management is a shortcut into the canonical account connection editor (openApiKeysModal, STU-384) — not a second independent editor. Closing the dialog returns focus to the composer and never reopens the walkthrough.

#code-generator-model remains the single element the generation pipeline reads (no key-storage or transport change), so selections flow into the actual generation request. It stays on the page and the dialog's model control is a synchronized mirror that forwards to it — this was required to preserve the hero's vertical-scroll invariant at 390×844 (stu389 hard-fails if the on-page selector is removed: base scrollHeight 908 vs 844 after removal).

Acceptance criteria

  • Free/paid/unresolved fixtures show correct availability + transparent upgrade — stu385-composer-settings.spec.js "model availability by subscription tier": free labels Pro models "(PRO)" and shows "Upgrade for all models" (selecting a Pro model reverts to Gemini and opens pricing); paid unlocks Pro models unlabelled with no upgrade notice; unresolved never claims a free-only limitation.
  • Selected options appear in the actual generation request; no key-storage/transport change — "a model chosen in the dialog is what the generation request carries" intercepts ENDPOINTS.pipeline and asserts body.model === "anthropic/claude-opus-5" for the generator stage. #code-generator-model is unchanged as the request source; no key-storage code touched.
  • Close/cancel returns to the initiating surface; ordinary close never reopens the walkthrough — focus-return test (activeElement back to the gear) and an ordinary-close test that asserts #walkthrough-modal never opens via Escape or Done. Consistent with and non-regressing STU-391 (walkthroughSettingsActive untouched by normal close; ghost of the app.js:949–960 bug exercised).
  • Browser tests cover model switching, image capability change and focus return — the three named behaviors above plus the provider-key shortcut test.

CI results (all on this branch, hermetic CI=true)

Step Result
npm test 346 passed
npm run build ok
npm run test:accessibility 7 modal shells passed (incl. composer-settings-modal)
npm run test:deploy-guard 9/9
npm run test:proxies 30/30
bundle leak scan "No API key/endpoint leaks found"
Browser suite CI=true run 1 181 passed, 1 flaky (stu389 timing, passed on retry)
Browser suite CI=true run 2 180 passed, 2 flaky (stu389 timing, passed on retry)
Base full suite CI=true (control) 166 passed, 5 flaky (761e incl. stu389, stu375, stu376)

The base control run (15 min, heavier load) produced more flakiness than this branch, confirming the stu389 timing flakes are pre-existing in this repo (its own playwright.config.js notes CI is "SERIAL on a slower runner"); they were not introduced here and passed on retry in both runs.

Falsification (each behavior broken → test failed → restored → passed)

Every test was written against a concrete break I actually induced; observed fail + pass for each:

  1. Gear wiring — reverted onclick to openApiKeysModal() → all 8 STU-385 tests failed (#composer-settings-modal never gets .open). Restored → 8 pass.
  2. Focus return — closeModal(..., { restoreFocus: false }) → "closing the dialog returns focus to the composer" failed (activeElement ≠ gear). Restored → pass.
  3. Ordinary close → walkthrough — forced walkthrough open in closeComposerSettings → "an ordinary close never reopens the walkthrough" failed. Restored → pass.
  4. Model → request — getEffectiveModel stubbed to FREE_MODEL → request test failed (generatorModels ≠ selected). Restored → pass.
  5. Dialog mirror forward — removed canonical.value = mirror.value → request test failed (request stayed Gemini). Restored → pass.
    Grep confirmed no FALSIFICATION or other break marker remains in the tree.

Screenshots (changed surfaces)

  • screenshots/stu-385/stu385-composer-settings-free-1440.png (1440×900, free tier)
  • screenshots/stu-385/stu385-composer-settings-free-360.png (360×800, free tier)
  • screenshots/stu-385/stu385-composer-settings-paid-1440.png (1440×900, paid tier)

Visual slices validate 360×800 and 1440×900 plus the dialog's own responsive column (the modal is centered modal-overlay); the milestone's full nine-viewport matrix is owned by final acceptance.

Files changed

  • index.html — gear bound to openComposerSettings(); composer-settings-modal (mirror model select + image-input generation option + provider-key shortcut → account editor); hero-settings kept as the canonical selector surface.
  • app.js — openComposerSettings / closeComposerSettings / syncComposerSettingsModel / renderComposerSettingsOptions + window exports. Closing never touches walkthrough state.
  • e2e/stu376-composer.spec.js — settings-control test now asserts the generation-settings dialog + provider-key shortcut.
  • e2e/stu385-composer-settings.spec.js — new (8 tests).
  • dist/index.html — tracked deployable artifact regenerated; orphaned bundle pruned.
  • screenshots/stu-385/*.png — review captures.

Branch: sgardoll/stu-385-move-generation-options-into-the-composer-settings. Top commit b392fd8; push containment verified: CONTAINED.

The composer's gear opens a generation-settings dialog that presents the
existing model selector and the supported generation option (image input).
#code-generator-model remains the single element the pipeline reads, so
selections reach the actual request unchanged; provider keys stay a shortcut
into the canonical account connection editor; ordinary close returns focus to
the composer and never reopens the walkthrough.

Regenerates the tracked dist/index.html and prunes the orphaned bundle.
…s it

Removing the on-page #code-generator-model flattened the hero so it no longer
overflowed at 390x844, breaking the hard 'page is reachable' scroll invariant
(stu389). Keep the canonical selector in hero-settings and make the composer
settings dialog's model control a synchronized mirror that forwards to it, so
dialog selections still reach the actual generation request while availability
labels and the free-tier upgrade action stay in step. stu377 back to baseline.
Free-tier and paid-tier dialog at 360x800, 1440x900.
@sgardoll
sgardoll marked this pull request as ready for review September 23, 2026 18:08
@greptile-apps

greptile-apps Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

This PR does not match any of the 1 configured review trigger rule.

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 4 potential issues.

Devin Review

Comment thread index.html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Settings migration remains incomplete

The original model selector and API-key link remain visible below the composer. Users now see duplicate controls outside the new dialog.

(Refers to this code)

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread dist/index.html Outdated
}
</style>
<script type="module" crossorigin src="/assets/index-oXwmwfKe.js"></script>
<script type="module" crossorigin src="/assets/index-BdVVPYTC.js"></script>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Production bundle is missing

A fresh checkout references index-BdVVPYTC.js, which is absent from HEAD. The app cannot load, and the deployment guard rejects the release.

Learn more

The production HTML names a content-hashed JavaScript bundle. The PR deletes the old tracked bundle but does not commit the new one. ensure_dist_self_consistent rejects exactly this state because publishing it would upload broken HTML and prune the working remote bundle.

Example: On a fresh checkout, /assets/index-BdVVPYTC.js returns 404. python3 scripts/deploy_ftp.py --dry-run exits because that referenced file does not exist.

Recommended fix: Run the production build, then force-add and commit the generated dist/assets/index-BdVVPYTC.js. Confirm every local dist/ file is tracked and rerun the deploy guard.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread app.js
Comment on lines +1208 to +1209
syncComposerSettingsModel();
renderComposerSettingsOptions();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Early Pro selection bypasses gating

Before subscription reconciliation, openComposerSettings exposes ungated Pro choices. A free user's selection skips pricing, then generation silently falls back to Gemini.

Learn more

The page becomes interactive while the asynchronous auth and subscription startup still runs. The canonical Pro-gating listener is installed only when updateModelSelectorGating runs after reconciliation. Opening this dialog earlier copies the raw canonical options, and its change handler can select a Pro model before that listener exists. The pipeline later waits for reconciliation, then getEffectiveModel replaces that selection with the free model without opening pricing.

Example: A returning free user clicks the gear during a slow subscription request and selects Claude Opus 5. The dialog retains Claude and no pricing modal appears, but the generator request uses Gemini.

Recommended fix: Keep the model control disabled or in a loading state until sessionReadiness settles. Then run updateModelSelectorGating(), synchronize the mirror, and enable selection.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread index.html
</div>

<div class="rounded-lg border border-gray-200 overflow-hidden">
<button type="button" class="w-full flex items-center justify-between gap-3 px-3 py-2.5 text-left hover:bg-gray-50 transition-colors" onclick="openApiKeysModal()" aria-haspopup="dialog" aria-controls="api-keys-modal">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 API-key shortcut loses focus

Opening API keys makes the hidden shortcut button its return target. Closing API keys leaves keyboard focus without a visible control.

Learn more

Opening a new modal automatically closes the active modal without restoring focus. openModal then records the currently focused shortcut as the API-key modal's return target. That shortcut belongs to the now-hidden composer settings dialog, so closeModal cannot return keyboard focus to a visible control.

Example: A keyboard user opens Generation settings, activates “API keys & connection,” and closes API Keys. Focus does not return to the gear or another visible composer control.

Recommended fix: Close the composer settings dialog before opening API keys so focus first returns to the gear, or extend the modal API to accept an explicit return target and pass the gear button. Rebuild dist/ after changing the source HTML.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

…broken

The branch regenerated dist/index.html and deleted dist/assets/index-oXwmwfKe.js
without committing a replacement bundle, so dist/index.html referenced
/assets/index-BdVVPYTYC.js while dist/assets held only a font and a CSS file —
a built page whose entire application script 404s.

dist/ is regenerated by CI (`npm run build`) and by the documented deploy path,
and tracked build output is what caused a cross-PR merge conflict earlier today.
Reverted to the integration branch's copy; no source change.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Devin Review

Comment thread app.js
Comment on lines +1162 to +1167
mirror.value = canonical.value;
const canonicalOptions = Array.from(canonical.options);
Array.from(mirror.options).forEach((mo, i) => {
const co = canonicalOptions[i];
if (co) mo.textContent = co.textContent;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Plan resolution leaves model mirror stale

When plan resolution finishes with settings open, composer-settings-model retains its earlier selection and labels. A free-tier reset changes the canonical model to Gemini, while the dialog still displays Pro.

Learn more

The dialog mirror synchronizes only when the dialog opens or its own model control changes. Startup intentionally leaves composer controls interactive while authentication and subscription requests run. Later, updateModelSelectorGating can relabel options and reset a Pro selection to FREE_MODEL, but it never synchronizes the open mirror.

Example: A user opens settings before startup plan resolution, selects Claude, and then resolves as free-tier. The canonical selector resets to Gemini, but the open dialog still displays Claude without the current free-tier labels. The next generation uses Gemini despite the visible selection.

Recommended fix: Synchronize the composer mirror whenever updateModelSelectorGating changes the canonical selector or its labels. Also refresh the dialog capability row after a value reset.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread index.html
Comment on lines +3966 to +3975
<select id="composer-settings-model">
<option value="google/gemini-3.7-flash" selected>Gemini 3.7 Flash</option>
<option value="anthropic/claude-opus-5">Claude Opus 5</option>
<option value="openai/gpt-5.6-sol">GPT-5.6 Sol</option>
<option value="z-ai/glm-5.2">GLM 5.2</option>
<option value="moonshotai/kimi-k3">Kimi K3</option>
<option value="openrouter/auto-beta">OpenRouter: Auto Router</option>
<option value="openrouter/free">OpenRouter: Free Models</option>
<option value="openrouter/deepseek/deepseek-v4-pro">DeepSeek v4 Pro</option>
</select>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Model lists require lockstep maintenance

The dialog duplicates the canonical option list and copies labels by position. Model additions or reordering can silently mislabel or omit dialog choices.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@sgardoll
sgardoll merged commit d3708da into milestone/stu-implement-updated-ui-ux Sep 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant