feat: Sprint 8 — E2E Tests, Documentation & Launch Prep#52
Merged
Conversation
…I, and security hardening - Add comprehensive Playwright E2E test suite (15 spec files, 55+ tests) covering auth, specs, servers, console, MCP SSE, billing, export, and landing page flows with proper fixtures and helpers - Finalize all documentation for launch: README rewrite, comprehensive API reference (docs/API.md), 7 new ADRs in ARCHITECTURE.md, expanded SECURITY.md with disclosure timeline, v0.1.0 changelog - Add performance CI: Lighthouse assertions upgraded to error level, bundle analysis workflow, size-limit for transformer package, slow query monitoring in runtime - Harden security CI: pin Trivy and Snyk action versions, add Dockerfile scanning, remove continue-on-error on audit jobs - Fix CORS default in code generator to require explicit origin (was '*') - Add fetch timeout (30s) to generated standalone servers - Review and fix all 13 Fumadocs MDX pages for accuracy
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Playwright E2E infrastructure and many tests, introduces a DB query monitoring wrapper with logging, integrates Next.js bundle analysis and Lighthouse CI assertion changes, tightens CI/security workflows and transformer size enforcement, expands docs/ADRs/security, and updates web runtime/export behavior and CORS/runtime timeouts. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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)
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 |
Contributor
Coverage Report for packages/transformer
File CoverageNo changed files found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
109-109:⚠️ Potential issue | 🟡 MinorUpdate Next.js version reference in README.
Line 109 incorrectly states "Next.js 14 dashboard" when the project uses Next.js 15 (
^15.5.14). Change to "Next.js 15 dashboard".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 109, Update the README entry for the Web App to reference the correct Next.js version: change the phrase "Next.js 14 dashboard" to "Next.js 15 dashboard" in the table row containing the Web App description (the line with "| **Web App** | [`apps/web`](apps/web) | Next.js 14 dashboard. ... |").
🟡 Minor comments (9)
apps/web/lib/export/generator.ts-59-59 (1)
59-59:⚠️ Potential issue | 🟡 MinorDocumentation mismatch:
CORS_ORIGINvsCORS_ORIGINSThe generated standalone server requires
CORS_ORIGIN(singular), but the relevant code snippet from.env.example:94-95shows the project usesCORS_ORIGINS(plural). This inconsistency will cause confusion for users deploying generated servers—they'll lack clear guidance on the required variable and hit startup crashes.Consider either:
- Aligning the generated server to read
CORS_ORIGINS(and handle comma-separated values), or- Adding
CORS_ORIGINto.env.examplewith documentation distinguishing it fromCORS_ORIGINSAlso applies to: 250-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/lib/export/generator.ts` at line 59, Update the generated standalone server to resolve the env var mismatch: either change the generator in apps/web/lib/export/generator.ts so the server reads CORS_ORIGINS (plural) and parses comma-separated values into the allowed origins list, or document and support both by checking process.env.CORS_ORIGINS first then falling back to process.env.CORS_ORIGIN; reference the CORS_ORIGINS/CORS_ORIGIN env names in the generator logic and update the generated .env example accordingly so the produced server and .env.example remain consistent.apps/web/content/docs/self-hosting.mdx-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorRepository URL points to a personal account.
The clone URL references
Work90210/APIFold(a personal account). For launch-ready documentation, consider using an organization-owned repository URL (e.g.,github.com/apifold/APIFold) to ensure long-term stability and discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/content/docs/self-hosting.mdx` around lines 23 - 24, The documentation uses a personal GitHub repo URL in the git clone command ("git clone https://github.com/Work90210/APIFold.git" and "cd APIFold"); update this to the organization-owned repository URL (for example "git clone https://github.com/apifold/APIFold.git" and adjust the subsequent "cd" if necessary) and ensure any other references in the same file to Work90210/APIFold are replaced with the org-owned repo name for consistency.e2e/tests/auth/signup.spec.ts-9-21 (1)
9-21:⚠️ Potential issue | 🟡 MinorTest intent and assertions are slightly mismatched.
The test name says it verifies both email and password fields, but it only asserts email + heading. Either add a password assertion or rename the test for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/auth/signup.spec.ts` around lines 9 - 21, The test "renders the sign-up form with email and password fields" currently only asserts the email input (emailInput) and a heading; either update the assertions to match the title by locating the password field (e.g., use a locator like 'input[name="password"], input[type="password"]' and add await expect(passwordInput).toBeVisible(...)) or change the test title string in the test(...) call to reflect that it only checks the email input and heading; adjust the test body accordingly (modify or add the locator referenced by passwordInput) so the name and assertions are consistent.e2e/tests/billing/upgrade.spec.ts-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorTest may fail for users without Stripe customer ID.
Per
apps/web/app/api/billing/portal/route.ts(lines 17-19), this endpoint returns 404 if the user has nostripeCustomerId. A fresh test user likely won't have one. Consider including 404 in expected statuses or documenting the precondition.🛠️ Proposed fix
- // Should either redirect (303) or return a URL - expect([200, 303, 302]).toContain(response.status()); + // Should return a URL (200), redirect (302/303), or 404 if no billing account exists + expect([200, 302, 303, 404]).toContain(response.status());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/billing/upgrade.spec.ts` around lines 106 - 112, The test "billing portal link is accessible" assumes the user has a Stripe customer ID but route /api/billing/portal (route.ts) returns 404 when stripeCustomerId is missing; update the assertion in the test (test("billing portal link is accessible", ...) in e2e/tests/billing/upgrade.spec.ts) to include 404 in the allowed response.status() values or alternatively add a setup step that ensures the test user has a stripeCustomerId before calling page.request.post("/api/billing/portal"); adjust the expect([...]).toContain(...) accordingly so the test passes for fresh users without a Stripe customer ID.e2e/tests/landing/page-load.spec.ts-19-20 (1)
19-20:⚠️ Potential issue | 🟡 MinorNon-null assertion may cause false failures.
textContent()can returnnulleven for visible elements (e.g., if the element contains only child elements). The non-null assertionheadingText!will throw if this happens.🛡️ Proposed fix
const headingText = await heading.textContent(); - expect(headingText!.length).toBeGreaterThan(0); + expect(headingText?.length ?? 0).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/landing/page-load.spec.ts` around lines 19 - 20, The test uses a non-null assertion on headingText from heading.textContent(), which can be null and cause false failures; update the assertion to handle null safely by first asserting headingText is not null (e.g., expect(headingText).not.toBeNull()) and only then assert the length, or use a null-coalescing check like expect(headingText?.length ?? 0).toBeGreaterThan(0); reference the variables/methods heading, headingText, and textContent() when making the change.e2e/tests/servers/configure-server.spec.ts-62-65 (1)
62-65:⚠️ Potential issue | 🟡 MinorAssertion doesn't verify the update persisted correctly.
The assertion only checks that
authModeproperty exists, butMcpServeralways has this field (perpackages/types/src/server.ts). To verify the update persisted, assert the actual value.🛠️ Proposed fix
// Verify via API that the change persisted const server = await getServer(page, serverId); - expect(server).toHaveProperty("authMode"); + expect(server.authMode).toBe("api_key");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/servers/configure-server.spec.ts` around lines 62 - 65, The test currently only checks that the returned server object from getServer(page, serverId) has an authMode property; instead assert that server.authMode equals the specific value you updated it to earlier in the test (e.g., the variable or literal used in the update request). Locate the getServer call and replace the loose expect(server).toHaveProperty("authMode") with a strict equality assertion against the expectedAuthMode (or the update payload variable) so the test verifies the persisted value.e2e/tests/mcp/sse-connection.spec.ts-100-102 (1)
100-102:⚠️ Potential issue | 🟡 MinorAssertion always passes — should require at least one event.
The test comment at line 100 states "Should have received at least one event", but the assertion
expect(events.length).toBeGreaterThanOrEqual(0)always passes since array length is never negative. This defeats the purpose of the test.🐛 Proposed fix
- // Should have received at least one event (e.g., session init) - expect(events.length).toBeGreaterThanOrEqual(0); + // Should have received at least one event (e.g., session init) + expect(events.length).toBeGreaterThanOrEqual(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/mcp/sse-connection.spec.ts` around lines 100 - 102, The assertion in the test currently checks events.length >= 0 which always passes; update the assertion in the sse-connection.spec.ts test so it requires at least one event (e.g., change the expectation on events.length to beGreaterThanOrEqual(1) or toBeGreaterThan(0)) and optionally add a clear failure message; locate the check referencing the events array and the expect(...) call to modify it accordingly.e2e/tests/specs/delete-spec.spec.ts-49-55 (1)
49-55:⚠️ Potential issue | 🟡 MinorAdd visibility wait before clicking delete button for consistency.
The first test (line 27) waits for the delete button to be visible before clicking, but this test clicks immediately without the visibility check. This could cause flaky failures if the page hasn't fully rendered.
🐛 Proposed fix
const deleteButton = page.getByRole("button", { name: /delete/i }); + await expect(deleteButton).toBeVisible({ timeout: 10_000 }); await deleteButton.click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/specs/delete-spec.spec.ts` around lines 49 - 55, The test clicks deleteButton immediately but should wait for it to be visible to match the first test and avoid flakiness; update the block that uses page.getByRole("button", { name: /delete/i }) (deleteButton) to wait for visibility before clicking (use the locator's waitFor/visibility check) and then proceed to click and interact with confirmButton (page.locator(...)) as before.e2e/helpers/api.ts-57-63 (1)
57-63:⚠️ Potential issue | 🟡 MinorHandle non-JSON responses before calling
response.json().
response.json()runs before any status check here, so an HTML/empty-body failure will surface asSyntaxErrorand hide the real HTTP status/body. That makes redirects, proxy failures, and no-content DELETEs much harder to diagnose.Suggested hardening
const response = await fetch(`${BASE_URL}${endpoint}`, options); - const json = (await response.json()) as ApiResponse<T>; - - if (!response.ok || !json.success) { + const contentType = response.headers.get("content-type") ?? ""; + const text = await response.text(); + const json = + contentType.includes("application/json") && text + ? (JSON.parse(text) as ApiResponse<T>) + : undefined; + + if (!response.ok) { throw new Error( - `API ${method} ${endpoint} failed: ${json.error ?? response.statusText}`, + `API ${method} ${endpoint} failed: ${json?.error ?? (text || response.statusText)}`, ); } + + if (!json?.success) { + throw new Error( + `API ${method} ${endpoint} failed: ${json?.error ?? "Unexpected response shape"}`, + ); + } return json.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/api.ts` around lines 57 - 63, The code calls response.json() unconditionally which can throw SyntaxError for non-JSON responses and hide the real HTTP status/body; modify the fetch handling around the fetch(`${BASE_URL}${endpoint}`, options) call so you first check response.ok and the Content-Type (via response.headers.get('content-type')) and, for non-JSON or empty bodies, call response.text() (or handle 204/empty) instead of response.json(), then construct the thrown Error using method, endpoint, response.status, response.statusText and the raw body text if available; adjust references to response, json, ApiResponse<T>, method and endpoint when reading/parsing so errors include the actual HTTP status and body.
🧹 Nitpick comments (17)
apps/runtime/__tests__/unit/query-monitor.test.ts (2)
51-68: Test timing is flaky-prone.The test relies on a 50ms
setTimeoutexceeding a 10ms threshold. While there's margin, real-time delays in CI can be unpredictable. Consider mockingperformance.now()to make the test deterministic.♻️ Suggested deterministic approach
it('warns for slow queries', async () => { + let callCount = 0; + vi.spyOn(performance, 'now').mockImplementation(() => { + // First call returns 0, second call returns 150 (simulating 150ms elapsed) + return callCount++ === 0 ? 0 : 150; + }); + const db = createMockDb(async () => { - // Simulate a slow query - await new Promise((resolve) => setTimeout(resolve, 50)); return { rows: [] }; }); const monitored = createMonitoredDb({ logger, db, slowThresholdMs: 10 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/runtime/__tests__/unit/query-monitor.test.ts` around lines 51 - 68, The test is flaky because it depends on real time delays; make it deterministic by mocking performance.now() around the call so measured duration exceeds slowThresholdMs without setTimeout. In the test for createMonitoredDb / monitored.query, replace the 50ms setTimeout approach with a jest.spyOn or similar on global.performance.now() that returns a start value (e.g., 1000) before calling monitored.query and a later value (e.g., 1011) after to simulate elapsed time > slowThresholdMs; ensure you restore the spy after the test and keep assertions on logger.warn the same.
96-108: Same flakiness concern applies here.This test also uses real
setTimeoutfor slow query simulation. Same suggestion to mockperformance.now()applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/runtime/__tests__/unit/query-monitor.test.ts` around lines 96 - 108, The test uses a real setTimeout inside createMockDb which causes flakiness; instead simulate the slow query by mocking time—replace the sleep-based callback with a deterministic mock of performance.now so that createMockDb's query handler returns immediately but performance.now reports a start and end that exceed slowThresholdMs; update the test to stub performance.now (used by createMonitoredDb/monitored.query) to return two values (start and end) that produce the desired elapsed time and remove reliance on setTimeout.lighthouserc.json (1)
1-27: Duplicate Lighthouse configuration may cause confusion.This
lighthouserc.jsonduplicates.github/lighthouse/config.json(with the only addition beingupload.target). The GitHub Actions workflow explicitly usesconfigPath: .github/lighthouse/config.jsonand already setstemporaryPublicStorage: true, making this root config redundant.Consider either:
- Removing this file if it's not used by local
lhciruns- Consolidating to a single config and updating the workflow reference
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lighthouserc.json` around lines 1 - 27, The lighthouserc.json duplicates the GitHub Actions Lighthouse config and adds only "upload.target"; either remove this root lighthouserc.json to avoid confusion or consolidate into one config and update the workflow to reference it — e.g., remove the duplicate lighthouserc.json (which contains the "ci.collect", "ci.assert", and "ci.upload.target" entries) if local lhci runs are unnecessary, or merge its "upload.target": "temporary-public-storage" into the single canonical config used by the workflow and ensure the workflow's configPath (currently set to .github/lighthouse/config.json) and temporaryPublicStorage flag reflect the consolidated file.docs/CONTRIBUTING.md (1)
17-18: Consider using an organization-owned repository URL.The clone URL points to a personal GitHub account (
Work90210/APIFold). If this project may transfer ownership or use an organization in the future, consider whether this URL will remain stable. Otherwise, the capitalization change toAPIFoldis consistent with other documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONTRIBUTING.md` around lines 17 - 18, Update the repository URL in the CONTRIBUTING.md clone instruction so it points to the organization-owned repository (or a stable canonical URL) instead of the personal account; specifically replace the "git clone https://github.com/Work90210/APIFold.git" string with the org's repo URL (or a generic placeholder like "git clone https://github.com/ORG_NAME/APIFold.git") to ensure future ownership changes won't break the docs while preserving the existing "cd APIFold" line.docs/ARCHITECTURE.md (1)
150-163: ADR-010 duplicates ADR-002.ADR-002 ("Dual Licensing (AGPL-3 + MIT)") already documents this exact decision. Consider either removing ADR-010 or consolidating it into ADR-002 to avoid redundancy and potential drift between the two records.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ARCHITECTURE.md` around lines 150 - 163, ADR-010 duplicates the decision already recorded in ADR-002; remove ADR-010 or merge its content into ADR-002 to avoid redundancy. Update ADR-002 to include any unique wording from ADR-010 (including the explicit "packages/transformer is MIT" boundary and consequences) and delete ADR-010; also search for and update any cross-references or index entries that point to ADR-010 so they now reference ADR-002.e2e/tests/servers/manage-tools.spec.ts (2)
69-84: Fragile state detection and hardcoded wait.Two concerns:
Silent error swallowing: The
.catch()pattern masks all errors, not just "element is a switch." IfisChecked()fails for other reasons, the test silently falls back to incorrect logic.Hardcoded timeout:
waitForTimeout(1_000)is flaky. Prefer waiting for a specific condition (e.g.,expect(toggle).toBeChecked()ortoHaveAttribute('aria-checked', ...)).♻️ Suggested improvement
- // Get initial state - const wasChecked = await toggle.isChecked().catch(() => { - // role="switch" uses aria-checked - return toggle.getAttribute("aria-checked").then((v) => v === "true"); - }); - - // Toggle it - await toggle.click(); - - // Wait for the state to update - await page.waitForTimeout(1_000); - - // Verify state changed - const isChecked = await toggle.isChecked().catch(() => { - return toggle.getAttribute("aria-checked").then((v) => v === "true"); - }); - expect(isChecked).not.toBe(wasChecked); + // Determine element type and get initial state + const role = await toggle.getAttribute("role"); + const isSwitch = role === "switch"; + + const getCheckedState = async () => + isSwitch + ? (await toggle.getAttribute("aria-checked")) === "true" + : await toggle.isChecked(); + + const wasChecked = await getCheckedState(); + + await toggle.click(); + + // Wait for state change using assertion retry + await expect(async () => { + const nowChecked = await getCheckedState(); + expect(nowChecked).not.toBe(wasChecked); + }).toPass({ timeout: 5_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/servers/manage-tools.spec.ts` around lines 69 - 84, The test currently swallows all errors when calling toggle.isChecked() and uses a hardcoded waitForTimeout(1_000), making it flaky; change the logic in the block that computes wasChecked/isChecked to first try toggle.isChecked() but only fallback if the element truly lacks isChecked support (e.g., detect presence of aria-checked via await toggle.getAttribute('aria-checked') !== null) rather than catching all errors, then replace page.waitForTimeout(1_000) with an explicit wait for the toggle state to change such as await expect(toggle).toHaveAttribute('aria-checked', expectedValue) or use page.waitForFunction to poll the attribute/value until it differs from the previous value; update references to toggle, wasChecked, isChecked, toggle.isChecked, toggle.getAttribute and page.waitForTimeout accordingly.
21-27: Best-effort cleanup with empty catch may hide test setup issues.Silently catching all errors in
afterEachcan mask failures in test infrastructure (e.g., authentication issues, API changes). Consider logging the error for debugging:♻️ Suggested improvement
test.afterEach(async ({ page }) => { try { await deleteSpec(page, specId); - } catch { - // Best-effort cleanup + } catch (error) { + console.warn(`Cleanup failed for spec ${specId}:`, error); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/servers/manage-tools.spec.ts` around lines 21 - 27, The afterEach currently swallows all errors which can hide infra failures; update the test.afterEach cleanup to catch the error from deleteSpec(page, specId) as a named variable (e) and log it (e.g., console.error or test.info().log) with context including specId and page URL/session info, rather than leaving the catch empty—keep the best-effort intent but surface failures for debugging by logging the exception from deleteSpec in the test.afterEach block.e2e/global-setup.ts (1)
39-42: Warning message implies skipping, but setup doesn’t skip tests.Line 41 says auth tests “will be skipped,” but this file only logs a warning. Either wire an actual skip mechanism in auth specs or reword to “may fail”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/global-setup.ts` around lines 39 - 42, The warning in e2e/global-setup.ts currently logs that auth tests "will be skipped" but does not implement any skipping; update the behavior to match reality by either (A) changing the console.warn message in the block that references the missing array (the console.warn call using `${missing.join(", ")}`) to say the auth tests "may fail" or "will likely fail", or (B) implement an actual skip mechanism by exporting a flag (e.g., export const SKIP_AUTH_TESTS = true/false) from this setup and make auth specs read that flag to conditionally call test.skip or set Jest/Mocha to skip those suites; choose one approach and apply it consistently (modify the console.warn text if you pick rewording, or add the exported flag and update auth spec files to reference SKIP_AUTH_TESTS if you add skipping).e2e/tests/billing/upgrade.spec.ts (2)
66-78: Same issue: movetest.skip()to the beginning.As noted above, the skip check should precede navigation and UI interactions to avoid wasted effort.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/billing/upgrade.spec.ts` around lines 66 - 78, Move the test.skip invocation to the top of the "can fill in Stripe test card details" test so the skip check runs before any navigation or UI interactions; specifically, add the test.skip(!process.env["STRIPE_SECRET_KEY"], "STRIPE_SECRET_KEY not configured") at the start of the test body for the test named "can fill in Stripe test card details" (the async ({ page }) => { ... } block) and then proceed with page.goto and the upgradeButton clicks only if the test is not skipped.
39-53: Movetest.skip()to the beginning of the test.The
test.skip()check runs after navigating and clicking the upgrade button (lines 42-47). This wastes resources and could cause confusing failures if the button click triggers errors before the skip is evaluated.♻️ Proposed fix
test("Stripe checkout page renders with test mode badge", async ({ page, }) => { + test.skip( + !process.env["STRIPE_SECRET_KEY"], + "STRIPE_SECRET_KEY not configured", + ); + await page.goto("/dashboard/settings"); const upgradeButton = page.getByRole("button", { name: /upgrade|go pro|subscribe/i, }).or(page.getByRole("link", { name: /upgrade|go pro|subscribe/i })); await upgradeButton.first().click(); - // Wait to land on Stripe checkout - test.skip( - !process.env["STRIPE_SECRET_KEY"], - "STRIPE_SECRET_KEY not configured", - ); - await page.waitForURL(/checkout\.stripe\.com/, { timeout: 30_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/billing/upgrade.spec.ts` around lines 39 - 53, The test's conditional skip (test.skip(!process.env["STRIPE_SECRET_KEY"], ...)) should be moved to the very start of the test block so the test is skipped before any page navigation or interaction occurs; update the test in the "test(...)" function so the test.skip call runs before page.goto("/dashboard/settings") and before using upgradeButton (page.getByRole(...).or(...).first().click()) to avoid performing unnecessary actions when STRIPE_SECRET_KEY is not set.docs/API.md (1)
9-11: Add language specifiers to HTTP endpoint code blocks.Static analysis flagged 25 code blocks without language specifiers. For HTTP endpoint definitions, use
httportextto satisfy linters and improve syntax highlighting.♻️ Example fix for one block
-``` -Authorization: Bearer <session-token> -``` +```http +Authorization: Bearer <session-token> +```Apply similar changes to endpoint definition blocks throughout the document (lines 55, 82, 100, 110, 125, 139, 149, 167, 177, 193, 207, 217, 232, 266, 285, 299, 332, 359, 387, 412, 435, 467, 491, 505).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/API.md` around lines 9 - 11, Several HTTP endpoint code blocks (for example the block containing "Authorization: Bearer <session-token>") lack language specifiers; update each triple-backtick fence that contains HTTP endpoint definitions or header lines to use a language tag (prefer `http`, or `text` where appropriate). Locate each code block that shows request/response headers or endpoint examples (e.g., the block with "Authorization: Bearer <session-token>") and change the opening fence from ``` to ```http and keep the closing fence as ```, applying the same transformation to all similar endpoint definition blocks throughout the document.e2e/tests/specs/import-from-file.spec.ts (1)
4-5: Combine imports from the same module.Both imports are from
../../helpers/api. They can be consolidated.♻️ Proposed fix
-import { cleanupTestSpecs } from "../../helpers/api"; -import { getPetstoreFixturePath } from "../../helpers/api"; +import { cleanupTestSpecs, getPetstoreFixturePath } from "../../helpers/api";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/specs/import-from-file.spec.ts` around lines 4 - 5, Combine the two separate imports from the same module into a single import statement: replace the separate imports of cleanupTestSpecs and getPetstoreFixturePath (from "../../helpers/api") with one consolidated import that imports both symbols together; update the import line that currently references cleanupTestSpecs and getPetstoreFixturePath so both named exports are imported in a single import declaration.e2e/tests/servers/configure-server.spec.ts (1)
67-99: Consider adding persistence verification for other fields.The
baseUrlandrateLimittests only verify the success message but don't confirm persistence via API like theauthModetest attempts. For consistency and thoroughness, consider addinggetServercalls to verify the values persisted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/servers/configure-server.spec.ts` around lines 67 - 99, Update the "updates the base URL" and "updates the rate limit" tests to assert persistence by calling the same API helper used in the authMode test (e.g., getServer) after the UI save completes; locate the tests by the test names and the locators baseUrlInput and rateLimitInput, then after awaiting the save confirmation call getServer(serverId) and assert the returned object's baseUrl equals "https://api.example.com/v2" in the first test and rateLimitPerMinute equals 120 (or "120" normalized to a number) in the second test so the tests verify server-side persistence not just the success message.e2e/tests/landing/navigation.spec.ts (1)
50-68: Conditional tests silently pass when links are missing.If the
docsorchangeloglinks are accidentally removed from the landing page, these tests will silently pass (theifblock is skipped). This could mask regressions. Consider either:
- Making visibility a hard requirement if these links should always exist
- Using
test.skip()with a clear message when the link is absent- Adding a test annotation or console log to indicate the test was skipped
♻️ Option: Fail if link unexpectedly missing
test("docs link navigates to documentation", async ({ page }) => { const docsLink = page.getByRole("link", { name: /docs|documentation/i }); - - if (await docsLink.first().isVisible().catch(() => false)) { - await docsLink.first().click(); - await page.waitForURL(/docs/, { timeout: 15_000 }); - expect(page.url()).toContain("docs"); - } + await expect(docsLink.first()).toBeVisible({ timeout: 10_000 }); + await docsLink.first().click(); + await page.waitForURL(/docs/, { timeout: 15_000 }); + expect(page.url()).toContain("docs"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/landing/navigation.spec.ts` around lines 50 - 68, The tests currently use an "if (await ...isVisible().catch(() => false))" guard so they silently pass when links are missing; update both tests ("docs link navigates to documentation" and "changelog link navigates to changelog page") to assert visibility as a hard requirement: replace the conditional guards around docsLink and changelogLink with explicit Playwright assertions (e.g., await expect(docsLink.first()).toBeVisible() and await expect(changelogLink.first()).toBeVisible()) before attempting .click(), then proceed to click and waitForURL as before so the test fails immediately if the links are missing.e2e/tests/specs/delete-spec.spec.ts (1)
41-42: Static analysis false positive — IDs are backend-generated UUIDs.The
specIdandserverIdvalues come fromcreateTestSpec, which returns backend-generated UUIDs containing only hex characters and hyphens — no special regex characters that could cause ReDoS. The static analysis warning is a false positive in this context.That said, you could simplify by using exact text matching:
♻️ Suggested simplification
- await expect(page.getByText(new RegExp(specId))).not.toBeVisible({ + await expect(page.getByText(specId)).not.toBeVisible({ timeout: 5_000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/specs/delete-spec.spec.ts` around lines 41 - 42, The test uses a regex locator getByText(new RegExp(specId)) which triggered a static-analysis ReDoS false positive; replace the regex-based lookup with exact text matching (use getByText(specId) or the equivalent exact locator) for both specId and serverId checks so they match the backend-generated UUIDs directly; locate the usages created from createTestSpec (references to specId and serverId) and update the assertions that call getByText(new RegExp(...)) to use the exact-string form and keep the same timeout behavior.e2e/fixtures/test-user.ts (1)
42-45: Consider using a far-future expiry date.The test card expiry
12/30will become invalid after December 2030. While this gives several years of runway, using a further date like12/99would prevent future test failures.♻️ Suggested change
export const STRIPE_TEST_CARD_DETAILS = { - expiry: "12/30", + expiry: "12/99", cvc: "123", zip: "10001", } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fixtures/test-user.ts` around lines 42 - 45, Update the test card expiry in the STRIPE_TEST_CARD_DETAILS object to a far-future date (e.g., change expiry "12/30" to "12/99") so the fixture doesn't expire; keep the same string format and leave cvc/zip unchanged when modifying the STRIPE_TEST_CARD_DETAILS constant.e2e/tests/mcp/sse-connection.spec.ts (1)
110-129: Consider adding a timeout to the JSON-RPC fetch call for consistency.The SSE reachability test (lines 39-46) uses an
AbortControllerwith a 10-second timeout, but this JSON-RPC call has no equivalent. While Playwright's test timeout provides a backstop, adding an explicit timeout would make the failure mode clearer and keep the tests consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/mcp/sse-connection.spec.ts` around lines 110 - 129, The JSON-RPC fetch inside the page.evaluate (the POST to messageUrl) needs an explicit timeout like the earlier SSE test: create an AbortController with a 10s timeout, pass controller.signal into the fetch options, and clear the timer on success; ensure the catch branch handles AbortError similarly (returning status 0 and the error string) so the test fails fast and consistently with the SSE reachability check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/content/docs/api-reference.mdx`:
- Around line 305-314: Add an explicit exception paragraph under the "Stripe
Webhook" section clarifying that the /api/webhooks/stripe route does not use the
global Bearer token auth or the standard API response envelope; instead it
authenticates via the stripe-signature header and returns raw JSON ({
"received": true }) on success to allow raw webhook payload verification.
Reference the route (/api/webhooks/stripe) and the stripe-signature header in
that note so integrators know to bypass the usual Bearer auth and envelope for
this webhook endpoint.
In `@e2e/global-setup.ts`:
- Around line 13-14: The readiness polling uses fetch(HEALTH_URL) with no
timeout, so a hung request blocks the retry loop; wrap each attempt with an
AbortController and a per-attempt timer (e.g., create AbortController, pass
controller.signal to fetch(HEALTH_URL, { signal }), set a setTimeout to call
controller.abort() after your desired timeout, and clear the timeout when the
request completes) and handle the abort/error so the loop continues to the next
retry instead of hanging; update the block that checks response.ok to use this
abortable fetch and ensure any errors from abort are caught and treated as a
failed attempt.
In `@e2e/helpers/api.ts`:
- Around line 84-88: createTestSpec currently allows arbitrary names while
cleanupTestSpecs only deletes specs with the "E2E Test" prefix, which can leak
or accidentally delete other runs; modify createTestSpec and related functions
(e.g., cleanupTestSpecs) to tie creation and teardown to a run-specific
identifier: generate a single runId at module init (or accept it via env) and
prefix all created spec names with `${runId}-E2E Test ...`, or alternatively
record and return the created spec IDs from createTestSpec in a module-level
array (e.g., createdSpecIds) and update cleanupTestSpecs to delete only those
recorded IDs; ensure both name construction (in createTestSpec) and deletion
logic (in cleanupTestSpecs and any other cleanup helper mentioned around the
149-156 region) reference the same runId or createdSpecIds so created fixtures
are reliably cleaned up.
- Around line 72-78: loadPetstoreSpec currently returns only { _raw: content }
which forces createTestSpec to hit the public Petstore; change loadPetstoreSpec
to either parse the checked-in YAML into a JS object (using the existing YAML
parser used elsewhere) and return that parsed spec (so callers can send it as
rawSpec when creating specs) or else change its contract to return the raw YAML
string/path explicitly; update any call sites (notably createTestSpec and the
usage around the helper at the other occurrence mentioned) to use the parsed
object as rawSpec when calling the /api/specs creation flow (or use the returned
string for upload tests), referencing loadPetstoreSpec and createTestSpec to
locate where to adjust behavior.
In `@e2e/helpers/auth.ts`:
- Line 51: The URL matcher passed to page.waitForURL is too strict and rejects
valid post-sign-out redirects with query params; update the matcher used in
page.waitForURL (in e2e/helpers/auth.ts) to allow optional query strings — for
example replace /\/(sign-in)?$/ with a regex that permits trailing query params
like /^\/(sign-in)?(\?.*)?$/ or use a starts-with/glob pattern that matches "/"
or any "/sign-in" plus queries so sign-out waits don't flake.
In `@e2e/tests/auth/signin.spec.ts`:
- Around line 24-42: Replace the hardcoded email string with the known test
account so the failure is due to a wrong password rather than a missing user:
use getTestUser().email when filling the emailInput in the invalid-password
scenario (keep the intentionally bad password), ensure the test imports or
accesses getTestUser() and that the rest of the flow still targets the same
selectors (emailInput, continueButton, passwordInput, signInButton,
errorMessage) so the assertion checks the “invalid credentials” error path.
In `@e2e/tests/billing/plan-limits.spec.ts`:
- Around line 50-56: The test named "health endpoint is accessible without auth"
is running in an authenticated suite and asserting the wrong response shape;
update it to use an unauthenticated request context instead of page.request
(e.g., use the global request fixture or create a new unauthenticated request
context) and change assertions to match the real response shape returned by
/api/health: assert response.status() is 200 and that the parsed JSON has the
keys/status values like "status" (string or boolean as appropriate), "db"
(object or status), and "timestamp" (present and a valid value) rather than
expecting a "success" property; keep the test title and use the existing call
site (page.request.get) as the reference to replace with the unauthenticated
request call.
In `@e2e/tests/billing/upgrade.spec.ts`:
- Around line 23-36: The test currently swallows waitForURL timeouts and treats
remaining on settings as success; update the logic in upgrade.spec.ts so
waitForURL errors are not silently ignored and the assertion requires a real
navigation: remove the empty .catch or replace it with capturing the error and
failing the test (or logging then rethrowing), and change the final expectation
to assert that either isOnStripe or isOnCheckoutApi is true (i.e. disallow
isStillOnSettings as a pass); reference the waitForURL call, the page.url()
usage, and the isStillOnSettings/isOnStripe/isOnCheckoutApi variables when
making this change.
In `@e2e/tests/console/test-tool-call.spec.ts`:
- Around line 98-106: The test currently only asserts behavior when
executeButton is enabled, so it can silently pass if the button is disabled;
update the test around the executeButton check (the executeButton variable and
the error locator created via page.locator('[data-testid="error-message"],
[role="alert"], .text-destructive')) to assert the disabled path as well—i.e.,
add an explicit expectation when executeButton.isEnabled() is false (for example
use expect(executeButton).toBeDisabled() or an equivalent assertion) so both
enabled and disabled branches are covered.
- Around line 7-24: The afterEach cleanup currently calls deleteSpec(specId)
even when specId/serverId may be undefined from a failed beforeEach; make specId
and serverId nullable (e.g., string | undefined) and guard the deletions in
test.afterEach by checking for truthiness before calling deleteSpec and any
server deletion function; update references to specId/serverId in the
surrounding test.beforeEach/test.afterEach blocks to use the nullable types and
only invoke deleteSpec/deleteServer when the IDs are defined to avoid deleting
the wrong resource or calling endpoints with "undefined".
In `@e2e/tests/specs/import-from-url.spec.ts`:
- Around line 61-63: The success assertion in import-from-url.spec.ts uses
page.getByText(/imported successfully|spec/i) which is too broad; update the
test to assert a specific success indicator instead (e.g., a unique text string
or data-testid) by replacing the regex locator used in page.getByText with a
precise locator such as page.getByText('Import completed successfully') or
page.getByTestId('import-success') (or the actual testid used in the app), and
keep the same timeout option; ensure you change any references to the loose
regex in this test to the new specific locator so the assertion cannot
accidentally match generic "spec" text.
- Around line 14-16: The teardown misses specs created by this import flow
because cleanupTestSpecs only deletes names starting with "E2E Test"; update the
test in import-from-url.spec.ts (the test using test.afterEach) so imported
Petstore specs are created with a deterministic prefix that matches
cleanupTestSpecs (e.g., prepend "E2E Test" to the imported spec name) or capture
the created spec IDs during the import and pass them to cleanupTestSpecs;
reference the test's import call and the cleanupTestSpecs function to either
modify the created spec name or return/store IDs for removal so CI artifacts are
reliably cleaned up.
---
Outside diff comments:
In `@README.md`:
- Line 109: Update the README entry for the Web App to reference the correct
Next.js version: change the phrase "Next.js 14 dashboard" to "Next.js 15
dashboard" in the table row containing the Web App description (the line with "|
**Web App** | [`apps/web`](apps/web) | Next.js 14 dashboard. ... |").
---
Minor comments:
In `@apps/web/content/docs/self-hosting.mdx`:
- Around line 23-24: The documentation uses a personal GitHub repo URL in the
git clone command ("git clone https://github.com/Work90210/APIFold.git" and "cd
APIFold"); update this to the organization-owned repository URL (for example
"git clone https://github.com/apifold/APIFold.git" and adjust the subsequent
"cd" if necessary) and ensure any other references in the same file to
Work90210/APIFold are replaced with the org-owned repo name for consistency.
In `@apps/web/lib/export/generator.ts`:
- Line 59: Update the generated standalone server to resolve the env var
mismatch: either change the generator in apps/web/lib/export/generator.ts so the
server reads CORS_ORIGINS (plural) and parses comma-separated values into the
allowed origins list, or document and support both by checking
process.env.CORS_ORIGINS first then falling back to process.env.CORS_ORIGIN;
reference the CORS_ORIGINS/CORS_ORIGIN env names in the generator logic and
update the generated .env example accordingly so the produced server and
.env.example remain consistent.
In `@e2e/helpers/api.ts`:
- Around line 57-63: The code calls response.json() unconditionally which can
throw SyntaxError for non-JSON responses and hide the real HTTP status/body;
modify the fetch handling around the fetch(`${BASE_URL}${endpoint}`, options)
call so you first check response.ok and the Content-Type (via
response.headers.get('content-type')) and, for non-JSON or empty bodies, call
response.text() (or handle 204/empty) instead of response.json(), then construct
the thrown Error using method, endpoint, response.status, response.statusText
and the raw body text if available; adjust references to response, json,
ApiResponse<T>, method and endpoint when reading/parsing so errors include the
actual HTTP status and body.
In `@e2e/tests/auth/signup.spec.ts`:
- Around line 9-21: The test "renders the sign-up form with email and password
fields" currently only asserts the email input (emailInput) and a heading;
either update the assertions to match the title by locating the password field
(e.g., use a locator like 'input[name="password"], input[type="password"]' and
add await expect(passwordInput).toBeVisible(...)) or change the test title
string in the test(...) call to reflect that it only checks the email input and
heading; adjust the test body accordingly (modify or add the locator referenced
by passwordInput) so the name and assertions are consistent.
In `@e2e/tests/billing/upgrade.spec.ts`:
- Around line 106-112: The test "billing portal link is accessible" assumes the
user has a Stripe customer ID but route /api/billing/portal (route.ts) returns
404 when stripeCustomerId is missing; update the assertion in the test
(test("billing portal link is accessible", ...) in
e2e/tests/billing/upgrade.spec.ts) to include 404 in the allowed
response.status() values or alternatively add a setup step that ensures the test
user has a stripeCustomerId before calling
page.request.post("/api/billing/portal"); adjust the
expect([...]).toContain(...) accordingly so the test passes for fresh users
without a Stripe customer ID.
In `@e2e/tests/landing/page-load.spec.ts`:
- Around line 19-20: The test uses a non-null assertion on headingText from
heading.textContent(), which can be null and cause false failures; update the
assertion to handle null safely by first asserting headingText is not null
(e.g., expect(headingText).not.toBeNull()) and only then assert the length, or
use a null-coalescing check like expect(headingText?.length ??
0).toBeGreaterThan(0); reference the variables/methods heading, headingText, and
textContent() when making the change.
In `@e2e/tests/mcp/sse-connection.spec.ts`:
- Around line 100-102: The assertion in the test currently checks events.length
>= 0 which always passes; update the assertion in the sse-connection.spec.ts
test so it requires at least one event (e.g., change the expectation on
events.length to beGreaterThanOrEqual(1) or toBeGreaterThan(0)) and optionally
add a clear failure message; locate the check referencing the events array and
the expect(...) call to modify it accordingly.
In `@e2e/tests/servers/configure-server.spec.ts`:
- Around line 62-65: The test currently only checks that the returned server
object from getServer(page, serverId) has an authMode property; instead assert
that server.authMode equals the specific value you updated it to earlier in the
test (e.g., the variable or literal used in the update request). Locate the
getServer call and replace the loose expect(server).toHaveProperty("authMode")
with a strict equality assertion against the expectedAuthMode (or the update
payload variable) so the test verifies the persisted value.
In `@e2e/tests/specs/delete-spec.spec.ts`:
- Around line 49-55: The test clicks deleteButton immediately but should wait
for it to be visible to match the first test and avoid flakiness; update the
block that uses page.getByRole("button", { name: /delete/i }) (deleteButton) to
wait for visibility before clicking (use the locator's waitFor/visibility check)
and then proceed to click and interact with confirmButton (page.locator(...)) as
before.
---
Nitpick comments:
In `@apps/runtime/__tests__/unit/query-monitor.test.ts`:
- Around line 51-68: The test is flaky because it depends on real time delays;
make it deterministic by mocking performance.now() around the call so measured
duration exceeds slowThresholdMs without setTimeout. In the test for
createMonitoredDb / monitored.query, replace the 50ms setTimeout approach with a
jest.spyOn or similar on global.performance.now() that returns a start value
(e.g., 1000) before calling monitored.query and a later value (e.g., 1011) after
to simulate elapsed time > slowThresholdMs; ensure you restore the spy after the
test and keep assertions on logger.warn the same.
- Around line 96-108: The test uses a real setTimeout inside createMockDb which
causes flakiness; instead simulate the slow query by mocking time—replace the
sleep-based callback with a deterministic mock of performance.now so that
createMockDb's query handler returns immediately but performance.now reports a
start and end that exceed slowThresholdMs; update the test to stub
performance.now (used by createMonitoredDb/monitored.query) to return two values
(start and end) that produce the desired elapsed time and remove reliance on
setTimeout.
In `@docs/API.md`:
- Around line 9-11: Several HTTP endpoint code blocks (for example the block
containing "Authorization: Bearer <session-token>") lack language specifiers;
update each triple-backtick fence that contains HTTP endpoint definitions or
header lines to use a language tag (prefer `http`, or `text` where appropriate).
Locate each code block that shows request/response headers or endpoint examples
(e.g., the block with "Authorization: Bearer <session-token>") and change the
opening fence from ``` to ```http and keep the closing fence as ```, applying
the same transformation to all similar endpoint definition blocks throughout the
document.
In `@docs/ARCHITECTURE.md`:
- Around line 150-163: ADR-010 duplicates the decision already recorded in
ADR-002; remove ADR-010 or merge its content into ADR-002 to avoid redundancy.
Update ADR-002 to include any unique wording from ADR-010 (including the
explicit "packages/transformer is MIT" boundary and consequences) and delete
ADR-010; also search for and update any cross-references or index entries that
point to ADR-010 so they now reference ADR-002.
In `@docs/CONTRIBUTING.md`:
- Around line 17-18: Update the repository URL in the CONTRIBUTING.md clone
instruction so it points to the organization-owned repository (or a stable
canonical URL) instead of the personal account; specifically replace the "git
clone https://github.com/Work90210/APIFold.git" string with the org's repo URL
(or a generic placeholder like "git clone
https://github.com/ORG_NAME/APIFold.git") to ensure future ownership changes
won't break the docs while preserving the existing "cd APIFold" line.
In `@e2e/fixtures/test-user.ts`:
- Around line 42-45: Update the test card expiry in the STRIPE_TEST_CARD_DETAILS
object to a far-future date (e.g., change expiry "12/30" to "12/99") so the
fixture doesn't expire; keep the same string format and leave cvc/zip unchanged
when modifying the STRIPE_TEST_CARD_DETAILS constant.
In `@e2e/global-setup.ts`:
- Around line 39-42: The warning in e2e/global-setup.ts currently logs that auth
tests "will be skipped" but does not implement any skipping; update the behavior
to match reality by either (A) changing the console.warn message in the block
that references the missing array (the console.warn call using
`${missing.join(", ")}`) to say the auth tests "may fail" or "will likely fail",
or (B) implement an actual skip mechanism by exporting a flag (e.g., export
const SKIP_AUTH_TESTS = true/false) from this setup and make auth specs read
that flag to conditionally call test.skip or set Jest/Mocha to skip those
suites; choose one approach and apply it consistently (modify the console.warn
text if you pick rewording, or add the exported flag and update auth spec files
to reference SKIP_AUTH_TESTS if you add skipping).
In `@e2e/tests/billing/upgrade.spec.ts`:
- Around line 66-78: Move the test.skip invocation to the top of the "can fill
in Stripe test card details" test so the skip check runs before any navigation
or UI interactions; specifically, add the
test.skip(!process.env["STRIPE_SECRET_KEY"], "STRIPE_SECRET_KEY not configured")
at the start of the test body for the test named "can fill in Stripe test card
details" (the async ({ page }) => { ... } block) and then proceed with page.goto
and the upgradeButton clicks only if the test is not skipped.
- Around line 39-53: The test's conditional skip
(test.skip(!process.env["STRIPE_SECRET_KEY"], ...)) should be moved to the very
start of the test block so the test is skipped before any page navigation or
interaction occurs; update the test in the "test(...)" function so the test.skip
call runs before page.goto("/dashboard/settings") and before using upgradeButton
(page.getByRole(...).or(...).first().click()) to avoid performing unnecessary
actions when STRIPE_SECRET_KEY is not set.
In `@e2e/tests/landing/navigation.spec.ts`:
- Around line 50-68: The tests currently use an "if (await
...isVisible().catch(() => false))" guard so they silently pass when links are
missing; update both tests ("docs link navigates to documentation" and
"changelog link navigates to changelog page") to assert visibility as a hard
requirement: replace the conditional guards around docsLink and changelogLink
with explicit Playwright assertions (e.g., await
expect(docsLink.first()).toBeVisible() and await
expect(changelogLink.first()).toBeVisible()) before attempting .click(), then
proceed to click and waitForURL as before so the test fails immediately if the
links are missing.
In `@e2e/tests/mcp/sse-connection.spec.ts`:
- Around line 110-129: The JSON-RPC fetch inside the page.evaluate (the POST to
messageUrl) needs an explicit timeout like the earlier SSE test: create an
AbortController with a 10s timeout, pass controller.signal into the fetch
options, and clear the timer on success; ensure the catch branch handles
AbortError similarly (returning status 0 and the error string) so the test fails
fast and consistently with the SSE reachability check.
In `@e2e/tests/servers/configure-server.spec.ts`:
- Around line 67-99: Update the "updates the base URL" and "updates the rate
limit" tests to assert persistence by calling the same API helper used in the
authMode test (e.g., getServer) after the UI save completes; locate the tests by
the test names and the locators baseUrlInput and rateLimitInput, then after
awaiting the save confirmation call getServer(serverId) and assert the returned
object's baseUrl equals "https://api.example.com/v2" in the first test and
rateLimitPerMinute equals 120 (or "120" normalized to a number) in the second
test so the tests verify server-side persistence not just the success message.
In `@e2e/tests/servers/manage-tools.spec.ts`:
- Around line 69-84: The test currently swallows all errors when calling
toggle.isChecked() and uses a hardcoded waitForTimeout(1_000), making it flaky;
change the logic in the block that computes wasChecked/isChecked to first try
toggle.isChecked() but only fallback if the element truly lacks isChecked
support (e.g., detect presence of aria-checked via await
toggle.getAttribute('aria-checked') !== null) rather than catching all errors,
then replace page.waitForTimeout(1_000) with an explicit wait for the toggle
state to change such as await expect(toggle).toHaveAttribute('aria-checked',
expectedValue) or use page.waitForFunction to poll the attribute/value until it
differs from the previous value; update references to toggle, wasChecked,
isChecked, toggle.isChecked, toggle.getAttribute and page.waitForTimeout
accordingly.
- Around line 21-27: The afterEach currently swallows all errors which can hide
infra failures; update the test.afterEach cleanup to catch the error from
deleteSpec(page, specId) as a named variable (e) and log it (e.g., console.error
or test.info().log) with context including specId and page URL/session info,
rather than leaving the catch empty—keep the best-effort intent but surface
failures for debugging by logging the exception from deleteSpec in the
test.afterEach block.
In `@e2e/tests/specs/delete-spec.spec.ts`:
- Around line 41-42: The test uses a regex locator getByText(new RegExp(specId))
which triggered a static-analysis ReDoS false positive; replace the regex-based
lookup with exact text matching (use getByText(specId) or the equivalent exact
locator) for both specId and serverId checks so they match the backend-generated
UUIDs directly; locate the usages created from createTestSpec (references to
specId and serverId) and update the assertions that call getByText(new
RegExp(...)) to use the exact-string form and keep the same timeout behavior.
In `@e2e/tests/specs/import-from-file.spec.ts`:
- Around line 4-5: Combine the two separate imports from the same module into a
single import statement: replace the separate imports of cleanupTestSpecs and
getPetstoreFixturePath (from "../../helpers/api") with one consolidated import
that imports both symbols together; update the import line that currently
references cleanupTestSpecs and getPetstoreFixturePath so both named exports are
imported in a single import declaration.
In `@lighthouserc.json`:
- Around line 1-27: The lighthouserc.json duplicates the GitHub Actions
Lighthouse config and adds only "upload.target"; either remove this root
lighthouserc.json to avoid confusion or consolidate into one config and update
the workflow to reference it — e.g., remove the duplicate lighthouserc.json
(which contains the "ci.collect", "ci.assert", and "ci.upload.target" entries)
if local lhci runs are unnecessary, or merge its "upload.target":
"temporary-public-storage" into the single canonical config used by the workflow
and ensure the workflow's configPath (currently set to
.github/lighthouse/config.json) and temporaryPublicStorage flag reflect the
consolidated file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7858a63-7285-4d31-8310-e3c6bed493f9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.github/lighthouse/config.json.github/workflows/bundle-analysis.yml.github/workflows/ci.yml.github/workflows/security.ymlREADME.mdapps/runtime/__tests__/unit/query-monitor.test.tsapps/runtime/src/index.tsapps/runtime/src/observability/query-monitor.tsapps/web/content/docs/api-reference.mdxapps/web/content/docs/architecture.mdxapps/web/content/docs/changelog.mdxapps/web/content/docs/meta.jsonapps/web/content/docs/self-hosting.mdxapps/web/lib/export/generator.tsapps/web/next.config.mjsapps/web/package.jsondocs/API.mddocs/ARCHITECTURE.mddocs/CONTRIBUTING.mddocs/SECURITY.mde2e/fixtures/petstore.yamle2e/fixtures/test-user.tse2e/global-setup.tse2e/helpers/api.tse2e/helpers/auth.tse2e/playwright.config.tse2e/tests/auth/signin.spec.tse2e/tests/auth/signup.spec.tse2e/tests/billing/plan-limits.spec.tse2e/tests/billing/upgrade.spec.tse2e/tests/console/test-tool-call.spec.tse2e/tests/export/download-server.spec.tse2e/tests/landing/navigation.spec.tse2e/tests/landing/page-load.spec.tse2e/tests/mcp/sse-connection.spec.tse2e/tests/servers/configure-server.spec.tse2e/tests/servers/connection-snippets.spec.tse2e/tests/servers/manage-tools.spec.tse2e/tests/specs/delete-spec.spec.tse2e/tests/specs/import-from-file.spec.tse2e/tests/specs/import-from-url.spec.tslighthouserc.jsonpackages/transformer/.size-limit.jsonpackages/transformer/README.mdpackages/transformer/package.json
Comment on lines
+72
to
+78
| function loadPetstoreSpec(): Record<string, unknown> { | ||
| const yamlPath = path.resolve(__dirname, "../fixtures/petstore.yaml"); | ||
| const content = readFileSync(yamlPath, "utf-8"); | ||
|
|
||
| // Simple YAML-to-JSON: import the spec via the API using sourceUrl instead | ||
| // For file upload tests, we return the raw string | ||
| return { _raw: content } as unknown as Record<string, unknown>; |
There was a problem hiding this comment.
Make the local Petstore fixture actually usable for spec creation.
loadPetstoreSpec() is documented as a parsed spec, but it only returns { _raw: content } via a double cast. Because of that, createTestSpec() still depends on the public Petstore endpoint for every setup. That adds third-party network flakiness even though /api/specs already accepts rawSpec (apps/web/lib/validation/spec.schema.ts, Lines 3-11; apps/web/app/api/specs/route.ts, Lines 120-138). Parse the checked-in fixture or store a JSON version and send it via rawSpec; if this helper is only for upload flows, make its contract a string/path instead.
Also applies to: 90-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/api.ts` around lines 72 - 78, loadPetstoreSpec currently returns
only { _raw: content } which forces createTestSpec to hit the public Petstore;
change loadPetstoreSpec to either parse the checked-in YAML into a JS object
(using the existing YAML parser used elsewhere) and return that parsed spec (so
callers can send it as rawSpec when creating specs) or else change its contract
to return the raw YAML string/path explicitly; update any call sites (notably
createTestSpec and the usage around the helper at the other occurrence
mentioned) to use the parsed object as rawSpec when calling the /api/specs
creation flow (or use the returned string for upload tests), referencing
loadPetstoreSpec and createTestSpec to locate where to adjust behavior.
Comment on lines
+84
to
+88
| export async function createTestSpec( | ||
| page: Page, | ||
| options?: { readonly name?: string }, | ||
| ): Promise<CreateSpecResult> { | ||
| const name = options?.name ?? `E2E Test Spec ${Date.now()}`; |
There was a problem hiding this comment.
Tie creation and cleanup to the same identifier.
createTestSpec() can create arbitrary names via options.name, but cleanupTestSpecs() only deletes specs whose names start with E2E Test. That means custom-named fixtures can leak, and the shared prefix can also wipe another concurrent run's data if the same account is reused. Always stamp created resources with a run-specific prefix, or track created IDs and delete only those.
Also applies to: 149-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/api.ts` around lines 84 - 88, createTestSpec currently allows
arbitrary names while cleanupTestSpecs only deletes specs with the "E2E Test"
prefix, which can leak or accidentally delete other runs; modify createTestSpec
and related functions (e.g., cleanupTestSpecs) to tie creation and teardown to a
run-specific identifier: generate a single runId at module init (or accept it
via env) and prefix all created spec names with `${runId}-E2E Test ...`, or
alternatively record and return the created spec IDs from createTestSpec in a
module-level array (e.g., createdSpecIds) and update cleanupTestSpecs to delete
only those recorded IDs; ensure both name construction (in createTestSpec) and
deletion logic (in cleanupTestSpecs and any other cleanup helper mentioned
around the 149-156 region) reference the same runId or createdSpecIds so created
fixtures are reliably cleaned up.
added 5 commits
March 22, 2026 07:23
- Add webhook auth exception note in API reference docs - Add fetch timeout (5s) to global-setup health polling - Fix sign-out URL matcher to allow query params - Use test user email for invalid-password test scenario - Fix health endpoint test to use unauthenticated context - Replace permissive upgrade test with modal fallback assertion - Guard console test cleanup against null IDs - Add disabled-button assertion for missing params test - Track created spec IDs for reliable URL import cleanup - Narrow success assertion to match specific text
…dcoded key - Fix trivy-action version: 0.28.0 -> v0.28.0 (needs v prefix) - Fix snyk/actions/node version: 0.4.0 -> v0.4.0 (needs v prefix) - Fix hashicorp/nextjs-bundle-analysis: v0.7.2 -> v0.5.0 (latest valid) - Move Clerk publishable key to secrets to avoid gitleaks false positive
- Use trivy-action@v0.35.0 (latest stable, v0.28.0 had broken dep) - Use snyk/actions/node@v1 (v0.4.0 tag format incompatible) - Remove broken hashicorp/nextjs-bundle-analysis action (no action.yml) - Fix gitleaks: rename sk_test_ placeholder, allowlist workflow files
…uild The Next.js build attempts static page generation for dashboard routes which requires a running PostgreSQL instance. Bundle analysis cannot run in CI without database infrastructure.
7 tasks
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.
Summary
docs/API.md), 7 new Architecture Decision Records, expanded security policy with disclosure timeline, v0.1.0 changelog across all 8 sprintssize-limitfor transformer package (50KB gzip cap), slow query monitoring in runtimecontinue-on-erroron audit jobs, fixed CORS*default in code generator (now requires explicit origin), added 30s fetch timeout in generated serversTest plan
pnpm buildpasses (verified locally)pnpm lintpasses (verified locally)pnpm typecheckpasses (verified locally)pnpm exec playwright test)Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes & Improvements