Issue 80 - Updating bboxes array after rendering PDF doesn't create bbox on pdf #95
Conversation
📝 WalkthroughWalkthroughBumps package to 1.7.26 and refactors PdfPage to store the loaded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/pdfPage/PdfPage.tsx (1)
114-168: Avoid mutatingbboxListitems in place inside the effectInside the
bboxList.mapyou mutate eachbboxfrom props (e.g.,bbox.location = …). This was likely pre‑existing, but moving it into an effect that can run multiple times makes the side effects more noticeable and harder to reason about.To keep props immutable and avoid surprises for parents that reuse the same bbox objects, consider cloning before mutation:
- const errorBboxes = bboxList.map((bbox) => { + const errorBboxes = bboxList.map((bbox) => { + const nextBbox = { ...bbox }; let opData = operationData, posData = positionData, nMcidData = noMCIDData; … - if (bbox.mcidList) { - bbox.location = parseMcidToBbox(…); + if (nextBbox.mcidList) { + nextBbox.location = parseMcidToBbox(…); … - } else if (bbox.contentItemPath) { + } else if (nextBbox.contentItemPath) { … - bbox.location = [ + nextBbox.location = [ … ]; } catch (err) { console.log('NoMCIDDataParseError:', err.message || err); - bbox.location = [0, 0, 0, 0]; + nextBbox.location = [0, 0, 0, 0]; } } - if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { - bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); + if (_.isNumber(nextBbox.operatorIndex) && _.isNumber(nextBbox.glyphIndex)) { + nextBbox.location = getBboxForGlyph(nextBbox.operatorIndex, nextBbox.glyphIndex, opData, page.view, page.rotate, left, bottom); } - return bbox; + return nextBbox; });This keeps the component’s state updates self‑contained and avoids mutating upstream data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/public/pdf.worker.mjsis excluded by!**/dist/**
📒 Files selected for processing (2)
package.json(1 hunks)src/components/pdfPage/PdfPage.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/pdfPage/PdfPage.tsx (1)
src/services/bboxService.ts (4)
getFormattedAnnotations(65-82)createAllBboxes(247-272)parseMcidToBbox(584-638)getBboxForGlyph(563-582)
🔇 Additional comments (1)
package.json (1)
3-3: Version bump is consistent and scopedUpdating
"version"to1.7.26with no other manifest changes is fine for this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/pdfPage/PdfPage.tsx (1)
106-176: Structural bboxes won't render when error bboxes are empty.The effect gates the entire computation on
bboxList.length(line 107), so when there are no error bboxes but structural bboxes exist inprops.treeElementsBboxes, the else branch (lines 172-174) clearsbboxesAlland prevents structural bboxes from rendering. Decouple the logic socreateAllBboxesruns wheneverpageorprops.treeElementsBboxeschange, and only gate error-bbox computation onbboxList.length > 0.🔎 Proposed fix to decouple structural and error bbox computation
useEffect(() => { - if (page && bboxList.length) { + if (page) { Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => { const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3]; const operationData = operatorList.argsArray[operatorList.argsArray.length - 2]; const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1]; const annotsFormatted = getFormattedAnnotations(annotations); const allBboxes = createAllBboxes(props.treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate); - const errorBboxes = bboxList.map((bbox) => { + + const errorBboxes = bboxList.length > 0 ? bboxList.map((bbox) => { let opData = operationData, posData = positionData, nMcidData = noMCIDData; let left = 0, bottom = 0; const { annotIndex } = bbox; if (annotIndex != null) { left = annotations[annotIndex]?.rect[0] ?? 0; bottom = annotations[annotIndex]?.rect[1] ?? 0; opData = annotBBoxesAndOpPos[annotIndex]?.[0] ?? []; [posData, nMcidData] = annotBBoxesAndOpPos[annotIndex]?.[1] ?? [[], []]; } if (bbox.mcidList) { bbox.location = parseMcidToBbox( bbox.mcidList, posData, refPositionData, annotsFormatted, page.view, page.rotate, left, bottom, ); if (_.isEmpty(bbox.location)) { return null; } } else if (bbox.contentItemPath) { const contentItemsPath = bbox.contentItemPath.slice(2); let contentItemsBBoxes = nMcidData[bbox.contentItemPath[1]]; try { contentItemsPath.forEach((ci, i) => { if (contentItemsPath.length > i + 1 || !contentItemsBBoxes.final) { contentItemsBBoxes = contentItemsBBoxes.contentItems[0]; } contentItemsBBoxes = contentItemsBBoxes.contentItems[ci]; }); bbox.location = [ contentItemsBBoxes.contentItem.x + left, contentItemsBBoxes.contentItem.y + bottom, contentItemsBBoxes.contentItem.w, contentItemsBBoxes.contentItem.h ]; } catch (err) { console.log('NoMCIDDataParseError:', err.message || err); bbox.location = [0, 0, 0, 0]; } } if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); } return bbox; - }); + }) : []; + setBboxesAll(allBboxes); setBboxesErrors(errorBboxes); }); } else { setBboxesAll([]); setBboxesErrors([]); } }, [page, props.bboxList, props.treeElementsBboxes]);
🧹 Nitpick comments (1)
src/components/pdfPage/PdfPage.tsx (1)
106-176: Consider explicit memoization of per-page bbox props to prevent unnecessary re-renders.The parent component (PdfDocument) wraps the PdfPage rendering in
useMemowithbboxMapandtreeElementsBboxesin the dependency array (line 271-306). However, the individual propsbboxList={bboxMap[page]}andtreeElementsBboxes={treeElementsBboxes[page]}are property accesses on state objects rather than explicitly memoized values. If the parent's state objects are recreated on each update (even when the specific page's data hasn't changed), PdfPage will receive new prop references and trigger the effect in PdfPage to re-run. For more granular memoization, consider wrapping these per-page values withuseMemoin PdfDocument or extracting individual page prop objects separately to avoid re-rendering PdfPage when sibling pages' data changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/components/pdfPage/PdfPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/pdfPage/PdfPage.tsx (1)
src/services/bboxService.ts (4)
getFormattedAnnotations(65-82)createAllBboxes(247-272)parseMcidToBbox(584-638)getBboxForGlyph(563-582)
🔇 Additional comments (2)
src/components/pdfPage/PdfPage.tsx (2)
61-61: LGTM! Page state addition enables deferred bbox computation.Adding local state to store the page callback is a clean approach that allows bbox computation to be handled in a separate effect, avoiding potential issues with heavy computation blocking the load callback.
99-104: LGTM! Callback dependencies correctly aligned.The
onPageLoadSuccesscallback now properly includesprops.onPageLoadSuccessin its dependency array, ensuring the latest handler is captured when it changes. React state setters are stable and correctly omitted from the dependencies.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Vladimir-Burshnev
force-pushed
the
update-bboxes-after-render
branch
from
e153608 to
f8fac27
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/components/pdfPage/PdfPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/pdfPage/PdfPage.tsx (1)
src/services/bboxService.ts (4)
getFormattedAnnotations(65-82)parseMcidToBbox(584-638)getBboxForGlyph(563-582)createAllBboxes(247-272)
🔇 Additional comments (2)
src/components/pdfPage/PdfPage.tsx (2)
1-70: Setup looks good.The addition of
usePreviousto track page and treeElementsBboxes changes, along with the newpagestate, provides the foundation for deferring bbox computation until after the page loads. The destructuring and state initialization are correct.
101-106: Callback dependencies correctly aligned.The dependency array now correctly includes only
props.onPageLoadSuccess, matching what's actually used in the callback body. React state setters are stable and don't need to be listed. This addresses the previous review concern.
| useEffect(() => { | ||
| const triggeredByBboxList = prevPageBboxes | ||
| && prevPageBboxes.page === page | ||
| && prevPageBboxes.treeElementsBboxes === treeElementsBboxes; | ||
| if (page) { | ||
| if (triggeredByBboxList && !bboxList?.length) setBboxesErrors([]); | ||
| else { | ||
| Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => { | ||
| const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3]; | ||
| const operationData = operatorList.argsArray[operatorList.argsArray.length - 2]; | ||
| const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1]; | ||
| const annotsFormatted = getFormattedAnnotations(annotations); | ||
| const errorBboxes = (bboxList ?? []).map((bbox) => { | ||
| let opData = operationData, | ||
| posData = positionData, | ||
| nMcidData = noMCIDData; | ||
| let left = 0, bottom = 0; | ||
| const { annotIndex } = bbox; | ||
| if (annotIndex != null) { | ||
| left = annotations[annotIndex]?.rect[0] ?? 0; | ||
| bottom = annotations[annotIndex]?.rect[1] ?? 0; | ||
| opData = annotBBoxesAndOpPos[annotIndex]?.[0] ?? []; | ||
| [posData, nMcidData] = annotBBoxesAndOpPos[annotIndex]?.[1] ?? [[], []]; | ||
| } | ||
|
|
||
| bbox.location = [ | ||
| contentItemsBBoxes.contentItem.x + left, | ||
| contentItemsBBoxes.contentItem.y + bottom, | ||
| contentItemsBBoxes.contentItem.w, | ||
| contentItemsBBoxes.contentItem.h | ||
| ]; | ||
| } catch (err) { | ||
| console.log('NoMCIDDataParseError:', err.message || err); | ||
| bbox.location = [0, 0, 0, 0]; | ||
| } | ||
| } | ||
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | ||
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | ||
| } | ||
| if (bbox.mcidList) { | ||
| bbox.location = parseMcidToBbox( | ||
| bbox.mcidList, | ||
| posData, | ||
| refPositionData, | ||
| annotsFormatted, | ||
| page.view, | ||
| page.rotate, | ||
| left, | ||
| bottom, | ||
| ); | ||
| if (_.isEmpty(bbox.location)) { | ||
| return null; | ||
| } | ||
| } else if (bbox.contentItemPath) { | ||
| const contentItemsPath = bbox.contentItemPath.slice(2); | ||
| let contentItemsBBoxes = nMcidData[bbox.contentItemPath[1]]; | ||
| try { | ||
| contentItemsPath.forEach((ci, i) => { | ||
| if (contentItemsPath.length > i + 1 || !contentItemsBBoxes.final) { | ||
| contentItemsBBoxes = contentItemsBBoxes.contentItems[0]; | ||
| } | ||
| contentItemsBBoxes = contentItemsBBoxes.contentItems[ci]; | ||
| }); | ||
|
|
||
| return bbox; | ||
| }); | ||
| setBboxesAll(allBboxes); | ||
| setBboxesErrors(errorBboxes); | ||
| }); | ||
| props.onPageLoadSuccess?.(page); | ||
| }, [bboxList, props.treeElementsBboxes, props.width, props.height, scale]); | ||
| bbox.location = [ | ||
| contentItemsBBoxes.contentItem.x + left, | ||
| contentItemsBBoxes.contentItem.y + bottom, | ||
| contentItemsBBoxes.contentItem.w, | ||
| contentItemsBBoxes.contentItem.h | ||
| ]; | ||
| } catch (err) { | ||
| console.log('NoMCIDDataParseError:', err.message || err); | ||
| bbox.location = [0, 0, 0, 0]; | ||
| } | ||
| } | ||
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | ||
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (bboxList.length) { | ||
| setBboxesErrors((prev) => _.map(prev, (bbox, index) => { | ||
| if (_.isNil(bbox)) return null; | ||
| return { | ||
| ...bbox, | ||
| isVisible: bboxList[index].hasOwnProperty('isVisible') ? bboxList[index].isVisible : true, | ||
| }; | ||
| })); | ||
| return bbox; | ||
| }); | ||
| if (!triggeredByBboxList) { | ||
| const allBboxes = createAllBboxes(treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate); | ||
| setBboxesAll(allBboxes); | ||
| } | ||
| setBboxesErrors(errorBboxes); | ||
| }); | ||
| } | ||
| } | ||
| }, [bboxList]); | ||
| }, [page, bboxList, treeElementsBboxes]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PdfPage.tsx" | head -20Repository: veraPDF/verapdf-js-viewer
Length of output: 105
🏁 Script executed:
head -n 300 src/components/pdfPage/PdfPage.tsx | tail -n +100Repository: veraPDF/verapdf-js-viewer
Length of output: 8752
🏁 Script executed:
# Get the complete useEffect hook and surrounding context
sed -n '100,190p' src/components/pdfPage/PdfPage.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 3899
🏁 Script executed:
# Check the component's prop interface
rg -A 30 "interface.*Props|type.*Props" src/components/pdfPage/PdfPage.tsx | head -50Repository: veraPDF/verapdf-js-viewer
Length of output: 1942
🏁 Script executed:
# Find where PdfPage is imported and used
rg "import.*PdfPage|<PdfPage" --type tsx --type ts | head -20Repository: veraPDF/verapdf-js-viewer
Length of output: 96
🏁 Script executed:
rg "PdfPage" --type ts --type jsx --type js | grep -E "import|<PdfPage" | head -20Repository: veraPDF/verapdf-js-viewer
Length of output: 96
🏁 Script executed:
# Search for component instantiation more broadly
rg "<PdfPage" --glob="*.tsx" --glob="*.jsx" | head -20Repository: veraPDF/verapdf-js-viewer
Length of output: 175
🏁 Script executed:
# Let's look at the full file to understand context better
wc -l src/components/pdfPage/PdfPage.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 107
🏁 Script executed:
# Get the beginning of the file to see imports and how treeElementsBboxes is used
head -n 150 src/components/pdfPage/PdfPage.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 6051
🏁 Script executed:
# Find PdfDocument to see how treeElementsBboxes is passed
cat src/components/pdfDocument/PdfDocument.tsx | head -n 300Repository: veraPDF/verapdf-js-viewer
Length of output: 11277
🏁 Script executed:
# Search for treeElementsBboxes usage in PdfDocument
rg "treeElementsBboxes" src/components/pdfDocument/PdfDocument.tsx -B 5 -A 5Repository: veraPDF/verapdf-js-viewer
Length of output: 2662
🏁 Script executed:
# Check how many times treeElementsBboxes appears and in what context
rg "treeElementsBboxes" --type ts | head -30Repository: veraPDF/verapdf-js-viewer
Length of output: 1395
🏁 Script executed:
# Check the end of PdfPage.tsx to see the export statement
tail -n 30 src/components/pdfPage/PdfPage.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 1146
🏁 Script executed:
# Search specifically for PdfPage export
rg "export.*PdfPage|export default" src/components/pdfPage/PdfPage.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 98
🏁 Script executed:
# Check createBboxMap implementation to understand what it returns
rg -A 20 "export.*createBboxMap|function createBboxMap" src/services/Repository: veraPDF/verapdf-js-viewer
Length of output: 1598
🏁 Script executed:
# Find the createBboxMap function
fd -e ts -e tsx src/services/ | xargs grep -l "createBboxMap"Repository: veraPDF/verapdf-js-viewer
Length of output: 428
🏁 Script executed:
# Look at how createBboxMap is called and what it returns
sed -n '1,100p' src/services/bboxService.ts | grep -A 30 "export const createBboxMap"Repository: veraPDF/verapdf-js-viewer
Length of output: 51
🏁 Script executed:
# Check the complete createBboxMap function
rg -A 50 "export const createBboxMap" src/services/bboxService.ts | head -60Repository: veraPDF/verapdf-js-viewer
Length of output: 1961
🏁 Script executed:
# Check if PdfDocument's useMemo creates new PdfPage instances due to props dependency
rg -B 5 -A 15 "useMemo.*loaded.*shownPages" src/components/pdfDocument/PdfDocument.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 865
🏁 Script executed:
# Check how parsedTree is updated and if it changes frequently
rg -B 5 -A 5 "setParsedTree" src/components/pdfDocument/PdfDocument.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 1345
🏁 Script executed:
# Look at the overall component re-render patterns - check if PdfDocument itself is memoized
rg "export default" src/components/pdfDocument/PdfDocument.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 102
🏁 Script executed:
# Check how frequently parsedTree might change and trigger new treeElementsBboxes objects
rg -B 10 "setParsedTree" src/components/pdfDocument/PdfDocument.tsx | head -20Repository: veraPDF/verapdf-js-viewer
Length of output: 877
🏁 Script executed:
# Verify the useMemo dependency array one more time and check onPageLoadSuccess callback
rg -B 2 -A 25 "const onPageLoadSuccess = useCallback" src/components/pdfDocument/PdfDocument.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 1242
🏁 Script executed:
# Check if there's memo wrapping on passed props or any other optimization
rg -B 10 "treeElementsBboxes=\{treeElementsBboxes" src/components/pdfDocument/PdfDocument.tsxRepository: veraPDF/verapdf-js-viewer
Length of output: 643
🏁 Script executed:
# Look at the complete useMemo dependencies to understand re-render triggers
rg -A 1 "useMemo\(\(\)" src/components/pdfDocument/PdfDocument.tsx | tail -20Repository: veraPDF/verapdf-js-viewer
Length of output: 580
Reconsider treeElementsBboxes dependency to prevent unnecessary effect runs.
The treeElementsBboxes dependency in the effect causes unnecessary re-runs due to reference equality issues. Since createBboxMap() in the parent component creates a new object on every call (whenever parsedTree changes), the treeElementsBboxes[page] prop receives a new array reference even when the actual data hasn't changed. This triggers the effect to re-execute the Promise.all() chain and recompute bboxes redundantly, degrading performance.
The triggeredByBboxList check (line 111) attempts to optimize this by detecting when only treeElementsBboxes changed, but the reference equality comparison fails because the parent doesn't stabilize the object reference.
Solutions:
- Recommended: Memoize
treeElementsBboxes[page]in the parent usinguseMemoso it only changes when the actual bbox data changes - Use a deep equality check for
treeElementsBboxesinstead of reference equality - Split into two effects: one for bbox structure updates and one for error bbox updates
Option 1 aligns with React best practices and is the simplest fix. Both components already use memo(), so stabilizing the object reference in the parent will prevent the unnecessary effect runs.
🤖 Prompt for AI Agents
In src/components/pdfPage/PdfPage.tsx around lines 108-183 the effect re-runs
unnecessarily because treeElementsBboxes reference changes even when its data
for the current page is identical; fix by stabilizing that page-specific prop in
the parent (memoize treeElementsBboxes[page] with useMemo so the reference only
changes when its actual bbox data changes) and then change this component to
depend on that stable page-specific value (or, if you cannot change the parent,
add a local stablePageBboxes = useMemo(() => treeElementsBboxes?.[page],
[treeElementsBboxes?.[page], page]) or perform a deep-equality guard at the
start of the effect to skip the Promise.all when content is equal), ensuring the
effect no longer triggers from reference-only changes.
| Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => { | ||
| const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3]; | ||
| const operationData = operatorList.argsArray[operatorList.argsArray.length - 2]; | ||
| const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1]; | ||
| const annotsFormatted = getFormattedAnnotations(annotations); | ||
| const errorBboxes = (bboxList ?? []).map((bbox) => { | ||
| let opData = operationData, | ||
| posData = positionData, | ||
| nMcidData = noMCIDData; | ||
| let left = 0, bottom = 0; | ||
| const { annotIndex } = bbox; | ||
| if (annotIndex != null) { | ||
| left = annotations[annotIndex]?.rect[0] ?? 0; | ||
| bottom = annotations[annotIndex]?.rect[1] ?? 0; | ||
| opData = annotBBoxesAndOpPos[annotIndex]?.[0] ?? []; | ||
| [posData, nMcidData] = annotBBoxesAndOpPos[annotIndex]?.[1] ?? [[], []]; | ||
| } | ||
|
|
||
| bbox.location = [ | ||
| contentItemsBBoxes.contentItem.x + left, | ||
| contentItemsBBoxes.contentItem.y + bottom, | ||
| contentItemsBBoxes.contentItem.w, | ||
| contentItemsBBoxes.contentItem.h | ||
| ]; | ||
| } catch (err) { | ||
| console.log('NoMCIDDataParseError:', err.message || err); | ||
| bbox.location = [0, 0, 0, 0]; | ||
| } | ||
| } | ||
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | ||
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | ||
| } | ||
| if (bbox.mcidList) { | ||
| bbox.location = parseMcidToBbox( | ||
| bbox.mcidList, | ||
| posData, | ||
| refPositionData, | ||
| annotsFormatted, | ||
| page.view, | ||
| page.rotate, | ||
| left, | ||
| bottom, | ||
| ); | ||
| if (_.isEmpty(bbox.location)) { | ||
| return null; | ||
| } | ||
| } else if (bbox.contentItemPath) { | ||
| const contentItemsPath = bbox.contentItemPath.slice(2); | ||
| let contentItemsBBoxes = nMcidData[bbox.contentItemPath[1]]; | ||
| try { | ||
| contentItemsPath.forEach((ci, i) => { | ||
| if (contentItemsPath.length > i + 1 || !contentItemsBBoxes.final) { | ||
| contentItemsBBoxes = contentItemsBBoxes.contentItems[0]; | ||
| } | ||
| contentItemsBBoxes = contentItemsBBoxes.contentItems[ci]; | ||
| }); | ||
|
|
||
| return bbox; | ||
| }); | ||
| setBboxesAll(allBboxes); | ||
| setBboxesErrors(errorBboxes); | ||
| }); | ||
| props.onPageLoadSuccess?.(page); | ||
| }, [bboxList, props.treeElementsBboxes, props.width, props.height, scale]); | ||
| bbox.location = [ | ||
| contentItemsBBoxes.contentItem.x + left, | ||
| contentItemsBBoxes.contentItem.y + bottom, | ||
| contentItemsBBoxes.contentItem.w, | ||
| contentItemsBBoxes.contentItem.h | ||
| ]; | ||
| } catch (err) { | ||
| console.log('NoMCIDDataParseError:', err.message || err); | ||
| bbox.location = [0, 0, 0, 0]; | ||
| } | ||
| } | ||
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | ||
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (bboxList.length) { | ||
| setBboxesErrors((prev) => _.map(prev, (bbox, index) => { | ||
| if (_.isNil(bbox)) return null; | ||
| return { | ||
| ...bbox, | ||
| isVisible: bboxList[index].hasOwnProperty('isVisible') ? bboxList[index].isVisible : true, | ||
| }; | ||
| })); | ||
| return bbox; | ||
| }); | ||
| if (!triggeredByBboxList) { | ||
| const allBboxes = createAllBboxes(treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate); | ||
| setBboxesAll(allBboxes); | ||
| } | ||
| setBboxesErrors(errorBboxes); | ||
| }); |
There was a problem hiding this comment.
Add error handling to prevent silent failures.
The Promise.all call has no error handler. If getOperatorList() or getAnnotations() fail, the promise will reject and state updates won't occur, leaving bboxes in a stale or inconsistent state. This could result in missing bboxes or rendering issues with no visible error.
🔎 Proposed fix
Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => {
const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3];
const operationData = operatorList.argsArray[operatorList.argsArray.length - 2];
const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1];
const annotsFormatted = getFormattedAnnotations(annotations);
const errorBboxes = (bboxList ?? []).map((bbox) => {
// ... bbox computation ...
});
if (!triggeredByBboxList) {
const allBboxes = createAllBboxes(treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate);
setBboxesAll(allBboxes);
}
setBboxesErrors(errorBboxes);
+ }).catch((err) => {
+ console.error('Failed to compute bboxes:', err);
+ setBboxesAll([]);
+ setBboxesErrors([]);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => { | |
| const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3]; | |
| const operationData = operatorList.argsArray[operatorList.argsArray.length - 2]; | |
| const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1]; | |
| const annotsFormatted = getFormattedAnnotations(annotations); | |
| const errorBboxes = (bboxList ?? []).map((bbox) => { | |
| let opData = operationData, | |
| posData = positionData, | |
| nMcidData = noMCIDData; | |
| let left = 0, bottom = 0; | |
| const { annotIndex } = bbox; | |
| if (annotIndex != null) { | |
| left = annotations[annotIndex]?.rect[0] ?? 0; | |
| bottom = annotations[annotIndex]?.rect[1] ?? 0; | |
| opData = annotBBoxesAndOpPos[annotIndex]?.[0] ?? []; | |
| [posData, nMcidData] = annotBBoxesAndOpPos[annotIndex]?.[1] ?? [[], []]; | |
| } | |
| bbox.location = [ | |
| contentItemsBBoxes.contentItem.x + left, | |
| contentItemsBBoxes.contentItem.y + bottom, | |
| contentItemsBBoxes.contentItem.w, | |
| contentItemsBBoxes.contentItem.h | |
| ]; | |
| } catch (err) { | |
| console.log('NoMCIDDataParseError:', err.message || err); | |
| bbox.location = [0, 0, 0, 0]; | |
| } | |
| } | |
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | |
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | |
| } | |
| if (bbox.mcidList) { | |
| bbox.location = parseMcidToBbox( | |
| bbox.mcidList, | |
| posData, | |
| refPositionData, | |
| annotsFormatted, | |
| page.view, | |
| page.rotate, | |
| left, | |
| bottom, | |
| ); | |
| if (_.isEmpty(bbox.location)) { | |
| return null; | |
| } | |
| } else if (bbox.contentItemPath) { | |
| const contentItemsPath = bbox.contentItemPath.slice(2); | |
| let contentItemsBBoxes = nMcidData[bbox.contentItemPath[1]]; | |
| try { | |
| contentItemsPath.forEach((ci, i) => { | |
| if (contentItemsPath.length > i + 1 || !contentItemsBBoxes.final) { | |
| contentItemsBBoxes = contentItemsBBoxes.contentItems[0]; | |
| } | |
| contentItemsBBoxes = contentItemsBBoxes.contentItems[ci]; | |
| }); | |
| return bbox; | |
| }); | |
| setBboxesAll(allBboxes); | |
| setBboxesErrors(errorBboxes); | |
| }); | |
| props.onPageLoadSuccess?.(page); | |
| }, [bboxList, props.treeElementsBboxes, props.width, props.height, scale]); | |
| bbox.location = [ | |
| contentItemsBBoxes.contentItem.x + left, | |
| contentItemsBBoxes.contentItem.y + bottom, | |
| contentItemsBBoxes.contentItem.w, | |
| contentItemsBBoxes.contentItem.h | |
| ]; | |
| } catch (err) { | |
| console.log('NoMCIDDataParseError:', err.message || err); | |
| bbox.location = [0, 0, 0, 0]; | |
| } | |
| } | |
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | |
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | |
| } | |
| useEffect(() => { | |
| if (bboxList.length) { | |
| setBboxesErrors((prev) => _.map(prev, (bbox, index) => { | |
| if (_.isNil(bbox)) return null; | |
| return { | |
| ...bbox, | |
| isVisible: bboxList[index].hasOwnProperty('isVisible') ? bboxList[index].isVisible : true, | |
| }; | |
| })); | |
| return bbox; | |
| }); | |
| if (!triggeredByBboxList) { | |
| const allBboxes = createAllBboxes(treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate); | |
| setBboxesAll(allBboxes); | |
| } | |
| setBboxesErrors(errorBboxes); | |
| }); | |
| Promise.all([page.getOperatorList(), page.getAnnotations()]).then(([operatorList, annotations]) => { | |
| const annotBBoxesAndOpPos = operatorList.argsArray[operatorList.argsArray.length - 3]; | |
| const operationData = operatorList.argsArray[operatorList.argsArray.length - 2]; | |
| const [positionData, noMCIDData, refPositionData] = operatorList.argsArray[operatorList.argsArray.length - 1]; | |
| const annotsFormatted = getFormattedAnnotations(annotations); | |
| const errorBboxes = (bboxList ?? []).map((bbox) => { | |
| let opData = operationData, | |
| posData = positionData, | |
| nMcidData = noMCIDData; | |
| let left = 0, bottom = 0; | |
| const { annotIndex } = bbox; | |
| if (annotIndex != null) { | |
| left = annotations[annotIndex]?.rect[0] ?? 0; | |
| bottom = annotations[annotIndex]?.rect[1] ?? 0; | |
| opData = annotBBoxesAndOpPos[annotIndex]?.[0] ?? []; | |
| [posData, nMcidData] = annotBBoxesAndOpPos[annotIndex]?.[1] ?? [[], []]; | |
| } | |
| if (bbox.mcidList) { | |
| bbox.location = parseMcidToBbox( | |
| bbox.mcidList, | |
| posData, | |
| refPositionData, | |
| annotsFormatted, | |
| page.view, | |
| page.rotate, | |
| left, | |
| bottom, | |
| ); | |
| if (_.isEmpty(bbox.location)) { | |
| return null; | |
| } | |
| } else if (bbox.contentItemPath) { | |
| const contentItemsPath = bbox.contentItemPath.slice(2); | |
| let contentItemsBBoxes = nMcidData[bbox.contentItemPath[1]]; | |
| try { | |
| contentItemsPath.forEach((ci, i) => { | |
| if (contentItemsPath.length > i + 1 || !contentItemsBBoxes.final) { | |
| contentItemsBBoxes = contentItemsBBoxes.contentItems[0]; | |
| } | |
| contentItemsBBoxes = contentItemsBBoxes.contentItems[ci]; | |
| }); | |
| bbox.location = [ | |
| contentItemsBBoxes.contentItem.x + left, | |
| contentItemsBBoxes.contentItem.y + bottom, | |
| contentItemsBBoxes.contentItem.w, | |
| contentItemsBBoxes.contentItem.h | |
| ]; | |
| } catch (err) { | |
| console.log('NoMCIDDataParseError:', err.message || err); | |
| bbox.location = [0, 0, 0, 0]; | |
| } | |
| } | |
| if (_.isNumber(bbox.operatorIndex) && _.isNumber(bbox.glyphIndex)) { | |
| bbox.location = getBboxForGlyph(bbox.operatorIndex, bbox.glyphIndex, opData, page.view, page.rotate, left, bottom); | |
| } | |
| return bbox; | |
| }); | |
| if (!triggeredByBboxList) { | |
| const allBboxes = createAllBboxes(treeElementsBboxes, positionData, refPositionData, annotsFormatted, page.view, page.rotate); | |
| setBboxesAll(allBboxes); | |
| } | |
| setBboxesErrors(errorBboxes); | |
| }).catch((err) => { | |
| console.error('Failed to compute bboxes:', err); | |
| setBboxesAll([]); | |
| setBboxesErrors([]); | |
| }); |
🤖 Prompt for AI Agents
In src/components/pdfPage/PdfPage.tsx around lines 115-180, the
Promise.all([page.getOperatorList(), page.getAnnotations()]) usage has no
rejection handler which allows silent failures and leaves component state stale;
add a .catch handler on the promise chain to log the error (with context), set
bboxes state to a safe fallback (e.g., [] or previous known safe value) and
optionally set an error flag/state so the UI can respond, and ensure the .then
path still only runs when the component is mounted (guard with a mounted/ref
check) to avoid setting state after unmount.
Closes: https://trello.com/c/XYKb0fkl
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.