From a024d8e26a4faf266f428859de165ad86649cd07 Mon Sep 17 00:00:00 2001 From: Carlo <1778532+cdcme@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:37:06 -0400 Subject: [PATCH] Fix policy and report pages showing previously viewed content (#47767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves #43310 Read-only policy and report detail pages rendered their displayed fields (name, description, resolution, platforms, query) from the editing context (`PolicyContext`/`QueryContext`), which only updates in react-query's `onSuccess`. On a cached revisit, `isLoading` is `false` (no spinner) and the freshly-loaded entity is available immediately, but the context still held the previously-viewed entity's values for a frame — briefly showing the wrong policy/report. This change makes both detail pages render directly from the fresh `useQuery` result (`storedPolicy`/`storedQuery`) and drops their coupling to the editing context, matching the existing `HostDetailsPage`/`SoftwareTitleDetailsPage` pattern. `QueryDetailsPage` (Reports) had the identical latent bug and is fixed here too. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually New regression tests (`PolicyDetailsPage.tests.tsx`, `QueryDetailsPage.tests.tsx`) seed the context with stale values and assert the page renders the freshly-loaded entity instead. Verified live against a running Fleet instance with Playwright: navigating between cached detail pages no longer flashes the previous entity's content. [6f7d6d22134dc1fd968f8473c552ee49.webm](https://github.com/user-attachments/assets/c8ef5f3f-278c-4e13-adc1-689b3eff4e59) ## Summary by CodeRabbit ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Resolved an issue where policy and query details pages could briefly display previously viewed policy/report information when switching between items. * **Tests** * Added regression coverage to ensure the UI renders freshly loaded policy/query name, description, and query/report details, and does not show stale values. --- changes/43310-policy-report-details-flash | 1 + .../PolicyDetailsPage.tests.tsx | 138 +++++++++++++++++- .../PolicyDetailsPage/PolicyDetailsPage.tsx | 60 +------- .../QueryDetailsPage.tests.tsx | 136 +++++++++++++++++ .../QueryDetailsPage/QueryDetailsPage.tsx | 56 ++----- .../components/QueryReport/QueryReport.tsx | 9 +- 6 files changed, 298 insertions(+), 102 deletions(-) create mode 100644 changes/43310-policy-report-details-flash diff --git a/changes/43310-policy-report-details-flash b/changes/43310-policy-report-details-flash new file mode 100644 index 0000000000..3c687ed9c1 --- /dev/null +++ b/changes/43310-policy-report-details-flash @@ -0,0 +1 @@ +- Fixed the policy and report details pages briefly showing the previously-viewed policy/report's content when navigating between them. diff --git a/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tests.tsx b/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tests.tsx index 514d1a62e7..ef51a3deb9 100644 --- a/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tests.tsx +++ b/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tests.tsx @@ -1,13 +1,51 @@ +import React from "react"; +import { screen } from "@testing-library/react"; +import { http, HttpResponse } from "msw"; + import { IPolicy } from "interfaces/policy"; import { ILabelPolicy } from "interfaces/label"; +import { + createCustomRenderer, + baseUrl, + createMockRouter, +} from "test/test-utils"; +import mockServer from "test/mock-server"; +import createMockUser from "__mocks__/userMock"; +import createMockConfig from "__mocks__/configMock"; -import { getLabelModalData } from "./PolicyDetailsPage"; +import PolicyDetailsPage, { getLabelModalData } from "./PolicyDetailsPage"; // Stub SoftwareIcon to avoid asset resolution when importing the page module. jest.mock("pages/SoftwarePage/components/icons/SoftwareIcon", () => { return () => null; }); +// Avoid depending on react-router's browserHistory inside BackButton. +jest.mock("components/BackButton", () => ({ + __esModule: true, + default: ({ text }: { text: string }) => ( + + ), +})); + +// Surface the modal's `query` prop as plain text (the real modal renders it in +// an Ace editor that isn't reliably assertable in jsdom). +jest.mock("components/modals/ShowQueryModal", () => ({ + __esModule: true, + default: ({ query }: { query?: string }) => ( +
{query}
+ ), +})); + +// Activities table fetches on mount; stub it out so the render test stays +// focused on the policy's own fields. +jest.mock("../components/PolicyAutomationsActivitiesTable", () => ({ + __esModule: true, + default: () => null, +})); + const labels = (...names: string[]): ILabelPolicy[] => names.map((name, i) => ({ id: i + 1, name })); @@ -169,3 +207,101 @@ describe("getLabelModalData", () => { }); }); }); + +const POLICY_ID = 8; + +const createProps = () => ({ + router: createMockRouter(), + params: { id: String(POLICY_ID) }, + location: { + pathname: `/policies/${POLICY_ID}`, + search: "", + query: {}, + }, +}); + +const baseAppContext = { + isGlobalAdmin: true, + isOnGlobalTeam: true, + // Free tier short-circuits useTeamIdParam's redirect logic when no fleet_id is + // set, keeping the test focused on which data source the page renders from. + isFreeTier: true, + isPremiumTier: false, + currentUser: createMockUser({ global_role: "admin" }), + config: createMockConfig(), + availableTeams: [], +}; + +describe("PolicyDetailsPage - renders fresh policy data (regression #43310)", () => { + it("renders the loaded policy's fields, not stale PolicyContext values", async () => { + mockServer.use( + // team_id: null keeps the team query disabled, so no second endpoint to mock. + http.get(baseUrl(`/policies/${POLICY_ID}`), () => + HttpResponse.json({ + policy: createMockPolicy({ + id: POLICY_ID, + team_id: null, + name: "Fresh policy name", + description: "Fresh policy description", + resolution: "Fresh resolution steps", + platform: "darwin", + query: "SELECT 'fresh';", + critical: true, + }), + }) + ) + ); + + const render = createCustomRenderer({ + withBackendMock: true, + context: { + app: baseAppContext, + // Stale values left over from a previously-viewed policy. The page must + // ignore all of these and render the freshly-loaded policy instead. + policy: { + lastEditedQueryName: "Stale policy name", + lastEditedQueryDescription: "Stale policy description", + lastEditedQueryResolution: "Stale resolution steps", + lastEditedQueryPlatform: "windows", + lastEditedQueryBody: "SELECT 'stale';", + lastEditedQueryCritical: false, + }, + }, + }); + const { user, container } = render( + + ); + + // name + description + expect(await screen.findByText("Fresh policy name")).toBeInTheDocument(); + expect(screen.getByText("Fresh policy description")).toBeInTheDocument(); + expect(screen.queryByText("Stale policy name")).not.toBeInTheDocument(); + expect( + screen.queryByText("Stale policy description") + ).not.toBeInTheDocument(); + + // resolution + expect(screen.getByText("Fresh resolution steps")).toBeInTheDocument(); + expect( + screen.queryByText("Stale resolution steps") + ).not.toBeInTheDocument(); + + // platform ("darwin" displays as "macOS"; stale "windows" must not appear) + expect(screen.getByText("macOS")).toBeInTheDocument(); + expect(screen.queryByText("Windows")).not.toBeInTheDocument(); + + // critical (drives the critical-policy icon) + expect( + container.querySelector(".critical-policy-icon") + ).toBeInTheDocument(); + + // query (shown via the "Show query" modal) + await user.click(screen.getByRole("button", { name: "Show query" })); + expect(screen.getByTestId("show-query-modal")).toHaveTextContent( + "SELECT 'fresh';" + ); + expect(screen.getByTestId("show-query-modal")).not.toHaveTextContent( + "SELECT 'stale';" + ); + }); +}); diff --git a/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx b/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx index 55c4b5d6be..2829709533 100644 --- a/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx +++ b/frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx @@ -4,7 +4,6 @@ import { InjectedRouter, Params } from "react-router/lib/Router"; import { useErrorHandler } from "react-error-boundary"; import PATHS from "router/paths"; import { AppContext } from "context/app"; -import { PolicyContext } from "context/policy"; import { IPolicy, IStoredPolicyResponse, @@ -12,9 +11,7 @@ import { } from "interfaces/policy"; import { ILabelPolicy } from "interfaces/label"; import { - API_ALL_TEAMS_ID, API_NO_TEAM_ID, - APP_CONTEXT_ALL_TEAMS_ID, APP_CONTEXT_ALL_TEAMS_SUMMARY, APP_CONTEXT_NO_TEAM_SUMMARY, } from "interfaces/team"; @@ -113,25 +110,6 @@ const PolicyDetailsPage = ({ config, } = useContext(AppContext); - const { - lastEditedQueryName, - lastEditedQueryDescription, - lastEditedQueryResolution, - lastEditedQueryBody, - lastEditedQueryPlatform, - setLastEditedQueryId, - setLastEditedQueryName, - setLastEditedQueryDescription, - setLastEditedQueryBody, - setLastEditedQueryResolution, - setLastEditedQueryCritical, - setLastEditedQueryPlatform, - setLastEditedQueryLabelsIncludeAny, - setLastEditedQueryLabelsIncludeAll, - setLastEditedQueryLabelsExcludeAny, - setPolicyTeamId, - } = useContext(PolicyContext); - const { isRouteOk, teamIdForApi, @@ -169,30 +147,6 @@ const PolicyDetailsPage = ({ refetchOnWindowFocus: false, retry: false, select: (data: IStoredPolicyResponse) => data.policy, - onSuccess: (returnedPolicy) => { - setLastEditedQueryId(returnedPolicy.id); - setLastEditedQueryName(returnedPolicy.name); - setLastEditedQueryDescription(returnedPolicy.description); - setLastEditedQueryBody(returnedPolicy.query); - setLastEditedQueryResolution(returnedPolicy.resolution); - setLastEditedQueryCritical(returnedPolicy.critical); - setLastEditedQueryPlatform(returnedPolicy.platform); - setLastEditedQueryLabelsIncludeAny( - returnedPolicy.labels_include_any || [] - ); - setLastEditedQueryLabelsIncludeAll( - returnedPolicy.labels_include_all || [] - ); - setLastEditedQueryLabelsExcludeAny( - returnedPolicy.labels_exclude_any || [] - ); - const deNulledTeamId = returnedPolicy.team_id ?? undefined; - setPolicyTeamId( - deNulledTeamId === API_ALL_TEAMS_ID - ? APP_CONTEXT_ALL_TEAMS_ID - : deNulledTeamId - ); - }, onError: (error) => handlePageError(error), }); @@ -286,8 +240,8 @@ const PolicyDetailsPage = ({ }; const renderPlatforms = (): JSX.Element | null => { - if (!lastEditedQueryPlatform) return null; - const platforms = lastEditedQueryPlatform + if (!storedPolicy?.platform) return null; + const platforms = storedPolicy.platform .split(",") .map((p) => p.trim()) .filter((p): p is Platform => p in PLATFORM_DISPLAY_NAMES); @@ -348,12 +302,12 @@ const PolicyDetailsPage = ({ }; const renderResolution = () => { - if (!lastEditedQueryResolution) return null; + if (!storedPolicy?.resolution) return null; return ( ); @@ -413,7 +367,7 @@ const PolicyDetailsPage = ({

- {lastEditedQueryName} + {storedPolicy?.name} {storedPolicy?.critical && (

@@ -508,7 +462,7 @@ const PolicyDetailsPage = ({ )} {showQueryModal && ( setShowQueryModal(false)} /> )} diff --git a/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tests.tsx b/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tests.tsx index 95deca9c2c..8a49c34378 100644 --- a/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tests.tsx +++ b/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tests.tsx @@ -26,6 +26,34 @@ jest.mock("components/BackButton", () => ({ ), })); +// Surface the modal's `query` prop as plain text (the real modal renders it in +// an Ace editor that isn't reliably assertable in jsdom). +jest.mock("components/modals/ShowQueryModal", () => ({ + __esModule: true, + default: ({ query }: { query?: string }) => ( +
{query}
+ ), +})); + +// Surface the report-setting props derived from the loaded query so we can +// assert they reflect storedQuery rather than stale context. +jest.mock("../components/NoResults/NoResults", () => ({ + __esModule: true, + default: ({ + discardDataEnabled, + loggingSnapshot, + }: { + discardDataEnabled: boolean; + loggingSnapshot: boolean; + }) => ( +
+ ), +})); + const QUERY_ID = 1; const HOST_ID = 42; const FILTERED_QUERIES_PATH = "/queries/manage?fleet_id=1"; @@ -79,6 +107,114 @@ const renderPage = ( return screen.findByTestId("back-button"); }; +describe("QueryDetailsPage - renders fresh query data (regression #43310)", () => { + it("renders the loaded query's fields, not stale QueryContext values", async () => { + mockServer.use( + http.get(baseUrl(`/reports/${QUERY_ID}`), () => + HttpResponse.json({ + query: createMockSchedulableQuery({ + id: QUERY_ID, + team_id: null, + name: "Fresh report name", + description: "Fresh report description", + query: "SELECT 'fresh';", + logging: "differential", // not "snapshot" + discard_data: true, + }), + }) + ), + http.get(baseUrl(`/reports/${QUERY_ID}/report`), () => + HttpResponse.json( + createMockQueryReport({ query_id: QUERY_ID, results: [] }) + ) + ) + ); + + const render = createCustomRenderer({ + withBackendMock: true, + context: { + app: baseAppContext, + // Stale values left over from a previously-viewed report. The page must + // ignore all of these and render the freshly-loaded query instead. + query: { + lastEditedQueryName: "Stale report name", + lastEditedQueryDescription: "Stale report description", + lastEditedQueryBody: "SELECT 'stale';", + lastEditedQueryLoggingType: "snapshot", + lastEditedQueryDiscardData: false, + }, + }, + }); + const { user } = render(); + + // name + description + expect(await screen.findByText("Fresh report name")).toBeInTheDocument(); + expect(screen.getByText("Fresh report description")).toBeInTheDocument(); + expect(screen.queryByText("Stale report name")).not.toBeInTheDocument(); + expect( + screen.queryByText("Stale report description") + ).not.toBeInTheDocument(); + + // logging + discard_data (drive the report's caching state) + const noResults = screen.getByTestId("no-results"); + expect(noResults).toHaveAttribute("data-discard", "true"); + expect(noResults).toHaveAttribute("data-snapshot", "false"); + + // query (shown via the "Show query" modal) + await user.click(screen.getByRole("button", { name: "Show query" })); + expect(screen.getByTestId("show-query-modal")).toHaveTextContent( + "SELECT 'fresh';" + ); + expect(screen.getByTestId("show-query-modal")).not.toHaveTextContent( + "SELECT 'stale';" + ); + }); + + it("derives Live report visibility from the loaded query's observer_can_run, not stale context", async () => { + mockServer.use( + http.get(baseUrl(`/reports/${QUERY_ID}`), () => + HttpResponse.json({ + query: createMockSchedulableQuery({ + id: QUERY_ID, + team_id: null, + observer_can_run: true, + }), + }) + ), + http.get(baseUrl(`/reports/${QUERY_ID}/report`), () => + HttpResponse.json( + createMockQueryReport({ query_id: QUERY_ID, results: [] }) + ) + ) + ); + + const render = createCustomRenderer({ + withBackendMock: true, + context: { + // A plain observer: the only thing that can grant live-query access here + // is the query's own observer_can_run, so the button proves the source. + app: { + ...baseAppContext, + isGlobalAdmin: false, + isGlobalMaintainer: false, + isObserverPlus: false, + isGlobalTechnician: false, + isTeamMaintainerOrTeamAdmin: false, + isTeamTechnician: false, + isOnGlobalTeam: false, + currentUser: createMockUser({ global_role: "observer" }), + }, + query: { lastEditedQueryObserverCanRun: false }, + }, + }); + render(); + + expect( + await screen.findByRole("button", { name: /Live report/i }) + ).toBeInTheDocument(); + }); +}); + describe("QueryDetailsPage - back navigation", () => { beforeEach(() => setupQueryHandlers()); diff --git a/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx b/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx index b4270419b3..163ff5ab16 100644 --- a/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx +++ b/frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx @@ -5,7 +5,6 @@ import { useErrorHandler } from "react-error-boundary"; import PATHS from "router/paths"; import { AppContext } from "context/app"; -import { QueryContext } from "context/query"; import { IGetQueryResponse, @@ -110,25 +109,6 @@ const QueryDetailsPage = ({ isGlobalTechnician, isTeamTechnician, } = useContext(AppContext); - const { - lastEditedQueryName, - lastEditedQueryDescription, - lastEditedQueryBody, - lastEditedQueryObserverCanRun, - lastEditedQueryDiscardData, - lastEditedQueryLoggingType, - setLastEditedQueryId, - setLastEditedQueryName, - setLastEditedQueryDescription, - setLastEditedQueryBody, - setLastEditedQueryObserverCanRun, - setLastEditedQueryFrequency, - setLastEditedQueryLoggingType, - setLastEditedQueryMinOsqueryVersion, - setLastEditedQueryPlatforms, - setLastEditedQueryDiscardData, - } = useContext(QueryContext); - const [showQueryModal, setShowQueryModal] = useState(false); const [disabledCachingGlobally, setDisabledCachingGlobally] = useState(true); @@ -138,8 +118,6 @@ const QueryDetailsPage = ({ } }, [config]); - // disabled on page load so we can control the number of renders - // else it will re-populate the context on occasion const { isLoading: isStoredQueryLoading, data: storedQuery, @@ -151,18 +129,6 @@ const QueryDetailsPage = ({ enabled: !!queryId, refetchOnWindowFocus: false, select: (data) => data.query, - onSuccess: (returnedQuery) => { - setLastEditedQueryId(returnedQuery.id); - setLastEditedQueryName(returnedQuery.name); - setLastEditedQueryDescription(returnedQuery.description); - setLastEditedQueryBody(returnedQuery.query); - setLastEditedQueryObserverCanRun(returnedQuery.observer_can_run); - setLastEditedQueryFrequency(returnedQuery.interval); - setLastEditedQueryPlatforms(returnedQuery.platform); - setLastEditedQueryLoggingType(returnedQuery.logging); - setLastEditedQueryMinOsqueryVersion(returnedQuery.min_osquery_version); - setLastEditedQueryDiscardData(returnedQuery.discard_data); - }, onError: (error) => handlePageError(error), } ); @@ -192,7 +158,9 @@ const QueryDetailsPage = ({ data: queryReport, error: queryReportError, } = useQuery( - [], + // Key must include every queryFn parameter; an empty key bled one report's + // cached rows into another on revisit (and suppressed refetch on sort). + ["queryReport", queryId, currentTeamId, serverSortBy], () => queryReportAPI.load({ teamId: currentTeamId, @@ -236,7 +204,7 @@ const QueryDetailsPage = ({ const isLiveQueryDisabled = config?.server_settings.live_query_disabled; const canLiveQuery = - lastEditedQueryObserverCanRun || + storedQuery?.observer_can_run || isObserverPlus || isGlobalAdmin || isGlobalMaintainer || @@ -280,7 +248,7 @@ const QueryDetailsPage = ({

- {lastEditedQueryName} + {storedQuery?.name}

@@ -346,7 +314,7 @@ const QueryDetailsPage = ({
@@ -406,9 +374,10 @@ const QueryDetailsPage = ({ ); const renderReport = () => { - const loggingSnapshot = lastEditedQueryLoggingType === "snapshot"; + const discardData = !!storedQuery?.discard_data; + const loggingSnapshot = storedQuery?.logging === "snapshot"; const disabledCaching = - disabledCachingGlobally || lastEditedQueryDiscardData || !loggingSnapshot; + disabledCachingGlobally || discardData || !loggingSnapshot; const emptyCache = (queryReport?.results?.length ?? 0) === 0; if (isLoading) { @@ -420,7 +389,7 @@ const QueryDetailsPage = ({ } // Empty state with varying messages explaining why there's no results - if (emptyCache || lastEditedQueryDiscardData) { + if (emptyCache || discardData) { return ( @@ -453,7 +423,7 @@ const QueryDetailsPage = ({ {renderReport()} {showQueryModal && ( )} diff --git a/frontend/pages/queries/details/components/QueryReport/QueryReport.tsx b/frontend/pages/queries/details/components/QueryReport/QueryReport.tsx index f590b4a550..90c8616e8e 100644 --- a/frontend/pages/queries/details/components/QueryReport/QueryReport.tsx +++ b/frontend/pages/queries/details/components/QueryReport/QueryReport.tsx @@ -1,8 +1,7 @@ -import React, { useState, useContext, useMemo, useCallback } from "react"; +import React, { useState, useMemo, useCallback } from "react"; import { Row, Column } from "react-table"; import FileSaver from "file-saver"; -import { QueryContext } from "context/query"; import { generateCSVFilename, @@ -25,6 +24,7 @@ import generateReportColumnConfigsFromResults from "./QueryReportTableConfig"; interface IQueryReportProps { queryReport?: IQueryReport; queryId: number; + queryName?: string; isClipped?: boolean; canLiveQuery?: boolean; } @@ -49,11 +49,10 @@ const flattenResults = (results: IQueryReportResultRow[]) => { const QueryReport = ({ queryReport, queryId, + queryName, isClipped, canLiveQuery, }: IQueryReportProps): JSX.Element => { - const { lastEditedQueryName } = useContext(QueryContext); - const [filteredResults, setFilteredResults] = useState( flattenResults(queryReport?.results || []) ); @@ -72,7 +71,7 @@ const QueryReport = ({ FileSaver.saveAs( generateCSVQueryResults( filteredResults, - generateCSVFilename(`${lastEditedQueryName || CSV_TITLE} - Report`), + generateCSVFilename(`${queryName || CSV_TITLE} - Report`), columnConfigs ) );