fix(cloud-preview): 🐛 Load all paginated task pages by jorben · Pull Request #59 · MarkPDFdown/markpdfdown-desktop

jorben · 2026-03-04T09:24:44Z

Summary

Load all paginated task-page chunks from the cloud API instead of only the first chunk.
Deduplicate and sort merged page records by page number before rendering preview data.
Add a fallback fetch to ensure the currently selected page can still be loaded when backend page-size caps apply.

Test Plan

Open a cloud task with many pages and verify all pages are visible in preview metadata.
Navigate directly to a page not in the initial chunk and verify the image/content still loads.
Run renderer tests and verify CloudPreview pagination coverage passes.

🤖 Generated with Codex Cli

…agination

github-actions · 2026-03-04T09:25:05Z

AI Code Review Summary

PR: #59 (fix(cloud-preview): 🐛 Load all paginated task pages)
Preferred language: English

Overall Assessment

Detected 4 actionable findings, prioritize CRITICAL/HIGH before merge.

Major Findings by Severity

MEDIUM (3)
- src/renderer/pages/tests/CloudPreview.test.tsx:320 - Missing assertion for chunk-fetch error observability path
- src/renderer/pages/tests/CloudPreview.test.tsx:982 - Route-change reset test asserts fallback call but not primary reload call, risking false positives
- src/renderer/pages/CloudPreview.tsx:123 - Fallback may query previous task pages after route id change due to stale currentPage
LOW (1)
- src/renderer/pages/tests/CloudPreview.test.tsx:550 - Pagination interaction tests rely on CSS selector querying (li[title] a), increasing flakiness

Actionable Suggestions

Reset currentPage on id change and ensure fallback effect cannot run with stale page index.
Adjust the route-switch test to match intended UX/state reset behavior, preventing regression masking.
Consider adding a guard in ensureCurrentPageLoaded to skip when task is not loaded or currentPage is out of bounds.
Instrument console.error with vi.spyOn(console, 'error') in failure-case tests and assert expected messages.
In route-change test, assert getTaskPages('task-2', 1, 100) in addition to fallback calls.
Introduce stable test hooks for pagination controls (or semantic role/name queries) to avoid fragile CSS selectors.
Where possible, assert call order (toHaveBeenNthCalledWith) for primary fetch then chunk/fallback fetches to better encode behavior.

Potential Risks

Stale UI state leading to wrong API page requests after navigation.
Hidden regression if tests validate buggy behavior instead of desired behavior.
Async effect ordering races between initial fetch and fallback fetch.
Async fallback logic may regress while tests remain green due to broad toHaveBeenCalledWith checks.
UI library DOM changes could intermittently break tests that use raw selectors.
Operational debugging signal (logs) may silently disappear without test detection.

Test Suggestions

Route-switch regression test with old task on page N and new task with fewer pages.
Race test: slow initial fetch + fast fallback trigger should not duplicate/misfetch.
Assertion that dedupe keeps newest page item when same page appears across initial/fallback fetches.
Add a dedicated test for fetchPages when result.success === false that also verifies no extra chunk requests are attempted.
Add a test ensuring fallback is not re-triggered repeatedly for the same page after a successful fallback insertion.
Add call-order assertions for dynamic page_size correction path (page 3,size2 then corrected page 2,size3).

File-Level Coverage Notes

src/renderer/pages/CloudPreview.tsx: Behavioral changes (chunked loading + fallback fetch) are substantial and generally well-covered by new tests, but observability/error-path expectations are not fully validated by tests. (Implementation introduces multiple async branches; tests should lock both user-visible behavior and debug/ops signals.)
src/renderer/pages/tests/CloudPreview.test.tsx: Test suite expansion is strong and targets many edge cases, but includes some brittle DOM querying and misses a few key assertions that would make coverage more robust and less flaky. (Great breadth added; tightening assertion quality would significantly improve long-term reliability.)

Inline Downgraded Items (processed but not inline)

None

Coverage Status

Target files: 2
Covered files: 2
Uncovered files: 0
No-patch/binary covered as file-level: 0
Findings with unknown confidence (N/A): 0

Uncovered list:

None

No-patch covered list:

None

Runtime/Budget

Rounds used: 1/4
Planned batches: 1
Executed batches: 1
Sub-agent runs: 2
Planner calls: 1
Reviewer calls: 2
Model calls: 3/64
Structured-output summary-only degradation: NO

github-actions

Automated PR review completed.

Findings kept: 0
Findings with unknown confidence: 0
Inline comments attempted: 0
Target files: 0
Covered files: 0
Uncovered files: 0
See the summary comment for detailed analysis and coverage details.

github-actions

Automated PR review completed.

Findings kept: 6
Findings with unknown confidence: 0
Inline comments attempted: 6
Target files: 2
Covered files: 2
Uncovered files: 0
See the summary comment for detailed analysis and coverage details.

src/renderer/pages/__tests__/CloudPreview.test.tsx

    expect(window.api.cloud.getPageImage).toHaveBeenCalledWith({ taskId: 'task-1', pageNumber: 1 })
  })

+  it('fetches all API page chunks when task pages are paginated by backend', async () => {


src/renderer/pages/CloudPreview.tsx

  }, [id, cloudContext]);

+  // Fallback: ensure current page data is available even if backend enforces small page-size caps.
+  const ensureCurrentPageLoaded = useCallback(async () => {


src/renderer/pages/CloudPreview.tsx

+        const allPages = [...(result.data || [])];
+        const totalApiPages = result.pagination?.total_pages || 1;
+
+        for (let apiPage = 2; apiPage <= totalApiPages; apiPage++) {


src/renderer/pages/CloudPreview.tsx

  }, [id, cloudContext]);

+  // Fallback: ensure current page data is available even if backend enforces small page-size caps.
+  const ensureCurrentPageLoaded = useCallback(async () => {


src/renderer/pages/CloudPreview.tsx

+    if (pages.some(page => page.page === currentPage)) return;
+
+    try {
+      const result = await cloudContext.getTaskPages(id, currentPage, 1);


src/renderer/pages/CloudPreview.tsx

+          }
+        }
+
+        const dedupedSortedPages = Array.from(


github-actions

Automated PR review completed.

Findings kept: 4
Findings with unknown confidence: 0
Inline comments attempted: 4
Target files: 2
Covered files: 2
Uncovered files: 0
See the summary comment for detailed analysis and coverage details.

src/renderer/pages/CloudPreview.tsx

-        setPages(result.data);
+      const requestedPageSize = 100;
+      const result = await cloudContext.getTaskPages(id, 1, requestedPageSize);
+      if (result.success) {


src/renderer/pages/CloudPreview.tsx

+
+    inFlightFallbackPagesRef.current.add(currentPage);
+    try {
+      const result = await cloudContext.getTaskPages(id, currentPage, 1);


src/renderer/pages/CloudPreview.tsx

+      if (result.success && pageItems?.length) {
+        setPages(prev => dedupeAndSortPages([...prev, ...pageItems]));
+      }
+    } catch {


src/renderer/pages/__tests__/CloudPreview.test.tsx

+    cloudEventCb?.({ type: 'completed', data: { task_id: 'task-1', status: 6, pages_completed: 2, pages_failed: 0 } })
+    cloudEventCb?.({ type: 'completed', data: { task_id: 'task-1', status: 6, pages_completed: 2, pages_failed: 0 } })
+
+    await new Promise(resolve => setTimeout(resolve, 30))


github-actions

Automated PR review completed.

Findings kept: 3
Findings with unknown confidence: 0
Inline comments attempted: 2
Target files: 2
Covered files: 2
Uncovered files: 0
See the summary comment for detailed analysis and coverage details.

src/renderer/pages/CloudPreview.tsx

+    if (attemptedFallbackPagesRef.current.has(currentPage) || inFlightFallbackPagesRef.current.has(currentPage)) return;
+
+    const effectivePageSize = Math.max(1, taskPagesPageSizeRef.current || 1);
+    const fallbackApiPage = Math.max(1, Math.floor((currentPage - 1) / effectivePageSize) + 1);


src/renderer/pages/CloudPreview.tsx

+      }
+      const pageItems = (result.data || []).filter(page => page.page === currentPage);
+      if (result.success && pageItems?.length) {
+        setPages(prev => dedupeAndSortPages([...prev, ...pageItems]));


github-actions

Automated PR review completed.

Findings kept: 4
Findings with unknown confidence: 0
Inline comments attempted: 4
Target files: 2
Covered files: 2
Uncovered files: 0
See the summary comment for detailed analysis and coverage details.

github-actions · 2026-03-04T10:08:17Z

src/renderer/pages/__tests__/CloudPreview.test.tsx

+    })
+  })
+
+  it('keeps successfully fetched chunks when a later API page chunk fails', async () => {


[MEDIUM] Missing assertion for chunk-fetch error observability path

A test covers chunk failure recovery but does not assert that the error-reporting path is exercised, so logging/telemetry regressions could slip through unnoticed.

Suggestion: Spy on console.error (or your logging utility) and assert it is called with the chunk-specific message when a later page chunk throws. This ensures observability behavior is protected, not just UI continuity.

Risk: Silent failures in production become harder to diagnose if logging behavior regresses while tests still pass.

Confidence: 0.88

[From SubAgent: testing]

github-actions · 2026-03-04T10:08:17Z

src/renderer/pages/__tests__/CloudPreview.test.tsx

+
+    fireEvent.click(screen.getByRole('button', { name: 'Switch Task' }))
+
+    await waitFor(() => {


[MEDIUM] Route-change reset test asserts fallback call but not primary reload call, risking false positives

The route-change test validates a fallback fetch for task-2 but does not assert the expected initial fetch (task-2, page 1, size 100), so it may pass even if core page reload behavior breaks.

Suggestion: Add an explicit expectation that getTaskPages('task-2', 1, 100) is called after switching task IDs, then separately assert fallback behavior as needed.

Risk: A regression where new task pages are not fully reloaded on route change could go undetected while the test still passes due to fallback-only calls.

Confidence: 0.85

[From SubAgent: testing]

github-actions · 2026-03-04T10:08:17Z

src/renderer/pages/CloudPreview.tsx

  }, [id, cloudContext]);

+  // Fallback: ensure current page data is available even if backend enforces small page-size caps.
+  const ensureCurrentPageLoaded = useCallback(async () => {


[MEDIUM] Fallback may query previous task pages after route id change due to stale currentPage

When the task id changes, fallback logic can request a computed API page for the previous task's selected page index because currentPage is not reset with id transition.

Suggestion: Reset currentPage deterministically when id changes (e.g., to 1) before fallback runs, or guard ensureCurrentPageLoaded with current task metadata readiness and page bounds (currentPage <= task.page_count). Also update tests to assert first-page fetch after switch, not stale-page fallback.

Risk: Incorrect page requests after navigation can show wrong/empty content, add unnecessary API traffic, and create confusing UX on task switches.

Confidence: 0.93

[From SubAgent: general]

github-actions · 2026-03-04T10:08:17Z

src/renderer/pages/__tests__/CloudPreview.test.tsx

+      expect(screen.getByText(/6 pages/)).toBeInTheDocument()
+    })
+
+    const pageFiveAnchor = document.querySelector('li[title="5"] a') as HTMLElement | null


[LOW] Pagination interaction tests rely on CSS selector querying (li[title] a), increasing flakiness

Several new tests use implementation-detail CSS selectors for pagination controls, which are fragile against AntD markup/version changes and can cause flaky failures.

Suggestion: Prefer RTL semantic queries (e.g., getByRole('button', { name: '5' }) if available) or add stable data-testid hooks around pagination items in component markup for deterministic targeting.

Risk: Non-functional DOM structure changes may break tests unexpectedly, reducing signal quality and increasing maintenance churn.

Confidence: 0.93

[From SubAgent: testing]

jorben added 2 commits March 4, 2026 17:21

fix(cloud-preview): load all paginated task pages

c4afcff

Merge remote-tracking branch 'origin/master' into fix/cloud-preview-p…

226d6d0

…agination

github-actions bot reviewed Mar 4, 2026

View reviewed changes

jorben added 2 commits March 4, 2026 17:31

ci(workflow): 👷 Include TSX files in PR review filter

acdde73

chore(husky): 🔧 Disable pre-commit hook

3e26a93

github-actions bot reviewed Mar 4, 2026

View reviewed changes

fix(cloud-preview): 🐛 Harden paginated loading and fallback guards

c30bd53

github-actions bot reviewed Mar 4, 2026

View reviewed changes

fix(cloud-preview): 🐛 Correct fallback page-index mapping and tests

a6324b5

github-actions bot reviewed Mar 4, 2026

View reviewed changes

fix(cloud-preview): 🐛 Recompute fallback target on page-size drift

5e2552c

github-actions bot reviewed Mar 4, 2026

View reviewed changes

jorben merged commit aed8878 into master Mar 4, 2026
3 checks passed

jorben deleted the fix/cloud-preview-pagination branch March 4, 2026 10:09


		fireEvent.click(screen.getByRole('button', { name: 'Switch Task' }))

		await waitFor(() => {

Conversation

jorben commented Mar 4, 2026

Summary

Test Plan

Uh oh!

github-actions bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Code Review Summary

Overall Assessment

Major Findings by Severity

Actionable Suggestions

Potential Risks

Test Suggestions

File-Level Coverage Notes

Inline Downgraded Items (processed but not inline)

Coverage Status

Runtime/Budget

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions bot commented Mar 4, 2026 •

edited

Loading