Skip to content

fix(android): preserve chat back stack in the PR viewer - #18839

Open
PixPMusic wants to merge 2 commits into
pingdotgg:mainfrom
PixPMusic:t3/fix-pr-viewer-back-stack
Open

PixPMusic wants to merge 2 commits into
pingdotgg:mainfrom
PixPMusic:t3/fix-pr-viewer-back-stack

Conversation

@PixPMusic

@PixPMusic PixPMusic commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Opening a PR from an Android chat currently replaces the chat history, so Back lands on the PR list. Contextual PR links now preserve that history; Git controls sheets are replaced when appropriate. Toolbar Back and native Back return through the preceding routes.

Explicit workspace, tab, and sidebar selections still reset peer history. PR-linked threads and unsent drafts retain their PR return path. After an immediate first send is durably saved in the local outbox, Android selects Chats and resets to Home plus the submitted thread. Offline or upload-waiting sends retain the previous return behavior. iOS navigation remains unchanged.

Based on @PixPMusic’s work in #18839. Main was merged without rewriting contributor commits. The shared list-switching change was replaced with a separate detail Back callback so explicit navigation keeps its existing behavior; current root-targeted navigation and the route-free sidebar opener are preserved.

Verification:

  • 32 focused tests pass across Android workspace navigation, Home thread selection, and Git overview navigation.
  • PR-added coverage reduced from nine cases in a new one-off suite to seven in the existing router suite. All 18 baseline Android navigation cases remain, for 25 in that suite.
  • Selectively restoring main reproduces three contextual-navigation failures; restoring the original PR reproduces the explicit list-switching failure. These substitutions do not prove the full first-send persistence flow.
  • Mobile typecheck, targeted lint and formatting, and whitespace checks against merged main pass. Lint reports nine existing warnings on unchanged source contexts.
  • Two full-diff review rounds with Claude Opus 5.5 and Codex GPT-6-Sol. Round one caught a multi-PR sidebar sheet that accidentally preserved unrelated history. Its workspace override and a regression test fix that path; round two returned no code blockers. Current-head CI is followed after publication.

The tests reduce real router actions and check surviving route keys and Back destinations. The asynchronous outbox/UI caller, linked-thread opener, draft cancellation, keyboard priority, and native rendering were inspected in source rather than exercised end to end. Continuing a thread already below the PR intentionally opens a contextual instance above it, preserving Back to the PR; there is no name-only pop across environment identities.

Live native verification is unavailable: the Device tool reports “Agent device access is turned off for this environment.” The contributor’s prior captures predate this rebuild and are not current-head proof. Android phone/foldable layout, keyboard and hardware Back, draft cancellation, immediate and queued first sends, and iOS smoke coverage still need a native pass. macOS, Windows, and Electron desktop were not exercised. No server, provider, or wire-contract behavior changes.

Tests: GPT-6-Astra. Implementation: GPT-6.1-Sol. Harness: Codex in T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 12, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes the default Android PR-opening behavior and coordinates stack, toolbar Back, hardware Back, and draft-submit navigation across several existing flows. The user-visible navigation impact warrants human validation, especially for native Android behavior.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c74c49d1-30be-4550-9059-f0f7dc9e8443
📥 Commits

Reviewing files that changed from the base of the PR and between 81338cb and 4adc260.

📒 Files selected for processing: 10
  • apps/mobile/src/features/pull-requests/usePullRequestThreads.ts
  • apps/mobile/src/features/threads/NewTaskDraftScreen.tsx
  • apps/mobile/src/features/threads/ThreadHeader.tsx
  • apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx
  • apps/mobile/src/features/threads/ThreadRouteScreen.tsx
  • apps/mobile/src/features/threads/new-task-submit-navigation.ts
  • apps/mobile/src/features/workspace/useAndroidWorkspaceNavigation.ts
  • apps/mobile/src/lib/android-workspace-navigation.test.ts
  • apps/mobile/src/lib/android-workspace-navigation.ts
  • apps/mobile/src/native/useOpenPullRequest.ts

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

📝 Walkthrough

Pull-request, thread, and task-submission navigation now use updated route actions. Android Back handling now accounts for keyboard visibility and the current navigation stack. Tests cover pull-request history, workspace Back behavior, and task-submission return destinations.

Changes: Mobile navigation behavior

Layer / File(s) Summary
Pull-request navigation modes
apps/mobile/src/native/useOpenPullRequest.ts, apps/mobile/src/lib/android-workspace-navigation.ts, apps/mobile/src/features/pull-requests/usePullRequestThreads.ts, apps/mobile/src/features/threads/ThreadPullRequestsSheet.tsx, apps/mobile/src/lib/android-workspace-navigation.test.ts
Pull-request navigation now supports contextual and workspace modes. Android workspace resets use a shared root-state helper. Thread opening navigates to the Thread route across platforms. Tests cover opening a pull request from a sidebar sheet.
Workspace detail Back behavior
apps/mobile/src/features/workspace/useAndroidWorkspaceNavigation.ts, apps/mobile/src/features/threads/ThreadHeader.tsx, apps/mobile/src/features/threads/ThreadRouteScreen.tsx
Thread back actions now return to the previous route when the stack allows it, and otherwise use workspace-list navigation. Android hardware Back dismisses a visible keyboard, delegates to navigation when possible, or returns to the list.
Task submission destinations
apps/mobile/src/features/threads/NewTaskDraftScreen.tsx, apps/mobile/src/features/threads/new-task-submit-navigation.ts, apps/mobile/src/lib/android-workspace-navigation.test.ts
Queued submissions return to the previous route. Immediate submissions reset Android navigation to Thread or replace the current route on other platforms. Tests cover task return destinations and navigation history.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant NewTaskDraftScreen
  participant androidWorkspaceRootState
  participant newTaskSubmitNavigationAction
  participant Navigation
  NewTaskDraftScreen->>androidWorkspaceRootState: get Android root state when available
  NewTaskDraftScreen->>newTaskSubmitNavigationAction: build action from state and task parameters
  alt Task is queued
    newTaskSubmitNavigationAction-->>NewTaskDraftScreen: goBack action
  else Immediate Android task
    newTaskSubmitNavigationAction-->>NewTaskDraftScreen: reset to Thread action
  else Immediate task on another platform
    newTaskSubmitNavigationAction-->>NewTaskDraftScreen: replace with Thread action
  end
  NewTaskDraftScreen->>Navigation: dispatch submission action
Loading

Possibly related PRs

  • pingdotgg/t3code#21071: Changes the same Android workspace reset and navigation hooks, including root-targeted resets and workspace-list navigation.

Suggested reviewers: maria-rcks, juliusmarminge

Merge Risk: ⚪ Minimal · up to 4adc2

The supplied evidence identifies no actionable issue that would prevent merging after normal checks.

Security architecture risk: 🔵 Low · up to 4adc2

The changes preserve existing task validation, permission checks, and environment-scoped identities while changing return destinations. No introduced security bypass was established. Remaining uncertainty concerns navigation during interruption and nested-screen transitions, rather than demonstrated broader access or authority.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated changed exposure is within mobile navigation history and post-acceptance return destinations. URL-derived PR identity reaches the same destination builder, and accepted task navigation uses the same environment/thread identity; the inspected changes do not establish broader service authority.

Security Findings and Attack Paths

  • inferred — No introduced authorization bypass was established along the inspected URL-to-PR and draft-to-outbox paths. Navigation mode changes stack handling rather than destination scope or submission permission checks. This conclusion is client-side and does not verify backend authorization for arbitrary identifiers.

Trust Boundaries and Controls

  • observed — PR action availability remains gated by connection state, operation permissions, capabilities, and returned viewer permissions. Preserving route history is not used as the permission grant for these actions.

Resilience and Maintainability Implications

  • observed — The existing outbox path retains environment/thread-scoped identity and message deduplication. Delivery rechecks authorization and revisions; upload and recovery handling preserve ownership rather than treating successful navigation as proof of delivery. These controls were not introduced by this PR.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: preserving the Android chat back stack when viewing a pull request.
Description check Passed The description explains the problem, implementation scope, behavior changes, test results, limitations, and unavailable native verification. It is detailed and mostly matches the required template, a…
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.
Full details: Description check

Explanation

The description explains the problem, implementation scope, behavior changes, test results, limitations, and unavailable native verification. It is detailed and mostly matches the required template, although it does not use the template headings and does not provide explicit maintainer approval evidence beyond the linked prior pull request.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@TimPeterNL

Copy link
Copy Markdown

Confirmed on Android 2.0.0 beta. Back from a PR opened in a thread lands on the PR list; this fixes it. 🙏

Small aside, not blocking: "Continue thread" to the same thread you came from adds a second copy of it to the stack (RN 7 navigate doesn't pop without getId). navigate(…, { pop: true }) in openThread might avoid that.

@PixPMusic
PixPMusic force-pushed the t3/fix-pr-viewer-back-stack branch from 457acb4 to 81338cb Compare October 17, 2026 05:33
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 17, 2026
@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one for batch pull-request-11. I’ll preserve the contextual PR return path, reconcile current Android navigation changes, and verify shared Back handling and first-send behavior across entry points.

@juliusmarminge

Copy link
Copy Markdown
Member

@PixPMusic thanks for the original reproduction and fix. The takeover preserves the contextual chat return path and first-send behavior, keeps explicit workspace selection resets, and trims the regression suite. Two independent review rounds are clear after fixing the multi-PR sidebar badge selection.

A separate existing limitation surfaced in review: PR A → a PR B link in its description updates the focused PR route, so Back skips A. Main already loses that route too. I kept that follow-up outside this chat-return fix. Continuing a linked thread intentionally keeps a contextual instance above the PR so Back returns to the PR; a name-only pop would lose that destination and can select the wrong thread.

Device access is disabled here, so the updated body distinguishes passing router/typecheck proof from the remaining native verification.

@macroscopeapp
macroscopeapp Bot dismissed their stale review October 19, 2026 08:20

Dismissing prior approval to re-evaluate 4adc260

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Oct 19, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants