Repository navigation
Protect Dashboard: Add tabbed layout and section framework - #53190
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Feature] Protect Dashboard
[Plugin] Jetpack
|
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! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
20 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
c50d3ec to
46219b4
Compare
46219b4 to
10c62dc
Compare
enejb
added
the
Coverage tests to be added later
Ports the foundation onto the dashboard's new home in packages/protect: the Dashboard_Section registry, threat formatter, Overview/Settings tabs, settings hook, IP list field and section slots, plus their PHP and JS tests.
Section files now only declare a Sections\<Name> class; Dashboard::init() finds and registers them once, so the classmap autoloader can load them safely. Also print the initial state through a tested method, keep a later save when an earlier save of the same key fails, test the stage's tab resolution, and document the dashboard's dependency on the Jetpack plugin.
b531356 to
6567e4b
Compare
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Approving — these are all optional. The force-push landed every one of the review follow-ups correctly, and nothing in the shipped, reachable code regressed, so merge whenever you're happy with it. I left a few inline suggestions worth a look if you're still iterating:
- The shared settings hook has a latent rollback edge case (two overlapping failed saves of the same key) that no section can trigger today, but every feature section will inherit it — cheapest to fix here, before the five section PRs build on it.
load_sections()skips a misnamed section file silently and would fatal on an abstract or constructor-arg class; a guard that warns via_doing_it_wrong()would fail loud instead.- A few test-coverage gaps on claimed fixes (IpListField comma-list detection, the threat field mapping, the tab
?tab=round-trip) and one American-English docblock nit.
None of these block merge — full context in the inline comments.
| async ( patch: ProtectSettings, path = '/jetpack/v4/settings' ) => { | ||
| const keys = Object.keys( patch ); | ||
| const before = settingsRef.current; | ||
| const previous = Object.fromEntries( |
There was a problem hiding this comment.
Latent rollback edge case — no shipped caller hits it yet (the toggle and IP field disable themselves while saving), but every feature section inherits this hook. previous is captured from settingsRef.current at save time, i.e. the optimistic value. If two saves of the same key both fail, the second save's rollback restores the first save's optimistic value — one the server never accepted. Consider capturing a per-key last-server-confirmed baseline (seeded on load, updated on each save success) and rolling back to that instead. A both-fail concurrent-save test would pin it.
|
|
||
| export type DashboardContext< S = unknown > = { | ||
| /** This section's state from PHP, or undefined when PHP registered no section by this key. */ | ||
| state: S; |
There was a problem hiding this comment.
state is typed S, but as the comment on the line above notes, it is undefined at runtime when PHP registered no section for this key. Typing it S | undefined (or defaulting the lookup in stage.tsx) would make section authors handle the missing-state case at compile time rather than hitting a runtime undefined.
| ); | ||
|
|
||
| describe( 'IpListField', () => { | ||
| it( 'keeps the typed draft when its save fails', async () => { |
There was a problem hiding this comment.
The comma-separated-list detection and the "Add my IP address" button added in this PR have no test — the only case here is failed-save draft retention. Reverting the split(/[\s,]+/) back to newline-only would still pass CI. An it.each over drafts with currentIp set (comma list with and without the IP, newline list, a substring like 1.2.3.45, empty) plus a click-appends-IP assertion would cover it.
| * @param mixed $expected The expected value. | ||
| */ | ||
| #[DataProvider( 'provider_format' )] | ||
| public function test_format( $threat, $path, $expected ) { |
There was a problem hiding this comment.
test_format asserts only the single field each provider row targets, so the snake_case to camelCase mapping that is the formatter's main job (first_detected to firstDetected, fixed_in to fixedIn, fixed_on to fixedOn, and so on) is never checked end to end. One row that formats a fully-populated threat and assertSames the whole expected camelCase array would guard against a dropped or mis-mapped field.
| import { stage as Stage } from '../stage'; | ||
|
|
||
| jest.mock( '@wordpress/api-fetch', () => ( { __esModule: true, default: jest.fn() } ) ); | ||
| jest.mock( '@wordpress/route', () => ( { useSearch: jest.fn(), useNavigate: () => jest.fn() } ) ); |
There was a problem hiding this comment.
useNavigate is mocked as a throwaway () => jest.fn(), so the tab-change path added in this PR — writing ?tab=settings and dropping tab for Overview — is never exercised. Hoisting the mock into a shared jest.fn() and clicking each tab (asserting the search updater returns { ...prev, tab: 'settings' } for Settings and clears tab for Overview) would cover the round-trip.
| if ( ! class_exists( $class ) ) { | ||
| require_once $file; | ||
| } | ||
| if ( ! is_subclass_of( $class, Dashboard_Section::class ) || self::has_section_of_class( $class ) ) { |
There was a problem hiding this comment.
Two edge cases here. A class-*.php file whose declared class does not match the filename convention is skipped silently (is_subclass_of is false, so continue). And a class that implements the interface but is not instantiable (abstract, or a constructor with required args) would fatal at new $class() on the next line — inside init(), taking the admin menu down with it. A ReflectionClass::isInstantiable() guard plus _doing_it_wrong() on a mismatch (mirroring the duplicate-key notice) would fail loud instead of silent-or-fatal.
| } | ||
|
|
||
| /** | ||
| * A labelled figure, such as a count of blocked requests. |
There was a problem hiding this comment.
American English: "labelled" should be "labeled", per the repo's spelling convention.
github-actions
Bot
added
[Status] UI Changes
2 participants
Part of JETPACK-2875
Contributes to JETPACK-2874
Proposed changes
This is the foundation for the Protect dashboard in
packages/protect, which #53182 added. It ships the page shell and a section framework, so each feature (Scan, Monitor, Firewall, Login protection, Scan history) can land as its own PR without touching shared files.routes/dashboard/stage.tsxrendersAdminPagefrom@automattic/jetpack-components, with Overview and Settings tabs in the fixed header. It uses the sharedjetpack-admin-page-layout-wp-buildstyles, like Scan, Newsletter and Jetpack AI. The active tab lives in?tab=; an unknown tab falls back to Overview.src/sections/class-<name>.php, namedAutomattic\Jetpack\Protect\Sections\<Name>(class-login-protection.phpdeclaresSections\Login_Protection), implementingDashboard_Section(get_key(),get_state(),register_routes()).Dashboard::init()finds those files, registers each class once, prints every section's state aswindow.jetpackProtectDashboard, and registers their REST routes onrest_api_init. Section files only declare their class, so the classmap autoloader can load them safely.Dashboard:can_manage(),get_module_state(),has_scan_plan(), andDashboard_Threats, which formats threats for@automattic/jetpack-scan(used by Scan and History).routes/dashboard/sections/index.tsalready imports one slot per feature. A section can provide an Overview card, a Settings card and/or its own tab, so a feature PR only edits its own folder. In this PR the slots are empty, so both tabs show an empty state.ProtectCard,CardRow,Stat),SettingsLink,SettingToggle,IpListField,isModuleActive(), and the settings hookuseProtectSettings. The hook fetches the settings the first time the Settings tab opens and saves optimistically, rolling back on failure, tojetpack/v4/settingsor another path such asjetpack/v4/waf.route.scssalso loads the DataViews styles once for the sections that use them.packages/protect):@automattic/jetpack-components,@automattic/jetpack-base-styles,@automattic/jetpack-scan,@wordpress/route,@wordpress/dataviews,@wordpress/uiand friends, plus a Jest setup for the package.packages/protect):automattic/jetpack-protect-status,automattic/jetpack-protect-models,automattic/jetpack-status.get_module_state()and the settings endpoints are the Jetpack plugin's. The README says so; the Jetpack Protect plugin will need its own equivalents before it can use the page.Related product discussion/links
protect-dashboardmodule and the blank page inpackages/protect.packages/protectthe same way:Does this pull request change what data or activity we track or use?
No.
Testing instructions
JETPACK_AUTOLOAD_DEVset.wp companion feature-flag enable jetpack-protect-dashboard) and the module (wp jetpack module activate protect-dashboard).p=/?tab=settingsand the tab should say "There's nothing to set up yet." Reload, and Settings should stay selected. In the Network panel,jetpack/v4/settingsandjetpack/v4/wafshould be requested only once the Settings tab is open, not on Overview.wp eval 'echo wp_json_encode( Automattic\Jetpack\Protect\Dashboard::get_initial_state() );'should print[], since no sections are registered yet, and the page source should containwindow.jetpackProtectDashboard = {};.wp eval 'var_dump( Automattic\Jetpack\Protect\Dashboard::has_scan_plan() );'should printfalseon a site without a Scan plan.jp test php packages/protectandjp test js packages/protectshould pass.Review follow-ups
Fixed:
Dashboard::init()from side-effect-free classes instead of each file callingregister_section()at file scope, which the package's classmap autoloader could otherwise trigger. Loading twice registers each class once.Dashboard::print_initial_state(), tested for its escaping and for printing{}with no sections.IpListFielddetects the current IP in comma-separated lists too.Dashboard_Threats::format_all()iterates arrays andTraversableobjects instead of casting to an array.register_section()keeps the first section and calls_doing_it_wrong()on a duplicate key.route.scssuses logical sizing and a--wpds-*warning token instead of a hex colour.Deferred:
admin_menu/rest_api_init: the cost is oneglob()per request while the flag-gated module is on. Loading lazily needs both the page render andrest_api_initpaths to trigger it, so it is better done once the feature PRs have landed.Coverage tests to be added later):can_manage(),has_scan_plan(),get_module_state(),SettingsLink,SettingToggle,isModuleActive()and the card pieces are first used by the feature PRs, which test them alongside the code that calls them.Screenshots
Overview tab with the tab strip in the fixed header (no sections registered yet):
Settings tab: