Conversation
- MCPAuthorizePage: catch best-effort getClientInfo failures, narrow params via a single guard so `params!` assertions disappear. - MCPSettings: memoize approvedClients so list iteration keeps a stable reference between renders. - mcp-api: refresh the session before constructing the authorize-code request so the forwarded refresh_token and expires_at match the access token actually sent. - routes: limit the MCP marketing-path carve-out to /oauth/mcp/. - dev.env: point APPFLOWY_MCP_BASE_URL at the dedicated 8002 port. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements end-to-end MCP OAuth authorization and workspace-level MCP administration for AppFlowy Web, including a dedicated /oauth/mcp/authorize consent page, workspace MCP settings UI, HTTP/domain services, runtime configuration, and safe redirect handling for nested OAuth query parameters. Sequence diagram for MCP OAuth consent and authorization code issuancesequenceDiagram
actor User
participant BrowserApp as AppFlowyWeb
participant MCPAuthorizePage
participant McpService
participant Token as getTokenParsed
participant GoTrue as refreshToken
participant McpServer as MCP_API
User->>BrowserApp: Click "Allow access"
BrowserApp->>MCPAuthorizePage: onAllow()
MCPAuthorizePage->>McpService: createAuthorizationCode(req)
activate McpService
McpService->>Token: getTokenParsed()
Token-->>McpService: token
alt token expired
McpService->>GoTrue: refreshToken(refresh_token)
GoTrue-->>McpService: new session
McpService->>Token: getTokenParsed()
Token-->>McpService: refreshed token
end
McpService->>McpServer: POST /api/mcp/authorize/code
McpServer-->>McpService: { redirect_url }
deactivate McpService
McpService-->>MCPAuthorizePage: redirect_url
MCPAuthorizePage->>BrowserApp: window.location.href = redirect_url
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
deploy/routes.ts,MARKETING_PATHSadds'/oauth/mcp/'while the React route is'/oauth/mcp/authorize'; if the router comparison is exact (as with the other entries), this path won’t match and the MCP authorize page may not be served correctly—consider aligning the string to the exact route or updating the matching logic to be prefix-based.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `deploy/routes.ts`, `MARKETING_PATHS` adds `'/oauth/mcp/'` while the React route is `'/oauth/mcp/authorize'`; if the router comparison is exact (as with the other entries), this path won’t match and the MCP authorize page may not be served correctly—consider aligning the string to the exact route or updating the matching logic to be prefix-based.
## Individual Comments
### Comment 1
<location path="src/pages/MCPAuthorizePage.tsx" line_range="42" />
<code_context>
+ scope?: string;
+}
+
+function readParams(search: URLSearchParams): OAuthParams | { error: string } {
+ const required = [
+ 'client_id',
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the pure helper functions and the state/effect logic into dedicated utility and hook modules so the page component remains a lean, declarative UI layer.
You can keep all functionality but reduce cognitive load by separating orchestration/state from rendering and by moving pure helpers out of the component file.
### 1. Move pure helpers into a small module
`readParams`, `safeHttpUrl`, `isOwner` don’t depend on React and can live in a small utility file. This shortens the main page file and makes it easier to test them in isolation.
```ts
// mcpAuthorize.utils.ts
import { Role, Workspace } from '@/application/types';
export interface OAuthParams {
client_id: string;
redirect_uri: string;
response_type: string;
code_challenge: string;
code_challenge_method: string;
state?: string;
scope?: string;
}
export function readParams(search: URLSearchParams): OAuthParams | { error: string } {
// ... existing implementation ...
}
export function safeHttpUrl(value: string | null | undefined): string | undefined {
// ... existing implementation ...
}
export function isOwner(workspace: Workspace | undefined, userUid: string | undefined): boolean {
// ... existing implementation ...
}
```
Then in the page:
```ts
import { readParams, safeHttpUrl, isOwner, type OAuthParams } from './mcpAuthorize.utils';
```
### 2. Extract a `useMcpAuthorize` hook for the state machine
You can encapsulate the effects and callbacks (`onAllow`, `onApproveAndAllow`, `onDeny`, workspace fetching, client info fetching) into a hook so `MCPAuthorizePage` becomes mostly UI wiring.
Example skeleton (keep logic the same, just move it):
```ts
// useMcpAuthorize.ts
import { useEffect, useMemo, useState } from 'react';
import { useSearchParams, useNavigate } from 'react-router-dom';
import { McpService } from '@/application/services/domains';
import { getWorkspaces } from '@/application/services/js-services/http/workspace-api';
import { useCurrentUserOptional, useIsAuthenticatedOptional } from '@/components/main/app.hooks';
import type { Workspace, Role } from '@/application/types';
import type { OAuthParams } from './mcpAuthorize.utils';
import { readParams, safeHttpUrl, isOwner } from './mcpAuthorize.utils';
export function useMcpAuthorize() {
const [search] = useSearchParams();
const navigate = useNavigate();
const isAuthenticated = useIsAuthenticatedOptional();
const currentUser = useCurrentUserOptional();
const parsed = useMemo(() => readParams(search), [search]);
const params = 'error' in parsed ? null : parsed;
const paramError = 'error' in parsed ? parsed.error : null;
const [workspaces, setWorkspaces] = useState<Workspace[] | null>(null);
const [selectedWorkspace, setSelectedWorkspace] = useState('');
const [submitting, setSubmitting] = useState(false);
const [approvingClient, setApprovingClient] = useState(false);
const [submitError, setSubmitError] = useState<string | null>(null);
const [clientInfo, setClientInfo] = useState<McpClientInfo>({});
const [approvalNeeded, setApprovalNeeded] = useState(false);
// ... move existing useEffects and callbacks here ...
// expose only what the component needs:
const selectedWorkspaceInfo = workspaces?.find((w) => w.id === selectedWorkspace);
const canApproveSelectedWorkspace = isOwner(selectedWorkspaceInfo, currentUser?.uid);
const usePicker = (workspaces?.length ?? 0) > 1;
return {
params,
paramError,
isAuthenticated,
workspaces,
selectedWorkspace,
submitting,
approvingClient,
submitError,
clientInfo,
approvalNeeded,
canApproveSelectedWorkspace,
usePicker,
onAllow,
onApproveAndAllow,
onDeny,
onSelectWorkspace,
};
}
```
Then the page component becomes mostly declarative:
```tsx
// MCPAuthorizePage.tsx
import { useMcpAuthorize } from './useMcpAuthorize';
function MCPAuthorizePage() {
const {
params,
paramError,
isAuthenticated,
workspaces,
selectedWorkspace,
submitting,
approvingClient,
submitError,
clientInfo,
approvalNeeded,
canApproveSelectedWorkspace,
usePicker,
onAllow,
onApproveAndAllow,
onDeny,
onSelectWorkspace,
} = useMcpAuthorize();
if (!params) {
return <ErrorPanel title='Invalid request' message={paramError ?? 'Invalid authorization request.'} />;
}
if (!isAuthenticated) {
return <Centered>Redirecting to sign in…</Centered>;
}
if (workspaces === null) {
return <Centered>Loading…</Centered>;
}
if (workspaces.length === 0) {
return (
<ErrorPanel
title='No workspace'
message='Your account has no workspaces. Create one in AppFlowy first, then retry.'
/>
);
}
// ... existing JSX, now with much less embedded logic ...
}
```
This keeps all behavior and API calls intact, but:
- Localizes side-effects and state transitions inside `useMcpAuthorize`.
- Makes `MCPAuthorizePage` read as a simple flow: guard clauses + JSX.
- Allows you to test the hook’s logic separately from the UI.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| scope?: string; | ||
| } | ||
|
|
||
| function readParams(search: URLSearchParams): OAuthParams | { error: string } { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the pure helper functions and the state/effect logic into dedicated utility and hook modules so the page component remains a lean, declarative UI layer.
You can keep all functionality but reduce cognitive load by separating orchestration/state from rendering and by moving pure helpers out of the component file.
1. Move pure helpers into a small module
readParams, safeHttpUrl, isOwner don’t depend on React and can live in a small utility file. This shortens the main page file and makes it easier to test them in isolation.
// mcpAuthorize.utils.ts
import { Role, Workspace } from '@/application/types';
export interface OAuthParams {
client_id: string;
redirect_uri: string;
response_type: string;
code_challenge: string;
code_challenge_method: string;
state?: string;
scope?: string;
}
export function readParams(search: URLSearchParams): OAuthParams | { error: string } {
// ... existing implementation ...
}
export function safeHttpUrl(value: string | null | undefined): string | undefined {
// ... existing implementation ...
}
export function isOwner(workspace: Workspace | undefined, userUid: string | undefined): boolean {
// ... existing implementation ...
}Then in the page:
import { readParams, safeHttpUrl, isOwner, type OAuthParams } from './mcpAuthorize.utils';2. Extract a useMcpAuthorize hook for the state machine
You can encapsulate the effects and callbacks (onAllow, onApproveAndAllow, onDeny, workspace fetching, client info fetching) into a hook so MCPAuthorizePage becomes mostly UI wiring.
Example skeleton (keep logic the same, just move it):
// useMcpAuthorize.ts
import { useEffect, useMemo, useState } from 'react';
import { useSearchParams, useNavigate } from 'react-router-dom';
import { McpService } from '@/application/services/domains';
import { getWorkspaces } from '@/application/services/js-services/http/workspace-api';
import { useCurrentUserOptional, useIsAuthenticatedOptional } from '@/components/main/app.hooks';
import type { Workspace, Role } from '@/application/types';
import type { OAuthParams } from './mcpAuthorize.utils';
import { readParams, safeHttpUrl, isOwner } from './mcpAuthorize.utils';
export function useMcpAuthorize() {
const [search] = useSearchParams();
const navigate = useNavigate();
const isAuthenticated = useIsAuthenticatedOptional();
const currentUser = useCurrentUserOptional();
const parsed = useMemo(() => readParams(search), [search]);
const params = 'error' in parsed ? null : parsed;
const paramError = 'error' in parsed ? parsed.error : null;
const [workspaces, setWorkspaces] = useState<Workspace[] | null>(null);
const [selectedWorkspace, setSelectedWorkspace] = useState('');
const [submitting, setSubmitting] = useState(false);
const [approvingClient, setApprovingClient] = useState(false);
const [submitError, setSubmitError] = useState<string | null>(null);
const [clientInfo, setClientInfo] = useState<McpClientInfo>({});
const [approvalNeeded, setApprovalNeeded] = useState(false);
// ... move existing useEffects and callbacks here ...
// expose only what the component needs:
const selectedWorkspaceInfo = workspaces?.find((w) => w.id === selectedWorkspace);
const canApproveSelectedWorkspace = isOwner(selectedWorkspaceInfo, currentUser?.uid);
const usePicker = (workspaces?.length ?? 0) > 1;
return {
params,
paramError,
isAuthenticated,
workspaces,
selectedWorkspace,
submitting,
approvingClient,
submitError,
clientInfo,
approvalNeeded,
canApproveSelectedWorkspace,
usePicker,
onAllow,
onApproveAndAllow,
onDeny,
onSelectWorkspace,
};
}Then the page component becomes mostly declarative:
// MCPAuthorizePage.tsx
import { useMcpAuthorize } from './useMcpAuthorize';
function MCPAuthorizePage() {
const {
params,
paramError,
isAuthenticated,
workspaces,
selectedWorkspace,
submitting,
approvingClient,
submitError,
clientInfo,
approvalNeeded,
canApproveSelectedWorkspace,
usePicker,
onAllow,
onApproveAndAllow,
onDeny,
onSelectWorkspace,
} = useMcpAuthorize();
if (!params) {
return <ErrorPanel title='Invalid request' message={paramError ?? 'Invalid authorization request.'} />;
}
if (!isAuthenticated) {
return <Centered>Redirecting to sign in…</Centered>;
}
if (workspaces === null) {
return <Centered>Loading…</Centered>;
}
if (workspaces.length === 0) {
return (
<ErrorPanel
title='No workspace'
message='Your account has no workspaces. Create one in AppFlowy first, then retry.'
/>
);
}
// ... existing JSX, now with much less embedded logic ...
}This keeps all behavior and API calls intact, but:
- Localizes side-effects and state transitions inside
useMcpAuthorize. - Makes
MCPAuthorizePageread as a simple flow: guard clauses + JSX. - Allows you to test the hook’s logic separately from the UI.
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
1 participant
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.
Description
Adds MCP authorization and workspace MCP settings support to AppFlowy Web. This includes a new /mcp/authorize flow, MCP API/domain wiring, runtime MCP environment configuration, proactive token refresh handling, and sign-in test coverage for external login redirects.
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Add MCP OAuth authorization flow, workspace-level MCP administration UI, and runtime configuration wiring for MCP endpoints, along with safer redirect handling for OAuth login flows.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Deployment:
Tests: