Repository navigation
Backup: report an activity log failure like a backups failure - #53790
Conversation
…tice Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
added
[Package] Backup
[Status] In Progress
[Tests] Includes Tests
labels
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 2 files.
|
…ows while it retries Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dhasilva
left a comment
dhasilva
left a comment
There was a problem hiding this comment.
The approach holds up. TanStack applies placeholderData only while a query is pending, and retrying an errored query that holds no data puts it back to pending, so the isPlaceholderData && error !== null check in the second commit is the right signal. The DataViews 20 footer hides on ! totalItems, so keeping the last totals is what keeps the pager.
No blockers. There are two test changes I'd ask for and two optional ones, all inline.
I mutation-tested the new tests at e7a3b31 to check that each one fails without the change it covers:
| Mutation | Result |
|---|---|
showsPlaceholder = false |
"does not fill a failed page…" fails (2 "Backup complete" vs 1) |
activityFailed = false |
3 tests fail |
lastAnswer.current = answer (no keeping) |
2 tests fail, as the description says |
in-card returnFocusTo={ undefined } |
all 1048 tests still pass (see inline) |
Two notes outside the code:
- Changelog: the empty entry is right while the dashboard is behind its flag. If #53443 (the flip) merges first, this becomes a fix users can see and needs a real entry.
- Labels: the PR has both
[Status] In Progressand[Status] Needs Review.
Generated by Claude.
| const lastAnswer = useRef( answer ); | ||
| lastAnswer.current = answer ?? lastAnswer.current; | ||
| const totalItems = lastAnswer.current?.totalItems ?? 0; | ||
| const totalPages = lastAnswer.current?.totalPages ?? null; |
There was a problem hiding this comment.
Optional: the kept totals belong to this hook instance, not to the query, which shows up two ways:
- A failed per-page change keeps
totalPagescounted at the old size. DataViews resets to page 1 with the newperPage. With 30 rows at 10 per page, switching to 20 per page shows "1 of 3", and page 3 is past the end. - A return trip from Download or Restore after
gcTime(5 min) to a remembered page 2 or later starts with an empty ref. If that page fails, Overview shows the full-width notice with no pager. Try again is then the only way off the page until a reload resetslastView. The description lists "counts as a first load again" as known, but not that it strands the reader.
One change fixes both: keep only totalItems, work out totalPages from the current pageSize, and when nothing is kept, fall back to the page-1 query's totalItems. useHasRestorePoints keeps that query mounted in Overview, so it's there whenever page 1 has loaded.
Generated by Claude.
| code: 'activity_log_fetch_failed', | ||
| message: 'Service unavailable', | ||
| } ) | ||
| : new Promise( () => {} ); |
There was a problem hiding this comment.
Nothing checks where focus goes after a retry of the in-card notice succeeds. On trunk, error-persists-during-retry.test.tsx asserted that it lands on the "Latest backups" heading. This PR points that test at the first-load path instead. With returnFocusTo={ undefined } on the in-card QueryError (activity-list/index.tsx L312), all 1048 tests still pass.
The bug it would catch: the heading loses its ref, and focus drops to <body> after a successful retry. That is the regression the old assertion was there for.
The cheapest fix is here. Replace this never-settling promise with a deferred one, resolve it after the "same count" assertion, then await waitFor( () => expect( screen.getByRole( 'heading', { name: 'Latest backups' } ) ).toHaveFocus() ). One test then covers both behaviours.
Generated by Claude.
| // The detail pane keeps its heading; only the list rows are in question. | ||
| const before = screen.getAllByText( 'Backup complete' ).length; | ||
| failPage2 = false; | ||
| await userEvent.click( screen.getAllByRole( 'button', { name: 'Try again' } ).at( -1 ) ); |
There was a problem hiding this comment.
.at( -1 ) is choosing between two notices. The catch-all Promise.resolve( {} ) also answers /jetpack/v4/backups, so "We couldn't check your site's backup status." is on screen for the whole test (confirmed by dumping .jpb-query-error mid-test). It finds the right button only because of DOM order. The test above has the same setup.
The mockActivityLog helper this PR adds to error-persists-during-retry.test.tsx answers that route with [] for exactly this reason. I'd suggest one helper for the two copies of the page-2 routing in this file that does the same. This line can then be a plain getByRole( 'button', { name: 'Try again' } ).
Generated by Claude.
| // the wrong answer used to be. A log that never answered has no list to | ||
| // keep, so Overview reports that one instead. |
There was a problem hiding this comment.
This reason is also in overview.tsx L250 ("Only a log that never answered has no list to keep"). Overview makes the decision, so it can keep the reason and this comment can just point there:
| // the wrong answer used to be. A log that never answered has no list to | |
| // keep, so Overview reports that one instead. | |
| // the wrong answer used to be. Overview reports a log that never answered. |
Generated by Claude.
| screen.findByText( "We couldn't load your site's activity." ) | ||
| ).resolves.toBeInTheDocument(); | ||
| expect( screen.getByRole( 'heading', { name: 'Latest backups' } ) ).toBeInTheDocument(); | ||
| // The pager is what gets the reader back; DataViews hides it when the page has no rows. |
There was a problem hiding this comment.
This repeats the hook's comment on lastAnswer, and has drifted from it: the DataViews footer hides on ! totalItems, not on the page's rows. The hook owns the reason:
| // The pager is what gets the reader back; DataViews hides it when the page has no rows. | |
| // The pager is what gets the reader back. |
Generated by Claude.
| expect( screen.getByText( 'Service unavailable' ) ).toBeInTheDocument(); | ||
| // The regression: a 5xx telling the reader their site has no activity. | ||
| expect( screen.queryByText( 'No results' ) ).not.toBeInTheDocument(); | ||
| // The notice stands in for the list card instead of sitting inside a narrow one. |
There was a problem hiding this comment.
"Instead of sitting inside a narrow one" describes the layout this PR removes, which a reader won't find after merge:
| // The notice stands in for the list card instead of sitting inside a narrow one. | |
| // The notice replaces the list card. |
Generated by Claude.
…failure tests Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
removed
the
[Status] Needs Review
2 participants
Fixes JETPACK-3067
Proposed changes
useActivityLogkeeps the last answered totals, because DataViews hides its pager when the total drops to zero.totalPages === null. No new state.Known and left alone:
OverviewandActivityList. One copy needs a new shared component or a bigger refactor.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Screenshots
Local connected site with a Backup plan, 1400px and 390px wide. Only the request under test is failed (HTTP 503) with Playwright route interception. Everything else is real data.
Testing instructions
Checked:
pnpm run test(1047 tests),pnpm run typecheckandlint-changedinprojects/packages/backup. All pass. The new page-2 test fails if the hook change is reverted, and if Overview is made to fail on any error.The screenshots come from the built dashboard on a local connected site. To reproduce a case by hand, open Jetpack › Backup on a site with the modernized dashboard and a Backup plan, block the request
/wp-json/jetpack/v4/site/rewindable-activity(or/wp-json/jetpack/v4/backups) in the browser's network tools, and reload. I did not run these exact DevTools steps. The screenshots used the same request blocking through Playwright.