Skip to content

improvement(mship): questions improvement - #6385

Merged
Sg312 merged 2 commits into
stagingfrom
credential-continue
Aug 7, 2026
Merged

improvement(mship): questions improvement#6385
Sg312 merged 2 commits into
stagingfrom
credential-continue

Conversation

@Sg312

@Sg312 Sg312 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Mship credential continue ui --> questions ui
Companion: https://github.com/simstudioai/mothership/pull/414

Type of Change

  • Improvement

Testing

Manual

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 7, 2026 7:43pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR improves Mothership question and credential interactions, including exact desktop OAuth-attempt correlation.

  • Carries chatAttemptId through the renderer-to-main bridge and binds it to the accepted loopback state.
  • Echoes the correlated attempt on completion so overlapping launches cannot update the wrong credential row.
  • Adds attempt-scoped credential verification, question UI updates, Trello return routing, and related tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; current desktop completions are correlated using the attempt ID stored with the accepted handoff state, preventing completion from being applied to a different overlapping launch.

Important Files Changed

Filename Overview
apps/desktop/src/main/handoff.ts Binds connect scope, including the chat attempt ID, to the one-shot handoff state and returns it only after successful state consumption.
apps/sim/lib/credentials/oauth-chat-attempt.ts Resolves current-shell desktop completions by the explicit attempt ID while retaining an omission-only fallback for legacy shells.
apps/sim/hooks/use-oauth-return.ts Uses the correlated desktop completion to update the matching attempt and refresh credential caches.
apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/use-oauth-chip-connection.ts Creates row-scoped OAuth attempts and passes their IDs into desktop handoffs.
packages/desktop-bridge/src/index.ts Extends the typed desktop bridge completion and launch contracts with nullable attempt correlation.

Sequence Diagram

sequenceDiagram
  participant Row as Credential row
  participant Renderer
  participant Main as Electron main
  participant Browser as System browser
  Row->>Renderer: Create chatAttemptId
  Renderer->>Main: beginOAuthConnect(scope + chatAttemptId)
  Main->>Main: Bind scope to generated state
  Main->>Browser: Open OAuth URL
  Browser-->>Main: Loopback callback(state)
  Main->>Main: Consume matching state and scope
  Main-->>Renderer: Completion(chatAttemptId)
  Renderer-->>Row: Update exact attempt
Loading

Reviews (2): Last reviewed commit: "fixes" | Re-trigger Greptile

Comment thread apps/sim/lib/credentials/oauth-chat-attempt.ts
@github-actions github-actions Bot added the requires-mothership-merge Has a companion PR on the mothership/copilot side — merge in lockstep label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

⚠️ Cross-repo companion check

One or more companion PRs aren't merged into staging yet. Merging this without them will leave copilot and sim out of sync — merge them in lockstep.

@Sg312

Sg312 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor

cursor Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches OAuth return paths, desktop handoff IPC, credential persistence, and transcript pairing; changes are heavily tested but span auth-adjacent flows and desktop/web parity.

Overview
Aligns credential terminal tags with the same interaction card UX as questions: shared InteractionCard chrome, multi-row batches (OAuth links + secret inputs in one card), Continue/Submit stepping for questions, and transcript pairing that hides the user’s follow-up message behind a status recap.

Credential cards now parse <credential> bodies as an array (singleton objects still work). Submit emits a safe Credential setup submitted — {…} payload (provider/secret names and connected/skipped status only—no secret values) so Mothership can continue; legacy “setup complete” messages still pair on reload.

OAuth from chat chips is correlated per row via oauthAttempt / chatAttemptId: baseline credential snapshots, return-URL verification with retries, desktop handoff echoing chatAttemptId to the renderer, and Trello authorize/callback returnUrl cookies (same-origin) so flows can land back on the chat page. Service-account modals call onCreated on reconnect as well as create.

Reconnect now bumps updatedAt when the user re-authorizes the same account so chip completion can detect token refresh. oauth_request_access remains registered only for old checkpoints.

Reviewed by Cursor Bugbot for commit 5ba1185. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 817ca0d. Configure here.

@Sg312

Sg312 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile

@Sg312

Sg312 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5ba1185. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Want your agent to iterate on Greptile's feedback? Try greploops.

@Sg312
Sg312 merged commit a84260f into staging Aug 7, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the credential-continue branch August 8, 2026 03:19
waleedlatif1 added a commit that referenced this pull request Aug 8, 2026
waleedlatif1 added a commit that referenced this pull request Aug 8, 2026
…ial UI

Reverts #6403 and #6385 to restore the credential UI users had on main —
individual inline chips, no combined card — and re-adds only the part that was
worth keeping: the connect no longer strands you on a second copy of the chat.

The chip opens the flow in a popup whose return leg is rewritten to a
self-closing completion page, so this tab is never navigated. A refused popup
falls through to the anchor pointed at the same URL, so that tab still closes
itself rather than loading the app again.

Nothing tracks per-chip attempt state, which is what the reverted work needed
to drive the card's connected/pending labels. Main's chip is stateless, so the
ledger, the status hook, the popup watcher and its deadlines all go with it —
along with the races they kept producing.

Keeps oauth/chat-complete out of the strict COOP rule: same-origin would disown
the popup from its opener the moment it loads, leaving it not reliably
script-closable, which is the one thing the page exists to do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-mothership-merge Has a companion PR on the mothership/copilot side — merge in lockstep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant