207 reconcile for robins#243
Merged
Merged
Conversation
…yBowman/corates into 207-reconcile-for-robins
Closed
📝 WalkthroughWalkthroughThis PR introduces a complete ROBINS-I reconciliation feature with new components for comparing two ROBINS-I checklists across sections and domains in a split-screen layout with PDF viewer integration, adds Local-First Sync documentation, enhances PDF lifecycle management with memory cleanup and LRU caching, updates Dialog wrapper API usage, and refines checklist/appraisal terminology in UI text. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant RW as RobinsIReconciliation<br/>WithPdf
participant RIR as RobinsIReconciliation
participant Comp as Comparison<br/>Logic
participant Page as Page<br/>Component
participant Panel as Answer/Judgment<br/>Panel
participant PDF as PDF Viewer
User->>RW: Navigate to reconciliation
RW->>RIR: Initialize with checklist data
RIR->>Comp: compareChecklists(reviewer1, reviewer2)
Comp-->>RIR: Return comparison object
RIR->>RIR: buildNavigationItems(protocol)
RIR->>RW: setNavbarStore(navItems, currentPage, etc.)
RW->>RW: Render split-screen layout
RW->>PDF: Load pdfData/pdfUrl (read-only)
User->>RW: Click nav pill or change page
RW->>RIR: goToPage(index)
RIR->>Page: Render SectionBQuestionPage<br/>(or DomainQuestionPage)
Page->>Panel: Render 3 panels<br/>(reviewer1, reviewer2, final)
User->>Panel: Select reviewer answer or<br/>click "Use This"
Panel->>RIR: onUseThis() or<br/>onAnswerChange(value)
RIR->>RIR: Update finalAnswers[key]
RIR->>RIR: Recompute navbarStore
RW->>RW: Re-render navbar with<br/>updated status indicators
User->>RW: Click Save
RIR->>RIR: Validate all items answered
alt All answered
RIR->>RIR: Show confirmation dialog
User->>RIR: Confirm save
RIR->>RIR: Persist reconciliation
RIR->>RW: Show success toast
else Missing answers
RIR->>User: Show validation error
end
User->>RW: View summary or<br/>navigate summary page
RIR->>RIR: Render RobinsISummaryView
RIR->>RIR: getGroupedNavigationItems()
RIR->>User: Display agreement rate,<br/>item summary cards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 1794e07 | Commit Preview URL | Jan 07 2026, 04:37 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @packages/docs/audits/local-first-roadmap.md:
- Line 7: Update the incorrect cross-reference string
`docs/plans/local-first-sync-strategy.md` to the correct path
`docs/audits/local-first-sync-strategy.md` in the sentence that begins "This
roadmap turns the strategy..." so the link points to the actual file location;
ensure any other occurrences of `docs/plans/local-first-sync-strategy.md` in
this document are likewise replaced with
`docs/audits/local-first-sync-strategy.md`.
- Line 69: The cross-reference in packages/docs/audits/local-first-roadmap.md
incorrectly points to docs/plans/local-first-sync-strategy.md; update that
link/text to reference the actual file path
docs/audits/local-first-sync-strategy.md so the roadmap points to the correct
strategy document.
In @packages/docs/audits/local-first-sync-strategy.md:
- Line 54: Update the phrase "BroadcastChannel based approach" to
"BroadcastChannel-based approach" in the sentence that describes the service
worker and BroadcastChannel approach so the compound modifier is hyphenated
correctly; locate the sentence containing "Service worker & BroadcastChannel
based approach" and replace that fragment with "Service worker &
BroadcastChannel-based approach".
- Around line 117-118: Replace the informal closing "If you want, I can:" with a
more formal alternative that matches the document tone; update the options to
begin with a complete sentence such as "Please indicate whether you would like
me to: (A) generate the endpoint inventory now, (B) move selected Zod schemas to
`packages/shared`, or (C) scaffold `/api/metadata` and CI typegen." Ensure
punctuation and capitalization are consistent with surrounding documentation.
In @packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx:
- Around line 236-255: The cleanup returned from useEffect is currently calling
an async closeActiveDocument without awaiting, which makes the cleanup itself
async; make the cleanup synchronous by defining the async function
(closeActiveDocument) inside the effect but invoking it in a fire-and-forget
manner (for example: void closeActiveDocument(); or
closeActiveDocument().catch(err => console.warn(...))). Ensure you still call
docManagerRef.current.closeDocument(activeDocumentIdRef.current) inside that
async function and preserve the ref-clearing (docManagerRef,
activeDocumentIdRef, previousSelectedPdfIdRef, isLoadingRef) and error logging,
but do not return an async function from useEffect.
- Line 44: Remove the commented-out ConsoleLogger import and all commented
references to ConsoleLogger in this file (delete the "// import { ConsoleLogger
} from '@embedpdf/models';" and any other commented ConsoleLogger usages),
leaving no obsolete commented code paths referencing ConsoleLogger; ensure no
other code depends on it and run a quick build to confirm no unused symbol
warnings remain.
In
@packages/web/src/components/project/reconcile-tab/robins-i-reconcile/RobinsIReconciliationWithPdf.jsx:
- Around line 56-84: The JSX assigned to the headerContent variable is evaluated
once and won't react to changes in props.reviewer1Name, props.reviewer2Name or
props.onCancel; change headerContent into a function (e.g., const headerContent
= () => ( ... )) or inline the JSX where used so Solid can re-evaluate it, then
pass headerContent() (or the function itself if SplitScreenLayout expects a
render function) to SplitScreenLayout; keep the same internal references
(AiOutlineArrowLeft, RobinsINavbar, navbarStore) but ensure you call the
function where headerContent was previously referenced.
In @packages/web/src/primitives/pdfCache.js:
- Around line 16-18: Replace the hardcoded per-file limit with the shared
constant: add an import for PDF_LIMITS from '@corates/shared' at the top of the
file and set MAX_SINGLE_FILE_SIZE to PDF_LIMITS.MAX_SIZE instead of 50 * 1024 *
1024; keep MAX_CACHE_SIZE_BYTES unchanged and mirror the pattern used in
pdfValidation.js so the single-source-of-truth PDF_LIMITS.MAX_SIZE is used
across the codebase.
🧹 Nitpick comments (6)
packages/web/src/components/project/reconcile-tab/robins-i-reconcile/panels/DirectionPanel.jsx (1)
10-25: Consider extracting shared button style helper.
getDirectionButtonStyleis nearly identical togetJudgementButtonStyleinJudgementPanel.jsx. Consider extracting this into a shared utility (e.g.,getPanelButtonStyle) to reduce duplication. However, this is optional given the localized scope.♻️ Potential shared utility
Create a shared helper in a
panel-utils.jsfile:// panel-utils.js export function getPanelButtonStyle(isSelected, panelType) { if (!isSelected) { return 'border-gray-200 bg-white text-gray-700 hover:bg-gray-50'; } switch (panelType) { case 'reviewer1': return 'border-blue-400 bg-blue-50 text-blue-800'; case 'reviewer2': return 'border-purple-400 bg-purple-50 text-purple-800'; case 'final': return 'border-green-400 bg-green-50 text-green-800'; default: return 'border-gray-400 bg-gray-50 text-gray-800'; } }Then import and use in both DirectionPanel and JudgementPanel.
packages/web/src/components/project/reconcile-tab/robins-i-reconcile/RobinsINavbar.jsx (1)
53-58: Minor:indexOfcalled per item creates O(n²) complexity.For each item,
indexOfscans the entire array. While acceptable for small navigation lists (typically <30 items), consider pre-computing indices if the list grows:♻️ Alternative approach
// Pre-compute a Map of item → index const itemIndexMap = createMemo(() => { const map = new Map(); props.store.navItems?.forEach((item, idx) => map.set(item, idx)); return map; }); // Then in the For: const itemIndex = () => itemIndexMap().get(item) ?? -1;packages/web/src/components/project/reconcile-tab/robins-i-reconcile/RobinsIReconciliation.jsx (2)
68-83: Potential re-initialization issue with localStorage effect.This effect re-reads localStorage on every
checklist1.idorchecklist2.idchange. If the user navigates and these IDs change, the current page position will be overwritten with the stored value (or reset to 0 if no stored value exists).Consider adding a flag to only initialize once per component mount:
♻️ Suggested fix
+ const [initialized, setInitialized] = createSignal(false); + // Initialize navigation state from localStorage createEffect(() => { - if (!props.checklist1?.id || !props.checklist2?.id) return; + if (!props.checklist1?.id || !props.checklist2?.id || initialized()) return; const key = getStorageKey(); if (!key) return; try { const stored = localStorage.getItem(key); if (stored) { const parsed = JSON.parse(stored); setCurrentPage(parsed.currentPage ?? 0); setViewMode(parsed.viewMode ?? 'questions'); } + setInitialized(true); } catch (e) { console.error('Failed to load navigation state:', e); + setInitialized(true); } });
251-281: Unnecessaryasynckeyword onhandleReset.The function doesn't contain any
awaitexpressions, soasyncisn't needed:♻️ Minor cleanup
- async function handleReset() { + function handleReset() {packages/web/src/components/project/reconcile-tab/robins-i-reconcile/RobinsISummaryView.jsx (1)
194-227: Consider extracting shared display helpers to navbar-utils.js.
getItemDisplayNumberandgetItemLabelare very similar togetDisplayLabelandgetSectionLabelinRobinsINavbar.jsx. Consider consolidating these intonavbar-utils.jsto reduce duplication.packages/web/src/primitives/pdfCache.js (1)
144-177: Consider batching deletes for efficiency.The eviction logic is correct, but each delete creates a separate transaction (lines 164-170). For better performance when evicting multiple entries, consider batching all deletes into a single transaction.
Additionally, while the current code is safe because
cachePdfenforcesMAX_SINGLE_FILE_SIZE, adding a defensive check at the start would make the function more robust:♻️ Optional optimization
async function evictIfNeeded(requiredSpace) { try { + // Defensive check: if required space exceeds cache limit, we can't satisfy it + if (requiredSpace > MAX_CACHE_SIZE_BYTES) { + console.warn(`Required space ${requiredSpace} exceeds cache limit ${MAX_CACHE_SIZE_BYTES}`); + return; + } + const { totalSize, entries } = await getCacheStats();For batching deletes in a single transaction:
const db = await getDb(); + const idsToDelete = []; - // Evict oldest entries until we're under the limit for (const entry of entries) { if (currentSize <= targetSize) break; - - await new Promise((resolve, reject) => { - const transaction = db.transaction(PDF_STORE_NAME, 'readwrite'); - const store = transaction.objectStore(PDF_STORE_NAME); - const request = store.delete(entry.id); - request.onsuccess = () => resolve(); - request.onerror = () => reject(request.error); - }); - + idsToDelete.push(entry.id); currentSize -= entry.size; } + + // Batch delete in single transaction + if (idsToDelete.length > 0) { + await new Promise((resolve, reject) => { + const transaction = db.transaction(PDF_STORE_NAME, 'readwrite'); + const store = transaction.objectStore(PDF_STORE_NAME); + let completed = 0; + + for (const id of idsToDelete) { + const request = store.delete(id); + request.onsuccess = () => { + completed++; + if (completed === idsToDelete.length) resolve(); + }; + request.onerror = () => reject(request.error); + } + }); + }
| author: Team | ||
| --- | ||
|
|
||
| This roadmap turns the strategy from `docs/plans/local-first-sync-strategy.md` into a concrete, time-boxed plan with owners, milestones, and acceptance criteria. It is intentionally pragmatic: ship small, measure, and expand. |
There was a problem hiding this comment.
Correct file path reference.
Line 7 references docs/plans/local-first-sync-strategy.md, but the file is located at docs/audits/local-first-sync-strategy.md. Fix this broken cross-reference.
🔗 Proposed fix
- This roadmap turns the strategy from `docs/plans/local-first-sync-strategy.md` into a concrete, time-boxed plan with owners, milestones, and acceptance criteria. It is intentionally pragmatic: ship small, measure, and expand.
+ This roadmap turns the strategy from `docs/audits/local-first-sync-strategy.md` into a concrete, time-boxed plan with owners, milestones, and acceptance criteria. It is intentionally pragmatic: ship small, measure, and expand.📝 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.
Suggested change
| This roadmap turns the strategy from `docs/plans/local-first-sync-strategy.md` into a concrete, time-boxed plan with owners, milestones, and acceptance criteria. It is intentionally pragmatic: ship small, measure, and expand. | |
| This roadmap turns the strategy from `docs/audits/local-first-sync-strategy.md` into a concrete, time-boxed plan with owners, milestones, and acceptance criteria. It is intentionally pragmatic: ship small, measure, and expand. |
🤖 Prompt for AI Agents
In @packages/docs/audits/local-first-roadmap.md at line 7, Update the incorrect
cross-reference string `docs/plans/local-first-sync-strategy.md` to the correct
path `docs/audits/local-first-sync-strategy.md` in the sentence that begins
"This roadmap turns the strategy..." so the link points to the actual file
location; ensure any other occurrences of
`docs/plans/local-first-sync-strategy.md` in this document are likewise replaced
with `docs/audits/local-first-sync-strategy.md`.
|
|
||
| Where to record progress | ||
|
|
||
| - Use `docs/plans/local-first-sync-strategy.md` for strategy context. |
There was a problem hiding this comment.
Correct file path reference.
Line 69 also references docs/plans/local-first-sync-strategy.md, but the file is located at docs/audits/local-first-sync-strategy.md. Fix this broken cross-reference to match the location of the strategy document.
🔗 Proposed fix
- Use `docs/plans/local-first-sync-strategy.md` for strategy context.
+ Use `docs/audits/local-first-sync-strategy.md` for strategy context.📝 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.
Suggested change
| - Use `docs/plans/local-first-sync-strategy.md` for strategy context. | |
| - Use `docs/audits/local-first-sync-strategy.md` for strategy context. |
🤖 Prompt for AI Agents
In @packages/docs/audits/local-first-roadmap.md at line 69, The cross-reference
in packages/docs/audits/local-first-roadmap.md incorrectly points to
docs/plans/local-first-sync-strategy.md; update that link/text to reference the
actual file path docs/audits/local-first-sync-strategy.md so the roadmap points
to the correct strategy document.
| - `minSupportedClient` (semver) | ||
| - `features`/capabilities | ||
| - Client reports its `clientVersion` on requests (header) and performs a boot-time check. If the server responds that the client is too old, the client shows a blocking upgrade UI. | ||
| - Service worker & BroadcastChannel based approach: server changes `minSupportedClient` — SW detects it and broadcasts a force-reload message to all tabs. This is less disruptive than immediate hard-blocks. |
There was a problem hiding this comment.
Hyphenate compound modifier.
Change "BroadcastChannel based approach" to "BroadcastChannel-based approach" for correct grammar.
📝 Proposed fix
- Service worker & BroadcastChannel based approach: server changes `minSupportedClient` — SW detects it and broadcasts a force-reload message to all tabs. This is less disruptive than immediate hard-blocks.
+ Service worker & BroadcastChannel-based approach: server changes `minSupportedClient` — SW detects it and broadcasts a force-reload message to all tabs. This is less disruptive than immediate hard-blocks.📝 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.
Suggested change
| - Service worker & BroadcastChannel based approach: server changes `minSupportedClient` — SW detects it and broadcasts a force-reload message to all tabs. This is less disruptive than immediate hard-blocks. | |
| - Service worker & BroadcastChannel-based approach: server changes `minSupportedClient` — SW detects it and broadcasts a force-reload message to all tabs. This is less disruptive than immediate hard-blocks. |
🧰 Tools
🪛 LanguageTool
[grammar] ~54-~54: Use a hyphen to join words.
Context: ... UI. - Service worker & BroadcastChannel based approach: server changes `minSuppo...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In @packages/docs/audits/local-first-sync-strategy.md at line 54, Update the
phrase "BroadcastChannel based approach" to "BroadcastChannel-based approach" in
the sentence that describes the service worker and BroadcastChannel approach so
the compound modifier is hyphenated correctly; locate the sentence containing
"Service worker & BroadcastChannel based approach" and replace that fragment
with "Service worker & BroadcastChannel-based approach".
Comment on lines
+117
to
+118
|
|
||
| If you want, I can: (A) generate the endpoint inventory now, (B) move selected Zod schemas to `packages/shared`, or (C) scaffold `/api/metadata` and CI typegen. Which should I start? |
There was a problem hiding this comment.
Adopt a more formal tone in the closing section.
The opening phrase "If you want, I can:" is too informal for documentation. Use more formal language to align with the professional tone of the rest of the document.
📝 Proposed fix
- If you want, I can: (A) generate the endpoint inventory now, (B) move selected Zod schemas to `packages/shared`, or (C) scaffold `/api/metadata` and CI typegen. Which should I start?
+ Next steps: (A) generate the endpoint inventory, (B) move selected Zod schemas to `packages/shared`, or (C) scaffold `/api/metadata` and CI typegen.🧰 Tools
🪛 LanguageTool
[style] ~117-~117: This phrasing can be overused. Try elevating your writing with a more formal alternative.
Context: ... reconciliation for doc snapshots --- If you want, I can: (A) generate the endpoint inven...
(IF_YOU_WANT)
🤖 Prompt for AI Agents
In @packages/docs/audits/local-first-sync-strategy.md around lines 117 - 118,
Replace the informal closing "If you want, I can:" with a more formal
alternative that matches the document tone; update the options to begin with a
complete sentence such as "Please indicate whether you would like me to: (A)
generate the endpoint inventory now, (B) move selected Zod schemas to
`packages/shared`, or (C) scaffold `/api/metadata` and CI typegen." Ensure
punctuation and capitalization are consistent with surrounding documentation.
| import { ThumbnailsSidebar } from './components/thumbnails-sidebar'; | ||
| import { PageControls } from './components/page-controls'; | ||
| import { ConsoleLogger } from '@embedpdf/models'; | ||
| // import { ConsoleLogger } from '@embedpdf/models'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ConsoleLogger usage across entire codebase
rg "ConsoleLogger" --type=ts --type=tsxRepository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Also check if this is a common pattern with commented debug code
rg "// import.*from.*embedpdf" --type=ts --type=tsxRepository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check the actual file to see the context of the commented lines
if [ -f "packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx" ]; then
echo "=== Lines 40-60 ==="
sed -n '40,60p' packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx
echo -e "\n=== Lines 80-90 ==="
sed -n '80,90p' packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx
echo -e "\n=== Lines 270-280 ==="
sed -n '270,280p' packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx
else
echo "File not found"
fiRepository: InfinityBowman/corates
Length of output: 1808
🏁 Script executed:
#!/bin/bash
# Search for any active (non-commented) usage of ConsoleLogger
grep -r "ConsoleLogger" packages/web/src --include="*.tsx" --include="*.ts" | grep -v "^[^:]*://.*//.*ConsoleLogger"Repository: InfinityBowman/corates
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if ConsoleLogger is imported anywhere without comment
find packages/web/src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "ConsoleLogger" {} \;Repository: InfinityBowman/corates
Length of output: 128
Delete commented ConsoleLogger lines per coding guidelines.
The ConsoleLogger import and all references (lines 44, 58, 85, 274) are completely unused and commented out. Remove these lines entirely rather than leaving obsolete code paths in comments, per the guideline that states: "Do NOT reference removed or obsolete code paths in comments."
🤖 Prompt for AI Agents
In @packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx at line 44,
Remove the commented-out ConsoleLogger import and all commented references to
ConsoleLogger in this file (delete the "// import { ConsoleLogger } from
'@embedpdf/models';" and any other commented ConsoleLogger usages), leaving no
obsolete commented code paths referencing ConsoleLogger; ensure no other code
depends on it and run a quick build to confirm no unused symbol warnings remain.
Comment on lines
+236
to
+255
| // Cleanup: close the active document and release resources on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| const closeActiveDocument = async () => { | ||
| if (docManagerRef.current && activeDocumentIdRef.current) { | ||
| try { | ||
| await docManagerRef.current.closeDocument(activeDocumentIdRef.current); | ||
| } catch (err) { | ||
| console.warn('Error closing document on unmount:', err); | ||
| } | ||
| } | ||
| // Clear refs to release memory | ||
| docManagerRef.current = null; | ||
| activeDocumentIdRef.current = null; | ||
| previousSelectedPdfIdRef.current = undefined; | ||
| isLoadingRef.current = false; | ||
| }; | ||
| closeActiveDocument(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Fix async cleanup function in useEffect.
The cleanup function returned by useEffect must be synchronous, but closeActiveDocument is async and called without await. This means the document close operation won't complete before unmount, potentially causing memory leaks or orphaned resources in the PDF engine.
🐛 Proposed fix for synchronous cleanup
The cleanup should fire-and-forget the async operation, acknowledging it may not complete:
- // Cleanup: close the active document and release resources on unmount
useEffect(() => {
return () => {
- const closeActiveDocument = async () => {
- if (docManagerRef.current && activeDocumentIdRef.current) {
- try {
- await docManagerRef.current.closeDocument(activeDocumentIdRef.current);
- } catch (err) {
- console.warn('Error closing document on unmount:', err);
- }
- }
- // Clear refs to release memory
- docManagerRef.current = null;
- activeDocumentIdRef.current = null;
- previousSelectedPdfIdRef.current = undefined;
- isLoadingRef.current = false;
- };
- closeActiveDocument();
+ // Fire async close without awaiting (may not complete before unmount)
+ if (docManagerRef.current && activeDocumentIdRef.current) {
+ docManagerRef.current.closeDocument(activeDocumentIdRef.current)
+ .catch(err => console.warn('Error closing document on unmount:', err));
+ }
+ // Clear refs immediately
+ docManagerRef.current = null;
+ activeDocumentIdRef.current = null;
+ previousSelectedPdfIdRef.current = undefined;
+ isLoadingRef.current = false;
};
}, []);🤖 Prompt for AI Agents
In @packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx around lines
236 - 255, The cleanup returned from useEffect is currently calling an async
closeActiveDocument without awaiting, which makes the cleanup itself async; make
the cleanup synchronous by defining the async function (closeActiveDocument)
inside the effect but invoking it in a fire-and-forget manner (for example: void
closeActiveDocument(); or closeActiveDocument().catch(err =>
console.warn(...))). Ensure you still call
docManagerRef.current.closeDocument(activeDocumentIdRef.current) inside that
async function and preserve the ref-clearing (docManagerRef,
activeDocumentIdRef, previousSelectedPdfIdRef, isLoadingRef) and error logging,
but do not return an async function from useEffect.
Comment on lines
+56
to
+84
| const headerContent = ( | ||
| <> | ||
| {/* Back button */} | ||
| <button | ||
| onClick={() => props.onCancel?.()} | ||
| class='shrink-0 rounded-lg p-2 transition-colors hover:bg-gray-100' | ||
| title='Go back' | ||
| > | ||
| <AiOutlineArrowLeft class='h-5 w-5 text-gray-600' /> | ||
| </button> | ||
|
|
||
| {/* Title */} | ||
| <div class='shrink-0'> | ||
| <h1 class='text-lg font-bold text-gray-900'>ROBINS-I Reconciliation</h1> | ||
| <p class='text-xs text-gray-500'> | ||
| {props.reviewer1Name || 'Reviewer 1'} vs {props.reviewer2Name || 'Reviewer 2'} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div class='h-8 w-px shrink-0 bg-gray-200' /> | ||
|
|
||
| {/* Navbar - navigation pills */} | ||
| <Show when={navbarStore.navItems?.length > 0}> | ||
| <div class='flex flex-1 items-center gap-4 overflow-x-auto'> | ||
| <RobinsINavbar store={navbarStore} /> | ||
| </div> | ||
| </Show> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Reactivity issue: headerContent won't update when props change.
In SolidJS, JSX assigned to a variable outside the render path is evaluated once. If props.reviewer1Name, props.reviewer2Name, or props.onCancel change, this headerContent won't re-render.
Consider wrapping in a function or moving inline:
🔧 Suggested fix
- // Build header content with back button, title, and navbar
- const headerContent = (
- <>
+ // Build header content with back button, title, and navbar
+ const headerContent = () => (
+ <>
{/* Back button */}
...
</>
);Then pass headerContent={headerContent()} to SplitScreenLayout, or if the layout component expects a function, pass headerContent={headerContent}.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@packages/web/src/components/project/reconcile-tab/robins-i-reconcile/RobinsIReconciliationWithPdf.jsx
around lines 56 - 84, The JSX assigned to the headerContent variable is
evaluated once and won't react to changes in props.reviewer1Name,
props.reviewer2Name or props.onCancel; change headerContent into a function
(e.g., const headerContent = () => ( ... )) or inline the JSX where used so
Solid can re-evaluate it, then pass headerContent() (or the function itself if
SplitScreenLayout expects a render function) to SplitScreenLayout; keep the same
internal references (AiOutlineArrowLeft, RobinsINavbar, navbarStore) but ensure
you call the function where headerContent was previously referenced.
Comment on lines
+16
to
+18
| // LRU cache limits | ||
| const MAX_CACHE_SIZE_BYTES = 200 * 1024 * 1024; // 200 MB total cache limit | ||
| const MAX_SINGLE_FILE_SIZE = 50 * 1024 * 1024; // 50 MB per file (matches PDF_LIMITS.MAX_SIZE) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for PDF_LIMITS definitions
rg -n "PDF_LIMITS\s*=" --type js --type ts -g '!node_modules' -g '!dist'Repository: InfinityBowman/corates
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for where PDF_LIMITS is imported or used
rg -n "PDF_LIMITS" --type js --type ts -g '!node_modules' -g '!dist' -B 2 -A 2Repository: InfinityBowman/corates
Length of output: 8542
🏁 Script executed:
#!/bin/bash
# Check the current content of pdfCache.js to see imports and the full context
head -40 packages/web/src/primitives/pdfCache.jsRepository: InfinityBowman/corates
Length of output: 1146
Import PDF_LIMITS.MAX_SIZE from @corates/shared instead of hardcoding the value.
The cache limit on line 18 hardcodes 50 * 1024 * 1024 with a comment referencing PDF_LIMITS.MAX_SIZE, but it should import and use the constant directly from @corates/shared (as done in pdfValidation.js) to maintain a single source of truth and prevent future desynchronization.
🤖 Prompt for AI Agents
In @packages/web/src/primitives/pdfCache.js around lines 16 - 18, Replace the
hardcoded per-file limit with the shared constant: add an import for PDF_LIMITS
from '@corates/shared' at the top of the file and set MAX_SINGLE_FILE_SIZE to
PDF_LIMITS.MAX_SIZE instead of 50 * 1024 * 1024; keep MAX_CACHE_SIZE_BYTES
unchanged and mirror the pattern used in pdfValidation.js so the
single-source-of-truth PDF_LIMITS.MAX_SIZE is used across the codebase.
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.