Address code review on documents and UI skeletons

Fold the review feedback from the documents page work into the shared
components and helpers:

- Toaster: give the close control the kit's focus-visible ring, and wrap
  the title/description in Toast.Content so stacked toasts get Base UI's
  height measurement and overflow handling.
- TabsSkeleton: spread rest before the fixed aria-hidden so the
  decorative subtree can't be exposed to assistive tech.
- DocumentsPage: reconcile the active tab against the tab the preloaded
  query actually loaded with, so a tab change during the initial preload
  no longer shows the wrong slice.
- Grouping: drop the duplicated bucketing helpers and reuse the shared
  groupBy from @probo/helpers, keeping field-specific sorting/labeling in
  the callers.
- Documents list items: extract a shared DocumentEntry row and a
  useExportAndOpen hook so the three item components stop repeating the
  export/access behavior while keeping their own fragments.
- Subprocessors skeleton: reuse SelectSkeleton/TextFieldSkeleton instead
  of hand-rolled placeholders.
- useDocumentTab: derive DocumentTab and URL validation from a single
  DOCUMENT_TABS source of truth.

Signed-off-by: Émile Ré <emile@probo.com>
This commit is contained in:
Émile Ré
2026-07-16 09:57:52 +02:00
parent 4c599c17f4
commit 99990d6f6b
13 changed files with 209 additions and 180 deletions

View File

@@ -18,6 +18,7 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { groupBy } from "@probo/helpers";
import { useEffect, useRef, useTransition } from "react";
import { useTranslation } from "react-i18next";
import type { PreloadedQuery } from "react-relay";
@@ -35,7 +36,6 @@ import { DocumentSection } from "./_components/DocumentSection";
import { DocumentsEmpty } from "./_components/DocumentsEmpty";
import { DocumentsToolbar } from "./_components/DocumentsToolbar";
import { TrustCenterFileListItem } from "./_components/TrustCenterFileListItem";
import { groupByField } from "./_lib/groupByField";
import { toQueryVariables } from "./_lib/toQueryVariables";
import { useDocumentTab } from "./_lib/useDocumentTab";
import { documentsLayout } from "./variants";
@@ -102,15 +102,19 @@ export function DocumentsPage({ queryRef }: DocumentsPageProps) {
const { tab } = useDocumentTab();
const [isRefetching, startTransition] = useTransition();
// The initial query already loaded with the URL's tab; only refetch on
// subsequent tab changes, inside a transition so the toolbar and current
// results stay mounted (dimmed via `isRefetching`) while the slice loads.
const isFirstRender = useRef(true);
// Keep the displayed slice in sync with the active tab. Seed from the tab the
// preloaded query actually loaded with (`queryRef.variables`) instead of
// assuming the first render matches the URL: if the tab changed while the
// initial preload was in flight, this reconciles by refetching rather than
// showing the wrong slice. Refetch inside a transition so the toolbar and
// current results stay mounted (dimmed via `isRefetching`) while it loads.
const fetchedVisibility = useRef(queryRef.variables.visibility ?? null);
useEffect(() => {
if (isFirstRender.current) {
isFirstRender.current = false;
const target = toQueryVariables(tab).visibility ?? null;
if (target === fetchedVisibility.current) {
return;
}
fetchedVisibility.current = target;
startTransition(() => {
refetch(toQueryVariables(tab), { fetchPolicy: "store-or-network" });
});
@@ -125,9 +129,11 @@ export function DocumentsPage({ queryRef }: DocumentsPageProps) {
const total = documentNodes.length + fileNodes.length + auditNodes.length;
const documentGroups = groupByField(documentNodes, node => node.documentType)
const documentGroups = Object.entries(groupBy(documentNodes, node => node.documentType))
.map(([key, nodes]) => ({ key, nodes }))
.sort((a, b) => t(`types.${a.key}`).localeCompare(t(`types.${b.key}`)));
const fileGroups = groupByField(fileNodes, node => node.category)
const fileGroups = Object.entries(groupBy(fileNodes, node => node.category))
.map(([key, nodes]) => ({ key, nodes }))
.sort((a, b) => a.key.localeCompare(b.key));
const { page, results } = documentsLayout({ busy: isRefetching });

View File

@@ -18,17 +18,13 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { Text } from "@probo/ui/src/v2/typography/Text";
import { graphql, useFragment } from "react-relay";
import { useMutation } from "#/lib/relay/useMutation";
import { openExportedFile } from "../_lib/openExportedFile";
import { useExportAndOpen } from "../_lib/useExportAndOpen";
import type { AuditReportListItem_audit$key } from "./__generated__/AuditReportListItem_audit.graphql";
import type { AuditReportListItemExportMutation } from "./__generated__/AuditReportListItemExportMutation.graphql";
import { DocumentAccessAction } from "./DocumentAccessAction";
import { documentListItem } from "./variants";
import { DocumentEntry } from "./DocumentEntry";
const auditReportListItemFragment = graphql`
fragment AuditReportListItem_audit on Audit @throwOnFieldError {
@@ -58,44 +54,29 @@ interface AuditReportListItemProps {
auditKey: AuditReportListItem_audit$key;
}
// A single audit report row: the framework name, the report file name, and an
// A single audit report entry: the framework name, the report file name, and an
// access action that opens the exported report when the viewer is authorized.
// Renders nothing when the audit has no report file.
export function AuditReportListItem({ auditKey }: AuditReportListItemProps) {
const audit = useFragment(auditReportListItemFragment, auditKey);
const [exportReport, isExporting] = useMutation<AuditReportListItemExportMutation>(exportReportMutation);
const { root, content } = documentListItem();
const [openReport, isExporting] = useExportAndOpen<AuditReportListItemExportMutation>(
exportReportMutation,
response => response.exportReportPDF.data,
);
const report = audit.reportFile;
if (report == null) {
return null;
}
const handleView = () => {
exportReport({
variables: { input: { reportId: report.id } },
onCompleted: response => openExportedFile(response.exportReportPDF.data),
}).catch(() => {
// The mutation failure is already surfaced through a toast.
});
};
return (
<div className={root()}>
<div className={content()}>
<Text size={2} weight="medium" color="neutral" highContrast className="truncate">
{audit.framework.name}
</Text>
<Text size={1} color="gold" className="truncate">
{report.fileName}
</Text>
</div>
<DocumentAccessAction
isAuthorized={report.isUserAuthorized}
requested={report.access?.status === "REQUESTED"}
onView={handleView}
isViewing={isExporting}
/>
</div>
<DocumentEntry
title={audit.framework.name}
meta={report.fileName}
isAuthorized={report.isUserAuthorized}
requested={report.access?.status === "REQUESTED"}
onView={() => openReport({ input: { reportId: report.id } })}
isViewing={isExporting}
/>
);
}

View File

@@ -0,0 +1,66 @@
// Copyright (c) 2026 Probo Inc <hello@probo.com>.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { Text } from "@probo/ui/src/v2/typography/Text";
import type { ReactNode } from "react";
import { DocumentAccessAction } from "./DocumentAccessAction";
import { documentListItem } from "./variants";
interface DocumentEntryProps {
// Primary line (document title, file name, or framework name).
title: ReactNode;
// Accent sub-label (document type, file category, or report file name).
meta: ReactNode;
// Whether the viewer may open the entry (public or granted access).
isAuthorized: boolean;
// Whether an access request is already pending for the entry.
requested: boolean;
// Opens the entry; only invoked when authorized.
onView: () => void;
// Whether the export/open is in flight.
isViewing: boolean;
}
// Presentational row shared by the document / file / report list items: a title
// with accent metadata and the trailing access action. The connection-item
// wrappers own their fragments and supply these values.
export function DocumentEntry({ title, meta, isAuthorized, requested, onView, isViewing }: DocumentEntryProps) {
const { root, content } = documentListItem();
return (
<div className={root()}>
<div className={content()}>
<Text size={2} weight="medium" color="neutral" highContrast className="truncate">
{title}
</Text>
<Text size={1} color="gold" className="truncate">
{meta}
</Text>
</div>
<DocumentAccessAction
isAuthorized={isAuthorized}
requested={requested}
onView={onView}
isViewing={isViewing}
/>
</div>
);
}

View File

@@ -18,18 +18,14 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { Text } from "@probo/ui/src/v2/typography/Text";
import { useTranslation } from "react-i18next";
import { graphql, useFragment } from "react-relay";
import { useMutation } from "#/lib/relay/useMutation";
import { openExportedFile } from "../_lib/openExportedFile";
import { useExportAndOpen } from "../_lib/useExportAndOpen";
import type { DocumentListItem_document$key } from "./__generated__/DocumentListItem_document.graphql";
import type { DocumentListItemExportMutation } from "./__generated__/DocumentListItemExportMutation.graphql";
import { DocumentAccessAction } from "./DocumentAccessAction";
import { documentListItem } from "./variants";
import { DocumentEntry } from "./DocumentEntry";
const documentListItemFragment = graphql`
fragment DocumentListItem_document on Document @throwOnFieldError {
@@ -55,39 +51,24 @@ interface DocumentListItemProps {
documentKey: DocumentListItem_document$key;
}
// A single Probo document row: title, its document type, and an access action
// A single Probo document entry: title, its document type, and an access action
// that opens the exported PDF when the viewer is authorized.
export function DocumentListItem({ documentKey }: DocumentListItemProps) {
const { t } = useTranslation("documents");
const document = useFragment(documentListItemFragment, documentKey);
const [exportDocument, isExporting] = useMutation<DocumentListItemExportMutation>(exportDocumentMutation);
const { root, content } = documentListItem();
const handleView = () => {
exportDocument({
variables: { input: { documentId: document.id } },
onCompleted: response => openExportedFile(response.exportDocumentPDF.data),
}).catch(() => {
// The mutation failure is already surfaced through a toast.
});
};
const [openDocument, isExporting] = useExportAndOpen<DocumentListItemExportMutation>(
exportDocumentMutation,
response => response.exportDocumentPDF.data,
);
return (
<div className={root()}>
<div className={content()}>
<Text size={2} weight="medium" color="neutral" highContrast className="truncate">
{document.title}
</Text>
<Text size={1} color="gold" className="truncate">
{t(`types.${document.documentType}`)}
</Text>
</div>
<DocumentAccessAction
isAuthorized={document.isUserAuthorized}
requested={document.access?.status === "REQUESTED"}
onView={handleView}
isViewing={isExporting}
/>
</div>
<DocumentEntry
title={document.title}
meta={t(`types.${document.documentType}`)}
isAuthorized={document.isUserAuthorized}
requested={document.access?.status === "REQUESTED"}
onView={() => openDocument({ input: { documentId: document.id } })}
isViewing={isExporting}
/>
);
}

View File

@@ -18,17 +18,13 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { Text } from "@probo/ui/src/v2/typography/Text";
import { graphql, useFragment } from "react-relay";
import { useMutation } from "#/lib/relay/useMutation";
import { openExportedFile } from "../_lib/openExportedFile";
import { useExportAndOpen } from "../_lib/useExportAndOpen";
import type { TrustCenterFileListItem_file$key } from "./__generated__/TrustCenterFileListItem_file.graphql";
import type { TrustCenterFileListItemExportMutation } from "./__generated__/TrustCenterFileListItemExportMutation.graphql";
import { DocumentAccessAction } from "./DocumentAccessAction";
import { documentListItem } from "./variants";
import { DocumentEntry } from "./DocumentEntry";
const trustCenterFileListItemFragment = graphql`
fragment TrustCenterFileListItem_file on TrustCenterFile @throwOnFieldError {
@@ -54,38 +50,23 @@ interface TrustCenterFileListItemProps {
fileKey: TrustCenterFileListItem_file$key;
}
// A single uploaded trust-center file row: name, its category, and an access
// A single uploaded trust-center file entry: name, its category, and an access
// action that opens the exported file when the viewer is authorized.
export function TrustCenterFileListItem({ fileKey }: TrustCenterFileListItemProps) {
const file = useFragment(trustCenterFileListItemFragment, fileKey);
const [exportFile, isExporting] = useMutation<TrustCenterFileListItemExportMutation>(exportTrustCenterFileMutation);
const { root, content } = documentListItem();
const handleView = () => {
exportFile({
variables: { input: { trustCenterFileId: file.id } },
onCompleted: response => openExportedFile(response.exportTrustCenterFile.data),
}).catch(() => {
// The mutation failure is already surfaced through a toast.
});
};
const [openFile, isExporting] = useExportAndOpen<TrustCenterFileListItemExportMutation>(
exportTrustCenterFileMutation,
response => response.exportTrustCenterFile.data,
);
return (
<div className={root()}>
<div className={content()}>
<Text size={2} weight="medium" color="neutral" highContrast className="truncate">
{file.name}
</Text>
<Text size={1} color="gold" className="truncate">
{file.category}
</Text>
</div>
<DocumentAccessAction
isAuthorized={file.isUserAuthorized}
requested={file.access?.status === "REQUESTED"}
onView={handleView}
isViewing={isExporting}
/>
</div>
<DocumentEntry
title={file.name}
meta={file.category}
isAuthorized={file.isUserAuthorized}
requested={file.access?.status === "REQUESTED"}
onView={() => openFile({ input: { trustCenterFileId: file.id } })}
isViewing={isExporting}
/>
);
}

View File

@@ -1,45 +0,0 @@
// Copyright (c) 2026 Probo Inc <hello@probo.com>.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
export interface FieldGroup<T> {
key: string;
nodes: T[];
}
// Groups nodes by a derived string key, preserving first-seen order of the
// keys. Presentational only — the server already applied the visibility filter.
export function groupByField<T>(
nodes: readonly T[],
getKey: (node: T) => string,
): FieldGroup<T>[] {
const groups = new Map<string, T[]>();
for (const node of nodes) {
const key = getKey(node);
const existing = groups.get(key);
if (existing) {
existing.push(node);
} else {
groups.set(key, [node]);
}
}
return [...groups.entries()].map(([key, groupNodes]) => ({ key, nodes: groupNodes }));
}

View File

@@ -21,9 +21,19 @@
import { useCallback } from "react";
import { useSearchParams } from "react-router";
export type DocumentTab = "all" | "public" | "private";
// Single source of truth for the tab set. The type, the rendered tab list, and
// URL validation all derive from this so a new tab can't be shown/written but
// read back as the default.
export const DOCUMENT_TABS = ["all", "public", "private"] as const;
export const DOCUMENT_TABS: readonly DocumentTab[] = ["all", "public", "private"];
export type DocumentTab = (typeof DOCUMENT_TABS)[number];
// The default (no-filter) tab; kept out of the URL by `setTab`.
const DEFAULT_DOCUMENT_TAB: DocumentTab = "all";
function isDocumentTab(value: string | null): value is DocumentTab {
return value != null && (DOCUMENT_TABS as readonly string[]).includes(value);
}
interface DocumentTabState {
tab: DocumentTab;
@@ -37,12 +47,12 @@ export function useDocumentTab(): DocumentTabState {
const [searchParams, setSearchParams] = useSearchParams();
const raw = searchParams.get("tab");
const tab: DocumentTab = raw === "public" || raw === "private" ? raw : "all";
const tab: DocumentTab = isDocumentTab(raw) ? raw : DEFAULT_DOCUMENT_TAB;
const setTab = useCallback((value: DocumentTab) => {
setSearchParams((previous) => {
const next = new URLSearchParams(previous);
if (value === "all") {
if (value === DEFAULT_DOCUMENT_TAB) {
next.delete("tab");
} else {
next.set("tab", value);

View File

@@ -0,0 +1,51 @@
// Copyright (c) 2026 Probo Inc <hello@probo.com>.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { useCallback } from "react";
import type { GraphQLTaggedNode, MutationParameters } from "relay-runtime";
import { useMutation } from "#/lib/relay/useMutation";
import { openExportedFile } from "./openExportedFile";
// Shared "export then open" behavior for the document/file/report list items:
// commit the resource's export mutation, open the returned base64 payload, and
// let failures surface through the mutation notifier's toast. Each caller
// supplies its typed mutation and a selector for the payload string.
export function useExportAndOpen<T extends MutationParameters>(
mutation: GraphQLTaggedNode,
selectData: (response: T["response"]) => string,
): readonly [(variables: T["variables"]) => void, boolean] {
const [commit, isExporting] = useMutation<T>(mutation);
const open = useCallback(
(variables: T["variables"]) => {
commit({
variables,
onCompleted: response => openExportedFile(selectData(response)),
}).catch(() => {
// The mutation failure is already surfaced through a toast.
});
},
[commit, selectData],
);
return [open, isExporting];
}

View File

@@ -18,6 +18,8 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { TextFieldSkeleton } from "@probo/ui/src/v2/form/TextFieldSkeleton";
import { SelectSkeleton } from "@probo/ui/src/v2/Select/SelectSkeleton";
import { HeadingSkeleton } from "@probo/ui/src/v2/typography/HeadingSkeleton";
import { TextSkeleton } from "@probo/ui/src/v2/typography/TextSkeleton";
@@ -32,9 +34,9 @@ export function SubprocessorsPageSkeleton() {
<div className="flex w-full flex-col gap-2">
<HeadingSkeleton size={7} className="w-64" />
<div className="flex min-h-16 items-center gap-3">
<div className="h-8 w-40 animate-pulse rounded-2 bg-sand-3" />
<div className="h-8 w-40 animate-pulse rounded-2 bg-sand-3" />
<div className="h-8 w-60 animate-pulse rounded-2 bg-sand-3" />
<SelectSkeleton />
<SelectSkeleton />
<TextFieldSkeleton />
</div>
</div>
</HeaderBand>

View File

@@ -18,29 +18,22 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
import { groupBy } from "@probo/helpers";
export interface CategoryGroup<T> {
category: string;
nodes: T[];
}
// Groups subprocessor nodes into their (non-empty) categories, ordered by the
// localized category label. Presentational only — filtering happens server-side.
// localized category label. Bucketing uses the shared `groupBy` primitive; the
// category labeling/sorting is the subprocessor-specific part kept here.
// Presentational only — filtering happens server-side.
export function groupByCategory<T extends { readonly category: string }>(
nodes: readonly T[],
getLabel: (category: string) => string,
): CategoryGroup<T>[] {
const groups = new Map<string, T[]>();
for (const node of nodes) {
const existing = groups.get(node.category);
if (existing) {
existing.push(node);
} else {
groups.set(node.category, [node]);
}
}
return [...groups.entries()]
return Object.entries(groupBy([...nodes], node => node.category))
.map(([category, groupNodes]) => ({ category, nodes: groupNodes }))
.sort((a, b) => getLabel(a.category).localeCompare(getLabel(b.category)));
}

View File

@@ -34,7 +34,7 @@ export function TabsSkeleton(props: TabsSkeletonProps) {
const { root, item } = tabsSkeleton();
return (
<div className={root({ className })} aria-hidden {...rest}>
<div className={root({ className })} {...rest} aria-hidden>
{Array.from({ length: count }, (_, index) => (
<span key={index} className={item()} style={{ width: `${64 + (index % 3) * 16}px` }} />
))}

View File

@@ -66,12 +66,12 @@ export function Toaster() {
<span aria-hidden className={typed.icon()}>
{typeIcons[type]}
</span>
<div className={typed.content()}>
<Toast.Content className={typed.content()}>
<Toast.Title className={typed.title()} />
{toast.description != null && (
<Toast.Description className={typed.description()} />
)}
</div>
</Toast.Content>
<Toast.Close className={typed.close()} aria-label="Close">
<XIcon />
</Toast.Close>

View File

@@ -41,7 +41,10 @@ export const toaster = tv({
content: "flex min-w-0 flex-1 flex-col gap-1",
title: "text-2 font-medium",
description: "text-1",
close: "-mr-1 -mt-1 shrink-0 rounded-2 p-1 opacity-70 transition-opacity hover:opacity-100 [&_svg]:size-4",
close: [
"-mr-1 -mt-1 shrink-0 rounded-2 p-1 opacity-70 outline-none transition-opacity hover:opacity-100 [&_svg]:size-4",
"focus-visible:opacity-100 focus-visible:ring-2 focus-visible:ring-sand-8 focus-visible:ring-offset-1 focus-visible:ring-offset-sand-1",
],
},
variants: {
// Mirrors Callout's surface tokens: bg step 2, border step 6, text step 11,