House Keeping#74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors the event-info block editor to run saveEventDates (the /sync call) before savePost via a document-level publish/update click interceptor, removes unused 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/e2e-tests.yml (1)
31-31: ⚡ Quick winConsider removing
--forceflag from npm ci.The
--forceflag bypasses dependency conflict checks and can mask genuine dependency issues. Unless there's a specific reason to use it, prefernpm ciwithout--forcefor more reliable builds.📦 Recommended change
- run: npm ci --force + run: npm ci🤖 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 @.github/workflows/e2e-tests.yml at line 31, The CI step using the run command currently invokes "npm ci --force"; remove the "--force" flag so the workflow step runs "npm ci" instead to allow npm to surface dependency conflicts; update the run: entry in the e2e-tests workflow (the run command shown in the diff) accordingly.
🤖 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/e2e-tests.yml:
- Line 18: The PHP version in the GitHub Actions workflow is inconsistent with
the dev config: update the workflow's php-version key (the entry labeled
"php-version: '8.2'") to match the project's PHP requirement (8.3) so CI uses
PHP 8.3 like .wp-env.json and composer.json; ensure any other workflow matrix
entries referencing PHP are also changed to '8.3' to keep CI and local
environments synchronized.
In `@tests/e2e/specs/editor/event-dates-block.spec.js`:
- Around line 23-38: The test currently only checks lockState.blockCount so it
doesn't verify removal locking; update the assertion to also verify
lockState.templateLock to match the test intent (e.g.
expect(lockState.templateLock).toBeTruthy() or
expect(lockState.templateLock).toEqual('all') ) after calling
openNewEvent(page). Locate the lockState object gathered via
window.wp.data.select('core/block-editor').getSettings() in the test and add the
templateLock assertion (or an explicit non-removability check against the blocks
returned by window.wp.data.select('core/block-editor').getBlocks()) to ensure
the event-info block cannot be removed.
In `@tests/e2e/specs/editor/event-dates-save.spec.js`:
- Around line 72-79: The two assertions referencing net.counts.sync and
net.counts.postSave in the event-dates-save test are known failing repros;
quarantine them so CI stays green by skipping the containing test block (use
it.skip or describe.skip) or conditionally returning early based on a quarantine
flag, and add a short TODO comment pointing to the production fix; specifically
locate the assertions that call expect( net.counts.sync ).toBe( 1 ) and expect(
net.counts.postSave ).toBe( 1 ) and wrap/disable their enclosing it/describe so
they do not run until the fix is merged.
---
Nitpick comments:
In @.github/workflows/e2e-tests.yml:
- Line 31: The CI step using the run command currently invokes "npm ci --force";
remove the "--force" flag so the workflow step runs "npm ci" instead to allow
npm to surface dependency conflicts; update the run: entry in the e2e-tests
workflow (the run command shown in the diff) accordingly.
🪄 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: 1d421493-25d2-401a-aa20-fa89b8d0e0d3
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.claude/settings.json.github/workflows/e2e-tests.yml.gitignore.wp-env.jsoncomposer.jsonpackage.jsonphpunit.xml.distsrc/blocks/event-info/index.jssrc/classes/class-se-event-post-type.phptests/e2e/.env.exampletests/e2e/fixtures/event-editor.jstests/e2e/fixtures/index.jstests/e2e/global-setup.jstests/e2e/playwright.config.jstests/e2e/specs/editor/event-dates-block.spec.jstests/e2e/specs/editor/event-dates-regressions.spec.jstests/e2e/specs/editor/event-dates-reload.spec.jstests/e2e/specs/editor/event-dates-save.spec.jstests/phpunit/EventDates/EventDatesCleanupTest.phptests/phpunit/Migration/EventMigrationTest.phptests/phpunit/Query/EventQueryUtilsTest.phptests/phpunit/Smoke/SampleTest.phptests/phpunit/Templates/TemplateFunctionsTest.php
1 participant
Changes proposed in this Pull Request
This pull request introduces several infrastructure and configuration updates, alongside documentation for a planned fix to the event dates dirty-flag bug. The main highlights are the addition of an E2E test workflow using Playwright, updates to PHP version requirements, and a detailed technical plan for addressing a persistent editor dirty-state issue. No production code changes are included yet for the event dates fix; only the fix plan is documented.
Testing and CI improvements:
.github/workflows/e2e-tests.ymlto automate end-to-end (E2E) testing with Playwright, including environment setup, dependency installation, and artifact upload on failure.package.jsonto add Playwright and related scripts for running E2E tests locally and in CI. [1] [2]Environment and dependency updates:
.wp-env.jsonandcomposer.json, and set Composer's platform PHP version accordingly. [1] [2]composer.jsondevelopment dependencies section.testdoxoutput in PHPUnit configuration for improved test readability.Permissions and configuration:
.claude/settings.jsonto explicitly allow web fetches fromapi.github.com.Documentation:
event-dates-fix-plan.md, a comprehensive technical plan for fixing the event dates dirty-flag bug in the editor, including root cause analysis, rejected alternatives, proposed solution, edge cases, and testing strategy.Testing instructions
Manual Testing — Event Dates Dirty-State Fix & Migration Version Stamp
End-user QA steps for the two fixes in this branch. Follow in a browser; check each ✅ expectation.
Prerequisites
build/):http://localhost:8888(loginadmin/password). If testing on devilbox instead, use that URL — behaviour is identical.Fix #1 — Dirty-state leak (event-info block)
Test A — First publish + reload (the core repro)
Test B — Edit + save again (the "always dirty" repro)
…/event-dates/{id}/syncand one to…/wp/v2/se-event/{id}— not 3× and 2× like before.Test C — Abandon changes (draft semantic must still hold)
Fix #2 — Migration version stamping
Test D — New event is not flagged for migration
Test E — Save with NO date interaction (the original intermittent bug)
/syncnever fires) → Publish.Verify the meta directly
For any event ID, check the stamped version:
✅ Expect: prints
2.0.0(the currentSE_MIGRATION_VERSION) for any newly created event — including ones where you never added a date.Test F — Legacy events still migrate (regression guard)
Hard to stage by hand (needs a genuine pre-2.0 event with no version meta). Covered by the automated test
EventMigrationTest("A genuinely legacy event…"). If you have a real legacy event: it should still show in the migration queue and migrate normally when run — the new stamp only affects events created after this fix.If anything doesn't match the ✅ expectations, note the step number and what you saw.
Mentions #
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Style