-
Notifications
You must be signed in to change notification settings - Fork 2k
Scan Dashboard: implement threat fix progress tracking with reusable hook #106085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
return { | ||
...result, | ||
threats: Object.entries( result.threats ).map( ( [ id, threat ] ) => ( { | ||
...threat, | ||
id, | ||
} ) ), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Centralized the custom mapping on the useFixThreats
hook instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, as this is apparently the opposite of what you suggested last week (p1758823754745099?thread_ts=1758820334.569579&cid=C09AAF76EQZ-slack-C09AAF76EQZ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My expectation was to use the same query hook (fixThreatsStatusQuery) across different components, such as the Fix Threat modal once you added the Bulk Fix modal. Since I centralized all of that logic into a single hook, we can call it directly there. This also made it easier to handle the transformation logic in one place while keeping the query return type as the raw FixThreatsStatusResponse. It also simplified tests because we can mock the fetcher without introducing intermediate “transformation” types in the fixThreatsStatusQuery hook.
siteId: number, | ||
threatIds: number[] | ||
): Promise< SiteScan > { | ||
): Promise< FixThreatsStatusResponse > { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced SiteScan
with the new FixThreatsStatusResponse
because the responses are not the same.
return; | ||
if ( status.allFixed ) { | ||
createSuccessNotice( | ||
_n( 'Threat fixed.', 'All threats were successfully fixed.', bulkFixableThreats.length ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, Rafa. I left a couple questions, but they are not blockers. LGTM. ✅
id: Number( id ), | ||
} ) ); | ||
}, | ||
gcTime: 0, // Always fetch fresh status data, never use cached results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using meta: { persist: false }
wasn't enough? Maybe we should drop it from there as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it wasn’t enough 😢. We were still getting stale cached results, which caused the snackbar to appear immediately on subsequent fix attempts. Setting the garbage collection time to 0 clears the results from memory as soon as they’re no longer needed, so if you unmount the modal and try again, it now works properly.
I removed the meta: { persist: false }
in 13d3522 as it is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you unmount the modal and try again, it now works properly
If you want to force a fetch when the hook is re-mounted maybe the refetchOnMount: 'always'
option will work? https://tanstack.com/query/latest/docs/framework/react/reference/useQuery#:~:text=in%20the%20background-,refetchOnMount,-%3A%20boolean%20%7C%20%22always%22%20%7C%20((query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋
This specific query hook is trying to fetch the progress of a threat that is being fixed. If for some reason the process fails and the user wants to try again, the query still gets the latest status from cache and reports it immediately without actually trying to fetch a new result.
I believe the refetchOnMount just attempts to fetch but still returns what's in the cache first. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hook returns a isFetchedAfterMount
boolean, is that what you need?
It might not be 😅 I'm not familiar with the scan code, so hopefully I'm suggesting things that don't make sense. But React Query does cover a lot of use cases.
return { | ||
...result, | ||
threats: Object.entries( result.threats ).map( ( [ id, threat ] ) => ( { | ||
...threat, | ||
id, | ||
} ) ), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, as this is apparently the opposite of what you suggested last week (p1758823754745099?thread_ts=1758820334.569579&cid=C09AAF76EQZ-slack-C09AAF76EQZ).
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/20323842 Hi @Initsogar, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Resolves DOTDASH-529
Proposed Changes
useFixThreats
hook.gcTime: 0
to prevent stale "fixed" status persisting after mutations fail.fetchFixThreatsStatus
return type to use correctFixThreatsStatusResponse
definition.useFixThreats
hookBonus:
Demo
demo-03.mp4
demo01.mp4
Why are these changes being made?
The previous implementation of the individual threat-fixing was only using a mutation to trigger the fix, but doesn't track the progress. After implementing the bulk threat fix modal, we identified an opportunity to reuse the same endpoints to initiate the fix and also to track the progress.
This refactor centralizes the logic in a reusable hook with proper state management and error handling for both implementations.
Testing Instructions
/v2/sites/{site-slug}/scan
in the Hosting DashboardUnit tests
Run the comprehensive test suite with coverage:
Or just the tests without checking coverage:
# Run all scan tests yarn test-client dashboard/sites/scan
Pre-merge Checklist