Revert "Revert "feat: add alpha playwright suite support to the cli" … #1065
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
## Walkthrough
This change introduces comprehensive support for Playwright checks in the CLI. It adds the `PlaywrightCheck` construct, Playwright project bundling and uploading, Playwright config parsing, and related utilities. CLI commands and project parsing now handle Playwright checks, and new dependencies and test fixtures are included. The release workflow is also updated for improved canary tag extraction.
## Changes
| Files/Paths | Change Summary |
|------------------------------------------------------------------------------------------------------------|---------------|
| .github/workflows/release-canary.yml | Refactored canary tag extraction to use a Node.js script parsing all PR labels; updated npm publish step to use shell variables and log the tag. |
| packages/cli/package.json | Added dependencies: `archiver`, `@types/archiver`, and `minimatch`. |
| packages/cli/src/commands/deploy.ts<br>packages/cli/src/commands/test.ts | Added Playwright check support: imports, flag parsing, project parsing with Playwright options, and logic to bundle and upload Playwright projects. |
| packages/cli/src/constructs/index.ts | Exported new `playwright-check` construct. |
| packages/cli/src/constructs/playwright-check.ts | New `PlaywrightCheck` class: represents Playwright checks, bundles projects, uploads code, and synthesizes config. |
| packages/cli/src/reporters/list.ts | Updated link formatting to apply color/underline per link in output. |
| packages/cli/src/rest/checkly-storage.ts | Added `uploadCodeBundle` method for uploading Playwright bundles. |
| packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts<br>packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts<br>packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts<br>packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts | Added Playwright config fixtures for tests. |
| packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts<br>packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts | Added Playwright test spec fixtures. |
| packages/cli/src/services/__tests__/playwright-config.spec.ts | Added tests for Playwright config parsing and browser detection. |
| packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts | Replaced previous dependency parsing tests with Playwright config-based file parsing tests. |
| packages/cli/src/services/check-parser/parser.ts | Refactored `getFilesAndDependencies` to use `PlaywrightConfig`, added Playwright-aware file collection and matching logic. |
| packages/cli/src/services/checkly-config-loader.ts | Refactored config loading, added Playwright config detection, default config generation, and modularized helpers. |
| packages/cli/src/services/playwright-config.ts | New module: defines `PlaywrightConfig` and `PlaywrightProject` classes for Playwright test/project parsing and file management. |
| packages/cli/src/services/project-parser.ts | Extended project parsing to support Playwright checks, including bundling and instantiation logic. |
| packages/cli/src/services/util.ts | Added utilities for Playwright project bundling, file loading, config file writing, and cleanup. |
| packages/cli/src/services/__tests__/project-parser.spec.ts | Modified test assertions to check project logical ID and resource types/IDs more granularly. |
| packages/cli/src/constructs/project.ts | Added validation to ensure `logicalId` is present on constructs, throwing error if missing. |
## Possibly related PRs
- [checkly/checkly-cli#1055](https://github.com/checkly/checkly-cli/pull/1055): Refactors and improves the GitHub Actions workflow for extracting and using canary tags from multiple pull request labels, directly related to the workflow changes in this PR.
- [checkly/checkly-cli#1042](https://github.com/checkly/checkly-cli/pull/1042): Also modifies `.github/workflows/release-canary.yml` and implements Playwright check support in CLI commands and services, closely matching the changes here.
- [checkly/checkly-cli#1064](https://github.com/checkly/checkly-cli/pull/1064): Reverts Playwright-related changes introduced in this PR, including the `PlaywrightCheck` construct and associated utilities.
## Suggested reviewers
- umutuzgur🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
packages/cli/src/services/project-parser.ts (1)
118-124: Same unused variable issue in fallback branch- const playwrightCheck = new PlaywrightCheck(path.basename(playwrightConfigPath), { + new PlaywrightCheck(path.basename(playwrightConfigPath), { name: path.basename(playwrightConfigPath), logicalId: path.basename(playwrightConfigPath), codeBundlePath: key, browsers, playwrightConfigPath: relativePlaywrightConfigPath, })
🧹 Nitpick comments (16)
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts (2)
1-10: Add browser cleanup to prevent resource leaks.This test fixture is missing browser cleanup. Even though it's a test fixture, it's good practice to include proper cleanup.
import {test, chromium} from '@playwright/test'; test('Google test', async () => { const browser = await chromium.launch(); const context = await browser.newContext(); const page = await context.newPage(); // check start page is displayed await page.goto('https://google.com'); + + // Cleanup resources + await context.close(); + await browser.close(); });
3-10: Consider using Playwright's fixtures for better resource management.Playwright provides built-in fixtures that automatically handle browser lifecycle. This approach is more efficient and prevents resource leaks.
-import {test, chromium} from '@playwright/test'; +import {test} from '@playwright/test'; -test('Google test', async () => { - const browser = await chromium.launch(); - const context = await browser.newContext(); - const page = await context.newPage(); +test('Google test', async ({page}) => { // check start page is displayed await page.goto('https://google.com'); });packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts (3)
1-11: Add browser cleanup to prevent resource leaks.This test fixture is missing browser cleanup. Even though it's a test fixture, it's good practice to include proper cleanup.
import {test, expect, chromium} from '@playwright/test'; test('Google test', async () => { const browser = await chromium.launch(); const context = await browser.newContext(); const page = await context.newPage(); // check start page is displayed await page.goto('https://google.com'); await expect(page).toHaveScreenshot() + + // Cleanup resources + await context.close(); + await browser.close(); });
3-11: Consider using Playwright's fixtures for better resource management.Playwright provides built-in fixtures that automatically handle browser lifecycle. This approach is more efficient and prevents resource leaks.
-import {test, expect, chromium} from '@playwright/test'; +import {test, expect} from '@playwright/test'; -test('Google test', async () => { - const browser = await chromium.launch(); - const context = await browser.newContext(); - const page = await context.newPage(); +test('Google test', async ({page}) => { // check start page is displayed await page.goto('https://google.com'); await expect(page).toHaveScreenshot() });
10-10: Missing semicolon on toHaveScreenshot call.For consistency, add a semicolon after the toHaveScreenshot() call.
await page.goto('https://google.com'); - await expect(page).toHaveScreenshot() + await expect(page).toHaveScreenshot();packages/cli/src/reporters/list.ts (1)
47-47: Consider updating TODO commentsThe TODO comments mention "print all video files URLs" and "print all trace files URLs", but the implementation already handles multiple links with proper formatting. These TODO comments may be outdated since the functionality appears to be implemented.
- // TODO: print all video files URLs + // Links are printed with individual styling for better readabilityAlso applies to: 51-51, 81-81, 85-85
packages/cli/src/rest/checkly-storage.ts (1)
22-22: Consider consistent header casingThe Content-Type header uses Pascal-Case while content-length uses lowercase. For consistency, consider standardizing the casing of HTTP headers.
- { headers: { 'Content-Type': 'application/octet-stream', 'content-length': size } }, + { headers: { 'Content-Type': 'application/octet-stream', 'Content-Length': size } },packages/cli/src/services/__tests__/playwright-config.spec.ts (1)
10-10: Remove redundant "it" in test descriptionsBoth test descriptions include a redundant "it" at the beginning, which is stylistically unnecessary since the
itfunction already provides this context.- it('it should load simple config correctly', async () => { + it('should load simple config correctly', async () => {Also applies to: 16-16
packages/cli/src/commands/test.ts (1)
237-248: Bundle Playwright projects once per unique config instead of once per checkThe current loop bundles (and uploads) a Playwright project for every
PlaywrightCheckthat does not have acodeBundlePath.
When multiple checks share the same Playwright config this results in redundant
tar/gzip + network uploads, which can slow the CLI considerably on large
suites.A simple optimisation is to cache by
playwrightConfigPath + include.join(','):- for (const check of checks) { - // TODO: Improve bundling and uploading - if (!(check instanceof PlaywrightCheck) || check.codeBundlePath) { - continue - } - const { - relativePlaywrightConfigPath, browsers, key, - } = await PlaywrightCheck.bundleProject(check.playwrightConfigPath, check.include) + const bundleCache = new Map<string, Awaited<ReturnType<typeof PlaywrightCheck.bundleProject>>>() + for (const check of checks) { + if (!(check instanceof PlaywrightCheck) || check.codeBundlePath) { + continue + } + const cacheKey = `${check.playwrightConfigPath}::${check.include.sort().join(',')}` + const bundleInfo = bundleCache.get(cacheKey) ?? + await PlaywrightCheck.bundleProject(check.playwrightConfigPath, check.include) + bundleCache.set(cacheKey, bundleInfo) + const { relativePlaywrightConfigPath, browsers, key } = bundleInfo check.codeBundlePath = key check.browsers = browsers check.playwrightConfigPath = relativePlaywrightConfigPath }This keeps behaviour identical while drastically reducing duplicate work.
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
5-7: Drop unusedpathToPosiximport
pathToPosixisn’t referenced in this spec. Keeping the import will trigger
no-unused-vars/ESLint warnings and can break the CI lint step.-import { pathToPosix, loadFile } from '../../util' +import { loadFile } from '../../util'packages/cli/src/services/checkly-config-loader.ts (1)
95-97: Replace customisStringhelper with the native check
isStringcan be simplified totypeof obj === 'string', avoidingObject.prototype.toStringoverhead and improving readability.-function isString (obj: any) { - return (Object.prototype.toString.call(obj) === '[object String]') -} +const isString = (value: unknown): value is string => typeof value === 'string'packages/cli/src/services/check-parser/parser.ts (1)
188-194: LeveragePromise.allto parallelise directory scans
collectFilesis invoked sequentially per project; with large suites this becomes noticeable I/O wait. Consider:- for (const project of projects) { - const files = await this.collectFiles(cachedFiles, project.testDir, ignoredFiles) - … - } + await Promise.all(projects.map(async project => { + const files = await this.collectFiles(cachedFiles, project.testDir, ignoredFiles) + … + }))Reduces overall wall-clock time without altering semantics.
packages/cli/src/services/playwright-config.ts (1)
141-155: Snapshot-template default should inherit from parentWhen a project defines no
snapshotTemplates, falling back to the module-level default ignores any templates already gathered on the parent config. Consider cloning the parent set instead of unconditionalDEFAULT_SNAPSHOT_TEMPLATE, e.g.:this.snapshotTemplates = new Set( playwrightConfig.snapshotTemplates.size ? playwrightConfig.snapshotTemplates : [DEFAULT_SNAPSHOT_TEMPLATE], )Ensures behaviour consistency across mixed project/global definitions.
packages/cli/src/services/util.ts (1)
321-326: Multipleglobcalls can be merged to a single patternCalling
archive.globthree times incurs extra I/O and forgets other lock-file variants.
A single call keeps intent clear:-archive.glob('**/package*.json', …) -archive.glob('**/pnpm*.yaml', …) -archive.glob('**/yarn.lock', …) +archive.glob('{**/package*.json,**/pnpm*.y*ml,**/yarn.lock}', { + cwd: dir, + ignore: ignoredFiles, +})packages/cli/src/constructs/playwright-check.ts (2)
52-55: Grammar & clarity of validation errorTiny nit: “doesnt” → “doesn’t” and include quotes around the path.
-throw new ValidationError(`Playwright config doesnt exist ${props.playwrightConfigPath}`) +throw new ValidationError(`Playwright config doesn’t exist: "${props.playwrightConfigPath}"`)
101-105:uploadPlaywrightProjectreceives files, not directoriesRename parameter for readability and document units to avoid mis-use.
-static async uploadPlaywrightProject (dir: string): Promise<AxiosResponse> { - const { size } = await fs.promises.stat(dir) - const stream = fs.createReadStream(dir) +static async uploadPlaywrightProject (bundlePath: string): Promise<AxiosResponse> { + const { size } = await fs.promises.stat(bundlePath) + const stream = fs.createReadStream(bundlePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts-snapshots/Google-test-1-Mobile-Chrome-linux.pngis excluded by!**/*.png
📒 Files selected for processing (21)
.github/workflows/release-canary.yml(2 hunks)packages/cli/package.json(2 hunks)packages/cli/src/commands/deploy.ts(3 hunks)packages/cli/src/commands/test.ts(4 hunks)packages/cli/src/constructs/index.ts(1 hunks)packages/cli/src/constructs/playwright-check.ts(1 hunks)packages/cli/src/reporters/list.ts(2 hunks)packages/cli/src/rest/checkly-storage.ts(1 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts(1 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts(1 hunks)packages/cli/src/services/__tests__/playwright-config.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(6 hunks)packages/cli/src/services/checkly-config-loader.ts(4 hunks)packages/cli/src/services/playwright-config.ts(1 hunks)packages/cli/src/services/project-parser.ts(4 hunks)packages/cli/src/services/util.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/cli/src/reporters/list.ts (1)
packages/cli/src/reporters/util.ts (1)
printLn(459-461)
packages/cli/src/commands/deploy.ts (1)
packages/cli/src/constructs/playwright-check.ts (1)
PlaywrightCheck(24-124)
packages/cli/src/commands/test.ts (1)
packages/cli/src/constructs/playwright-check.ts (1)
PlaywrightCheck(24-124)
packages/cli/src/services/util.ts (4)
packages/create-cli/src/utils/fileloader.ts (1)
loadFile(4-20)packages/cli/src/services/playwright-config.ts (1)
PlaywrightConfig(38-117)packages/cli/src/services/check-parser/parser.ts (1)
Parser(94-470)packages/cli/src/services/checkly-config-loader.ts (1)
ChecklyConfig(22-93)
🪛 GitHub Check: lint
packages/cli/src/commands/test.ts
[warning] 149-149:
'playwrightConfig' is assigned a value but never used
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (17)
packages/cli/src/constructs/index.ts (1)
35-35: Export added for Playwright check construct.The export for the new Playwright check module follows the established pattern in this file and makes the PlaywrightCheck construct available to consumers.
packages/cli/package.json (1)
80-80: Dependencies added for Playwright project bundling.The added dependencies (archiver, @types/archiver, and minimatch) are appropriate for supporting Playwright check functionality. The archiver package will enable bundling Playwright projects into archives, while minimatch provides file pattern matching capabilities needed for project parsing.
Also applies to: 84-84, 99-99
packages/cli/src/reporters/list.ts (1)
45-45: Improved link formatting in CLI outputThe changes now apply styling to individual links rather than the entire string, resulting in better visual separation and clickability in the terminal. Each URL is now independently underlined and colored cyan, making it easier for users to identify and click separate links.
Also applies to: 48-48, 52-52, 79-79, 82-82, 86-86
packages/cli/src/rest/checkly-storage.ts (1)
18-24: New method for uploading Playwright code bundlesThe new
uploadCodeBundlemethod properly handles streaming uploads with explicit content length, which is essential for Playwright project bundles. The method follows the same pattern as the existinguploadmethod but adds the required content-length header.packages/cli/src/services/__tests__/playwright-config.spec.ts (1)
9-22: Well-structured tests for Playwright configurationThe tests are well-structured and validate both the test pattern matching and browser detection capabilities of the PlaywrightConfig class, including handling of defaults when no browsers are specified.
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts (1)
1-52: Well-structured Playwright configuration fixtureThe Playwright configuration is well-structured with appropriate comments explaining each section. It includes proper browser configurations, test settings, and conditional CI environment handling. This serves as a good fixture for testing the Playwright integration.
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts (1)
1-59: LGTM - Well-structured Playwright configuration file.This is a standard Playwright configuration file following best practices. It properly configures test execution parameters for both CI and local environments, sets up mobile device testing, and includes a local development server configuration.
packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts (1)
1-43: LGTM - Valid minimal Playwright configuration.This is a valid minimal Playwright configuration that's useful for testing browser detection edge cases. It properly configures a Chromium project without explicitly specifying browser options.
packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts (1)
1-65: LGTM - Comprehensive Playwright test configuration.This config provides good test coverage for the Playwright integration by including both mobile and desktop browsers, custom snapshot configurations, and browser channel specifications.
.github/workflows/release-canary.yml (3)
22-32: Improved canary tag extraction logic.The Node.js script approach for parsing JSON labels is more robust than string manipulation, properly handling any label starting with "canary:".
39-43: Improved conditional check for canary tag publishing.Using direct shell variable comparison is cleaner and more explicit.
45-46: Explicit environment variable passing.Explicitly passing environment variables to the publishing step improves clarity and maintainability.
packages/cli/src/commands/deploy.ts (3)
14-14: LGTM - Added PlaywrightCheck import.Correctly importing the PlaywrightCheck construct to support the new Playwright integration.
117-119: LGTM - Added Playwright configuration to project parsing.Properly passing the Playwright-specific configuration options to the project parser.
135-146: LGTM - Added Playwright project bundling.This correctly implements the bundling of Playwright projects for checks that don't already have a bundle path. The implementation:
- Identifies PlaywrightCheck instances
- Bundles the project with appropriate config and include patterns
- Updates the check with bundle info and relative config path
The TODO comment suggests this approach might be refined in the future.
packages/cli/src/services/check-parser/parser.ts (2)
136-146: Do not parse static assetsGood catch adding
txt/jpeg/jpg/pngtoSTATIC_FILE_EXTENSION; this prevents unnecessary AST walks. 👍
220-255: 🛠️ Refactor suggestionEdge-case:
{projectName}token loses prefix in snapshot templateIn
createFileMatcherthe replacements keep a possible separator via$1(e.g.,/{testDir}→/my/dir).
projectNamelacks this placeholder ('$1' + config.projectNamemissing), causing the preceding char to be dropped:- .replace(/\{(.)?projectName\}/g, config.projectName) + .replace(/\{(.)?projectName\}/g, '$1' + config.projectName)Without it
foo{projectName}→myProject, losingfoo.
Please adjust to preserve user-defined prefixes.Likely an incorrect or invalid review comment.
| expect(Array.from(config.testMatch)).toEqual(['**/*.@(spec|test).?(c|m)[jt]s?(x)']) | ||
| expect(config.getBrowsers()).toEqual(['chromium', 'webkit', 'msedge', 'chrome']) | ||
| }) | ||
| it('it should load simple config correctly', async () => { |
There was a problem hiding this comment.
Fix duplicate test description
The second test has the same description as the first test, but it's testing a different configuration file (without browsers). Update the description to reflect what's being tested.
- it('it should load simple config correctly', async () => {
+ it('it should load config with no browsers correctly', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('it should load simple config correctly', async () => { | |
| it('it should load config with no browsers correctly', async () => { |
🤖 Prompt for AI Agents
In packages/cli/src/services/__tests__/playwright-config.spec.ts at line 16, the
test description is duplicated for two different tests. Update the second test's
description to clearly indicate it is testing the configuration file without
browsers, differentiating it from the first test's description.
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 7'] }, | ||
| }, | ||
|
|
||
| /* Test against mobile viewports. */ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 5'] }, | ||
| }, | ||
| { | ||
| name: 'Mobile Safari', | ||
| use: { ...devices['iPhone 12'] }, | ||
| }, |
There was a problem hiding this comment.
Fix duplicate project name
There are two projects with the same name "Mobile Chrome" at lines 38 and 44, but they use different device configurations (Pixel 7 and Pixel 5). This could cause confusion when selecting specific browser configurations or when reporting results.
{
name: 'Mobile Chrome',
use: { ...devices['Pixel 7'] },
},
/* Test against mobile viewports. */
{
- name: 'Mobile Chrome',
+ name: 'Mobile Chrome (Pixel 5)',
use: { ...devices['Pixel 5'] },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome (Pixel 5)', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, |
🤖 Prompt for AI Agents
In
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts
between lines 37 and 50, there are two projects named "Mobile Chrome" with
different device configurations (Pixel 7 and Pixel 5). Rename one of the
projects to a unique name, such as "Mobile Chrome Pixel 7" or "Mobile Chrome
Pixel 5," to avoid confusion in browser selection and test reporting.
| const playwrightCheck = new PlaywrightCheck(playwrightCheckProps.logicalId, { | ||
| ...playwrightCheckProps, | ||
| codeBundlePath: key, | ||
| browsers, | ||
| playwrightConfigPath: relativePlaywrightConfigPath, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Instantiated object is unused – triggers noUnusedLocals
playwrightCheck is created only for its side-effect (constructor registers the
construct). Assigning it to a variable that is never read will fail the
TypeScript build when noUnusedLocals is true.
- const playwrightCheck = new PlaywrightCheck(playwrightCheckProps.logicalId, {
+ new PlaywrightCheck(playwrightCheckProps.logicalId, {
...playwrightCheckProps,
codeBundlePath: key,
browsers,
playwrightConfigPath: relativePlaywrightConfigPath,
})Same applies to the else branch below.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const playwrightCheck = new PlaywrightCheck(playwrightCheckProps.logicalId, { | |
| ...playwrightCheckProps, | |
| codeBundlePath: key, | |
| browsers, | |
| playwrightConfigPath: relativePlaywrightConfigPath, | |
| }) | |
| } | |
| new PlaywrightCheck(playwrightCheckProps.logicalId, { | |
| ...playwrightCheckProps, | |
| codeBundlePath: key, | |
| browsers, | |
| playwrightConfigPath: relativePlaywrightConfigPath, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In packages/cli/src/services/project-parser.ts around lines 107 to 113, the
variable playwrightCheck is assigned but never used, causing a noUnusedLocals
TypeScript error. To fix this, remove the assignment and instantiate
PlaywrightCheck directly without assigning it to a variable in both the if and
else branches, since the constructor side-effect is the only needed action.
| export async function loadChecklyConfig ( | ||
| dir: string, | ||
| filenames = ['checkly.config.ts', 'checkly.config.mts', 'checkly.config.cts', 'checkly.config.js', 'checkly.config.mjs', 'checkly.config.cjs'], | ||
| ): Promise<{ config: ChecklyConfig, constructs: Construct[] }> { | ||
| Session.loadingChecklyConfigFile = true | ||
| Session.checklyConfigFileConstructs = [] | ||
| for (const filename of filenames) { | ||
| const filePath = path.join(dir, filename) | ||
| if (existsSync(filePath)) { | ||
| config = await loadFile(filePath) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| let config = await findConfigFile(dir, filenames) | ||
|
|
||
| if (!config) { | ||
| throw new ConfigNotFoundError(`Unable to locate a config at ${dir} with ${filenames.join(', ')}.`) | ||
| config = await handleMissingConfig(dir, filenames) | ||
| } | ||
|
|
||
| for (const field of ['logicalId', 'projectName']) { | ||
| const requiredField = config?.[field] | ||
| if (!requiredField || !(isString(requiredField))) { | ||
| throw new Error(`Config object missing a ${field} as type string`) | ||
| } | ||
| } | ||
| validateConfigFields(config, ['logicalId', 'projectName']) | ||
|
|
||
| const constructs = Session.checklyConfigFileConstructs | ||
| Session.loadingChecklyConfigFile = false | ||
| Session.checklyConfigFileConstructs = [] | ||
| return { config, constructs } |
There was a problem hiding this comment.
Fix type-narrowing to avoid a compile-time error
config is declared as ChecklyConfig | null, and TypeScript does not narrow the type after the if (!config) block.
Consequently validateConfigFields(config, …) produces:
Argument of type 'ChecklyConfig | null' is not assignable to parameter of type 'ChecklyConfig'.
Add an explicit guard (or non-null assertion) before validation:
if (!config) {
config = await handleMissingConfig(dir, filenames)
}
+ /* istanbul ignore next -- purely defensive, should never happen */
+ if (!config) {
+ throw new ConfigNotFoundError(
+ `Unable to locate a config at ${dir} with ${filenames.join(', ')}.`
+ )
+ }
+
validateConfigFields(config, ['logicalId', 'projectName'])This satisfies the compiler and prevents a possible undefined access path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function loadChecklyConfig ( | |
| dir: string, | |
| filenames = ['checkly.config.ts', 'checkly.config.mts', 'checkly.config.cts', 'checkly.config.js', 'checkly.config.mjs', 'checkly.config.cjs'], | |
| ): Promise<{ config: ChecklyConfig, constructs: Construct[] }> { | |
| Session.loadingChecklyConfigFile = true | |
| Session.checklyConfigFileConstructs = [] | |
| for (const filename of filenames) { | |
| const filePath = path.join(dir, filename) | |
| if (existsSync(filePath)) { | |
| config = await loadFile(filePath) | |
| break | |
| } | |
| } | |
| let config = await findConfigFile(dir, filenames) | |
| if (!config) { | |
| throw new ConfigNotFoundError(`Unable to locate a config at ${dir} with ${filenames.join(', ')}.`) | |
| config = await handleMissingConfig(dir, filenames) | |
| } | |
| for (const field of ['logicalId', 'projectName']) { | |
| const requiredField = config?.[field] | |
| if (!requiredField || !(isString(requiredField))) { | |
| throw new Error(`Config object missing a ${field} as type string`) | |
| } | |
| } | |
| validateConfigFields(config, ['logicalId', 'projectName']) | |
| const constructs = Session.checklyConfigFileConstructs | |
| Session.loadingChecklyConfigFile = false | |
| Session.checklyConfigFileConstructs = [] | |
| return { config, constructs } | |
| export async function loadChecklyConfig ( | |
| dir: string, | |
| filenames = [ | |
| 'checkly.config.ts', | |
| 'checkly.config.mts', | |
| 'checkly.config.cts', | |
| 'checkly.config.js', | |
| 'checkly.config.mjs', | |
| 'checkly.config.cjs', | |
| ], | |
| ): Promise<{ config: ChecklyConfig, constructs: Construct[] }> { | |
| Session.loadingChecklyConfigFile = true | |
| Session.checklyConfigFileConstructs = [] | |
| let config = await findConfigFile(dir, filenames) | |
| if (!config) { | |
| config = await handleMissingConfig(dir, filenames) | |
| } | |
| /* istanbul ignore next -- purely defensive, should never happen */ | |
| if (!config) { | |
| throw new ConfigNotFoundError( | |
| `Unable to locate a config at ${dir} with ${filenames.join(', ')}.` | |
| ) | |
| } | |
| validateConfigFields(config, ['logicalId', 'projectName']) | |
| const constructs = Session.checklyConfigFileConstructs | |
| Session.loadingChecklyConfigFile = false | |
| Session.checklyConfigFileConstructs = [] | |
| return { config, constructs } | |
| } |
🤖 Prompt for AI Agents
In packages/cli/src/services/checkly-config-loader.ts around lines 128 to 146,
the variable 'config' can be null, but it is passed directly to
'validateConfigFields' which expects a non-null 'ChecklyConfig'. To fix this,
add an explicit null check or use a non-null assertion on 'config' before
calling 'validateConfigFields' to ensure the compiler knows 'config' is not null
at that point.
packages/cli/src/services/util.ts
Outdated
| const pwConfigParsed = new PlaywrightConfig(dir, pwtConfig) | ||
|
|
||
| await loadPlaywrightProjectFiles(dir, pwConfigParsed, include, archive) | ||
| archive.file(playwrightConfig, { name: path.basename(playwrightConfig) }) | ||
| await archive.finalize() | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Archiving wrong file reference
archive.file(playwrightConfig …) points to the possibly relative argument that may not exist anymore
(after the fix above it is definitely wrong). Use the resolved filePath instead.
-archive.file(playwrightConfig, { name: path.basename(playwrightConfig) })
+archive.file(filePath, { name: path.basename(filePath) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pwConfigParsed = new PlaywrightConfig(dir, pwtConfig) | |
| await loadPlaywrightProjectFiles(dir, pwConfigParsed, include, archive) | |
| archive.file(playwrightConfig, { name: path.basename(playwrightConfig) }) | |
| await archive.finalize() | |
| return new Promise((resolve, reject) => { | |
| const pwConfigParsed = new PlaywrightConfig(dir, pwtConfig) | |
| await loadPlaywrightProjectFiles(dir, pwConfigParsed, include, archive) | |
| archive.file(filePath, { name: path.basename(filePath) }) | |
| await archive.finalize() | |
| return new Promise((resolve, reject) => { |
🤖 Prompt for AI Agents
In packages/cli/src/services/util.ts around lines 291 to 296, the archive.file
call uses the possibly relative variable playwrightConfig, which may not point
to the correct or existing file after previous changes. Replace playwrightConfig
with the resolved absolute filePath variable to ensure the correct file is
archived.
| const parser = new Parser({}) | ||
| const { files, errors } = await parser.getFilesAndDependencies(pwConfigParsed) | ||
| if (errors.length) { | ||
| throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loss of diagnostic details in dependency errors
errors.map((e: string) => e) coerces objects to '[object Object]'.
Surface the actual messages:
-throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`)
+throw new Error(`Error loading Playwright project files:\n${errors.map(e => (e as Error).message).join('\n')}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parser = new Parser({}) | |
| const { files, errors } = await parser.getFilesAndDependencies(pwConfigParsed) | |
| if (errors.length) { | |
| throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`) | |
| } | |
| const parser = new Parser({}) | |
| const { files, errors } = await parser.getFilesAndDependencies(pwConfigParsed) | |
| if (errors.length) { | |
| throw new Error(`Error loading Playwright project files:\n${errors.map(e => (e as Error).message).join('\n')}`) | |
| } |
🤖 Prompt for AI Agents
In packages/cli/src/services/util.ts around lines 311 to 315, the error handling
code maps errors to strings using errors.map((e: string) => e), which causes
objects to be coerced to '[object Object]', losing diagnostic details. To fix
this, update the mapping to extract and join the actual error messages, such as
using errors.map(e => e.message || String(e)) to ensure meaningful error
information is included in the thrown Error message.
| let dir = '' | ||
| try { | ||
| const { | ||
| outputFile, browsers, relativePlaywrightConfigPath, | ||
| } = await bundlePlayWrightProject(playwrightConfigPath, include) | ||
| dir = outputFile | ||
| const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(dir) | ||
| return { key, browsers, relativePlaywrightConfigPath } | ||
| } finally { | ||
| await cleanup(dir) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable dir shadows the temp file path & only deletes the file
dir first holds a directory name in other contexts, but here it becomes the tar.gz file path.
We delete the file but leave the temp directory behind. Rename for clarity and remove the folder:
-let dir = ''
+let bundlePath = ''
try {
const { outputFile, browsers, relativePlaywrightConfigPath } = await bundlePlayWrightProject(playwrightConfigPath, include)
- dir = outputFile
- const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(dir)
+ bundlePath = outputFile
+ const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(bundlePath)
return { key, browsers, relativePlaywrightConfigPath }
} finally {
- await cleanup(dir) // only removes the file
+ await cleanup(path.dirname(bundlePath)) // cleans up the whole tmp folder
}🤖 Prompt for AI Agents
In packages/cli/src/constructs/playwright-check.ts around lines 88 to 99, the
variable named 'dir' is misleading because it holds a tar.gz file path, not a
directory, causing only the file to be deleted while the temporary directory
remains. Rename this variable to reflect it is a file path, and update the
cleanup logic to remove both the tar.gz file and its containing temporary
directory to avoid leftover temp folders.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
packages/cli/src/services/util.ts (6)
12-12:⚠️ Potential issueIncorrect
archiverimport – will not compile
archiverusesexport =syntax; its typings do not export theArchivertype nor a namedcreatefunction.
Trying to import them withimport { Archiver, create } from 'archiver'breaks type-checking and, withesModuleInterop, also crashes at runtime.-import { Archiver, create } from 'archiver' +import archiver from 'archiver' +import type { Archiver } from 'archiver'…and later replace
create('tar', …)witharchiver('tar', …)(see next comment).
277-278:⚠️ Potential issue
filePathis computed incorrectly – duplicates the directory segment
diris already the absolute directory of the config file.
Doingpath.resolve(dir, playwrightConfig)appends the config path again, producing e.g.
/abs/e2e/e2e/playwright.config.ts.-const dir = path.resolve(path.dirname(playwrightConfig)) -const filePath = path.resolve(dir, playwrightConfig) +const filePath = path.resolve(playwrightConfig) +const dir = path.dirname(filePath)This bug prevents the config from being found and produces a wrong
relativePlaywrightConfigPath.
284-289:⚠️ Potential issueUse the correct
archiverfactoryAfter fixing the import, create the archive via the default function:
-const archive = create('tar', { +const archive = archiver('tar', {Failing to change this will throw
create is not a function.
299-299:⚠️ Potential issueArchiving wrong file reference
archive.file(playwrightConfig …)points to the possibly relative argument that may not exist anymore
(after the fix above it is definitely wrong). Use the resolvedfilePathinstead.-archive.file(playwrightConfig, { name: path.basename(playwrightConfig) }) +archive.file(filePath, { name: path.basename(filePath) })
350-352: 🛠️ Refactor suggestionLoss of diagnostic details in dependency errors
errors.map((e: string) => e)coerces objects to'[object Object]'.
Surface the actual messages:-throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`) +throw new Error(`Error loading Playwright project files:\n${errors.map(e => (e as Error).message).join('\n')}`)
387-392:⚠️ Potential issue
cleanupfails for directories and may leave tmp folders behind
fs.rm(dir)without options errors on non-empty directories; in our case the temp folder created by
fs.mkdtempremains.-return fs.rm(dir) +return fs.rm(dir, { recursive: true, force: true })Consider renaming the parameter to
targetPathto avoid confusion with directories vs. files.packages/cli/src/constructs/playwright-check.ts (2)
84-88:⚠️ Potential issueUnquoted config path & potential shell-escaping issues
--config ${playwrightConfigPath}breaks when the path contains spaces.
Quote the path and escape user-supplied values.-return `${testCommand} --config ${playwrightConfigPath}${playwrightProject?.length ? ' --project ' + playwrightProject.map(project => `"${project}"`).join(' ') : ''}${playwrightTag?.length ? ' --grep="' + playwrightTag.join('|') + '"' : ''}` +const quotedPath = `"${playwrightConfigPath}"` +const projectArg = playwrightProject?.length ? ' --project ' + playwrightProject.map(p => `"${p}"`).join(' ') : '' +const tagArg = playwrightTag?.length ? ' --grep "' + playwrightTag.join('|').replace(/"/g, '\\"') + '"' : '' +return `${testCommand} --config ${quotedPath}${projectArg}${tagArg}`
90-102:⚠️ Potential issueVariable
dirshadows the temp file path & only deletes the file
dirfirst holds a directory name in other contexts, but here it becomes the tar.gz file path.
We delete the file but leave the temp directory behind. Rename for clarity and remove the folder:-let dir = '' +let bundlePath = '' try { const { outputFile, browsers, relativePlaywrightConfigPath, cacheHash } = await bundlePlayWrightProject(playwrightConfigPath, include) - dir = outputFile - const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(dir) + bundlePath = outputFile + const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(bundlePath) return { key, browsers, relativePlaywrightConfigPath, cacheHash } } finally { - await cleanup(dir) // only removes the file + await cleanup(path.dirname(bundlePath)) // cleans up the whole tmp folder }
🧹 Nitpick comments (2)
packages/cli/src/constructs/playwright-check.ts (2)
45-53: Simplify array conversion logicThe repeated pattern for converting potential string or string[] props to arrays can be extracted to a helper function.
+ private toArray<T>(value?: T | T[]): T[] { + if (!value) return [] + return Array.isArray(value) ? value : [value] + } + constructor (logicalId: string, props: PlaywrightCheckProps) { // ... - this.pwProjects = props.pwProjects - ? (Array.isArray(props.pwProjects) ? props.pwProjects : [props.pwProjects]) - : [] - this.pwTags = props.pwTags - ? (Array.isArray(props.pwTags) ? props.pwTags : [props.pwTags]) - : [] - this.include = props.include - ? (Array.isArray(props.include) ? props.include : [props.include]) - : [] + this.pwProjects = this.toArray(props.pwProjects) + this.pwTags = this.toArray(props.pwTags) + this.include = this.toArray(props.include) // ... }
55-57: Use asynchronous file existence checkUsing synchronous
fs.existsSyncin a constructor can block the main thread. Consider restructuring to use async validation.- if (!fs.existsSync(props.playwrightConfigPath)) { - throw new ValidationError(`Playwright config doesnt exist ${props.playwrightConfigPath}`) - } + // Synchronous check in constructor for immediate validation + if (!fs.existsSync(props.playwrightConfigPath)) { + throw new ValidationError(`Playwright config doesn't exist: ${props.playwrightConfigPath}`) + }Additionally, fix the grammatical error by adding an apostrophe to "doesn't".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/cli/src/constructs/playwright-check.ts(1 hunks)packages/cli/src/services/project-parser.ts(4 hunks)packages/cli/src/services/util.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/services/project-parser.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: publish
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/cli/src/services/util.ts (5)
183-189:⚠️ Potential issueUse the correct
archiverfactory functionAfter fixing the import, you need to use the default export as a function instead of using
create.- const archive = create('tar', { + const archive = archiver('tar', { gzip: true, gzipOptions: { level: 9, }, })
286-291:⚠️ Potential issue
cleanupfails for directories and may leave tmp folders behindThe
fs.rm(dir)call without options will fail on non-empty directories, which could leave temporary folders behind.export function cleanup (dir: string) { if (!dir.length) { return } - return fs.rm(dir) + return fs.rm(dir, { recursive: true, force: true }) }Consider renaming the parameter from
dirtotargetPathfor clarity as this can be used for both files and directories.
198-198:⚠️ Potential issueArchiving wrong file reference
The
archive.filecall uses the possibly relativeplaywrightConfigpath, which may not point to the correct file. Use the resolvedfilePathinstead.- archive.file(playwrightConfig, { name: path.basename(playwrightConfig) }) + archive.file(filePath, { name: path.basename(filePath) })
10-10:⚠️ Potential issueFix incorrect
archiverimportThe current import statement for 'archiver' will not work because 'archiver' uses
export =syntax and doesn't export named members.-import { Archiver, create } from 'archiver' +import archiver from 'archiver' +import type { Archiver } from 'archiver'
247-251: 🛠️ Refactor suggestionLoss of diagnostic details in dependency errors
The current error mapping coerces error objects to strings, potentially losing diagnostic details.
- throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`) + throw new Error(`Error loading Playwright project files:\n${errors.map(e => (e as Error).message || String(e)).join('\n')}`)
🧹 Nitpick comments (2)
packages/cli/src/services/util.ts (2)
257-259: Improve globbing efficiencyThere's a TODO comment about combining multiple glob calls into a single one.
Consider consolidating these multiple glob patterns into a single call:
- // TODO: This code below should be a single glob - archive.glob('**/package*.json', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) - archive.glob('**/pnpm*.yaml', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) - archive.glob('**/yarn.lock', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) + // Package management files + archive.glob(['**/package*.json', '**/pnpm*.yaml', '**/yarn.lock'], { cwd: path.join(dir, '/'), ignore: ignoredFiles })
289-289: Improve early return handlingThis early return is missing the Promise return type, which could cause issues with async handling.
if (!dir.length) { - return + return Promise.resolve() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/cli/package.json(2 hunks)packages/cli/src/commands/test.ts(3 hunks)packages/cli/src/constructs/playwright-check.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(6 hunks)packages/cli/src/services/checkly-config-loader.ts(5 hunks)packages/cli/src/services/project-parser.ts(4 hunks)packages/cli/src/services/util.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli/package.json
- packages/cli/src/services/project-parser.ts
- packages/cli/src/commands/test.ts
- packages/cli/src/services/checkly-config-loader.ts
- packages/cli/src/services/check-parser/parser.ts
- packages/cli/src/constructs/playwright-check.ts
🧰 Additional context used
🪛 GitHub Actions: test
packages/cli/src/services/util.ts
[error] 178-178: TypeScript error TS2304: Cannot find name 'loadFile'.
🔇 Additional comments (1)
packages/cli/src/services/util.ts (1)
205-205:⚠️ Potential issueIncorrect relative path calculation
The
relativePlaywrightConfigPathcalculation is incorrect becausefilePathisn't properly resolved.- relativePlaywrightConfigPath: path.relative(dir, filePath), + relativePlaywrightConfigPath: path.basename(filePath),Likely an incorrect or invalid review comment.
packages/cli/src/services/util.ts
Outdated
| export async function bundlePlayWrightProject (playwrightConfig: string, include: string[]): | ||
| Promise<{outputFile: string, browsers: string[], relativePlaywrightConfigPath: string, cacheHash: string}> { | ||
| const dir = path.resolve(path.dirname(playwrightConfig)) | ||
| const filePath = path.resolve(dir, playwrightConfig) | ||
| const pwtConfig = await loadFile(filePath) |
There was a problem hiding this comment.
Missing loadFile function and incorrect path resolution
There are two issues here:
- The
loadFilefunction is not defined anywhere (causing the TS2304 error) - The path resolution is incorrect -
diris already the absolute directory, so resolving withplaywrightConfigagain duplicates the directory segment
export async function bundlePlayWrightProject (playwrightConfig: string, include: string[]):
Promise<{outputFile: string, browsers: string[], relativePlaywrightConfigPath: string, cacheHash: string}> {
- const dir = path.resolve(path.dirname(playwrightConfig))
- const filePath = path.resolve(dir, playwrightConfig)
- const pwtConfig = await loadFile(filePath)
+ const filePath = path.resolve(playwrightConfig)
+ const dir = path.dirname(filePath)
+ const pwtConfig = await fs.readFile(filePath, 'utf8')You need to either define a loadFile function or use fs.readFile directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function bundlePlayWrightProject (playwrightConfig: string, include: string[]): | |
| Promise<{outputFile: string, browsers: string[], relativePlaywrightConfigPath: string, cacheHash: string}> { | |
| const dir = path.resolve(path.dirname(playwrightConfig)) | |
| const filePath = path.resolve(dir, playwrightConfig) | |
| const pwtConfig = await loadFile(filePath) | |
| export async function bundlePlayWrightProject (playwrightConfig: string, include: string[]): | |
| Promise<{outputFile: string, browsers: string[], relativePlaywrightConfigPath: string, cacheHash: string}> { | |
| - const dir = path.resolve(path.dirname(playwrightConfig)) | |
| - const filePath = path.resolve(dir, playwrightConfig) | |
| - const pwtConfig = await loadFile(filePath) | |
| + const filePath = path.resolve(playwrightConfig) | |
| + const dir = path.dirname(filePath) | |
| + const pwtConfig = await fs.readFile(filePath, 'utf8') |
🧰 Tools
🪛 GitHub Actions: test
[error] 178-178: TypeScript error TS2304: Cannot find name 'loadFile'.
🤖 Prompt for AI Agents
In packages/cli/src/services/util.ts around lines 174 to 178, the function
loadFile is not defined, causing a TypeScript error, and the path resolution
duplicates directory segments by resolving an already absolute directory with
the config path again. Fix this by either defining the loadFile function or
replacing it with fs.readFile to load the file content, and correct the filePath
assignment by resolving only the playwrightConfig path relative to the directory
without duplicating the directory segment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/cli/src/services/util.ts (4)
200-200:⚠️ Potential issueUse resolved file path when archiving the config file
Using the original
playwrightConfigparameter can lead to issues as it might be a relative path that doesn't exist in the expected location for the archiver.Use the resolved
filePathinstead:- archive.file(playwrightConfig, { name: path.basename(playwrightConfig) }) + archive.file(filePath, { name: path.basename(filePath) })
252-252: 🛠️ Refactor suggestionImprove error message formatting for better diagnostics
The current error mapping coerces error objects to strings, which often results in unhelpful
'[object Object]'outputs.Enhance the error message formatting to extract and display useful error information:
- throw new Error(`Error loading playwright project files: ${errors.map((e: string) => e).join(', ')}`) + throw new Error(`Error loading Playwright project files:\n${errors.map(e => (e as Error).message || String(e)).join('\n')}`)This will provide more meaningful error details for debugging.
180-180:⚠️ Potential issueUse an existing file loading method instead of undefined
Session.loadFileThe function
Session.loadFileis not defined elsewhere in the codebase. This will cause a runtime error.Replace with an appropriate file loading mechanism:
- const pwtConfig = await Session.loadFile(filePath) + const pwtConfig = await fs.readFile(filePath, 'utf8')
178-180:⚠️ Potential issueFix path resolution to avoid directory duplication
The current implementation has a path resolution issue that would create duplicate directory segments. Currently
diris the absolute directory path, and thenfilePathresolves with that directory again plus the full config path.Modify the code to correctly resolve the file path:
- const dir = path.resolve(path.dirname(playwrightConfig)) - const filePath = path.resolve(dir, playwrightConfig) + const filePath = path.resolve(playwrightConfig) + const dir = path.dirname(filePath)This prevents paths like
/abs/e2e/e2e/playwright.config.tsfrom occurring.
🧹 Nitpick comments (4)
packages/cli/src/services/util.ts (4)
258-261: Consider consolidating package-related globsThe TODO comment suggests combining multiple package-related glob patterns, which would simplify the code.
Implement the suggested improvement in the TODO comment:
- // TODO: This code below should be a single glob - archive.glob('**/package*.json', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) - archive.glob('**/pnpm*.yaml', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) - archive.glob('**/yarn.lock', { cwd: path.join(dir, '/'), ignore: ignoredFiles }) + // Package manager dependency files + archive.glob('**/{package*.json,pnpm*.yaml,yarn.lock}', { cwd: path.join(dir, '/'), ignore: ignoredFiles })This simplifies the code and improves maintainability.
288-293: Improve parameter name for clarityThe parameter name
dirmight be confusing since it could refer to either a directory or a file path.Rename the parameter for clarity:
-export function cleanup (dir: string) { - if (!dir.length) { - return - } - return fs.rm(dir, { recursive: true, force: true }) +export function cleanup (targetPath: string) { + if (!targetPath.length) { + return + } + return fs.rm(targetPath, { recursive: true, force: true })This improves code readability by using a more descriptive parameter name.
295-316: Add options parameter for flexible default configurationThe default configuration is hardcoded with specific values that might not be suitable for all use cases.
Consider adding an options parameter to allow customization of default values:
-export function getDefaultChecklyConfig (directoryName: string, playwrightConfigPath: string): ChecklyConfig { +export function getDefaultChecklyConfig ( + directoryName: string, + playwrightConfigPath: string, + options?: { frequency?: number, locations?: string[], runLocation?: string } +): ChecklyConfig { + const frequency = options?.frequency ?? 10; + const locations = options?.locations ?? ['us-east-1']; + const runLocation = options?.runLocation ?? 'us-east-1'; + return { logicalId: directoryName, projectName: directoryName, checks: { playwrightConfigPath, playwrightChecks: [ { logicalId: directoryName, name: directoryName, - frequency: 10, - locations: ['us-east-1'], + frequency, + locations, }, ], - frequency: 10, - locations: ['us-east-1'], + frequency, + locations, }, cli: { - runLocation: 'us-east-1', + runLocation, }, }This provides more flexibility while maintaining the same defaults.
219-226: Add documentation for the cache hash functionThe
getCacheHashfunction is missing documentation to explain its purpose and usage.Add JSDoc comments to explain the function:
+/** + * Generates a SHA-256 hash of the project's package lock file. + * This hash is used to determine if dependencies have changed between deployments. + * + * @param dir The directory containing the lock file + * @returns A hex string representing the SHA-256 hash of the lock file + * @throws Error if no lock file is found + */ export async function getCacheHash (dir: string): Promise<string> { const lockFile = await findLockFile(dir) if (!lockFile) { throw new Error('No lock file found') } const fileBuffer = await readFile(lockFile); const hash = createHash('sha256'); hash.update(fileBuffer); return hash.digest('hex'); }This improves code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/util.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
| return resolve({ | ||
| outputFile, | ||
| browsers: pwConfigParsed.getBrowsers(), | ||
| relativePlaywrightConfigPath: path.relative(dir, filePath), |
There was a problem hiding this comment.
Fix relative path calculation for the Playwright config
Since the directory structure in the resolved path is incorrect, the relative path calculation will also be wrong.
After fixing the path resolution as suggested in the first comment, the relative path would be the basename since filePath is in dir. Consider simplifying:
- relativePlaywrightConfigPath: path.relative(dir, filePath),
+ relativePlaywrightConfigPath: path.basename(filePath),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| relativePlaywrightConfigPath: path.relative(dir, filePath), | |
| relativePlaywrightConfigPath: path.basename(filePath), |
🤖 Prompt for AI Agents
In packages/cli/src/services/util.ts at line 207, the relative path calculation
for the Playwright config is incorrect due to an earlier path resolution issue.
After fixing the path resolution, simplify the relative path calculation by
using the basename of filePath instead of path.relative(dir, filePath), since
filePath is located within dir.
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cli/src/services/checkly-config-loader.ts (1)
148-152:⚠️ Potential issueFix potential null reference before validation
The current code doesn't properly handle the case where
handleMissingConfigcould fail and leaveconfigasundefined. Although the function is designed to either return a valid config or throw an error, adding an explicit null check would be safer and satisfy the TypeScript compiler.Add an explicit null check before validation:
if (!config) { config = await handleMissingConfig(dir, filenames) } + /* istanbul ignore next -- purely defensive, should never happen */ + if (!config) { + throw new ConfigNotFoundError( + `Unable to locate a config at ${dir} with ${filenames.join(', ')}.` + ) + } + validateConfigFields(config, ['logicalId', 'projectName'] as const)
🧹 Nitpick comments (2)
packages/cli/src/services/checkly-config-loader.ts (2)
182-184: Simplify validation logicThe condition
!config?.[field] || !isString(config[field])has redundancy and a potential issue. Ifconfig[field]is falsy, the second part!isString(config[field])will still be evaluated, which could lead to errors ifconfig[field]is undefined or null. Additionally, the optional chaining in the first part but not in the second part is inconsistent.Simplify the condition:
for (const field of fields) { - if (!config?.[field] || !isString(config[field])) { + if (!config[field] || !isString(config[field])) { throw new Error(`Config object missing a ${field} as type string`) } }The optional chaining in
config?.[field]isn't necessary since we're already checking thatconfigis defined before calling this function.
174-178: Add caching for Playwright config path searchThe
findPlaywrightConfigPathfunction could benefit from caching its results, especially if it's called multiple times during CLI execution.Consider implementing a simple cache:
+ const playwrightConfigPathCache = new Map<string, string | undefined>(); + function findPlaywrightConfigPath (dir: string): string | undefined { + if (playwrightConfigPathCache.has(dir)) { + return playwrightConfigPathCache.get(dir); + } + - return ['playwright.config.ts', 'playwright.config.js'] + const result = ['playwright.config.ts', 'playwright.config.js'] .map(file => path.resolve(dir, file)) .find(filePath => existsSync(filePath)) + + playwrightConfigPathCache.set(dir, result); + return result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/checkly-config-loader.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/cli/src/services/checkly-config-loader.ts (4)
packages/cli/src/constructs/playwright-check.ts (1)
PlaywrightCheckProps(11-23)packages/cli/src/services/__tests__/fixtures/configs/no-export-config.js (1)
config(1-4)packages/cli/src/services/__tests__/fixtures/configs/no-logical-id-config.js (1)
config(1-3)packages/cli/src/services/util.ts (2)
getDefaultChecklyConfig(295-316)writeChecklyConfigFile(318-324)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (3)
packages/cli/src/services/checkly-config-loader.ts (3)
18-21: The newPlaywrightSlimmedProptype looks goodThe type captures all necessary properties from
PlaywrightCheckPropsand adds a requiredlogicalIdproperty. This ensures consistent configuration for Playwright checks.
69-81: The Playwright-specific additions toChecklyConfigare well-structuredThe new properties for Playwright support are well-documented with clear JSDoc comments explaining their purpose. The type definitions are also appropriate for the intended usage.
3-4: New imports align with added functionalityThe additional imports for filesystem operations and utility functions are appropriate for the new auto-configuration feature.
✅ Actions performedReviews paused. |
|
🎉 Experimental release successfully published on npm |
…(#1064)"
This reverts commit c5a47d6.
Affected Components
Notes for the Reviewer
New Dependency Submission