fix: make key generated on mcp json be shown, make placeholder show up if key not generated #10997
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughChanges enhance API key handling in MCP server components by shifting fallback logic from nullish coalescing to logical OR (treating empty strings as missing), adding a loading state to the Button component, updating test mocks, and expanding e2e test coverage with content verification and API key validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
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 |
github-actions
bot
added
bug
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.7.0 #10997 +/- ##
=================================================
+ Coverage 32.43% 33.01% +0.57%
=================================================
Files 1367 1368 +1
Lines 63315 63738 +423
Branches 9357 9388 +31
=================================================
+ Hits 20538 21042 +504
+ Misses 41744 41653 -91
- Partials 1033 1043 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts (1)
135-153: Remove/guard theconsole.log—it can leak newly generated API keys.
At minimum, don’t logres.api_key(or the entire response). Prefer dev-only logging + redaction.- console.log("res", res); + // Avoid logging sensitive credentials (API keys). + // If needed for local debugging, log only non-sensitive metadata. + // if (import.meta.env.DEV) console.debug("createApiKey success");
🧹 Nitpick comments (5)
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx (3)
89-115: Button mock:disabled={disabled || loading}+ “Loading...” swap is correct, but consider asserting it in the new flow test.
Right now the added “Generate API key” test only checks the handler call; if the component relies onisGeneratingApiKeyto disable the button / change label, this test won’t catch regressions in the loading UX.
234-251: JSON mode assertion updated to “Transport” text: OK, but adata-testidwould be less brittle.
Text-based assertions for labels tend to break on copy changes; if feasible, prefer a stable test id for the JSON mode container/selector.
302-328: Generate API key test: verify post-click UI contract (button disabled/label change and/or JSON placeholder removal).
Since the PR goal is “placeholder shows until key generated”, consider extending this unit test to assert either: (a) “API key generated” appears, (b) button is disabled while generating, and/or (c) the JSON code block no longer containsYOUR_API_KEYonceapiKeyis set in the mocked hook return.src/frontend/src/pages/MainPage/pages/homePage/utils/mcpServerUtils.tsx (1)
75-94:apiKey || "YOUR_API_KEY"matches the “show placeholder when key not generated” requirement; consider trimming/escaping for robustness.
IfapiKeycan be whitespace, you likely want(apiKey?.trim() ? apiKey.trim() : "YOUR_API_KEY"). Also consider escaping"/\before interpolating into the generated JSON string (defensive hardening).- return `"--headers","x-api-key","${apiKey || "YOUR_API_KEY"}"`; + const resolvedKey = apiKey?.trim() ? apiKey.trim() : "YOUR_API_KEY"; + return `"--headers","x-api-key","${resolvedKey}"`;src/frontend/tests/extended/features/mcp-server-tab.spec.ts (1)
219-233: Preferexpect.pollover manual retry loops for JSON updates (less flake).
This reduces timing sensitivity and makes failures easier to diagnose.- let jsonAfterGeneration = await preElement.textContent(); - let retries = 0; - while ( - jsonAfterGeneration?.includes("YOUR_API_KEY") && - retries < 10 - ) { - await page.waitForTimeout(500); - jsonAfterGeneration = await preElement.textContent(); - retries++; - } - - // Verify "YOUR_API_KEY" is no longer present - expect(jsonAfterGeneration).not.toContain("YOUR_API_KEY"); + const jsonAfterGeneration = await expect + .poll(async () => (await preElement.textContent()) ?? "", { + timeout: 5000, + }) + .toSatisfy((t) => !t.includes("YOUR_API_KEY"));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx(5 hunks)src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts(4 hunks)src/frontend/src/pages/MainPage/pages/homePage/utils/mcpServerUtils.tsx(1 hunks)src/frontend/tests/extended/features/mcp-server-tab.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
src/frontend/src/**/*.{ts,tsx}: Use React 18 with TypeScript for frontend development
Use Zustand for state management
Files:
src/frontend/src/pages/MainPage/pages/homePage/utils/mcpServerUtils.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsxsrc/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts
src/frontend/src/**/*.{tsx,jsx,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
Use Tailwind CSS for styling
Files:
src/frontend/src/pages/MainPage/pages/homePage/utils/mcpServerUtils.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
src/frontend/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
src/frontend/src/**/*.{tsx,jsx}: Implement dark mode support using the useDarkMode hook and dark store
Use Lucide React for icon components in the application
Files:
src/frontend/src/pages/MainPage/pages/homePage/utils/mcpServerUtils.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
src/frontend/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
Write component tests using React Testing Library with render(), screen, and fireEvent utilities
Files:
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
src/frontend/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
src/frontend/**/*.{test,spec}.{ts,tsx,js,jsx}: Use@pytest.mark.asynciodecorator for async frontend tests; structure tests to verify component behavior, state updates, and proper cleanup
Frontend tests should validate component rendering, user interactions, state management, and async operations using appropriate testing libraries (React Testing Library, Vitest, Jest, etc.)
Document each frontend test with a clear docstring/comment explaining its purpose, the scenario being tested, and expected outcomes
Files:
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsxsrc/frontend/tests/extended/features/mcp-server-tab.spec.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (Custom checks)
**/*.test.ts?(x): Review test files for excessive use of mocks that may indicate poor test design - check if tests have too many mock objects that obscure what's actually being tested
Warn when mocks are used instead of testing real behavior and interactions, and suggest using real objects or test doubles when mocks become excessive
Ensure mocks are used appropriately for external dependencies only, not for core logic
Frontend test files should follow the naming convention *.test.ts or *.test.tsx using Playwright
Test files should have descriptive test function names that explain what is being tested
Tests should be organized logically with proper setup and teardown
Consider including edge cases and error conditions for comprehensive test coverage
Verify tests cover both positive and negative scenarios where appropriate
Files:
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
src/frontend/src/**/{hooks,services}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
Use the useApi hook for API calls with proper error handling, including loading state, error state, and exception throwing
Files:
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts
src/frontend/src/**/{services,hooks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
Use async/await pattern for API calls instead of .then() chains
Files:
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts
🧠 Learnings (2)
📚 Learning: 2025-12-03T18:17:26.561Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-03T18:17:26.561Z
Learning: Applies to **/*.test.ts?(x) : Ensure mocks are used appropriately for external dependencies only, not for core logic
Applied to files:
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
📚 Learning: 2025-11-24T19:46:45.790Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/frontend_development.mdc:0-0
Timestamp: 2025-11-24T19:46:45.790Z
Learning: Applies to src/frontend/**/*.test.{ts,tsx} : Write component tests using React Testing Library with render(), screen, and fireEvent utilities
Applied to files:
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (52)
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 12/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 11/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 21/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 18/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 19/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 47/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 17/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 35/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 46/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 41/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 27/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 20/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 16/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 38/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 43/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 40/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 44/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 9/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 10/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 50/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 26/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 15/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 23/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 29/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 24/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 32/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 34/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 33/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 28/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 13/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 25/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 49/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 48/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 30/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 42/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 14/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 22/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 36/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 31/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 39/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 37/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 8/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 45/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 7/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 3/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 2/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 1/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 5/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 6/50
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 4/50
- GitHub Check: Test Docker Images / Test docker images
- GitHub Check: Test Starter Templates
🔇 Additional comments (4)
src/frontend/src/pages/MainPage/pages/homePage/components/__tests__/McpServerTab.test.tsx (2)
285-300: OAuth error test now ensures “Transport” is hidden: good coverage of the error UI gating.
This matches the intended “error takes over content” behavior.
147-154: The mock correctly intercepts thereact-syntax-highlighterimport as configured.
McpJsonContentimports{ Light as SyntaxHighlighter }from the root path"react-syntax-highlighter", and the mock at lines 147–154 provides exactly that named export with the correct prop signature (childrenandCodeTag). No false-positive risk from import shape mismatch. The mock is appropriate for this test.src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts (2)
247-256: Memo deps +apiKeyFromStore || s.apiKey: change is coherent and fixes stale memo risk.
Includings.apiKeyin dependencies is the right fix if the hook generates a key locally and expects derived values to update.
328-330: ReturnedapiKey: apiKeyFromStore || s.apiKeymatches the new fallback semantics.
Looks consistent with the header fragment logic.
| // Switch to JSON mode | ||
| await page.getByText("JSON", { exact: true }).last().click(); | ||
|
|
||
| await page.waitForSelector("pre", { state: "visible", timeout: 3000 }); | ||
|
|
||
| // Generate API key if not in auto login mode | ||
| const isAutoLogin = await page | ||
| .getByText("Generate API key") | ||
| .isVisible(); | ||
| if (isAutoLogin) { | ||
| await page.getByText("Generate API key").click(); | ||
| await expect(page.getByText("API key generated")).toBeVisible(); | ||
| // Test API key generation in JSON mode | ||
| const generateApiKeyButton = page.getByText("Generate API key"); | ||
| const isGenerateButtonVisible = await generateApiKeyButton | ||
| .isVisible() | ||
| .catch(() => false); | ||
|
|
||
| if (isGenerateButtonVisible) { | ||
| // Get the JSON configuration before generating | ||
| const preElement = page.locator("pre").first(); | ||
| const jsonBeforeGeneration = await preElement.textContent(); | ||
|
|
||
| // Verify "YOUR_API_KEY" is present in the JSON before generation | ||
| expect(jsonBeforeGeneration).toContain("YOUR_API_KEY"); | ||
|
|
||
| // Verify the button is visible and clickable | ||
| await expect(generateApiKeyButton).toBeVisible(); | ||
| await expect(generateApiKeyButton).toBeEnabled(); | ||
|
|
||
| // Click the Generate API key button | ||
| await generateApiKeyButton.click(); | ||
|
|
||
| // Wait for the API key to be generated and verify the state change | ||
| // The button text should change from "Generate API key" to "API key generated" | ||
| await expect(page.getByText("API key generated")).toBeVisible({ | ||
| timeout: 5000, | ||
| }); | ||
|
|
||
| // Wait for the JSON to update - it should no longer contain "YOUR_API_KEY" | ||
| // Retry until the JSON no longer contains "YOUR_API_KEY" | ||
| let jsonAfterGeneration = await preElement.textContent(); | ||
| let retries = 0; | ||
| while ( | ||
| jsonAfterGeneration?.includes("YOUR_API_KEY") && | ||
| retries < 10 | ||
| ) { | ||
| await page.waitForTimeout(500); | ||
| jsonAfterGeneration = await preElement.textContent(); | ||
| retries++; | ||
| } | ||
|
|
||
| // Verify "YOUR_API_KEY" is no longer present | ||
| expect(jsonAfterGeneration).not.toContain("YOUR_API_KEY"); | ||
|
|
||
| // Verify that an actual API key (not "YOUR_API_KEY") is present | ||
| // The JSON format has: "--headers", "x-api-key", "<api-key-value>" | ||
| // Match for "x-api-key" followed by comma and whitespace/newlines, then the API key | ||
| const apiKeyMatch = jsonAfterGeneration?.match( | ||
| /"x-api-key"[\s,]*"([^"]+)"/, | ||
| ); | ||
| expect(apiKeyMatch).not.toBeNull(); | ||
| if (apiKeyMatch) { | ||
| const generatedApiKey = apiKeyMatch[1]; | ||
| expect(generatedApiKey).not.toBe("YOUR_API_KEY"); | ||
| expect(generatedApiKey.length).toBeGreaterThan(0); | ||
| // API keys should be non-empty strings | ||
| expect(generatedApiKey.trim().length).toBeGreaterThan(0); | ||
| } | ||
|
|
||
| // Verify the Generate API key button text is no longer visible | ||
| // (it should be replaced by "API key generated") | ||
| await expect(generateApiKeyButton).not.toBeVisible(); | ||
| } else { | ||
| // If button is not visible, verify we're in a valid state: | ||
| // Either "API key generated" is shown (already generated) or | ||
| // we're in auto-login mode (no API key needed) | ||
| const apiKeyGeneratedText = page.getByText("API key generated"); | ||
| const hasApiKeyGenerated = await apiKeyGeneratedText | ||
| .isVisible() | ||
| .catch(() => false); | ||
|
|
||
| // In auto-login mode, neither button should be visible, which is expected | ||
| // In API key mode with key already generated, "API key generated" should be visible | ||
| expect( | ||
| hasApiKeyGenerated || | ||
| !(await page.getByText("Generate API key").isVisible()), | ||
| ).toBeTruthy(); | ||
| } |
There was a problem hiding this comment.
Else-branch assertion is currently non-validating; make it assert a real “already generated” or “auto-login” condition.
Because you’re already in the !isGenerateButtonVisible branch, re-checking that the button isn’t visible will almost always pass and won’t catch regressions.
} else {
// If button is not visible, verify we're in a valid state:
// Either "API key generated" is shown (already generated) or
// we're in auto-login mode (no API key needed)
const apiKeyGeneratedText = page.getByText("API key generated");
const hasApiKeyGenerated = await apiKeyGeneratedText
.isVisible()
.catch(() => false);
- // In auto-login mode, neither button should be visible, which is expected
- // In API key mode with key already generated, "API key generated" should be visible
- expect(
- hasApiKeyGenerated ||
- !(await page.getByText("Generate API key").isVisible()),
- ).toBeTruthy();
+ // Validate JSON content reflects a valid state too.
+ const preElement = page.locator("pre").first();
+ const json = (await preElement.textContent()) ?? "";
+
+ // If we didn't generate here, we should not be stuck showing the placeholder.
+ expect(json).not.toContain("YOUR_API_KEY");
+
+ // And either the UI explicitly says it's generated, or we're in a mode that doesn't require an API key header.
+ // (If auto-login removes headers entirely, ensure the header isn't present.)
+ if (!hasApiKeyGenerated) {
+ expect(json).not.toContain('"x-api-key"');
+ }
}
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
bug
lucaseduoli
force-pushed
the
fix/generate_key
branch
from
7b28038 to
b78b7fb
Compare
…ver.ts Co-authored-by: Cristhian Zanforlin Lousa <[email protected]>
github-actions
bot
added
bug
There was a problem hiding this comment.
LGTM!
Remeber to still create a main PR with these changes or else they won't be seen on main until after the offical release day (~18th). I general the flow should be to get the main PR merge first then cherrypick. But because it is so close I am okay with this pattern for today.
This pull request makes several small improvements to how API keys are handled and passed throughout the MCP server logic. The main changes involve consistently using the logical OR (
||) operator instead of the nullish coalescing operator (??) when selecting API keys, updating dependencies in React hooks, and adding a debug log statement.API key handling consistency and logic updates:
??with||for API key selection inuseMcpServer.tsandmcpServerUtils.tsxto ensure fallback values are correctly applied whenapiKeyFromStoreis falsy, not just nullish. [1] [2] [3]React hook dependency and import improvements:
useMemoinuseMcpServer.tsto includes.apiKey, ensuring the memoized value updates when the API key changes.useMcpServer.tsfor clarity and correctness.Debugging and developer experience:
console.logstatement to log the result when generating a new API key, aiding in debugging.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.