refactor(playwright-coverage): split index into focused modules#177
Merged
Conversation
Drop the noindex,nofollow opt-out from the design system page config so it inherits the default index,follow. The sitemap plugin auto-includes any page without a noindex robots meta, so /design/system now appears in sitemap.xml — update the SEO e2e to assert that. Also genericize the sitemap plugin's docstring and toPath comment so they no longer reference site-specific routes or host. /projects remains the sole noindex page.
Switch the Content wrapper from View to Flex with flex-grow: 1 so it grows to fill the parent's height instead of sizing to its content. Drop the now unused View import.
…lose #175 Extract the Playwright -> monocart e2e coverage harness out of apps/web into a new publishable workspace package, @soroush.tech/playwright-coverage: - auto fixture capturing per-test V8 JS coverage (Chromium only) - globalSetup/globalTeardown that clear and aggregate raw dumps into an lcov report - declarative include/exclude glob API (picomatch, repo-relative, cross-platform) that builds the monocart sourceFilter; a raw report.sourceFilter still works as a fallback (globs win when both are set) Mirrors @soroush.tech/vite-plugin-msw-server: peer deps (@playwright/test, monocart), tsdown dual-build + publishConfig, MIT LICENSE, README (install/usage/options/Codecov in CI/FAQ), 100% coverage. apps/web now consumes it via workspace:* — fixtures, coverage.setup and coverage.teardown re-export the instance from a shared coverage.ts that passes the Vike +Page include glob; coverage-options.ts is removed. E2E coverage output is unchanged (the 9 +Page.tsx files at 100%).
The workflow_dispatch `notes` input is a single-line string (dispatch has no multiline field). Expand `\n` escapes to real line breaks with printf '%b' so notes render as proper multiline markdown in the GitHub Release.
Lets the publish workflow target @soroush.tech/playwright-coverage. Requires a manual first publish + npm trusted-publisher config before OIDC releases work.
…r/lifecycle modules Extract the package internals from a single 195-line index.ts into focused sibling modules — types, report config helpers, the per-test collector, and the global setup/teardown lifecycle — each with co-located tests. index.ts keeps the public API (default factory + barrel re-exports) unchanged.
|
Warning Review limit reached
More reviews will be available in 23 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds a reusable Playwright coverage package, wires it into the web app’s E2E coverage setup, and updates the package release workflow. It also removes the design system page’s ChangesPlaywright coverage package
Design system indexing
Sequence Diagram(s)sequenceDiagram
participant PlaywrightTest
participant playwrightCoverage
participant page.coverage
participant CoverageReport
PlaywrightTest->>playwrightCoverage: call playwrightCoverage(options)
playwrightCoverage->>PlaywrightTest: return test, expect, globalSetup, globalTeardown
PlaywrightTest->>playwrightCoverage: run autoCoverage fixture
playwrightCoverage->>page.coverage: startJSCoverage({ resetOnNavigation: false })
PlaywrightTest->>playwrightCoverage: run test body
playwrightCoverage->>page.coverage: stopJSCoverage()
PlaywrightTest->>playwrightCoverage: run globalTeardown()
playwrightCoverage->>CoverageReport: add(parsed raw dumps)
playwrightCoverage->>CoverageReport: generate()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Use String#replaceAll over global-regex replace in defaultSourcePath, and flatten the nested ternary in toGlobs into a guard clause.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/playwright-coverage/src/index.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid exporting the internal collector helper from the barrel.
export * from './collector'turnscreateAutoCoverageFixtureinto a public API, which is broader than the factory-based surface described for this refactor. If that helper is meant to stay internal, this creates an unnecessary semver commitment.Suggested diff
export * from './types' -export * from './collector' export * from './lifecycle'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/playwright-coverage/src/index.ts` around lines 6 - 8, The barrel in index.ts is exporting the internal collector helper, which exposes createAutoCoverageFixture as public API; remove the export from ./collector and keep only the intended factory-based surface. Update the index.ts re-exports so types and lifecycle remain public while the collector module stays internal, and verify any needed symbols are imported directly from collector only within the package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cd-packages.yml:
- Around line 99-100: The release notes processing in the workflow is expanding
more than just newline escapes, which can mutate markdown before gh release
create runs. Update the release-notes handling in cd-packages workflow to
convert only literal \n sequences from the workflow_dispatch input, and avoid
using a formatter that interprets other backslash escapes; keep the fix
localized to the step that prepares the release body.
In `@packages/playwright-coverage/src/lifecycle.test.ts`:
- Around line 92-99: The no-op test in createGlobalTeardown uses a fixed rawDir
path in lifecycle.test that can collide with an existing temp directory entry;
update the test to generate a fresh temporary parent directory and set rawDir to
a never-created child path so the absence check is reliably unique. Keep the
change scoped to the no-op case in the createGlobalTeardown test and preserve
the existing makeFakeReport setup.
In `@packages/playwright-coverage/src/types.ts`:
- Around line 40-41: The browser option in the coverage types is too broad and
should be narrowed to Playwright’s browser-name union. Update the `browser`
field in `types.ts` from `string` to the exact `'chromium' | 'firefox' |
'webkit'` union so `collect()` can only compare against valid `browserName`
values and typos are caught at compile time.
---
Nitpick comments:
In `@packages/playwright-coverage/src/index.ts`:
- Around line 6-8: The barrel in index.ts is exporting the internal collector
helper, which exposes createAutoCoverageFixture as public API; remove the export
from ./collector and keep only the intended factory-based surface. Update the
index.ts re-exports so types and lifecycle remain public while the collector
module stays internal, and verify any needed symbols are imported directly from
collector only within the package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 358b9b65-0c6f-4f2c-89d2-0652a72930b1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (28)
.github/workflows/cd-packages.ymlapps/web/package.jsonapps/web/src/common/Blueprint/Blueprint.tsxapps/web/src/pages/design/system/+config.tsapps/web/src/test/e2e/coverage-options.tsapps/web/src/test/e2e/coverage.setup.tsapps/web/src/test/e2e/coverage.teardown.tsapps/web/src/test/e2e/coverage.tsapps/web/src/test/e2e/fixtures.tsapps/web/src/test/e2e/seo.e2e.tspackages/README.mdpackages/packages.mdpackages/playwright-coverage/LICENSEpackages/playwright-coverage/README.mdpackages/playwright-coverage/eslint.config.jspackages/playwright-coverage/package.jsonpackages/playwright-coverage/src/collector.test.tspackages/playwright-coverage/src/collector.tspackages/playwright-coverage/src/index.test.tspackages/playwright-coverage/src/index.tspackages/playwright-coverage/src/lifecycle.test.tspackages/playwright-coverage/src/lifecycle.tspackages/playwright-coverage/src/report.test.tspackages/playwright-coverage/src/report.tspackages/playwright-coverage/src/types.tspackages/playwright-coverage/tsconfig.jsonpackages/playwright-coverage/tsdown.config.tspackages/vite-plugin-sitemap/src/index.ts
💤 Files with no reviewable changes (2)
- apps/web/src/pages/design/system/+config.ts
- apps/web/src/test/e2e/coverage-options.ts
- playwright-coverage: narrow the `browser` option from `string` to 'chromium' | 'firefox' | 'webkit' so typos can't silently skip coverage. - playwright-coverage: point the teardown no-op test at a unique fresh temp path instead of a fixed name that could collide and flake. - CD: expand only literal `\n` in release notes via parameter expansion; the previous `printf '%b'` also interpreted `\t`/`\u`/`\c` and could mangle markdown.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will decrease total bundle size by 248 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: @soroush.tech/dev-esmAssets Changed:
Files in
|
The package had no vitest.config.ts, so coverage fell back to vitest's default reporters (clover/html/json) and never wrote coverage/lcov.info — breaking the CI step that rewrites SF: paths in that file. Add the same config the sibling packages use (v8, ['text', 'lcov'], 100% thresholds).
Document that every package ships vitest.config.ts with coverage.reporter ['text', 'lcov'] — CI reads each package's coverage/lcov.info, so omitting it breaks the coverage step.
|
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.
What
Splits the
@soroush.tech/playwright-coveragepackage's single ~195-linesrc/index.tsinto focused, co-located modules while keeping the public API unchanged.types.tsJsCoverageSource,CoverageFixtureArgs,PlaywrightCoverageOptions,PlaywrightCoverage)report.tsdefaultEntryFilter,defaultSourcePath,toGlobs,buildSourceFilter,resolveReportOptions,resolveRawDir) — package-internal, not re-exportedcollector.tscollect(private) +createAutoCoverageFixture+CoverageFixtureTuplelifecycle.tscreateGlobalSetup,createGlobalTeardownindex.tsplaywrightCoveragefactory +export *barrel re-exportsEach module gets a co-located
*.test.ts.report.test.tsnow unit-tests the helpers directly rather than driving them throughcreateGlobalTeardown.Why
The package was one chunky file. The public API stays in
index.ts(single entry, default factory) while internals move to siblings — matching the new guidance added topackages.md: extract internal modules once the entry file grows unwieldy; the app-level "one file per helper" rule is scoped to components, not packages.Also included
packages.md— documents when/how to split a package's internal modules.package.json— version bump0.1.0 → 1.0.0.pnpm-lock.yaml— sync so the workspace package resolves.Verification
pnpm lint— clean (0 warnings)pnpm typecheck— cleanpnpm test:coverage— 25 tests, 100% statements/branches/functions/linespnpm build— succeedsapps/webe2e) uses only the default export.Summary by CodeRabbit
New Features
\nline breaks.Bug Fixes
Documentation