Skip to content

Show OpenRouter first in onboarding provider dialog - #4379

Open
keppo-bot[bot] wants to merge 1 commit into
mainfrom
codex/openrouter-first-onboarding
Open

Show OpenRouter first in onboarding provider dialog#4379
keppo-bot[bot] wants to merge 1 commit into
mainfrom
codex/openrouter-first-onboarding

Conversation

@keppo-bot

@keppo-bot keppo-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Make OpenRouter the first bring-your-own-key choice in the onboarding provider dialog, while keeping Google available immediately beside it.

  • Changes only the presentation order; provider setup behavior, labels, and free-tier chips remain unchanged.
  • Adds an onboarding E2E assertion for the OpenRouter-first ordering while retaining navigation coverage for Google, OpenRouter, and other providers.
  • Records the Dugite rebuild needed when packaging from a worktree repaired with npm install --ignore-scripts.

Review in cubic

@dyad-assistant dyad-assistant 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.

Claude review: 1 inline finding(s).

Comment thread e2e-tests/setup.spec.ts

testSetup("setup ai provider", async ({ po }) => {
const dialog = await openAiSetupDialog(po);
const providerButtons = dialog.locator(".grid").getByRole("button");

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.

🟑 MEDIUM

Ordering assertion anchored to Tailwind .grid utility class

The new ordering assertion resolves the provider buttons through the Tailwind utility class .grid. The Base UI dialog popup itself also carries a grid class (src/components/ui/dialog.tsx:53); this works today only because Locator.locator() matches descendants rather than the root. Because a chained locator over several .grid matches does not raise a strict-mode error, adding any grid-classed wrapper inside the dialog later would silently change which elements nth(0)/nth(1) resolve to, turning a real ordering regression into a passing (or confusingly failing) test rather than a clear failure. Styling classes are not a stable contract for structural assertions.

πŸ’‘ Suggestion: Add a stable hook to the provider row in SetupBanner.tsx (e.g. data-testid="provider-options") and locate via dialog.getByTestId("provider-options").getByRole("button"), so the assertion no longer depends on Tailwind class names.

@dyad-assistant

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: πŸ€” NOT SURE - Potential issues
Recommendation: ready

Small, well-scoped presentation change: the three "use your own API key" buttons in SetupBanner are reordered so OpenRouter renders first and Google second, with a new E2E assertion covering that order. No handlers, labels, chips, analytics events, or navigation targets changed β€” handleOpenRouterSetupClick / handleGoogleSetupClick and their PostHog captures are untouched, so the swap is purely visual. The reorder applies to both the inline and dialog variants of the banner, which is consistent since there is only one place in the app that renders these buttons (googleIcon is imported nowhere else).

Existing coverage still holds: e2e-tests/setup_flow.spec.ts locates these buttons by accessible name rather than position, so reordering does not break it. Nothing else in the repo (snapshots, docs fixtures) encodes the old ordering.

The one thing worth flagging is the selector the new assertion relies on. It targets the provider row via the Tailwind utility class .grid, and the Base UI dialog popup itself also carries a grid class (src/components/ui/dialog.tsx:53). It works today only because Locator.locator() matches descendants and not the locator root β€” but a chained locator over multiple .grid matches does not raise a strict-mode error, so a future layout wrapper inside the dialog would silently shift what nth(0)/nth(1) resolve to instead of failing loudly. Class-based selectors do have precedent in this suite (edit_custom_models.spec.ts), so this is a maintainability note, not a defect.

The AGENTS.md addition (npm rebuild dugite before npm run build after an --ignore-scripts install) is documentation-only and reads consistently with the surrounding worktree-repair guidance.

Diff was complete (not truncated), so confidence in the scope assessment is high.

Issues Summary

Severity File Issue
🟑 MEDIUM e2e-tests/setup.spec.ts:10 Ordering assertion anchored to Tailwind .grid utility class
🟒 Low Priority Notes (3 items)
  • Ordering coverage added to the thinner of two duplicate specs - e2e-tests/setup.spec.ts ("setup ai provider") largely duplicates e2e-tests/setup_flow.spec.ts ("AI provider setup flow"), which asserts the full button inventory including "Other providers" and the Dyad Pro entries. The new ordering assertion would arguably live better next to that richer inventory check, or the duplicate spec could be consolidated. (e2e-tests/setup.spec.ts)
  • Third button not covered by the ordering assertion - Only positions 0 and 1 are asserted, so a regression that moved "Other providers" ahead of a provider would only be caught indirectly. Adding a nth(2) check (or asserting the full accessible-name list in one go) would make the intent explicit. (e2e-tests/setup.spec.ts)
  • Unanchored name regexes - /OpenRouter/ and /Google/ are substring matches, so a future sibling like "Google Vertex" would still satisfy them. Not a problem today given the fixed three-button set. (e2e-tests/setup.spec.ts)

Generated by Dyadbot persona-based code review

@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Aug 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

βœ… All tests passed!

OS Passed Flaky Skipped
🍎 macOS 294 0 12

Total: 294 tests passed (12 skipped)

πŸ“Š View full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant