Conversation
WalkthroughThis pull request refactors the module structure within the admin-x-framework package by reorganizing the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. 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 (2)
apps/admin-x-framework/src/hooks.ts (1)
1-1: LGTM — consider also exportingRequestOptionsfor TypeScript consumers.The new
useFetchApiexport looks correct. Optionally, exportingRequestOptionsalongside it would let TypeScript callers annotate their options without reaching into an internal module path.♻️ Suggested addition
-export {useFetchApi} from './hooks/use-fetch-api'; +export {useFetchApi} from './hooks/use-fetch-api'; +export type {RequestOptions} from './hooks/use-fetch-api';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-framework/src/hooks.ts` at line 1, Add a type-only re-export for RequestOptions from the use-fetch-api module so consumers can import it from the public barrel; specifically, in the file that currently exports useFetchApi (export {useFetchApi} from './hooks/use-fetch-api';) add a type export like export type { RequestOptions } from './hooks/use-fetch-api' (or combine into a single export statement) to expose the RequestOptions TypeScript type alongside useFetchApi.ghost/admin/app/components/koenig-lexical-editor.js (1)
546-549: Upload progress is now binary (0 → 100) — XHR progress events removed.The previous Ember AJAX implementation tracked incremental upload progress via XHR events and reflected it through
progressTracker. Withfetch, there are no upload progress events, soprogressTrackeris only updated to 100% upon completion. Large file uploads will show no progress until fully sent.This is a known UX regression; flagging it for completeness given the
progressTrackerinfrastructure is still in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/components/koenig-lexical-editor.js` around lines 546 - 549, The upload currently uses fetchApi (call around the upload: const uploadResponse = await fetchApi(...)) which provides only terminal progress (0→100) because fetch lacks upload progress events; replace this fetch-based upload with an XMLHttpRequest-based upload that wires xhr.upload.onprogress to update the existing progressTracker incrementally and resolves/rejects with the same response handling as uploadResponse so downstream code is unchanged; specifically, implement an XHR POST/PUT matching requestMethod, send fileFormData, update progressTracker during xhr.upload.onprogress, and on load/err parse the response so callers of the upload behave the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/components/koenig-lexical-editor.js`:
- Around line 575-576: Update the stale inline comment that reads "fall back to
EmberData/ember-ajax default message for error type" next to the conditional
checking for message (the if (!message) { block) to reference the current upload
implementation (useFetchApi) and the Fetch API/response default message instead;
locate the comment in koenig-lexical-editor.js near the if (!message) { branch
and replace the text to indicate fallback to the Fetch API/default response
message or the useFetchApi helper.
---
Nitpick comments:
In `@apps/admin-x-framework/src/hooks.ts`:
- Line 1: Add a type-only re-export for RequestOptions from the use-fetch-api
module so consumers can import it from the public barrel; specifically, in the
file that currently exports useFetchApi (export {useFetchApi} from
'./hooks/use-fetch-api';) add a type export like export type { RequestOptions }
from './hooks/use-fetch-api' (or combine into a single export statement) to
expose the RequestOptions TypeScript type alongside useFetchApi.
In `@ghost/admin/app/components/koenig-lexical-editor.js`:
- Around line 546-549: The upload currently uses fetchApi (call around the
upload: const uploadResponse = await fetchApi(...)) which provides only terminal
progress (0→100) because fetch lacks upload progress events; replace this
fetch-based upload with an XMLHttpRequest-based upload that wires
xhr.upload.onprogress to update the existing progressTracker incrementally and
resolves/rejects with the same response handling as uploadResponse so downstream
code is unchanged; specifically, implement an XHR POST/PUT matching
requestMethod, send fileFormData, update progressTracker during
xhr.upload.onprogress, and on load/err parse the response so callers of the
upload behave the same.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/admin-x-framework/src/api/current-user.tsapps/admin-x-framework/src/hooks.tsapps/admin-x-framework/src/hooks/use-fetch-api.tsapps/admin-x-framework/src/hooks/use-filterable-api.tsapps/admin-x-framework/src/utils/api/hooks.tsapps/admin-x-framework/test/unit/hooks/use-fetch-api.test.tsxapps/admin-x-framework/test/unit/hooks/use-filterable-api.test.tsghost/admin/app/components/koenig-lexical-editor.jsghost/admin/package.json
| // fall back to EmberData/ember-ajax default message for error type | ||
| if (!message) { |
There was a problem hiding this comment.
Stale comment references EmberData/ember-ajax.
The comment says "fall back to EmberData/ember-ajax default message for error type" but the upload now uses useFetchApi. Update to reflect the actual fallback source.
✏️ Proposed fix
- // fall back to EmberData/ember-ajax default message for error type
+ // fall back to the error's own message (e.g. from fetchApi/APIError)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // fall back to EmberData/ember-ajax default message for error type | |
| if (!message) { | |
| // fall back to the error's own message (e.g. from fetchApi/APIError) | |
| if (!message) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/admin/app/components/koenig-lexical-editor.js` around lines 575 - 576,
Update the stale inline comment that reads "fall back to EmberData/ember-ajax
default message for error type" next to the conditional checking for message
(the if (!message) { block) to reference the current upload implementation
(useFetchApi) and the Fetch API/response default message instead; locate the
comment in koenig-lexical-editor.js near the if (!message) { branch and replace
the text to indicate fallback to the Fetch API/default response message or the
useFetchApi helper.
EvanHahn
force-pushed
the
useFetchApi-in-koenig-file-upload-http-request
branch
from
3dadea5 to
368cc70
Compare
towards https://linear.app/ghost/issue/NY-1075 This change should have limited user impact. I want to move `useFileUpload` to a separate file. To do this, I need to remove its dependency on Ember's AJAX service. That meant exporting `useFetchApi` from `admin-x-framework`, depending on it there, and adding a new progress feature it needs. IMO, the "root" of the change is in `koenig-lexical-editor.js`. I'd start there if reviewing this patch.
EvanHahn
force-pushed
the
useFetchApi-in-koenig-file-upload-http-request
branch
from
368cc70 to
872324f
Compare
EvanHahn
added
ok to merge for me
1 participant
towards https://linear.app/ghost/issue/NY-1097
ref #26591
ref #26593
This change should have limited user impact.
I want to move
useFileUploadto a separate file. To do this, I need to remove its dependency on Ember's AJAX service. That meant exportinguseFetchApifromadmin-x-frameworkand depending on it there.IMO, the "root" of the change is in
koenig-lexical-editor.js. I'd start there if reviewing this patch.useFetchApiis slightly different from Ember's AJAX service, but it's our closest analog in the React world.