feat: add AI support structure and migrate to main branch#198
Conversation
mfilip
commented
May 29, 2026
mfilip
commented
- Add CLAUDE.md with project documentation
- Add .claude/skills for build, test, release workflows
- Update all references from master to main branch
- Configure semantic-release for main branch
- Update GitHub workflows for main branch
- Add CLAUDE.md with project documentation - Add .claude/skills for build, test, release workflows - Update all references from master to main branch - Configure semantic-release for main branch - Update GitHub workflows for main branch
WalkthroughThis PR migrates E2E testing from Cypress to Playwright (configs, scripts, devDependencies, new Playwright specs and shared helpers), updates CI and release workflows to trigger on main and use Node 24 (replacing the CI Cypress step with Playwright and wiring API_KEY), adds CLAUDE.md and SKILL.md documentation pages, updates .gitignore/.env.example/Makefile/README, and adds a registerEvent() fallback called after module install. 🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
PR Review: AI support structure + branch migrationThanks for setting up the AI assistant scaffolding. The CLAUDE.md and skill files are a useful addition. A few issues to flag, mostly around the branch migration half of this PR. 🚨 Blocking: branch rename is incompleteThe PR renames every reference from
Two safer options:
I'd recommend option 1 — it's cleaner and the transition window is short. Code quality / correctness
Performance / Security
Test coverage
Suggestions
Happy to re-review once the branch question is sorted out. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.claude/skills/release/SKILL.md (1)
24-24: 💤 Low valueCapitalise brand name.
"github" should be "GitHub" for the proper brand name.
♻️ Proposed fix
-- **git** - Commits release changes -- **github** - Creates GitHub release with `idealpostcodes.ocmod.zip` +- **git** - Commits release changes +- **GitHub** - Creates GitHub release with `idealpostcodes.ocmod.zip`🤖 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 @.claude/skills/release/SKILL.md at line 24, Update the markdown bullet in SKILL.md that currently reads "**github** - Creates GitHub release with `idealpostcodes.ocmod.zip`" to use the correct brand capitalization: change the bolded "github" token to "GitHub" (i.e., "**GitHub** - Creates GitHub release with `idealpostcodes.ocmod.zip`") so the brand name is capitalised consistently.CLAUDE.md (1)
19-27: 💤 Low valueAdd language specifier to fenced code block.
The code block would benefit from a language specifier for consistency with Markdown best practices. Consider adding
textorplaintext.♻️ Proposed fix
-``` +```text lib/ # TypeScript source (compiled by Rollup) src/🤖 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 `@CLAUDE.md` around lines 19 - 27, The fenced code block in CLAUDE.md showing the directory tree lacks a language specifier; update that triple-backtick fence to include a language (e.g., use ```text or ```plaintext) so Markdown renderers treat it as plain text—edit the fenced block containing "lib/ # TypeScript source (compiled by Rollup)" through "docker/ # Docker setup scripts" and add the language token immediately after the opening ``` to apply the change.
🤖 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/ci.yml:
- Line 9: The pull_request trigger in the GitHub Actions workflow currently only
lists 'main', which will skip CI for PRs targeting 'master'; update the
pull_request branches list in .github/workflows/ci.yml to include both 'main'
and 'master' (e.g., add '- master' alongside '- main') so PR CI runs for either
branch during migration, and plan to remove 'master' from that list after
cutover.
In @.github/workflows/release.yml:
- Line 6: Update the GitHub Actions trigger so releases run from both branches
during the rename: in the workflow's push/branches configuration (the entry
currently listing "main"), add "master" alongside "main" so the release job
triggers for either branch until the repo rename is finished; ensure the
branches array includes both "main" and "master" and keep any existing workflow
names and jobs intact.
---
Nitpick comments:
In @.claude/skills/release/SKILL.md:
- Line 24: Update the markdown bullet in SKILL.md that currently reads
"**github** - Creates GitHub release with `idealpostcodes.ocmod.zip`" to use the
correct brand capitalization: change the bolded "github" token to "GitHub"
(i.e., "**GitHub** - Creates GitHub release with `idealpostcodes.ocmod.zip`") so
the brand name is capitalised consistently.
In `@CLAUDE.md`:
- Around line 19-27: The fenced code block in CLAUDE.md showing the directory
tree lacks a language specifier; update that triple-backtick fence to include a
language (e.g., use ```text or ```plaintext) so Markdown renderers treat it as
plain text—edit the fenced block containing "lib/ # TypeScript source
(compiled by Rollup)" through "docker/ # Docker setup scripts" and add
the language token immediately after the opening ``` to apply the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1d9a873-6c9d-4793-9beb-175e787a8c2d
📒 Files selected for processing (11)
.claude/settings.json.claude/skills/build/SKILL.md.claude/skills/release/SKILL.md.claude/skills/test/SKILL.md.github/workflows/ci.yml.github/workflows/release.yml.gitignoreCLAUDE.mdMakefileREADME.mdrelease.config.js
- Update CI workflow to use Node.js 24.x - Update release workflow to use Node.js 24.x - Remove .claude/settings.json configuration file - Clarify bundle command requires build step first - Improve security documentation for Twig json_encode filter usage
Review — feat: add AI support structure and migrate to main branchThanks for putting the AI tooling and skills together — the That said, the Blocking — branch rename will break CI and releases on mergeThe PR base is
Recommended order of operations:
If you can't reorder, a safer interim is to keep the workflow triggers listing both branches ( Medium
Nits
Security & Test coverage
SummaryThe AI scaffolding looks great and is accurate to the codebase. The branch-rename half needs to be sequenced behind a GitHub-side default-branch rename (or made bi-branch tolerant) before merging, otherwise this PR will silently break releases and CI triggers. |
- Replace Cypress with Playwright test framework - Update test configuration and file structure - Move tests from cypress/e2e to tests/ directory - Update CI workflow to use Playwright with chromium browser - Replace cypress.config.ts with playwright.config.ts - Update npm scripts for Playwright commands (ui, headed, playwright) - Add .env.example for API key configuration - Update documentation and skill files to reflect Playwright usage - Remove
PR Review — feat: add AI support structure and migrate to main branchThanks for the migration. The Cypress→Playwright port and Bugs
{ path: "idealpostcodes.ocmod.zip", label: "..." }
await page.route('https://api.ideal-postcodes.co.uk/v1/keys/*', route => route.continue());
await page.goto('/index.php?route=checkout/checkout');
await page.waitForResponse('https://api.ideal-postcodes.co.uk/v1/keys/*');The const keysResponse = page.waitForResponse('https://api.ideal-postcodes.co.uk/v1/keys/*');
await page.goto('/index.php?route=checkout/checkout');
await keysResponse;
const apiKey = process.env.API_KEY || '';If the secret is missing in CI, the test will fill the API-key field with an empty string and fail later in a confusing way (likely on the verify-config step). Fail fast at module scope:
let token: string;
...
test.beforeEach(async ({ page, baseURL }) => {
if (token) return;
});This works today only because Code quality / consistency
The
Mixed concerns in one PR. SecurityNo new concerns. Performance
Test coverageCoverage is preserved at parity with the Cypress suite (admin config navigation + checkout autocomplete/postcode lookup). No regression, no expansion. Neither suite covers the uninstall path or the OpenCart 4 install/upgrade flow exercised in Net: directionally good, but I would want the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
tests/admin.spec.ts (3)
58-58: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Using
waitForTimeout(2000)introduces flakiness. Prefer waiting for specific conditions:// Wait for network idle after dropdown selection await page.waitForLoadState('networkidle'); // or wait for specific element that appears after selection await page.waitForSelector('expected-element-selector');🤖 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 `@tests/admin.spec.ts` at line 58, Replace the non-deterministic sleep call page.waitForTimeout(2000) with a deterministic wait: after the dropdown selection or action that triggers the change, use page.waitForLoadState('networkidle') or await page.waitForSelector('expected-element-selector') (replace with the actual selector that signals completion) so the test waits for the specific network or DOM condition instead of a fixed timeout.
77-77: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Using
waitForTimeout(1000)introduces flakiness. Wait for the save operation to complete by checking for success indicators:// Wait for success message or page reload await page.waitForLoadState('networkidle'); // or wait for toast/alert await expect(page.locator('.alert-success')).toBeVisible();🤖 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 `@tests/admin.spec.ts` at line 77, Replace the flaky explicit sleep call using page.waitForTimeout(1000) with deterministic waits that assert the actual save completion: remove page.waitForTimeout and instead wait for network idle via page.waitForLoadState('networkidle') or assert a visible success indicator using expect(page.locator('.alert-success')).toBeVisible(), targeting the same test step where the save action occurs (look for usages around page.waitForTimeout and the save action).
26-26: ⚡ Quick winToken reuse depends on single-worker configuration.
The token caching with
if (token) return;relies on tests running sequentially (workers: 1 in config). If the worker count increases, each worker will need to authenticate separately, and this shared token approach will fail.Consider documenting this dependency or using Playwright's project setup to authenticate once and reuse the state across tests.
🤖 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 `@tests/admin.spec.ts` at line 26, The guard "if (token) return;" in the tests relies on a single shared in-memory token and will break with multiple Playwright workers; replace this pattern by either documenting the single-worker requirement or, preferably, remove the global token cache and authenticate using Playwright's supported mechanisms — e.g., perform login in a globalSetup or use Playwright projects/storageState or a worker-scoped fixture so each worker obtains and reuses its own auth state (replace the shared "token" guard with per-worker storageState or a worker-scoped login helper).tests/helpers/suite.ts (6)
11-14: 💤 Low value
normalizeCityonly capitalises the first character.The function converts to lowercase then capitalises only the first character, which won't handle multi-word city names (e.g., "ST HELIER" → "St helier" instead of "St Helier").
If the test fixtures already use proper casing and OpenCart only modifies the first letter, this is fine. Otherwise, consider proper title-casing:
const normalizeCity = (city: string): string => { return city.toLowerCase().replace(/\b\w/g, c => c.toUpperCase()); };🤖 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 `@tests/helpers/suite.ts` around lines 11 - 14, normalizeCity currently lowercases the whole string then only capitalises the first character, which fails for multi-word city names; update the normalizeCity implementation to title-case each word (e.g., use a regex replacement like replace(/\b\w/g, c => c.toUpperCase()) or split/map/join) so "ST HELIER" becomes "St Helier" while still normalising case consistently.
51-51: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Instead of waiting 1 second after country selection, wait for the form to be ready:
await page.waitForLoadState('networkidle'); // or wait for specific country-dependent elements to update🤖 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 `@tests/helpers/suite.ts` at line 51, Replace the non-deterministic sleep at page.waitForTimeout(1000) in tests/helpers/suite.ts with a deterministic wait: remove the waitForTimeout call and instead wait for a reliable readiness signal after the country selection (e.g., call page.waitForLoadState('networkidle') or page.waitForSelector for the specific country-dependent element or form field that updates). Ensure the replacement targets the same test flow where country selection occurs so functions that rely on the updated form state proceed only after the actual UI/network work completes.
68-68: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Instead of waiting 1 second after clicking the lookup button, wait for the results to populate:
await page.waitForSelector('.idpc-select-container select option:not([value=""])', { state: 'attached' });🤖 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 `@tests/helpers/suite.ts` at line 68, Replace the brittle sleep in tests/helpers/suite.ts (the await page.waitForTimeout(1000) call) with a deterministic wait that waits for the lookup results to render; specifically, after triggering the lookup action use page.waitForSelector to wait for '.idpc-select-container select option:not([value=""])' to be attached so the test proceeds only when options are populated.
64-64: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Instead of waiting 1 second after country selection, wait for the country-dependent form updates:
await page.waitForLoadState('networkidle');🤖 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 `@tests/helpers/suite.ts` at line 64, The hard-coded sleep using page.waitForTimeout(1000) is flaky; replace the call to page.waitForTimeout in the test helper with a deterministic wait such as awaiting the page network idle state: use await page.waitForLoadState('networkidle') (or alternatively waitForResponse/waitForSelector for the specific country-dependent element) instead of page.waitForTimeout to ensure the form updates are complete after country selection.
42-42: 💤 Low valueDocument the hard-coded country code.
The country code
'257'is hard-coded but its meaning (presumably Great Britain in OpenCart's system) is not obvious. Consider adding a comment:await expect(scope.locator(selectors.country)).toHaveValue('257'); // GBR in OpenCart🤖 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 `@tests/helpers/suite.ts` at line 42, The assertion uses a hard-coded country code '257' which is unclear; update the test at the expect(scope.locator(selectors.country)).toHaveValue('257') site to clarify the value by either replacing '257' with a named constant (e.g., DEFAULT_COUNTRY_GBR) or adding an inline comment such as "// GBR in OpenCart" so future readers understand that '257' corresponds to Great Britain; touch the expect call, the selectors.country reference, and any nearby test setup to use the constant if you choose that approach.
54-54: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.Instead of waiting 2 seconds for autocomplete suggestions, wait for the suggestions to appear:
await page.waitForSelector('.idpc_ul li', { state: 'visible' });This is more reliable and often faster than arbitrary timeouts.
🤖 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 `@tests/helpers/suite.ts` at line 54, Replace the arbitrary sleep call using page.waitForTimeout(2000) with a deterministic wait for the autocomplete suggestions to become visible: use Playwright's waitForSelector (or locator.waitFor) to wait for the suggestion list selector ('.idpc_ul li') to be visible before proceeding; update the code path that calls page.waitForTimeout to instead await page.waitForSelector('.idpc_ul li', { state: 'visible' }) so tests become deterministic and faster.tests/checkout.spec.ts (1)
27-27: ⚡ Quick winReplace
waitForTimeoutwith deterministic waits.After clicking "Guest Checkout" and verifying the shipping address is visible, waiting 1 second is arbitrary. Consider waiting for the form to be ready for interaction:
await expect(page.locator('`#shipping-address` input').first()).toBeEnabled();🤖 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 `@tests/checkout.spec.ts` at line 27, The current arbitrary sleep using page.waitForTimeout(1000) should be replaced with a deterministic wait so the test proceeds only when the shipping form is ready; remove the waitForTimeout call and instead assert the shipping form input is interactive by awaiting the page locator for '`#shipping-address` input' to be enabled/visible (use the Playwright expect matcher such as toBeEnabled/toBeVisible) after the "Guest Checkout" click and shipping address visibility check, so subsequent interactions are stable and deterministic.playwright.config.ts (1)
16-16: ⚡ Quick winConsider making baseURL configurable.
The baseURL is hardcoded to
http://localhost:3000. Consider using an environment variable to allow different environments:baseURL: process.env.BASE_URL || 'http://localhost:3000',This would align with the existing pattern of using
.envfor configuration.🤖 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 `@playwright.config.ts` at line 16, Replace the hardcoded baseURL string in playwright.config.ts with a value read from the environment (use environment variable BASE_URL with a fallback to 'http://localhost:3000') so tests can run against different environments; update the baseURL assignment in the export (the baseURL property) and ensure the process.env is populated (import or initialize dotenv or import 'dotenv/config' near the top of the config file if not already).
🤖 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 `@tests/admin.spec.ts`:
- Line 3: The parameter type for the helper function activateExtension is too
permissive: replace the any type on the page parameter with the Playwright Page
type; import Page from '`@playwright/test`' at the top of the file and update the
function signature (activateExtension(page: Page, token: string) => ...) and any
other local uses that declared page as any to use Page for proper type safety
and autocompletion.
---
Nitpick comments:
In `@playwright.config.ts`:
- Line 16: Replace the hardcoded baseURL string in playwright.config.ts with a
value read from the environment (use environment variable BASE_URL with a
fallback to 'http://localhost:3000') so tests can run against different
environments; update the baseURL assignment in the export (the baseURL property)
and ensure the process.env is populated (import or initialize dotenv or import
'dotenv/config' near the top of the config file if not already).
In `@tests/admin.spec.ts`:
- Line 58: Replace the non-deterministic sleep call page.waitForTimeout(2000)
with a deterministic wait: after the dropdown selection or action that triggers
the change, use page.waitForLoadState('networkidle') or await
page.waitForSelector('expected-element-selector') (replace with the actual
selector that signals completion) so the test waits for the specific network or
DOM condition instead of a fixed timeout.
- Line 77: Replace the flaky explicit sleep call using page.waitForTimeout(1000)
with deterministic waits that assert the actual save completion: remove
page.waitForTimeout and instead wait for network idle via
page.waitForLoadState('networkidle') or assert a visible success indicator using
expect(page.locator('.alert-success')).toBeVisible(), targeting the same test
step where the save action occurs (look for usages around page.waitForTimeout
and the save action).
- Line 26: The guard "if (token) return;" in the tests relies on a single shared
in-memory token and will break with multiple Playwright workers; replace this
pattern by either documenting the single-worker requirement or, preferably,
remove the global token cache and authenticate using Playwright's supported
mechanisms — e.g., perform login in a globalSetup or use Playwright
projects/storageState or a worker-scoped fixture so each worker obtains and
reuses its own auth state (replace the shared "token" guard with per-worker
storageState or a worker-scoped login helper).
In `@tests/checkout.spec.ts`:
- Line 27: The current arbitrary sleep using page.waitForTimeout(1000) should be
replaced with a deterministic wait so the test proceeds only when the shipping
form is ready; remove the waitForTimeout call and instead assert the shipping
form input is interactive by awaiting the page locator for '`#shipping-address`
input' to be enabled/visible (use the Playwright expect matcher such as
toBeEnabled/toBeVisible) after the "Guest Checkout" click and shipping address
visibility check, so subsequent interactions are stable and deterministic.
In `@tests/helpers/suite.ts`:
- Around line 11-14: normalizeCity currently lowercases the whole string then
only capitalises the first character, which fails for multi-word city names;
update the normalizeCity implementation to title-case each word (e.g., use a
regex replacement like replace(/\b\w/g, c => c.toUpperCase()) or split/map/join)
so "ST HELIER" becomes "St Helier" while still normalising case consistently.
- Line 51: Replace the non-deterministic sleep at page.waitForTimeout(1000) in
tests/helpers/suite.ts with a deterministic wait: remove the waitForTimeout call
and instead wait for a reliable readiness signal after the country selection
(e.g., call page.waitForLoadState('networkidle') or page.waitForSelector for the
specific country-dependent element or form field that updates). Ensure the
replacement targets the same test flow where country selection occurs so
functions that rely on the updated form state proceed only after the actual
UI/network work completes.
- Line 68: Replace the brittle sleep in tests/helpers/suite.ts (the await
page.waitForTimeout(1000) call) with a deterministic wait that waits for the
lookup results to render; specifically, after triggering the lookup action use
page.waitForSelector to wait for '.idpc-select-container select
option:not([value=""])' to be attached so the test proceeds only when options
are populated.
- Line 64: The hard-coded sleep using page.waitForTimeout(1000) is flaky;
replace the call to page.waitForTimeout in the test helper with a deterministic
wait such as awaiting the page network idle state: use await
page.waitForLoadState('networkidle') (or alternatively
waitForResponse/waitForSelector for the specific country-dependent element)
instead of page.waitForTimeout to ensure the form updates are complete after
country selection.
- Line 42: The assertion uses a hard-coded country code '257' which is unclear;
update the test at the
expect(scope.locator(selectors.country)).toHaveValue('257') site to clarify the
value by either replacing '257' with a named constant (e.g.,
DEFAULT_COUNTRY_GBR) or adding an inline comment such as "// GBR in OpenCart" so
future readers understand that '257' corresponds to Great Britain; touch the
expect call, the selectors.country reference, and any nearby test setup to use
the constant if you choose that approach.
- Line 54: Replace the arbitrary sleep call using page.waitForTimeout(2000) with
a deterministic wait for the autocomplete suggestions to become visible: use
Playwright's waitForSelector (or locator.waitFor) to wait for the suggestion
list selector ('.idpc_ul li') to be visible before proceeding; update the code
path that calls page.waitForTimeout to instead await
page.waitForSelector('.idpc_ul li', { state: 'visible' }) so tests become
deterministic and faster.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88782c3c-442a-46d5-8a01-cff35b07cc77
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.claude/skills/test/SKILL.md.env.example.github/workflows/ci.yml.gitignoreCLAUDE.mdcypress.config.tscypress/e2e/00_admin.tscypress/e2e/01_checkout.tscypress/fixtures/example.jsoncypress/integration/01_checkout.tscypress/plugins/index.tscypress/support/commands.tscypress/support/e2e.tscypress/support/index.d.tscypress/support/index.tscypress/support/suite.tscypress/tsconfig.jsonpackage.jsonplaywright.config.tstests/admin.spec.tstests/checkout.spec.tstests/helpers/suite.ts
💤 Files with no reviewable changes (12)
- cypress/tsconfig.json
- cypress/integration/01_checkout.ts
- cypress/support/e2e.ts
- cypress/support/index.d.ts
- cypress/plugins/index.ts
- cypress/support/index.ts
- cypress/fixtures/example.json
- cypress/e2e/00_admin.ts
- cypress/support/commands.ts
- cypress.config.ts
- cypress/e2e/01_checkout.ts
- cypress/support/suite.ts
✅ Files skipped from review due to trivial changes (3)
- .env.example
- CLAUDE.md
- .claude/skills/test/SKILL.md
- Update CLAUDE.md to reference Playwright tests instead of Cypress - Add API_KEY validation in admin.spec.ts to fail fast if missing - Replace arbitrary timeouts with explicit visibility waits - Improve test reliability with proper wait conditions - Add event registration backup in docker setup script - Clean up release.config.js comments and asset path - Downgrade dotenv from ^17.4.2 to ~16.4.5
PR ReviewThanks for the migration work. This PR is doing substantially more than the description suggests — alongside the CLAUDE.md + main-branch updates, it migrates the E2E suite from Cypress to Playwright, bumps Node 18 → 24 in CI, and changes the docker setup script. Worth calling out in the description so reviewers know what they're looking at. 🐛 Bugs / correctness1.
2. \"test\": \"npm start && npm run playwright && make down\"With \"test\": \"npm start && (npm run playwright; status=$?; make down; exit $status)\"or move teardown to a Playwright 3. Redundant event registration in
If a workaround is genuinely needed, please add a comment explaining the failure mode (with a linked issue) and a TODO to remove it. 4.
Only ⚙️ CI / build5. Node 18 → 24 jump is unflagged (medium) 6. Playwright browser cache not cached in CI (low) - run: npx playwright install --with-deps chromiumEvery CI run re-downloads the Chromium build. Consider an 🧪 Tests7. Module-scoped login token in let token: string;
test.beforeEach(async ({ page }) => {
if (token) return;
...
});The comment correctly acknowledges this only works because of 8. 9. return lowerCase.charAt(0).toUpperCase() + lowerCase.slice(1);Breaks on multi-word cities ("New York" → "New york"). Carried over from the Cypress version, so out of scope, but worth a follow-up. 10. Top-level if (!process.env.API_KEY) {
throw new Error('API_KEY environment variable is required');
}This fires during file collection, which means a single missing env var aborts the whole test run rather than failing one suite. Prefer 🔐 Security11. CLAUDE.md note on Twig
Twig's 📝 Process12. PR description omits major scope (process) Overall: the Playwright migration looks well-executed, the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker/run-setup.php (1)
339-348: ⚡ Quick winConsider adding verification after insertion.
The
addModuleToListfunction (lines 267-283) follows a pattern of verifying successful insertion by querying the inserted row. Applying the same pattern here would improve consistency and provide better confidence that the event was registered successfully.♻️ Proposed verification pattern
"); $eventId = $db->getLastId(); - echo "Event registered with ID: {$eventId}\n"; + + if ($eventId) { + echo "Event registered with ID: {$eventId}\n"; + + // Verify the entry was added + $verify_query = $db->query("SELECT * FROM `" . DB_PREFIX . "event` WHERE `event_id` = '" . (int)$eventId . "'"); + if ($verify_query->num_rows) { + $row = $verify_query->row; + echo "Verified in database: ID={$row['event_id']}, code={$row['code']}\n"; + } else { + echo "ERROR: Could not verify the inserted event despite having an ID!\n"; + } + } else { + echo "ERROR: Failed to register event! No ID returned.\n"; + } } else {🤖 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 `@docker/run-setup.php` around lines 339 - 348, After inserting the event and getting $eventId, perform a verification query similar to addModuleToList: select the row from the event table using the returned $eventId and confirm a result exists; if no row is found, log an error and handle/fail appropriately. Update the insertion block that creates the ukaddresssearch event (the INSERT and $eventId retrieval) to include a follow-up SELECT on DB_PREFIX . "event" for that $eventId, check the result, and echo or process a failure path when verification fails.
🤖 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 `@tests/checkout.spec.ts`:
- Around line 24-25: Replace the brittle sleep call (page.waitForTimeout) with a
deterministic wait for the postcode widget initialization: remove the await
page.waitForTimeout(2000) and instead wait for the selector '`#shipping-address`
.idpc_lookup[idpc="true"]' to be visible (use Playwright's
expect(locator).toBeVisible with an increased timeout, e.g. 15000) so the test
reliably proceeds only after the extension widget has marked the context as
initialized.
---
Nitpick comments:
In `@docker/run-setup.php`:
- Around line 339-348: After inserting the event and getting $eventId, perform a
verification query similar to addModuleToList: select the row from the event
table using the returned $eventId and confirm a result exists; if no row is
found, log an error and handle/fail appropriately. Update the insertion block
that creates the ukaddresssearch event (the INSERT and $eventId retrieval) to
include a follow-up SELECT on DB_PREFIX . "event" for that $eventId, check the
result, and echo or process a failure path when verification fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93276171-fe9e-4961-b1ad-ce46ab37d60b
📒 Files selected for processing (7)
CLAUDE.mddocker/run-setup.phppackage.jsonrelease.config.jstests/admin.spec.tstests/checkout.spec.tstests/helpers/suite.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/helpers/suite.ts
- package.json
- Replace 2s timeout with explicit wait for .idpc_lookup element - Increase timeout to 15s for widget injection - Improve test reliability with proper wait condition - Update comment to clarify widget is hidden until UK selected
PR Review: Cypress → Playwright migration + main-branch renameOverall this is a clean, well-scoped infrastructure migration. The Playwright ports preserve test semantics, add reasonable timeouts in place of 🐛 Likely bugs / behavior changes
🔒 Security
⚙️ Quality / consistency
⚡ Performance
🧪 Test coverage
✨ Nice touches
Recommendation: approve once the — Reviewed by Claude (claude-opus-4-7) |
PR Review: feat: add AI support structure and migrate to main branchThanks for the thorough Cypress → Playwright migration and the Blockers / Likely Bugs1. Test coverage regression: billing-address checkout was not migratedThe old 2.
|
- Add Node.js version requirement (>=24.0.0) to package.json - Simplify dotenv path configuration in Playwright config - Add null check for user_token extraction in admin tests - Refactor address line assertions with clearer control flow and comments - Expand registerEvent() documentation explaining test bootstrap workaround - Fix .gitignore trailing newline
PR Review: feat: add AI support structure and migrate to main branchThanks for this work — the Cypress → Playwright migration is well-scoped and the AI tooling additions look clean. A few things worth considering before merging. Code quality & best practices
Bugs / risk areas
Performance
Security
Test coverage
Minor
SummaryThe migration itself is sound. Main asks before merge:
Nice work overall — the Playwright code is more readable than the Cypress equivalent. |
- Add development section to README with CI and testing information - Document Blacksmith runner usage and fallback behavior - Replace absolute href selectors with suffix selectors for better reliability - Extract OpenCart country IDs as named constants (COUNTRY_UK, COUNTRY_JERSEY) - Add inline documentation for country ID values from oc_country table - Reuse extensionLink locator to reduce duplication in admin tests
PR Review: feat: add AI support structure and migrate to main branchThanks for the substantial migration work — moving Cypress → Playwright cleanly plus standardising on Bugs / correctness
Code quality
CI / workflow
Test coverageThe migration is roughly 1:1 with the previous Cypress suite, but a few gaps are worth noting (not necessarily blocking):
Security
No security concerns blocking merge. Performance
SummarySolid migration overall. Before merging I'd prioritise: (a) the cleanup-on-failure issues in 🤖 Generated with Claude Code |
- Add always() cleanup step to CI workflow to ensure Docker teardown - Fix test script to preserve exit status during cleanup - Add worker parallelism validation in Playwright config - Improve security documentation for JSON data in templates - Add error handling to event registration in Docker setup - Update comments to clarify sequential test execution requirements
Code ReviewThanks for the migration — this is a thorough sweep across CI, branch naming, test framework, and tooling docs. Below are the issues I think are worth addressing before merge, ordered by impact. High impact1. Billing checkout test was dropped in the migration (test coverage regression).
The new Playwright suite only ports the shipping version ( 2. Implicit cross-spec ordering dependency.
3. if (process.env.PW_WORKERS && parseInt(process.env.PW_WORKERS) > 1) {
throw new Error('Parallel workers not supported: ...');
}Playwright doesn't read Medium impact4. Docs disagree on OpenCart version. 5.
6. Race in admin save verification. await page.locator('button.btn.btn-primary[title="Save"]').click();
await expect(extensionLink).toBeVisible({ timeout: 10000 });
await extensionLink.click();If the save fails (validation error, server error), the form page often stays open — and the form page may also contain 7. Case-sensitivity workaround in $fullModulePath = str_replace('Idealpostcodes', 'idealpostcodes', $fullModulePath);This patches the symptom. Worth tracking down where the capital- Low impact / nits8. 9. Token-sharing pattern in let token: string;
test.beforeEach(async ({ page }) => {
if (token) return;
// login...
});This works today because there's only one test, but the canonical Playwright equivalent is 10. CI doesn't cache Playwright browsers. 11. - path: ["idealpostcodes.ocmod.zip"],
+ path: "idealpostcodes.ocmod.zip",
12. Unscoped autocomplete dropdown selector (pre-existing, carried over from Cypress). Positives worth calling out
Happy to dig into any of these further if helpful. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docker/run-setup.php`:
- Around line 352-367: The current check only looks up by `code =
'ukaddresssearch'` and skips rows that exist even if their `trigger`,`action` or
`status` are wrong; modify the logic around the `$query` result for the DB table
(`DB_PREFIX . "event"`) to be self-healing: if `$query->num_rows` is zero INSERT
the correct row (as you already do and capture `$eventId = $db->getLastId()`),
but if a row exists compare its `trigger`, `action`, `status` (and
`description`/`sort_order` if desired) and perform an UPDATE to set the correct
values when they differ; ensure you update the existing row via an UPDATE query
using the existing `$query->row['event_id']`, and echo whether you inserted,
updated, or left the event unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31ec595e-1d9b-4626-ba56-15a00b0b8fc5
📒 Files selected for processing (6)
.github/workflows/ci.ymlCLAUDE.mddocker/run-setup.phppackage.jsonplaywright.config.tstests/admin.spec.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yml
- playwright.config.ts
- package.json
- tests/admin.spec.ts
- Add browser caching to CI workflow with cache key based on package-lock.json - Split browser installation into cached and dependency-only steps - Move OpenCart bootstrap after browser installation for better workflow order - Update README to reflect OpenCart 4.x testing - Enhance event registration to update existing rows if values differ - Add expected values documentation in registerEvent() function - Simplify Playwright config by removing validation code
PR Review: feat: add AI support structure and migrate to main branchThanks for this work — the Cypress → Playwright migration is a solid modernization and the AI scaffolding/docs are useful. Below are concerns grouped by impact. Headline issues are (1) and (3). 1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ci.yml:
- Line 27: Replace the mutable tag "uses: actions/cache@v4" with a pinned commit
SHA for the actions/cache action; locate the uses: actions/cache@v4 entry in the
CI workflow and change the reference to the full commit SHA (e.g.,
actions/cache@<full-commit-sha>) so the workflow is immutable and meets stricter
supply-chain policies.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa2c1f88-6097-4e08-be06-2ad6418418b5
📒 Files selected for processing (6)
.github/workflows/ci.ymlREADME.mddocker/run-setup.phpplaywright.config.tstests/admin.spec.tstests/helpers/suite.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/helpers/suite.ts
1 participant
