feat(web): Add roadmap gate metrics and MCP-first fallback policy#451
feat(web): Add roadmap gate metrics and MCP-first fallback policy#451LucasSantana-Dev wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIntroduces a new feature flag Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Generate API
participant Router as Provider Router
participant MCP as MCP Gateway
participant Provider as Direct Provider
Client->>API: POST /api/generate
API->>API: Read ENABLE_MCP_DIRECT_PROVIDER_FALLBACK flag
API->>Router: routeGeneration(allowDirectProviderFallback: bool)
alt MCP Enabled
Router->>MCP: routeViaMcp()
MCP-->>Router: MCP fails (no output)
alt Fallback Enabled (flag=true)
Router->>Provider: fallback to direct provider
Provider-->>Router: result
Router-->>API: success
else Fallback Disabled (flag=false)
Router-->>API: error (MCP failure, no fallback)
end
else MCP Disabled
Router->>Provider: route directly
Provider-->>Router: result
Router-->>API: success
end
API-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.3)apps/web/src/__tests__/lib/features/flags.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m apps/web/src/app/api/generate/route.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m apps/web/src/app/api/metrics/route.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code review (mandatory pass): Findings
Assumptions/Open questions
Summary
|
|
Project Scorecard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/__tests__/lib/services/provider-router.test.ts (1)
100-120: Consider verifying error capture consistency.The test correctly validates the core fallback-disabled behavior: no fallback event, no fallback chunk, error returned, and
generateWithProvidernot called. Theeslint-disablecomment on line 104 appropriately addresses the static analysis warning about the intentionally non-yielding generator.However, the fallback-enabled test (line 94) asserts that
captureServerErroris called when MCP fails. If the implementation also logs errors to Sentry when fallback is disabled, consider adding that assertion here for consistency and to verify error observability is maintained regardless of fallback behavior.💡 Optional: Add captureServerError assertion
it('returns an error on MCP failure when direct-provider fallback is disabled', async () => { const { generateComponentStream: mcpStream } = require('@/lib/mcp/client'); const { generateWithProvider } = require('@/lib/services/generation'); + const { captureServerError } = require('@/lib/sentry/server'); // eslint-disable-next-line require-yield mcpStream.mockImplementation(async function* () { throw new Error('Gateway down'); }); generateWithProvider.mockImplementation(async function* () { yield { type: 'chunk', content: 'fallback-code', timestamp: 11 }; }); const events = await collectEvents( routeGeneration({ ...baseOpts, mcpEnabled: true, accessToken: 'tok-123' }) ); + expect(captureServerError).toHaveBeenCalled(); expect(events.find((e) => e.type === 'fallback')).toBeUndefined(); expect(events.some((e) => e.type === 'chunk' && e.content === 'fallback-code')).toBe(false); expect(events.some((e) => e.type === 'error')).toBe(true); expect(generateWithProvider).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/__tests__/lib/services/provider-router.test.ts` around lines 100 - 120, The test for "returns an error on MCP failure when direct-provider fallback is disabled" is missing an assertion that the error telemetry function captureServerError is invoked when the MCP stream throws; update the test to mock/spy on captureServerError (the same spy used in the fallback-enabled test) and assert it was called after calling routeGeneration with mcpEnabled: true and accessToken set, while keeping the existing mcpStream and generateWithProvider mocks and the other expectations unchanged so error observability is verified even when fallback is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/__tests__/lib/services/provider-router.test.ts`:
- Around line 100-120: The test for "returns an error on MCP failure when
direct-provider fallback is disabled" is missing an assertion that the error
telemetry function captureServerError is invoked when the MCP stream throws;
update the test to mock/spy on captureServerError (the same spy used in the
fallback-enabled test) and assert it was called after calling routeGeneration
with mcpEnabled: true and accessToken set, while keeping the existing mcpStream
and generateWithProvider mocks and the other expectations unchanged so error
observability is verified even when fallback is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81fff63e-dc60-4bcd-bed2-70d8dd3bcfb7
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.md
📒 Files selected for processing (9)
apps/web/src/__tests__/lib/features/flags.test.tsapps/web/src/__tests__/lib/services/provider-router.test.tsapps/web/src/app/api/generate/route.tsapps/web/src/app/api/metrics/__tests__/route.test.tsapps/web/src/app/api/metrics/route.tsapps/web/src/lib/features/flags.tsapps/web/src/lib/features/types.tsapps/web/src/lib/mcp/client.tsapps/web/src/lib/services/provider-router.ts
|
Superseded by #452 due branch ruleset GH013 preventing updates to this PR branch. |
1 participant
Summary
GET /api/metricsfor:adoption.gate50)routing.mcp)ENABLE_MCP_DIRECT_PROVIDER_FALLBACKfeature flagfalseWhy
Roadmap priorities require validation/adoption metrics and completion of WebApp->Gateway integration with MCP as the primary flow before broader Phase 2 expansion.
Testing
npm run lintnpm run type-checknpm run testnpm run buildnpm --workspace @siza/web test -- src/app/api/metrics/__tests__/route.test.ts src/__tests__/lib/services/provider-router.test.ts src/__tests__/lib/features/flags.test.tsSummary by CodeRabbit
New Features
Tests