Feature/stricter excluded link enforcement#342
Conversation
…sion rule. Also prevents exlcluded links being handled on the front end
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request adds comprehensive support for distinguishing between manual and system-set link exclusions, then conditionally lifts only system exclusions when links are successfully repaired. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
🧹 Nitpick comments (3)
.github/workflows/e2e-tests.yml (2)
30-31: ⚡ Quick winConsider removing
--forceflag fromnpm ci.The
--forceflag can mask dependency conflicts and version mismatches that should be resolved. In CI environments, it's better to fail fast on dependency issues rather than force installation.♻️ Proposed change
- name: Install npm dependencies - 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 around lines 30 - 31, Remove the --force flag from the npm install step so CI fails on real dependency issues: update the workflow step named "Install npm dependencies" to run "npm ci" (remove the "--force" from the command "npm ci --force") to allow the job to surface dependency conflicts and version mismatches instead of masking them.
13-60: ⚖️ Poor tradeoffConsider pinning GitHub Actions to commit SHAs for improved security.
Static analysis flagged that actions are referenced by tag (
@v4,@v2) rather than commit SHA. While tag-based versions are more maintainable, SHA pinning provides stronger supply-chain security by preventing tag movements. This is a project policy decision.🤖 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 around lines 13 - 60, Replace tag-based action refs with pinned commit SHAs: change uses: actions/checkout@v4, shivammathur/setup-php@v2, actions/setup-node@v4, and actions/upload-artifact@v4 to their corresponding repository commit SHAs (e.g., uses: actions/checkout@<commit-sha>) so the workflow is immutably pinned; update each "uses:" line in the workflow to the exact SHA for that action, and add a short comment beside each pinned ref noting the original tag (for maintainability) and a link or note on how to update the SHA in future.composer.json (1)
41-41: ⚡ Quick winConsider a more specific version constraint.
Using
"*"allows any version ofvlucas/phpdotenv, including future major releases with breaking changes. While this is a dev dependency, a breaking change can still disrupt development and CI workflows.📦 Recommended version constraint
- "vlucas/phpdotenv": "*", + "vlucas/phpdotenv": "^5.5",Alternatively, if you need v6.x features, use
"^6.0".🤖 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 `@composer.json` at line 41, The composer.json dependency "vlucas/phpdotenv" currently uses a wildcard "*" which permits any version; update the version constraint to a safer, more specific range (for example "^6.0" or another targeted major like "^5.5") to avoid pulling breaking changes—edit the composer.json entry for "vlucas/phpdotenv" to replace "*" with the chosen semver constraint and run composer update/install to verify compatibility.
🤖 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 `@src/Link/Link.php`:
- Around line 217-226: The is_manual_exclusion() method relies on the
user-facing string 'User Requested To Exclude' in $this->message which is
fragile; change the implementation to use a stable machine-readable marker or
dedicated flag instead: add a constant (e.g. MANUAL_EXCLUSION_MARKER) or a
boolean property (e.g. $this->manual_excluded) and set that marker in
Report_Page::handle_link_details_form() when a user manually excludes a link;
update is_manual_exclusion() to check $this->is_excluded && (marker presence or
$this->manual_excluded) and keep display text separate (use the
'internet-archive-wayback-machine-link-fixer' text domain for any translatable
UI string).
---
Nitpick comments:
In @.github/workflows/e2e-tests.yml:
- Around line 30-31: Remove the --force flag from the npm install step so CI
fails on real dependency issues: update the workflow step named "Install npm
dependencies" to run "npm ci" (remove the "--force" from the command "npm ci
--force") to allow the job to surface dependency conflicts and version
mismatches instead of masking them.
- Around line 13-60: Replace tag-based action refs with pinned commit SHAs:
change uses: actions/checkout@v4, shivammathur/setup-php@v2,
actions/setup-node@v4, and actions/upload-artifact@v4 to their corresponding
repository commit SHAs (e.g., uses: actions/checkout@<commit-sha>) so the
workflow is immutably pinned; update each "uses:" line in the workflow to the
exact SHA for that action, and add a short comment beside each pinned ref noting
the original tag (for maintainability) and a link or note on how to update the
SHA in future.
In `@composer.json`:
- Line 41: The composer.json dependency "vlucas/phpdotenv" currently uses a
wildcard "*" which permits any version; update the version constraint to a
safer, more specific range (for example "^6.0" or another targeted major like
"^5.5") to avoid pulling breaking changes—edit the composer.json entry for
"vlucas/phpdotenv" to replace "*" with the chosen semver constraint and run
composer update/install to verify compatibility.
🪄 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: 452d6a4f-3772-4cbc-9cf4-b615126297f0
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.distignore.github/workflows/e2e-tests.yml.gitignore.wp-env.jsonREADME.mdcomposer.jsone2e/.env.examplee2e/fixtures/seed-exclusion-bug.phpe2e/global-setup.jse2e/playwright.config.jse2e/specs/link-exclusion.spec.jspackage.jsonsrc/Action/Link_Latest_Snapshot_Action.phpsrc/Event/Check_Snapshot_Status_Event.phpsrc/Event/Check_Validator_Status.phpsrc/Link/Link.phpsrc/Link/Link_Repository.phptests/Action/Test_Link_Latest_Snapshot_Action.phptests/Event/Test_Check_Snapshot_Status_Event.phptests/Event/Test_Check_Validator_Status.phptests/Link/Test_Link.phptests/Link/Test_Link_Repository.php
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
1 participant
Changes proposed in this Pull Request
This pull request introduces a comprehensive end-to-end (E2E) test harness for the plugin, adds Playwright-based E2E testing infrastructure, and implements a regression test for the per-link "Exclude this link" feature. It also refines exclusion logic so that only system-set (not manual) exclusions are automatically lifted when a link is fixed, and ensures that the front-end JS correctly ignores manually excluded links. Additionally, several developer experience and dependency updates are included.
E2E Test Infrastructure and Regression Coverage:
e2e/directory, including configuration, environment setup, and a regression test for per-link exclusion. This includes.wp-env.jsonfor local WordPress environments, Playwright config, and supporting fixtures. (.wp-env.json,e2e/global-setup.js,e2e/playwright.config.js,e2e/fixtures/seed-exclusion-bug.php,e2e/specs/link-exclusion.spec.js, [1] [2] [3] [4] [5].github/workflows/e2e-tests.yml, .github/workflows/e2e-tests.ymlR1-R60).distignoreto exclude E2E-only files from deploys. (.distignore, .distignoreR27-R32)package.jsonwith E2E test scripts and new dev dependencies. (package.json, [1] [2]README.md. (README.md, README.mdL12-R15)Exclusion Logic Improvements:
src/Action/Link_Latest_Snapshot_Action.php, [1];src/Event/Check_Snapshot_Status_Event.php, [2];src/Event/Check_Validator_Status.php, [3]is_manual_exclusion()method to theLinkmodel to distinguish manual from system exclusions. (src/Link/Link.php, src/Link/Link.phpR217-R227)Link Filtering and Front-End Behavior:
Link_Repository::get_links_for_post()to ensure per-link exclusions are always honored, so excluded links are not sent to the front-end JS for rewriting. (src/Link/Link_Repository.php, src/Link/Link_Repository.phpL367-R376)Dependency and Maintenance Updates:
composer.jsonto allow any version ofvlucas/phpdotenv. (composer.json, composer.jsonL41-R41).env.examplefor E2E test environment variables. (e2e/.env.example, e2e/.env.exampleR1-R5)These changes significantly improve test coverage for exclusion logic, prevent regressions, and enhance the reliability of both the code and the development workflow.
Testing instructions
Mentions #
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores