Skip to content

Conversation

@htdat
Copy link
Contributor

@htdat htdat commented Oct 31, 2025

Description

Testing instructions

Checklist

  • npm run test:php does not return errors.
  • npm run lint:php does not return errors.

Summary by CodeRabbit

  • Documentation

    • Updated Quick Start guide with revised integration path
    • Added instructions to enable advanced checkout capabilities through feature flag configuration
  • Chores

    • Optimized build process for production dependency management

@coderabbitai
Copy link

coderabbitai bot commented Oct 31, 2025

Walkthrough

This PR updates documentation guidance in the README to reference Agentic Commerce and adds a feature flag instruction, while modifying the production build script to install dependencies to a temporary directory before relocating them to the final build output.

Changes

Cohort / File(s) Summary
Documentation updates
README.md
Updated Step 4 in Quick Start to point to Agentic Commerce instead of OpenAI integrations path; added note documenting how to enable the agentic_checkout Woo core feature flag via CLI.
Build script modifications
bin/build-prod-zip.sh
Modified Step 3 to install production dependencies into a separate ./vendor_prod directory, then move the vendor directory to its final location in build/${PLUGIN_SLUG}/vendor, decoupling installation from the final build directory structure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • README.md: Verify the Agentic Commerce path reference is accurate and the feature flag name and CLI command syntax are correct.
  • bin/build-prod-zip.sh: Confirm the temporary directory approach (./vendor_prod) works reliably across different environments and that the move operation preserves all dependency files correctly.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description consists entirely of the empty template structure with no actual content provided by the author. The "Description" section contains only the template comment with no summary of changes, the "Testing instructions" section is completely empty, and the checklist items remain unchecked. While the template structure is followed, all required content sections are missing, leaving no meaningful information about what was changed, why it was changed, or how to test it.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Fix error when installing vendor dir directly to build" directly addresses the primary technical change in the PR, which is the modification to bin/build-prod-zip.sh that fixes how production dependencies are installed by using a separate vendor_prod directory before moving to the final build path. While the PR also includes README.md changes about Agentic Commerce, the title accurately captures the core fix without being misleading or off-topic. The guidance states titles don't need to cover every detail of the changeset, so focusing on the main technical fix is appropriate.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/vendor-build-folder

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@htdat htdat marked this pull request as ready for review October 31, 2025 09:21
@htdat
Copy link
Contributor Author

htdat commented Oct 31, 2025

Alright, the zip here looks good https://github.com/woocommerce/OpenAI-Product-Feed/actions/runs/18968180054

The current build on trunk https://github.com/woocommerce/OpenAI-Product-Feed/actions/runs/18967626322 is giving fatal error. I think that's because the relative path when installing vendor in the build directly.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
bin/build-prod-zip.sh (1)

29-30: Add cleanup trap for temporary vendor_prod directory on error.

If the build script exits unexpectedly (e.g., move fails at line 30), the temporary ./vendor_prod directory will be left behind. Consider adding an error trap to clean it up.

Add a cleanup trap at the top of the script (after set -e):

 set -e
+
+# Cleanup trap to remove temporary vendor directory on script exit/error
+trap "rm -rf ./vendor_prod" EXIT

This ensures ./vendor_prod is removed whether the script succeeds or fails, keeping the working directory clean.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31744e0 and 75c6ee4.

📒 Files selected for processing (2)
  • README.md (1 hunks)
  • bin/build-prod-zip.sh (1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md

23-23: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)

🔇 Additional comments (1)
bin/build-prod-zip.sh (1)

29-30: Good fix for decoupling vendor installation from build directory.

The change to install dependencies into a temporary vendor_prod directory before moving to the final location cleanly resolves the vendor installation issue. This approach avoids potential path/permission problems when installing directly to the final build directory.

@htdat
Copy link
Contributor Author

htdat commented Oct 31, 2025

FYI, I am going to merge this right away @RadoslavGeorgiev

@htdat htdat merged commit 44500f0 into trunk Oct 31, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants