chore(kit): close two CI gaps that let kit.openiap.dev ship broken#121
Conversation
This week shipped two production-breaking PRs (#119 fix-up and #120 runtime crash) that PR CI didn't catch. Both classes were addressable: 1. Docker build only ran on push-to-main, so the Dockerfile bug from PR #109 (Bun 1.3.13 --filter hoisting) only surfaced after merge. → Add a PR-level Docker build step in the verify job, with GHA layer caching so cold builds aren't paid every PR. 2. The smoke test only HTTP-probed /, /health, /api/v1, which all returned 200 even when the SPA bundle crashed at boot. PR #120's manualChunks cycle ("Cannot set properties of undefined (setting 'Activity')") shipped to kit.openiap.dev as a blank page through green CI. → Add scripts/smoke-browser.ts: load the SPA in headless Chromium, listen for pageerror + console.error, and assert #root mounts non-empty content. Wire it into smoke-server.sh after HTTP probes. The browser smoke is gated on SKIP_BROWSER_SMOKE=1 for dev iteration, and CI installs chromium via `bunx playwright install`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hyochan
added
kit
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a headless browser smoke test for the kit SPA to detect runtime bundle crashes that standard HTTP probes might miss. The implementation includes a new Playwright-based script and its integration into the existing server smoke test. Feedback was provided to improve error reporting by catching navigation failures and to prevent resource leaks by ensuring the browser is closed before the process exits.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR enhances the CI/verification pipeline by adding browser-based smoke testing and Docker image validation. It introduces a new headless Playwright test that validates SPA mounting and runtime behavior, updates placeholder Convex URLs, and implements PR-time Docker Buildx image building to catch Dockerfile/runtime incompatibilities before merge. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 24 seconds.Comment |
… ignored errors The browser smoke caught a real failure mode it wasn't designed for: the Convex client throws synchronously during module init when VITE_KIT_CONVEX_URL doesn't match the Convex deployment-name format, which blocks React from mounting #root and short-circuits the smoke even though the bundle itself is fine. Two-part fix: 1. Switch placeholder URL to a valid deployment-name format (`placeholder-build-1.convex.cloud`) in deploy-kit.yml + smoke script so Convex's parser accepts it without throwing. 2. Add an allow-list in smoke-browser.ts for known-OK placeholder-environment errors (Convex deployment-name parse, Sentry invalid DSN). Defense-in-depth in case other deps grow similar synchronous-init checks against placeholder env. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kit/scripts/smoke-browser.ts (1)
22-29: ⚡ Quick winRename constant to camelCase to match repo convention.
IGNORED_ERROR_PATTERNSviolates the project naming rule for TS/JS variables. Please rename to camelCase (for exampleignoredErrorPatterns) and update references.Suggested patch
-const IGNORED_ERROR_PATTERNS: RegExp[] = [ +const ignoredErrorPatterns: RegExp[] = [ /CONVEX FATAL ERROR.*Couldn't parse deployment name/i, /Sentry.*Invalid DSN/i, ]; function isIgnored(message: string): boolean { - return IGNORED_ERROR_PATTERNS.some((re) => re.test(message)); + return ignoredErrorPatterns.some((re) => re.test(message)); }As per coding guidelines "Use camelCase for variable and function names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kit/scripts/smoke-browser.ts` around lines 22 - 29, The constant name IGNORED_ERROR_PATTERNS violates the project's camelCase convention; rename it to ignoredErrorPatterns (keeping the RegExp[] type and same patterns) and update every reference, including the isIgnored function which currently uses IGNORED_ERROR_PATTERNS, to use ignoredErrorPatterns instead so the code compiles and follows naming rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/kit/scripts/smoke-browser.ts`:
- Around line 22-29: The constant name IGNORED_ERROR_PATTERNS violates the
project's camelCase convention; rename it to ignoredErrorPatterns (keeping the
RegExp[] type and same patterns) and update every reference, including the
isIgnored function which currently uses IGNORED_ERROR_PATTERNS, to use
ignoredErrorPatterns instead so the code compiles and follows naming rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9465500a-812b-4add-a633-6face363f0ca
📒 Files selected for processing (3)
.github/workflows/deploy-kit.ymlpackages/kit/scripts/smoke-browser.tspackages/kit/scripts/smoke-server.sh
There was a problem hiding this comment.
Pull request overview
Improves packages/kit PR verification so CI can catch (1) Docker build breakages and (2) SPA runtime crashes that still return HTTP 200, preventing broken kit.openiap.dev deploys from slipping through green PR CI.
Changes:
- Add a headless Chromium smoke test that loads the SPA, watches for runtime errors, and asserts
#rootmounts content. - Wire the browser smoke into
smoke-server.sh(skippable viaSKIP_BROWSER_SMOKE=1). - Extend the kit verify workflow to install Playwright Chromium and to perform a PR-level Docker build of
packages/kit/Dockerfilewith GHA layer caching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/kit/scripts/smoke-server.sh |
Runs the new browser smoke after existing HTTP probes (optional via env). |
packages/kit/scripts/smoke-browser.ts |
New Playwright-based headless SPA smoke to catch runtime boot/hydration failures. |
.github/workflows/deploy-kit.yml |
Adds Playwright Chromium install + PR-level Docker build check in the verify job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Gemini + Copilot review: process.exit() inside the try block short-circuits the finally block, leaving the headless chromium process running and suppressing any errors collected via the pageerror/console.error listeners. Restructure: - Wrap goto + assertion in an inner try/catch so navigation timeouts surface in the errors array instead of jumping out. - Move the failure report and exit decision out of the outer try so finally runs first. - main() now returns the exit code; the caller sets process.exitCode instead of calling process.exit, letting Node/Bun finish flushing before exit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a headless browser smoke test for the kit SPA to detect runtime bundle crashes that standard HTTP probes might miss. It adds a Playwright-based script (smoke-browser.ts) and integrates it into the existing smoke-server.sh workflow. Feedback was provided regarding the use of "networkidle" in the browser script, which may cause flakiness due to persistent WebSocket connections; using a more specific selector wait is recommended instead.
Per Gemini review: networkidle hangs in apps with persistent sockets (Convex uses WebSockets, Sentry posts beacons). Switch the smoke navigation to waitUntil: domcontentloaded and explicitly wait for #root to gain children. Mounted-children is the actual signal we care about; if the bundle crashes the wait surfaces as a clean TimeoutError that the existing inner try/catch reports. Keep the post-mount innerHTML emptiness check as a tripwire so a future refactor can't silently weaken the assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
This week's outage chain (PR #119 fix-up + PR #120 runtime crash) shipped through green PR CI because the verify job didn't catch either failure mode. Two targeted additions:
1. PR-level Docker build (catches Dockerfile bugs before merge)
Docker buildstep to theverifyjob that buildspackages/kit/Dockerfileagainst the workspace context, with GHA layer caching.--filterhoisting incompatibility — currently the Docker image only builds in theDeployjob which runs on push-to-main, so the bug only surfaced 5 minutes after merge.2. Headless-browser smoke (catches SPA runtime crashes)
packages/kit/scripts/smoke-browser.ts: load the SPA in headless Chromium, listen forpageerror+console.error, assert#rootmounts non-empty content.smoke-server.shafter the existing HTTP probes (gated onSKIP_BROWSER_SMOKE=1for dev iteration).manualChunkscross-chunk cycle ("Cannot set properties of undefined (setting 'Activity')") — HTTP probes returned 200 because the server still served a blankindex.html, but the bundle crashed at boot in the browser.bunx playwright install --with-deps chromium;@playwright/testwas already a dev dep.Test plan
smoke-browser.tscorrectly reportsmounted cleanly ✓against the current bundle.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Tests
Chores