Skip to content

fix(gitlab): preserve MR pagination when totals are missing - #34171

Merged
brennanb2025 merged 6 commits into
mainfrom
brennanb2025/tp-gitlab-mr-pages
Oct 27, 2026
Merged

brennanb2025 merged 6 commits into
mainfrom
brennanb2025/tp-gitlab-mr-pages

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 13 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​502 $\color{#cf222e}{\Huge{\mathbf{−}}}$​92 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​410
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​7 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4

GitLab sometimes returns a full page of merge requests without either total header. Orca treated the missing total as zero and reported exactly one page, even when more results could exist. The MR listing API now advertises one more page to probe after a full response and stops after a short or empty response, matching the existing issue-list and local CLI fallback rule.

Known totals retain their existing precedence: x-total-pages, then x-total. An unknown totalCount remains zero and the response shape stays unchanged. The common pattern uses observed page exhaustion; retaining our existing page-count contract is intended for compatibility with current IPC/RPC callers.

Reachability: this corrects API pagination metadata; no reachable shipped UI was found that uses this MR totalPages value to advance pages. Desktop Tasks (use-task-page-gitlab-loading.ts) requests page 1 with 50 items and discards the count; its MR list has no pager. Desktop workspace search and Cmd-J also request page 1 and consume items only. Mobile Tasks and workspace search call gitlab.listWorkItems with page 1 and consume items only. The combined list forwards the MR page count. Desktop IPC (gitlab:listMRs) and paired-server RPC (gitlab.listMRs, gitlab.listWorkItems) expose the corrected metadata, and the web bridge forwards it. No GitLab MR-list command or pagination reader was found in the CLI. This PR does not enable browsing beyond the first page in those screens.

Verification: the new missing-total regression failed against main (full page 3 expected totalPages: 4, received 1) and now passes. Coverage includes full first/later pages, short/empty exhaustion, unknown count, total-count fallback and page-header precedence. The full review added consumer assertions in existing harnesses for combined lists, IPC/RPC/web forwarding, desktop Tasks/search, Cmd-J, mobile search and mounted mobile Tasks. The mounted mobile file passed all 19 tests in Mobile verify. Final CI is green on 37e1ce9b99d, including static analysis/typechecking, unit shards, mobile bundle and Linux/Windows packaging. Full correctness, architecture, consumer, readiness and performance reviews found no production defect or negative user-facing trade-off. Tests-only QA applies; no running-app check or screenshot is required for unchanged screens.

Review path: full.

  • Size: one production file, 7 added and 3 removed lines; verification extends existing test files.
  • Risk boundary: the corrected pagination value is returned through desktop IPC and paired-server RPC, including combined listings; fields and types stay unchanged.
  • History: touched-file history has no same-symptom fix or revert in the previous 30 days; the preceding change was comment cleanup.
  • Effect: the provider result flows through combined lists and several transports, so the light path's local-effect criterion does not hold.

Builds on #33801 by @Hotragn — thank you! The contributor's fix and regression were cherry-picked with authorship preserved; the regression was extended with first-page, empty-page, and header-precedence controls.

Tracked in #33800

Tracked in STA-12217

@brennanb2025
brennanb2025 marked this pull request as ready for review October 27, 2026 01:20
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e0d95f3e-b72b-4017-be26-e06227a48c76
Commits

Reviewing files that changed from the base of the PR and between bfba354 and 37e1ce9.

Files selected for processing: 1
  • src/main/runtime/rpc/methods/gitlab.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

GitLab merge-request listings now infer page totals when pagination headers are missing. A full page without a total header indicates that another page may exist. Tests cover pagination metadata and its routing through API, runtime, renderer, and mobile paths. Renderer and mobile tests also cover item mapping and permission-denied responses.

Priority: Low

Merge Risk: Minimal · up to 37e1c

No actionable issue is established in the reviewed test change; it is mergeable subject to normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the primary change: preserving GitLab merge-request pagination when pagination totals are missing.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains the problem, implementation, scope, compatibility impact, linked issues, testing, CI results, and why visual proof is not required. It does not use the template headin…
Full details: Description check

Explanation

The description clearly explains the problem, implementation, scope, compatibility impact, linked issues, testing, CI results, and why visual proof is not required. It does not use the template headings or complete the checklist and AI Disclosure sections, but the required information is largely present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 14 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

GitLab can return a full merge-request page without either total header. Orca treated the missing total as zero and reported one page, so API consumers could stop early. The MR list now advertises one more page after a full response and stops after a short or empty response. Valid total headers keep their existing precedence, and the result shape is unchanged.

Current desktop/mobile Tasks and search screens still request page 1 and ignore this metadata. This PR corrects the API result; it does not add later-page browsing to those screens.

Full-path correctness, consumer, architecture, final readiness and performance checks found no production defects or negative user-facing trade-offs. Consumer verification was added to existing test harnesses. CI caught a double cast in a touched RPC fixture; it was replaced with typed runtime fixtures and spies. No production findings were deferred. The contributor-authored fix remains intact.

Verified: direct MR regression (failed against main), full/short/empty pages and authoritative-header precedence; combined-list metadata; IPC/RPC/web/owner forwarding; desktop Tasks and workspace-search rows; Cmd-J; mobile search/schema and mounted Tasks rows, refresh errors and page-1-only requests. Named local tests passed through tc-gate, and final CI passed on 37e1ce9b99d8a20d20c0a7e1edf4115bd555c319. Mobile verify executed the mounted Tasks file successfully (19 tests). No live GitLab or running-app QA was performed: this changes response metadata with unchanged rendered rows/actions and no shipped MR pager. No screenshot or agent run applies.

For Brennan's review: missing-total fallback and combined-list contract. The common exhaustion rule is adapted to the existing numeric paging contract; retaining exact header precedence is intended for API compatibility.

@brennanb2025
brennanb2025 merged commit 8f3d8c8 into main Oct 27, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants