Skip to content

Conversation

@GrabowskiM
Copy link
Contributor

🎫 Issue IBX-10173

Description:

That's why I vouched for not using path.resolve in requires - changes from root dir broke this bundle...

Also, most of these changes are only because of linter/prettier

For QA:

Documentation:

@GrabowskiM GrabowskiM requested review from a team and Copilot July 10, 2025 09:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR moves webpack configuration logic out of the project root into the @ibexa/frontend-config package, refactors the custom Encore setup into a reusable function, and applies linter/prettier-driven formatting fixes.

  • Swap root‐relative requires for the new frontend‐config manager package
  • Wrap the Encore build steps in a getCustomConfig function and export it
  • Clean up formatting in JS code and add the new frontend‐config devDependency

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/bundle/Resources/public/js/CKEditor/embed/image/embed-image-editing.js Removed // eslint-disable-next-line and adjusted blank line before innerHTML.
src/bundle/Resources/encore/ibexa.webpack.custom.config.js Replaced path.resolve manager require, refactored into getCustomConfig, and updated Encore chaining.
package.json Added @ibexa/frontend-config to devDependencies.
Comments suppressed due to low confidence (2)

src/bundle/Resources/public/js/CKEditor/embed/image/embed-image-editing.js:153

  • [nitpick] Removing this // eslint-disable-next-line may cause lint errors on the next line. Re-add it or update the ESLint config to allow this inline HTML construction.
                    // note: do not reformat - configuration value for image embeds cannot contain whitespaces

src/bundle/Resources/encore/ibexa.webpack.custom.config.js:5

  • [nitpick] Continuing to use path.resolve for this require can reintroduce root-path issues. Consider importing via a package alias or a relative path to maintain consistency.
const configManagers = require(path.resolve('./var/encore/ibexa.richtext.config.manager.js'));


configManagers.forEach((configManagerPath) => {
const configManager = require(path.resolve(configManagerPath));
Encore.reset();
Copy link

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

The second Encore.reset() right before returning customConfig will clear all previously chained settings, leading to an empty or default config. Remove this reset or relocate it.

Suggested change
Encore.reset();

Copilot uses AI. Check for mistakes.
"dependencies": {},
"devDependencies": {
"@ibexa/eslint-config": "https://github.com/ibexa/eslint-config-ibexa.git#~v2.0.0",
"@ibexa/frontend-config": "https://github.com/ibexa/frontend-config#^v5.0.0-beta1",
Copy link

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

The git URL fragment #^v5.0.0-beta1 mixes a caret with a git ref, which may not be supported by all package managers. Consider using a branch name (e.g. #v5.0.0-beta1) or a semver tag.

Suggested change
"@ibexa/frontend-config": "https://github.com/ibexa/frontend-config#^v5.0.0-beta1",
"@ibexa/frontend-config": "https://github.com/ibexa/frontend-config#v5.0.0-beta1",

Copilot uses AI. Check for mistakes.
@dew326 dew326 merged commit 10124ef into main Jul 10, 2025
16 of 17 checks passed
@dew326 dew326 deleted the IBX-10173 branch July 10, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants