Skip to content

Commit e853cd3

Browse files
northdpolecursoragent
northdpole
and
cursoragent
committed
fix: address CodeRabbit review on Explorer pagination PR
Fix buildTree sibling keyPath mutation, serialize loadPage via promise chain with exposed dataLoadError, hoist Explorer layout wrappers, surface load failures in graph views, restore viewport zoom, and harden pagination link parity test. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 694f61e commit e853cd3

9 files changed

Lines changed: 116 additions & 56 deletions

File tree

‎application/frontend/src/index.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<meta charset="utf-8" />
55
<meta
66
name="viewport"
7-
content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=1, user-scalable=no"
7+
content="width=device-width, initial-scale=1"
88
/>
99
<meta
1010
name="description"

‎application/frontend/src/pages/Explorer/explorer.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ import { GraphDebugPanel } from './GraphDebugPanel';
1313
import { LinkedStandards } from './LinkedStandards';
1414

1515
export const Explorer = () => {
16-
const { dataLoading, dataTree, dataStore, hasMore, isLoadingMore, loadNextPage } = useDataStore();
16+
const { dataLoading, dataTree, dataStore, hasMore, isLoadingMore, loadNextPage, dataLoadError } =
17+
useDataStore();
1718
const [loading, setLoading] = useState<boolean>(false);
1819
const [filter, setFilter] = useState('');
1920
const [filteredTree, setFilteredTree] = useState<TreeDocument[]>();
@@ -212,7 +213,7 @@ export const Explorer = () => {
212213

213214
{debugMode && <GraphDebugPanel dataStore={dataStore} />}
214215

215-
<LoadingAndErrorIndicator loading={loading} error={null} />
216+
<LoadingAndErrorIndicator loading={loading || dataLoading} error={dataLoadError} />
216217
<List>
217218
{filteredTree?.map((item) => {
218219
return processNode(item);

‎application/frontend/src/pages/Explorer/visuals/circles/circles.tsx‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { Button, Icon } from 'semantic-ui-react';
1010
export const ExplorerCircles = () => {
1111
const { height, width } = useWindowDimensions();
1212
const [useFullScreen, setUseFullScreen] = useState(false);
13-
const { dataLoading, dataTree, ensureFullExplorerData, fullLoadProgress } = useDataStore();
13+
const { dataLoading, dataTree, ensureFullExplorerData, fullLoadProgress, dataLoadError } = useDataStore();
1414
const [breadcrumb, setBreadcrumb] = useState<string[]>([]);
1515
const svgRef = React.useRef(null);
1616

@@ -22,7 +22,15 @@ export const ExplorerCircles = () => {
2222
const margin = 20;
2323

2424
useEffect(() => {
25-
ensureFullExplorerData();
25+
let active = true;
26+
void ensureFullExplorerData().catch((err) => {
27+
if (active) {
28+
console.error('Failed to load explorer graph data', err);
29+
}
30+
});
31+
return () => {
32+
active = false;
33+
};
2634
}, [ensureFullExplorerData]);
2735

2836
const defaultSize = width > height ? height - 100 : width;
@@ -451,7 +459,7 @@ export const ExplorerCircles = () => {
451459
<g transform={`translate(${size / 2},${size / 2})`}></g>
452460
</svg>
453461
</div>
454-
<LoadingAndErrorIndicator loading={dataLoading || !!fullLoadProgress} error={null} />
462+
<LoadingAndErrorIndicator loading={dataLoading || !!fullLoadProgress} error={dataLoadError} />
455463
{fullLoadProgress && (
456464
<p className="explorer-full-load-progress">Loading graph data ({fullLoadProgress})…</p>
457465
)}

‎application/frontend/src/pages/Explorer/visuals/force-graph/forceGraph.tsx‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,15 @@ export const ExplorerForceGraph = () => {
2222
const [ignoreTypes, setIgnoreTypes] = useState(['same']);
2323
const [maxCount, setMaxCount] = useState(0);
2424
const [maxNodeSize, setMaxNodeSize] = useState(0);
25-
const { dataLoading, dataTree, getStoreKey, dataStore, ensureFullExplorerData, fullLoadProgress } =
26-
useDataStore();
25+
const {
26+
dataLoading,
27+
dataTree,
28+
getStoreKey,
29+
dataStore,
30+
ensureFullExplorerData,
31+
fullLoadProgress,
32+
dataLoadError,
33+
} = useDataStore();
2734
const fgRef = useRef<ForceGraphMethods>();
2835
// ADDING STATE FOR FILTERING LOGIC
2936
const [filterTypeA, setFilterTypeA] = useState('');
@@ -34,7 +41,15 @@ export const ExplorerForceGraph = () => {
3441
const [combinedOptions, setCombinedOptions] = useState<DropdownOption[]>([]);
3542

3643
useEffect(() => {
37-
ensureFullExplorerData();
44+
let active = true;
45+
void ensureFullExplorerData().catch((err) => {
46+
if (active) {
47+
console.error('Failed to load explorer graph data', err);
48+
}
49+
});
50+
return () => {
51+
active = false;
52+
};
3853
}, [ensureFullExplorerData]);
3954

4055
// Adding a show all checkbox
@@ -436,7 +451,7 @@ export const ExplorerForceGraph = () => {
436451
}, [graphData]);
437452
return (
438453
<div>
439-
<LoadingAndErrorIndicator loading={dataLoading || !!fullLoadProgress} error={null} />
454+
<LoadingAndErrorIndicator loading={dataLoading || !!fullLoadProgress} error={dataLoadError} />
440455
{fullLoadProgress && (
441456
<p className="explorer-full-load-progress">Loading graph data ({fullLoadProgress})…</p>
442457
)}

‎application/frontend/src/providers/DataProvider.tsx‎

Lines changed: 63 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ type DataContextValues = {
3333
hasMore: boolean;
3434
isLoadingMore: boolean;
3535
fullLoadProgress: string | null;
36+
dataLoadError: Error | null;
3637
loadNextPage: () => Promise<void>;
3738
ensureFullExplorerData: () => Promise<void>;
3839
};
@@ -71,12 +72,14 @@ export const DataProvider = ({ children }: { children: React.ReactNode }) => {
7172
const [isLoadingMore, setIsLoadingMore] = useState<boolean>(false);
7273
const [isFullStoreLoaded, setIsFullStoreLoaded] = useState<boolean>(false);
7374
const [fullLoadProgress, setFullLoadProgress] = useState<string | null>(null);
75+
const [dataLoadError, setDataLoadError] = useState<Error | null>(null);
7476

7577
const dataStoreRef = useRef(dataStore);
7678
const loadedPagesRef = useRef(loadedPages);
7779
const totalPagesRef = useRef(totalPages);
7880
const isFullStoreLoadedRef = useRef(isFullStoreLoaded);
7981
const loadingPageRef = useRef(false);
82+
const loadChainRef = useRef(Promise.resolve<Record<string, TreeDocument>>({}));
8083
const fullLoadRef = useRef<Promise<void> | null>(null);
8184
const bootstrapDoneRef = useRef(false);
8285

@@ -102,23 +105,23 @@ export const DataProvider = ({ children }: { children: React.ReactNode }) => {
102105
const buildTree = useCallback(
103106
(doc: Document, store: Record<string, TreeDocument>, keyPath: string[] = []): TreeDocument => {
104107
const selfKey = getStoreKey(doc);
105-
keyPath.push(selfKey);
108+
const nextPath = [...keyPath, selfKey];
106109
const storedDoc = structuredClone(store[selfKey] ?? doc);
107110
const initialLinks = storedDoc.links || [];
108111
let creLinks = initialLinks.filter(
109-
(x) => !!x.document && !keyPath.includes(getStoreKey(x.document)) && getStoreKey(x.document) in store
112+
(x) => !!x.document && !nextPath.includes(getStoreKey(x.document)) && getStoreKey(x.document) in store
110113
);
111114
creLinks = creLinks.filter((x) => x.ltype === 'Contains');
112115
creLinks = creLinks.map((x) => ({
113116
ltype: x.ltype,
114-
document: buildTree(x.document, store, keyPath),
117+
document: buildTree(x.document, store, nextPath),
115118
}));
116119
storedDoc.links = [...creLinks];
117120
const standards = initialLinks.filter(
118121
(link) =>
119122
link.document &&
120123
link.document.doctype === 'Standard' &&
121-
!keyPath.includes(getStoreKey(link.document))
124+
!nextPath.includes(getStoreKey(link.document))
122125
);
123126
storedDoc.links = [...creLinks, ...standards];
124127
return storedDoc;
@@ -154,51 +157,68 @@ export const DataProvider = ({ children }: { children: React.ReactNode }) => {
154157
setDataTree([]);
155158
return [];
156159
}
157-
const result = await axios.get(`${apiUrl}/root_cres`);
158-
const treeData = result.data.data.map((x: Document) => buildTree(x, store));
159-
setDataTree(treeData);
160-
return treeData;
160+
try {
161+
const result = await axios.get(`${apiUrl}/root_cres`);
162+
const treeData = result.data.data.map((x: Document) => buildTree(x, store));
163+
setDataTree(treeData);
164+
return treeData;
165+
} catch (err) {
166+
const error = err instanceof Error ? err : new Error('Failed to load explorer tree');
167+
setDataLoadError(error);
168+
throw error;
169+
}
161170
},
162171
[apiUrl, buildTree]
163172
);
164173

165174
const loadPage = useCallback(
166175
async (page: number): Promise<Record<string, TreeDocument>> => {
167-
if (loadingPageRef.current) {
168-
return dataStoreRef.current;
169-
}
170-
loadingPageRef.current = true;
171-
setIsLoadingMore(true);
172-
try {
173-
const result = await axios.get(`${apiUrl}/all_cres`, {
174-
params: { page, per_page: EXPLORER_CRE_PAGE_SIZE },
175-
});
176-
const docs: Document[] = result.data.data || [];
177-
const pagesTotal = Number(result.data.total_pages) || 1;
178-
const nextStore = mergeDocsIntoStore(docs, dataStoreRef.current, getStoreKey);
179-
const pagesLoaded = page;
180-
181-
dataStoreRef.current = nextStore;
182-
setDataStore(nextStore);
183-
setLoadedPages(pagesLoaded);
184-
setTotalPages(pagesTotal);
185-
loadedPagesRef.current = pagesLoaded;
186-
totalPagesRef.current = pagesTotal;
187-
188-
const fullLoaded = pagesLoaded >= pagesTotal;
189-
if (fullLoaded) {
190-
isFullStoreLoadedRef.current = true;
191-
setIsFullStoreLoaded(true);
176+
const nextLoad = loadChainRef.current.then(async () => {
177+
if (isFullStoreLoadedRef.current || (loadedPagesRef.current >= page && loadedPagesRef.current > 0)) {
178+
return dataStoreRef.current;
192179
}
193180

194-
const treeData = await rebuildDataTree(nextStore);
195-
await persistCache(nextStore, pagesLoaded, pagesTotal, fullLoaded, treeData);
181+
loadingPageRef.current = true;
182+
setIsLoadingMore(true);
183+
try {
184+
const result = await axios.get(`${apiUrl}/all_cres`, {
185+
params: { page, per_page: EXPLORER_CRE_PAGE_SIZE },
186+
});
187+
const docs: Document[] = result.data.data || [];
188+
const pagesTotal = Number(result.data.total_pages) || 1;
189+
const nextStore = mergeDocsIntoStore(docs, dataStoreRef.current, getStoreKey);
190+
const pagesLoaded = page;
191+
192+
dataStoreRef.current = nextStore;
193+
setDataStore(nextStore);
194+
setLoadedPages(pagesLoaded);
195+
setTotalPages(pagesTotal);
196+
loadedPagesRef.current = pagesLoaded;
197+
totalPagesRef.current = pagesTotal;
198+
setDataLoadError(null);
199+
200+
const fullLoaded = pagesLoaded >= pagesTotal;
201+
if (fullLoaded) {
202+
isFullStoreLoadedRef.current = true;
203+
setIsFullStoreLoaded(true);
204+
}
205+
206+
const treeData = await rebuildDataTree(nextStore);
207+
await persistCache(nextStore, pagesLoaded, pagesTotal, fullLoaded, treeData);
208+
209+
return nextStore;
210+
} catch (err) {
211+
const error = err instanceof Error ? err : new Error('Failed to load CRE data');
212+
setDataLoadError(error);
213+
throw error;
214+
} finally {
215+
loadingPageRef.current = false;
216+
setIsLoadingMore(false);
217+
}
218+
});
196219

197-
return nextStore;
198-
} finally {
199-
loadingPageRef.current = false;
200-
setIsLoadingMore(false);
201-
}
220+
loadChainRef.current = nextLoad.catch(() => dataStoreRef.current);
221+
return nextLoad;
202222
},
203223
[apiUrl, getStoreKey, persistCache, rebuildDataTree]
204224
);
@@ -289,6 +309,8 @@ export const DataProvider = ({ children }: { children: React.ReactNode }) => {
289309
} else if (Object.keys(dataStoreRef.current).length) {
290310
await rebuildDataTree(dataStoreRef.current);
291311
}
312+
} catch {
313+
// loadPage/rebuildDataTree already set dataLoadError
292314
} finally {
293315
setDataLoading(false);
294316
}
@@ -332,6 +354,7 @@ export const DataProvider = ({ children }: { children: React.ReactNode }) => {
332354
hasMore,
333355
isLoadingMore,
334356
fullLoadProgress,
357+
dataLoadError,
335358
loadNextPage,
336359
ensureFullExplorerData,
337360
}}

‎application/frontend/src/routes.tsx‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ import { MyOpenCRE } from './pages/MyOpenCRE/MyOpenCRE';
2626
import { SearchName } from './pages/Search/SearchName';
2727
import { StandardSection } from './pages/Standard/StandardSection';
2828

29+
const ExplorerWithLayout = withExplorerLayout(Explorer);
30+
const ExplorerCirclesWithLayout = withExplorerLayout(ExplorerCircles);
31+
const ExplorerForceGraphWithLayout = withExplorerLayout(ExplorerForceGraph);
32+
2933
export interface IRoute {
3034
path: string;
3135
component: ReactNode | ReactNode[];
@@ -113,17 +117,17 @@ export const ROUTES = (capabilities: Capabilities): IRoute[] => [
113117
},
114118
{
115119
path: `${EXPLORER}/circles`,
116-
component: withExplorerLayout(ExplorerCircles),
120+
component: ExplorerCirclesWithLayout,
117121
showFilter: false,
118122
},
119123
{
120124
path: `${EXPLORER}/force_graph`,
121-
component: withExplorerLayout(ExplorerForceGraph),
125+
component: ExplorerForceGraphWithLayout,
122126
showFilter: false,
123127
},
124128
{
125129
path: `${EXPLORER}`,
126-
component: withExplorerLayout(Explorer),
130+
component: ExplorerWithLayout,
127131
showFilter: false,
128132
},
129133
];

‎application/frontend/www/bundle.js‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎application/frontend/www/index.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
<!doctype html><head><meta charset="utf-8"/><meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1,maximum-scale=1,user-scalable=no"/><meta name="description" content="Open CRE - Open Cybersecurity Requirement Expression: Link security requirements across standards and frameworks."/><title>Open CRE</title><link rel="icon" type="image/svg+xml" href="/logo.svg"/><link rel="alternate icon" href="/favicon.ico"/><link rel="stylesheet" href="//cdn.jsdelivr.net/npm/semantic-ui@2.4.1/dist/semantic.min.css"/><script defer="defer" src="/bundle.js"></script></head><body><div id="mount"></div></body>
1+
<!doctype html><head><meta charset="utf-8"/><meta name="viewport" content="width=device-width,initial-scale=1"/><meta name="description" content="Open CRE - Open Cybersecurity Requirement Expression: Link security requirements across standards and frameworks."/><title>Open CRE</title><link rel="icon" type="image/svg+xml" href="/logo.svg"/><link rel="alternate icon" href="/favicon.ico"/><link rel="stylesheet" href="//cdn.jsdelivr.net/npm/semantic-ui@2.4.1/dist/semantic.min.css"/><script defer="defer" src="/bundle.js"></script></head><body><div id="mount"></div></body>

‎application/tests/db_test.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2531,7 +2531,16 @@ def test_all_cres_with_pagination_hydrates_full_link_graph(self) -> None:
25312531

25322532
for paginated_cre in paginated_cres:
25332533
expected = collection.get_CREs(external_id=paginated_cre.id)[0]
2534-
self.assertEqual(expected.todict(), paginated_cre.todict())
2534+
expected_dict = expected.todict()
2535+
actual_dict = paginated_cre.todict()
2536+
self.assertEqual(expected_dict["id"], actual_dict["id"])
2537+
self.assertEqual(expected_dict["name"], actual_dict["name"])
2538+
self.assertEqual(
2539+
expected_dict.get("description"), actual_dict.get("description")
2540+
)
2541+
self.assertCountEqual(
2542+
expected_dict.get("links", []), actual_dict.get("links", [])
2543+
)
25352544

25362545
child1_paginated = next(c for c in paginated_cres if c.id == "101-101")
25372546
link_types = {link.ltype for link in child1_paginated.links}

0 commit comments

Comments
 (0)