Skip to content

Used shared Koenig file upload hook#26626

Merged
EvanHahn merged 5 commits intomainfrom
evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility
Mar 2, 2026
Merged

Used shared Koenig file upload hook#26626
EvanHahn merged 5 commits intomainfrom
evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility

Conversation

@EvanHahn
Copy link
Contributor

closes https://linear.app/ghost/issue/NY-1097

This change should have no user impact. It's just a cleanup.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da83040 and 9058e15.

📒 Files selected for processing (3)
  • apps/admin-x-design-system/src/utils/format-url.ts
  • ghost/admin/app/components/koenig-lexical-editor.js
  • ghost/admin/package.json

Walkthrough

This pull request refactors the admin application's file upload handling and updates project dependencies. The koenig-lexical-editor component's custom file upload logic is replaced with external hooks from a dedicated module, reducing code complexity. An import statement is updated to include a .js file extension for ES module resolution. A new devDependency for the admin-x-framework package is added to package.json, and the nx build configuration is updated to include this framework in the build:dev, build, and test dependency chains.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Used shared Koenig file upload hook' directly and clearly summarizes the main change: replacing the in-file custom file upload implementation with an external shared hook, which is the primary objective of this PR.
Description check ✅ Passed The description references the closed issue (NY-1097) and accurately characterizes the change as a cleanup with no user impact, which aligns with the changeset showing internal refactoring of file upload logic.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility

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.

@EvanHahn EvanHahn force-pushed the evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility branch from 2d11a58 to 38086dc Compare February 28, 2026 16:32
EvanHahn added a commit that referenced this pull request Feb 28, 2026
ref #26626
ghost-admin now imports @tryghost/admin-x-framework/hooks, which resolves via ember-auto-import to dist/hooks.cjs. Ensure build, build:dev, and test run @tryghost/admin-x-framework:build first so dist artifacts exist in CI.
@@ -1,4 +1,4 @@
import isEmail from 'validator/es/lib/isEmail';
import isEmail from 'validator/es/lib/isEmail.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I admit I'm not certain why this is required.

I think this is because validator/es/lib/isEmail isn't directly exported from the validator package, and we need to make sure it's resolving a file.

This change could be done separately, but it only started causing problems after this PR was made.

Let me know if you want me to understand this further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per Claude analysis, it's because Admin uses webpack v5 which uses fullySpecified: true so we need the extension.

EvanHahn added 5 commits March 1, 2026 08:18
closes https://linear.app/ghost/issue/NY-1097

This change should have no user impact. It's just a cleanup.
ref #26626
ghost-admin now imports @tryghost/admin-x-framework/hooks, which resolves via ember-auto-import to dist/hooks.cjs. Ensure build, build:dev, and test run @tryghost/admin-x-framework:build first so dist artifacts exist in CI.
ref https://linear.app/tryghost/issue/NY-1097/

The Ghost admin build resolves this module via webpack's strict ESM rules in CI, which require fully specified deep imports. Adding the .js extension keeps the existing @tryghost/admin-x-framework/hooks usage intact while making CI module resolution deterministic.
@EvanHahn EvanHahn force-pushed the evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility branch from f82bdfa to 9058e15 Compare March 1, 2026 14:18
@EvanHahn EvanHahn marked this pull request as ready for review March 1, 2026 14:46
@EvanHahn EvanHahn requested a review from 9larsons March 2, 2026 12:58
@EvanHahn EvanHahn merged commit 1aa3c66 into main Mar 2, 2026
30 checks passed
@EvanHahn EvanHahn deleted the evanhahn-ny-1097-move-koeniglexicaleditors-usefileupload-into-its-own-utility branch March 2, 2026 18:25
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