Skip to content

fix(mobile): show PR file diff when tapping row on single-repo tasks#907

Closed
luancm wants to merge 3 commits into
kdlbs:mainfrom
luancm:fix/pr-changes-on-mobile
Closed

fix(mobile): show PR file diff when tapping row on single-repo tasks#907
luancm wants to merge 3 commits into
kdlbs:mainfrom
luancm:fix/pr-changes-on-mobile

Conversation

@luancm
Copy link
Copy Markdown
Contributor

@luancm luancm commented May 15, 2026

Tapping a PR Changes file on mobile single-repo tasks rendered "No changes" because PRFileRow stamped repository_name="" (single-repo sentinel) into OpenDiffOptions, while useReviewSources tags PR entries in allFiles with pr.repo (e.g. "kandev") — filterVisibleFiles then dropped every row. Convert the empty stamp to undefined in mobile-changes-panel.tsx so the repo filter is skipped, matching the desktop path which never forwards repositoryName.

Validation

  • pnpm vitest run components/task/task-changes-panel.test.ts — 18/18 pass
  • pnpm lint on changed files — clean
  • New e2e at apps/web/e2e/tests/task/mobile-changes-panel.spec.ts:159 seeds a mock PR with patch content, taps the file row, asserts diff text renders (would fail before fix)
  • Manual: did not run mobile build (no device handy); covered by e2e

Possible Improvements

Low risk. Pre-existing gap (out of scope): multi-repo PR file taps still mismatch because useReviewSources only fetches the primary PR via usePRDiff and stamps with pr.repo, while the timeline stamps with the workspace repoName. File a separate issue if needed.

Checklist

  • I have performed a self-review of my code.
  • I have manually tested my changes and they work as expected.
  • My changes have tests that cover the new functionality and edge cases.
  • If my change touches UI files (apps/web/), I have added or updated Playwright e2e tests in apps/web/e2e/ and verified them with make test-e2e.

PR file rows stamp `repository_name=""` (single-repo sentinel) on the
timeline, but `useReviewSources` tags PR files in `allFiles` with
`pr.repo` (e.g. "kandev"). The previous code passed the empty stamp
straight into `DiffSheetMode`, so `filterVisibleFiles` matched
`(file.repository_name ?? "") === ""` and dropped every PR file —
"No changes". Convert empty string to `undefined` so the repo filter
is skipped, matching desktop behavior.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4e601208-2db4-417f-9810-8f3f198f81d9

📥 Commits

Reviewing files that changed from the base of the PR and between ca451a3 and ed4ccc6.

📒 Files selected for processing (7)
  • apps/web/components/task/dockview-desktop-layout.tsx
  • apps/web/components/task/mobile/mobile-changes-panel.tsx
  • apps/web/components/task/task-changes-panel.test.ts
  • apps/web/components/task/task-changes-panel.tsx
  • apps/web/hooks/domains/session/use-review-sources.ts
  • apps/web/lib/state/dockview-panel-actions.ts
  • apps/web/lib/state/dockview-store.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Fixed PR file display in mobile changes panel file mode to ensure PR files are properly shown even when they exist in other contexts
    • Improved PR diff handling and source filtering in file view mode
  • Tests

    • Added E2E test coverage for opening PR file diffs in the mobile changes panel

Walkthrough

Adds a non-deduplicated rawPRFiles list to review sources, uses it to show PR-only files in file-mode when sourceFilter === "pr", threads source through dockview panel actions, normalizes mobile diff params, and adds an E2E test verifying tapping a PR file opens its diff.

Changes

PR file source and diff plumbing

Layer / File(s) Summary
Expose rawPRFiles from useReviewSources
apps/web/hooks/domains/session/use-review-sources.ts
Adds rawPRFiles: ReviewFile[] to the hook return and computes it from prDiffFiles via addPRFiles without uncommitted-path filtering.
Visible-file filtering & TaskChangesPanel wiring
apps/web/components/task/task-changes-panel.tsx, apps/web/components/task/task-changes-panel.test.ts
Refactors filterVisibleFiles to accept an options object including rawPRFiles, adds a PR-bypass path for file-mode+sourceFilter === "pr", updates useVisibleDiffState and TaskChangesPanel to pass rawPRFiles, and updates tests to the new options API.
Dockview diff panel source plumbing
apps/web/components/task/dockview-desktop-layout.tsx, apps/web/lib/state/dockview-panel-actions.ts, apps/web/lib/state/dockview-store.ts
Adds source typing and passes options?.source from dockview layout into addFileDiffPanel, and stores params.source on opened file-diff preview panels.
Mobile diff opening normalization
apps/web/components/task/mobile/mobile-changes-panel.tsx
handleOpenDiffFile now normalizes repositoryName to undefined when options?.repositoryName is not provided.
E2E test for PR file diff display
apps/web/e2e/tests/task/mobile-changes-panel.spec.ts
New Playwright test that mocks a PR with a file/patch, opens the mobile changes panel, taps the PR file row, and asserts the diff sheet displays expected PR patch content.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • kdlbs/kandev#357: Related changes that integrate PR detection and websocket PR flows which interact with review-source behavior.
  • kdlbs/kandev#618: Prior edits to task-changes-panel.tsx affecting visible-file lifecycle (auto-close) that may interact with the new rawPRFiles behavior.

Poem

🐰 I hopped through code to fetch PR trails,
Raw files kept whole where dedupe fails,
Panels now pass sources true and neat,
Mobile taps bloom diffs—what a treat! 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically summarizes the main fix: enabling PR file diff display when tapping rows on mobile single-repo tasks.
Description check ✅ Passed The description includes all required sections: clear problem explanation, validation with specific test commands and results, and a completed checklist. Includes optional risk assessment.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

@luancm luancm marked this pull request as ready for review May 15, 2026 10:19
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR fixes a "No changes" regression on mobile single-repo tasks where tapping a PR file row passed an empty-string repositoryName into OpenDiffOptions, causing filterVisibleFiles to drop every PR entry. The fix converts the empty string to undefined in mobile-changes-panel.tsx and introduces a rawPRFiles bypass in filterVisibleFiles for the case where a PR file is also present in local changes (deduplication would otherwise hide it).

  • mobile-changes-panel.tsx: options?.repositoryName || undefined converts the single-repo empty-string sentinel to undefined so the repo filter is skipped, matching desktop behavior.
  • task-changes-panel.tsx / use-review-sources.ts: A new rawPRFiles field (non-deduplicated PR entries) is plumbed through useReviewSourcesuseChangesViewfilterVisibleFiles, which uses it as a fast-path when mode === "file" && sourceFilter === "pr".
  • dockview-desktop-layout.tsx: Desktop now also threads source through addFileDiffPanel so the sourceFilter prop on TaskChangesPanel is populated on the desktop path.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to the PR file diff display path and does not touch data mutation, auth, or any shared state beyond the diff viewer.

The fix is minimal and well-contained: a one-line || undefined coercion in mobile-changes-panel.tsx plus a targeted bypass in filterVisibleFiles that only activates when mode === "file" && sourceFilter === "pr" && rawPRFiles.length > 0. All 18 existing unit tests pass, a new regression test covers the deduplication-bypass scenario, and a new e2e test verifies the full tap-to-diff flow. The known multi-repo limitation is explicitly documented in both code and the PR description.

No files require special attention.

Important Files Changed

Filename Overview
apps/web/components/task/mobile/mobile-changes-panel.tsx Core fix: converts empty-string repositoryName to undefined so the repo filter is skipped for single-repo PR file rows
apps/web/components/task/task-changes-panel.tsx Adds rawPRFiles bypass in filterVisibleFiles and threads rawPRFiles through useChangesView and useVisibleDiffState; refactors filterVisibleFiles to accept an opts object
apps/web/hooks/domains/session/use-review-sources.ts Exposes rawPRFiles (non-deduplicated PR entries built via addPRFiles with empty uncommittedPaths) on UseReviewSourcesResult
apps/web/components/task/dockview-desktop-layout.tsx Forwards options.source through addFileDiffPanel and reads it back as sourceFilter in DiffViewerContent, aligning desktop with mobile behavior
apps/web/components/task/task-changes-panel.test.ts Updates call sites for refactored filterVisibleFiles signature and adds a new test covering the deduplication-bypass scenario
apps/web/e2e/tests/task/mobile-changes-panel.spec.ts Adds e2e test seeding a mock PR with patch content, tapping the file row, and asserting diff text is visible
apps/web/lib/state/dockview-panel-actions.ts Widens addFileDiffPanel opts type to include source and stores it in panel params
apps/web/lib/state/dockview-store.ts Type signature update only to match dockview-panel-actions.ts change

Reviews (3): Last reviewed commit: "fix: PR file row click shows PR diff ins..." | Re-trigger Greptile

Comment thread apps/web/components/task/mobile/mobile-changes-panel.tsx Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/web/components/task/mobile/mobile-changes-panel.tsx">

<violation number="1" location="apps/web/components/task/mobile/mobile-changes-panel.tsx:70">
P2: Hardcoding `sourceFilter: "all"` discards the `source: "pr"` passed by `PRFileRow`, so PR file taps will show changes from all sources instead of just the PR diff. The `repositoryName` fix alone addresses the "No changes" bug described in the PR — consider preserving the source filter: `sourceFilter: options?.source ?? "all"`.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread apps/web/components/task/mobile/mobile-changes-panel.tsx Outdated
@luancm
Copy link
Copy Markdown
Contributor Author

luancm commented May 15, 2026

@cubic-dev-ai The "all" hardcode is intentional. useReviewSources deduplicates across sources — a file that appears in both the PR diff and the current uncommitted working tree lands in allFiles as source: "uncommitted" (priority: uncommitted > committed > pr). Forwarding sourceFilter: "pr" would cause filterVisibleFiles to drop the entry and show "No changes" again for that case.

Desktop never passes a source filter at all (DiffViewerContent in dockview-desktop-layout.tsx omits the prop), so "all" correctly matches desktop behavior.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented May 15, 2026

@cubic-dev-ai The "all" hardcode is intentional. useReviewSources deduplicates across sources — a file that appears in both the PR diff and the current uncommitted working tree lands in allFiles as source: "uncommitted" (priority: uncommitted > committed > pr). Forwarding sourceFilter: "pr" would cause filterVisibleFiles to drop the entry and show "No changes" again for that case.

Desktop never passes a source filter at all (DiffViewerContent in dockview-desktop-layout.tsx omits the prop), so "all" correctly matches desktop behavior.

@luancm Makes sense — thanks for the explanation. The "all" hardcode is the right call given how useReviewSources prioritizes sources during dedup. Forwarding "pr" from PRFileRow would indeed reintroduce the "No changes" bug for files that appear in both the PR diff and the working tree, which is exactly the edge case your dedup logic handles.

Desktop already skips the source filter entirely (no prop forwarded), so "all" aligns cleanly — it's effectively the same behavior, just explicit because the mobile code passes through DiffSheetMode which requires a value. No divergence here.

@luancm
Copy link
Copy Markdown
Contributor Author

luancm commented May 15, 2026

I've just realized that the PR changes is not working correctly (not even for desktop)

In my example, I have a task with a PR opened that changes a file, and unstaged changes to that same file.
When I click in the file through the UNSTAGED section of the changes tab, it shows correctly the diff with only the local changes against the PR
But when I click in that file through the "PR Changes" section, it also shows the same diff, instead of the changes of the PR itself

I'll see if it is worth to fix in this PR.
If this gets merged before that, I'll open a new one with that fix

@luancm
Copy link
Copy Markdown
Contributor Author

luancm commented May 15, 2026

I'll open another PR with a more appropriate fix that also fixes the Desktop version

@luancm luancm closed this May 15, 2026
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.

1 participant