Port restore posts page to React#28767
Open
PedroMarianoAlmeida wants to merge 11 commits into
Open
Conversation
- first step of porting /restore from Ember to React: lock in the current behavior with a Playwright suite before any React code exists, so the same suite can re-validate the React port later - covers the seeded-revision list (title + Restore button), the restore click showing the success notification, and the empty state - seeds revisions straight into localStorage (load -> setItem -> reload) since that is how the Ember editor populates them; reuses the existing data-test-id selectors so one suite can target both implementations Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This creates a temporary React-owned restore path so the migration can be built and verified without displacing the existing Ember /restore route.
Add the React restore route probe and extract local revision reads into a tested localStorage module for the Ember-to-React migration.
Render local restore revisions in the React route using Shade components and add RTL coverage for empty and populated states.
Replaces the temporary custom restore revision list with shade table primitives, constrains the restore table to the members page width, and matches the Members Created column date and relative-age styling.
Add the temporary Restore button handler for the selected local revision and cover the click path in the route test.
Remove the Ember restore-posts route, template, controller, and acceptance test now that the React /restore route owns the canonical path.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request migrates the Ghost Admin "Restore Posts" feature from Ember.js to React. On the Ember side, the Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/admin/src/restore/local-revisions.test.ts (1)
42-61: ⚡ Quick winAdd malformed JSON coverage for revision keys.
Please add a regression test where a
post-revision-*key contains invalid JSON and assertfindAll()skips it instead of throwing.Suggested test addition
+ it("ignores malformed JSON for revision keys", () => { + window.localStorage.setItem("post-revision-bad-1000", "{bad-json"); + window.localStorage.setItem("post-revision-good-1001", JSON.stringify({ + id: "good", + revisionTimestamp: 1001, + title: "Good revision", + type: "post" + })); + + expect(findAll()).toEqual([ + { + key: "post-revision-good-1001", + id: "good", + revisionTimestamp: 1001, + title: "Good revision", + type: "post" + } + ]); + });🤖 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 `@apps/admin/src/restore/local-revisions.test.ts` around lines 42 - 61, Add a new test case that verifies findAll() gracefully handles malformed JSON in revision keys. Create a test where a localStorage key matching the post-revision-* pattern (e.g., "post-revision-draft-1000") contains invalid JSON instead of a valid JSON object, then assert that findAll() skips this entry without throwing an error. This ensures the function properly handles edge cases where stored revision data may be corrupted or improperly formatted.
🤖 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 `@apps/admin/src/restore/local-revisions.ts`:
- Around line 39-44: The find function's JSON.parse call can throw an exception
when encountering malformed JSON in localStorage, which crashes the entire
`/restore` route. Wrap the JSON.parse statement in a try-catch block to handle
the parsing error gracefully. When a JSON parsing error occurs, catch the
exception and return null to treat corrupt entries as non-revisions, allowing
findAll() to continue processing other entries without crashing the page.
In `@apps/admin/src/restore/restore.tsx`:
- Around line 121-133: The handleRestore function has a race condition where the
finally block unconditionally clears the restoringRevisionKey state, which can
interfere with overlapping restore mutations. To fix this, capture the specific
revision key at the start of the function (by storing revision.key in a local
variable), and in the finally block, only clear the restoringRevisionKey state
if it still matches the captured key. This prevents one completed mutation from
clearing the loading state while another mutation for a different revision is
still in-flight. Apply the same fix to the similar pattern mentioned at lines
190-204.
In `@e2e/helpers/pages/admin/restore/restore-page.ts`:
- Around line 31-42: The visit() and seedRevisionAndVisit() methods need to
include waitFor() guards to ensure the page has fully rendered before
proceeding. Add await this.waitFor() after the await this.goto() call in the
visit() method, and also add await this.waitFor() after the await
this.page.reload() call in the seedRevisionAndVisit() method. This ensures the
Page Object waits for the route to finish rendering rather than stopping at just
the load event.
---
Nitpick comments:
In `@apps/admin/src/restore/local-revisions.test.ts`:
- Around line 42-61: Add a new test case that verifies findAll() gracefully
handles malformed JSON in revision keys. Create a test where a localStorage key
matching the post-revision-* pattern (e.g., "post-revision-draft-1000") contains
invalid JSON instead of a valid JSON object, then assert that findAll() skips
this entry without throwing an error. This ensures the function properly handles
edge cases where stored revision data may be corrupted or improperly formatted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 486ed3e9-d32d-47de-a5fd-4776b8bfee57
📒 Files selected for processing (19)
apps/admin-x-framework/src/api/posts.tsapps/admin-x-framework/test/unit/api/posts.test.tsxapps/admin/src/restore/local-revisions.test.tsapps/admin/src/restore/local-revisions.tsapps/admin/src/restore/restore.test.tsxapps/admin/src/restore/restore.tsxapps/admin/src/routes.tsxapps/shade/src/components/ui/sonner.tsxe2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/restore/index.tse2e/helpers/pages/admin/restore/restore-page.tse2e/tests/admin/restore.test.tsghost/admin/app/controllers/restore-posts.jsghost/admin/app/router.jsghost/admin/app/routes/restore-posts.jsghost/admin/app/services/state-bridge.jsghost/admin/app/templates/restore-posts.hbsghost/admin/tests/acceptance/restore-post-test.jsghost/admin/tests/unit/services/state-bridge-test.js
💤 Files with no reviewable changes (5)
- ghost/admin/app/router.js
- ghost/admin/app/routes/restore-posts.js
- ghost/admin/app/templates/restore-posts.hbs
- ghost/admin/app/controllers/restore-posts.js
- ghost/admin/tests/acceptance/restore-post-test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin/src/restore/restore.test.tsx (1)
165-213: ⚡ Quick winAdd assertion that error toast was not called.
For consistency with the successful restore test (lines 107-163), this test should verify that
mockToastErrorwas not called after the success toast assertion.✨ Suggested consistency fix
await waitFor(() => { expect(mockToastSuccess).toHaveBeenCalledWith("Post restored successfully"); }); + +expect(mockToastError).not.toHaveBeenCalled();🤖 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 `@apps/admin/src/restore/restore.test.tsx` around lines 165 - 213, The restoreTest function "ignores duplicate restore clicks while the revision is already restoring" is missing an assertion to verify that mockToastError was not called. After the existing waitFor block that asserts mockToastSuccess was called with "Post restored successfully", add another waitFor block or assertion to ensure that mockToastError was not called, maintaining consistency with the successful restore test pattern.
🤖 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.
Nitpick comments:
In `@apps/admin/src/restore/restore.test.tsx`:
- Around line 165-213: The restoreTest function "ignores duplicate restore
clicks while the revision is already restoring" is missing an assertion to
verify that mockToastError was not called. After the existing waitFor block that
asserts mockToastSuccess was called with "Post restored successfully", add
another waitFor block or assertion to ensure that mockToastError was not called,
maintaining consistency with the successful restore test pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddac607d-ae85-45ce-89e4-4e956c2ef09e
📒 Files selected for processing (5)
apps/admin/src/restore/local-revisions.test.tsapps/admin/src/restore/local-revisions.tsapps/admin/src/restore/restore.test.tsxapps/admin/src/restore/restore.tsxe2e/helpers/pages/admin/restore/restore-page.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/helpers/pages/admin/restore/restore-page.ts
- apps/admin/src/restore/local-revisions.test.ts
- apps/admin/src/restore/local-revisions.ts
- apps/admin/src/restore/restore.tsx
Handle malformed local revision JSON, prevent duplicate restore creates, and add the restore page object wait guard.
PedroMarianoAlmeida
force-pushed
the
migrate-restore-page-to-react
branch
from
49d85f5 to
743400f
Compare
Use moment-timezone for restore revision dates so the absolute and relative labels follow the same pattern as the members page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR ports the Ghost Admin
/restorepage from Ember to React. The route lets users recover local editor revisions that were saved in the browser, and the new implementation keeps that behavior while moving the route into the React admin shell.The final diff is a clean cutover: React owns
/restore, the temporary/restore-reactcomparison route is gone, and the old Ember route/controller/template are removed. The Emberlocal-revisionsservice stays because the Ember editor still writes the local revision data.Check live demo
Why Make It
This is part of the Ember to React migration based on routes.
/restoreis a good candidate because it is small, isolated, and only reachable by direct URL rather than from the main admin navigation.The main risk was whether restore data came from an API or from the browser. The migration task confirmed it is browser-local: the Ember editor writes revisions to
window.localStorageusingpost-revision-<id>-<timestamp>keys. That means the React route can safely take over the read and restore UI without changing the editor writer path.What Changed
/restoreroute that reads local revisions fromlocalStorage, sorted newest-first.apps/admin/src/restore/local-revisions.tsas a small read layer for the existingpost-revision-*storage contract.ListPageandTableprimitives.data-test-id="restore-post-title"data-test-id="restore-post-button"useAddPosttoadmin-x-frameworkso restore can create a draft throughPOST /posts/./restorefromEMBER_ROUTESand registering the React route at/restore.restore-postsroute, controller, template, router entry, and old Ember acceptance test.Restore Behavior
The restore action intentionally follows Ember parity:
(Restored) <title>{id}, tags, andstatus: 'draft'Post restored successfullyorFailed to restore postWhy Users And Developers Need It
Users keep the same recovery path for local editor revisions if they lose draft content. Developers get another Ember-owned admin route migrated to React with the old Ember UI removed, focused tests in place, and no change to the editor’s local revision writer.
Migration Notes
ghost-revisionsis intentionally ignored. Ember reads use a rawpost-revision-*prefix scan, so React does the same.useLocalStoragehook was added. React reads once on mount, matching Ember routemodel()semantics. React does not own or write this data.Please check your PR against these items:
Testing
pnpm --filter @tryghost/admin-x-framework test:unit -- test/unit/api/posts.test.tsxpnpm --filter @tryghost/admin-x-framework test:typespnpm --filter @tryghost/admin-x-framework exec tsc -p tsconfig.declaration.jsonpnpm --filter @tryghost/shade exec tsc -p tsconfig.declaration.jsonpnpm --filter @tryghost/shade test:typespnpm --filter @tryghost/shade lint:codepnpm --filter @tryghost/admin test:unit -- src/restore/restore.test.tsx src/restore/local-revisions.test.tspnpm --filter @tryghost/admin typecheckpnpm --filter @tryghost/admin lintpnpm --filter @tryghost/e2e test:typespnpm --filter @tryghost/e2e lintpnpm --filter @tryghost/e2e test tests/admin/restore.test.ts --timeout=60000The Playwright restore e2e suite now covers the deleted Ember acceptance-test contract against canonical
/restore: seeded local revision renders title + Restore button, clicking Restore shows the success notification, and empty localStorage shows the empty state.