Skip to content

Move plugin configuration into a drawer - #694

Open
pratikbuilds wants to merge 4 commits into
corbitsdev:mainfrom
pratikbuilds:plugin-configuration-drawer
Open

Move plugin configuration into a drawer#694
pratikbuilds wants to merge 4 commits into
corbitsdev:mainfrom
pratikbuilds:plugin-configuration-drawer

Conversation

@pratikbuilds

Copy link
Copy Markdown
Contributor

Summary

  • Keep OAuth-only connections on their direct authorization flow.
  • Move API-key and token setup, plus MCP management, into a right-side drawer.
  • Keep catalog rows compact and make room for future management controls.

Validation

  • bun run check
  • Browser walkthrough of the GitHub MCP drawer

@pratikbuilds

Copy link
Copy Markdown
Contributor Author

Demo Video here

Screen.Recording.2026-09-08.at.9.06.54.PM.mov

@TheGreatAxios

Copy link
Copy Markdown
Contributor

That looks so good

@TheGreatAxios

TheGreatAxios commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Recommendation: scope or explicitly review the app-wide focus-style change; treat drawer ownership and dismissal as follow-ups, not demonstrated blockers. The reopen-probe finding is withdrawn.

Updated after a factual second pass. The token/disconnect flows move into the shared right-side drawer, and their tests move into the panel suite. The OAuth reopen behavior is correct on the inspected dependency path.

Should address

  1. The focus-style change is app-wide, not drawer-scoped. apps/web/src/app.css:42 removes the outline for every matching [data-slot="input"]. Scope this change to the intended surface or explicitly review its app-wide effect. The earlier numerical contrast downgrade was not verified against the rendered remaining indicator and should not have been presented as a measurement.

  2. Use accurate credential copy. The masked field at packages/plugins-ui/src/plugin-connect-panel.tsx:315 says API key while collecting an access token. Prefer the credential-specific label.

  3. Pin the moved interaction wiring with tests. Add integration assertions for close-after-success and close/reopen behavior. The page callbacks at apps/web/src/pages/plugins-page.tsx:379 close after both connect and disconnect; confirm that this is the intended management flow, rather than assuming it is a defect.

Maintainability and UX recommendations

  • The additional hosted-OAuth check at packages/plugins-ui/src/plugin-card.tsx:42 duplicates the decision in the connect panel. Consider sharing the predicate next to the auth-kind domain definition. This is duplication worth addressing, not evidence of a current misrouted auth kind.
  • The page owns a drawer at apps/web/src/pages/plugins-page.tsx:374, and the gallery owns another at packages/plugins-ui/src/plugins-gallery.tsx:419. Consolidating ownership may simplify coordination, but this review did not demonstrate a double-open or deep-link malfunction.
  • Preset token state is discarded on dismissal. Decide whether that is the intended UX and verify focus restoration for the moved flow. The connector drawer already lacked an explicit trigger/custom focus-restoration connection and dismissed unconditionally at base (packages/plugins-ui/src/plugin-connect-panel.tsx:234); it is not a wholly new regression. Exact focus landing was not browser-verified.

Corrections to the original review

  • OAuth probing does re-run on reopen. Closing makes plugin null at packages/plugins-ui/src/plugin-connect-panel.tsx:358; reopening changes it back, triggering [plugin, tenantId, oauthProbeKey] at packages/plugins-ui/src/plugin-connect-panel.tsx:389. Reusing the resolved object does not bypass that intervening null dependency. Finding 5 was incorrect.
  • Inline-only card error handling at packages/plugins-ui/src/mcp-preset-cards.tsx:91 predates this PR. It should not have been presented as a newly introduced telemetry regression.
  • The previous unconditional focus-drop and numerical contrast claims are withdrawn pending runtime verification.

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.

2 participants