Fix routing bugs in security dashboards
- Related issue: #11412 (closed)
Context
In the security dashboard, we sync the store and the router so that filters values are reflected in the URL, and vice-versa. This means that:
- Whenever the user changes a filter's value, we update the store, then the router
- Whenever the user navigates backwards, we update the store based on the new URL
This is all handled in a Vuex plugin: sync_with_router.js
We discovered that it's impossible to go back more than one entry in the history: using the back button once works as expected, but subsequent usages seem to have no effect on the filters.
What's happening?
When the user navigates backwards, we trigger some store actions to update the store based on the query string: sync_with_router.js#L17-35
While the store is being updated, we "freeze" the plugin by setting a syncingRouter
flag to true.
The store->router
update process works by programmatically triggering navigation events when specific mutations are being committed: sync_with_router.js#L38-49.
The plugin currently subscribes to 2 mutations:
vulnerabilities/SET_VULNERABILITIES_HISTORY_DAY_RANGE
vulnerabilities/RECEIVE_VULNERABILITIES_SUCCESS
The problem here is that vulnerabilities/RECEIVE_VULNERABILITIES_SUCCESS
is an async mutation which is committed once vulnerabilities have been successfully fetched, at which point the plugin has been "unfreezed" already (syncingRouter === false
). This means that whenever we refresh the vulnerabilities list, an entry is pushed to the browser's history. Thus, when using the back button, the current route ends up being duplicated in the history.
graph LR;
subgraph store
setAllFilters --> fetchVulnerabilities --> RECEIVE_VULNERABILITIES_SUCCESS[vulnerabilities/RECEIVE_VULNERABILITIES_SUCCESS];
end
subgraph router
Router[Navigation event] -- queryString --> syncingRouter["syncingRouter: true"] --> setAllFilters;
notSyncingRouter["syncingRouter: false"];
Router[Navigation event] --> History[New history entry];
end
setAllFilters --> notSyncingRouter;
RECEIVE_VULNERABILITIES_SUCCESS[vulnerabilities/RECEIVE_VULNERABILITIES_SUCCESS] -- state --> Router
What does this MR do?
This MR drops store.subscribe()
in favor of store.subscribeAction()
in order to hook into vulnerabilities/fetchVulnerabilities
and vulnerabilities/setVulnerabilitiesHistoryDayRange
actions instead of low-level mutations. This lets us perform store->router
updates more reliably without polluting the browser's history because we process filters changes synchronously. This approach has another benefit in terms of UX: URL updates happen immediately after filters change as opposed to the previous approach where URL updates would be delayed until the XHR query for retrieving vulnerabilities resolved.
One more thing...
This MR fixes another bug where the Project Security Dashboard would not init the filters based on the query string. Instead, the store would initialize the filters with their default values and call router.push()
to remove custom filter values from the query string. This was due to the fact that this dashboard was initialized without a router and thus ignored the URL completely until the store triggered some routing event. This has been fixed by properly installing the router in the Project Security Dashboard.
Screenshots
Before | After |
---|---|
bad-gsd-history-back-button | sd_history |
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry - [-] Documentation created/updated or follow-up review issue created
-
Code review guidelines - [-] Merge request performance guidelines
-
Style guides - [-] Database guides
- [-] Separation of EE specific content
Performance and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers - During cross-browser testing, it appeared that the Security Dashboard does not work properly on IE11 for reasons that aren't related to this MR. At the very least, we would need to include
Number.isFinite
andNumber.isInteger
polyfills but that might not suffice. To be continued in a follow up MR: !17644 (closed)
- During cross-browser testing, it appeared that the Security Dashboard does not work properly on IE11 for reasons that aren't related to this MR. At the very least, we would need to include