Repository navigation
feat(mobile): diff review surface — inline comments, native review toolbar, rebuilt diff renderer - #5838
Conversation
…sions, restyled home list - Home list: drop the chat-card background; sessions hang off a thread rail under their workspace, date sections separate on whitespace alone - Workspace rows navigate to a new workspace page; a trailing secondary icon Button takes over the start-chat interaction - New workspace/[id] landing screen: chat list default tab + placeholder diff tab in a bottom tab bar - Home sessions and the workspace page list ACP sessions host-wide via acpSessions.list (new useHostAcpSessions hook); listSessions input is now optional/host-wide on the client - Composer target flow creates an ACP session and prompts it, landing in the ACP thread
…ss tabs - workspace/[id] uses the native stack header: floating back circle, centered GlassHeaderTitle pill with the workspace name, and a glass ellipsis menu (copy branch / rename) on the right - tabs move to expo-router NativeTabs ((tabs)/index + (tabs)/diff) so the bottom bar is the native liquid glass one - branch shown as a small centered line under the header on the chats tab; custom WorkspaceTabBar/DiffPlaceholder components removed
…itle + toolbar menu - plain native centered title (no GlassHeaderTitle pill) on the transparent header - ellipsis becomes a native Stack.Toolbar.Menu (copy branch / rename) composed on the (tabs) Stack.Screen, replacing the custom GlassView button + ActionSheetIOS
… subtitle, compose & menu toolbar - flatten navigation: the (tabs) screen now lives directly in the root authenticated stack (intermediate workspace Stack removed), so pushing from home gets the native back chevron for free - header config moves into the (tabs) layout via the Stack.Screen composition API (Stack.Title asChild + Stack.Toolbar must be DIRECT children — wrapper components are dropped by the matcher) - title view is name + branch subtitle, width-capped so it never slides under the trailing buttons - right side: native toolbar with a compose button (creates an ACP session, lands in the thread) and an ellipsis menu (copy branch, rename), sharing one glass capsule on iOS 26
…ons surface - tridot mirrors the home row menu: rename, copy branch / copy ID / share (inline group), destructive delete with the force-delete recovery flow (pops back to home on success); delete hidden for main - drop the Live sessions screen, route, and row-menu entry — ACP sessions are the chat list now
…, destructive delete last)
…me, delete, copy id/share)
…composer polish - Changes screen (files/commits/PR checks), files-changed + per-file views, commits formSheet with native pinned header and a continuous opaque TimelineRow rail - workspace page consolidated to a single screen: chat list, pinned composer with glass View-changes chip, full-viewport scroll surface - chat threads flattened into the root stack (native back chevron) with the session-title + workspace-name glass header - composer: whole-pill tap-to-focus (contentShape + onTapGesture), tighter SwiftUI edge padding; git router gains commit-files support
- files-changed and file-viewer cached different shapes (computed rows vs raw pair) under the same query key with infinite staleTime — whichever screen ran first poisoned the other; keys are now distinct - diff-row keys include the file's +/− stats so fresh edits bust the otherwise-immortal cache; viewer contents get a finite staleTime - changesets over 15 files rest collapsed so first open doesn't fire a full-contents git.getDiff per file through the relay
…mail doesn't match
…zontal pan, sticky headers, context expansion, jump-to-file
…roll handler crash)
…g during pill resize
…redate gesture-handler serialization API
…ton in thumb cluster
… chevron in bottom toolbar
…stom toolbar views get squeezed)
…List v2 + per-file item cache
…LOC in jump sheet
- CharWidthProbe: absoluteFill stretched the probe to screen width, so the
measured char advance came out ~2x too small; measure intrinsic width with a
probe short enough to never clamp
- reset a file's collapse toggle when its viewed baseline flips, so marking a
collapsed file viewed no longer re-expands it
- pull-to-refresh invalidates the per-file diff queries (staleTime Infinity +
count-keyed queryKey served stale diffs for same-count edits)
- persist lineType/tokens on draft comments so editing keeps add/del styling
and syntax highlighting in the anchor card
- drop the phantom trailing line from split("\n") on newline-terminated
files (spurious 1-line expander at EOF)
- move the maxScrollX write out of useMemo into an effect and clamp scrollX
when the max shrinks
- jump-to-file force-expands manually-collapsed unviewed targets
- RefreshControl progressViewOffset so the spinner clears the glass header
- weight CJK/emoji as two monospace cells in maxLineChars so wide lines don't
wrap inside fixed-height segments
- solid themed nav header replacing glass; drops all transparent-header inset/offset compensation from the list - file bars restyled to the native reference: chevron, monospace path, divider, ellipsis, viewed circle; height 56; +/- counts removed from bar - per-file tridot now opens a tap-triggered native UIMenu (@expo/ui SwiftUI Menu) with SF symbols and destructive Delete, replacing ActionSheetIOS - PressableScale no longer discards the caller's style prop, so fixed row heights (file bar 56, expander 38) actually apply
…dentity - collapse/expand and mark-viewed animate via LayoutAnimation + FlashList prepareForLayoutAnimationRender (flag auto-resets on commit) - useChangesetListItems now keeps stable object identity for file-header, note, and comment items (per-path/id caches), so toggling one section no longer re-renders every mounted file bar and its SwiftUI menu host - FileHeaderRow memoized so renderItem identity changes skip the host subtree - white +/- signs and add/del gutter numbers; anchor card sign matches
|
Too many files changed for review. ( Bypass the limit by tagging |
📝 WalkthroughWalkthroughThe mobile workspace review flow now includes virtualized tokenized diffs, expandable file context, draft line comments, viewed-file navigation, review submission sheets, updated workspace routing, and related file-viewing and shared UI improvements. ChangesMobile workspace review flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant FilesChangedScreen
participant DiffAPI
participant computeFileDiff
participant useChangesetListItems
participant HunkSegmentCell
participant LineCommentSheet
participant FinishReviewSheet
FilesChangedScreen->>DiffAPI: fetch changed file contents
FilesChangedScreen->>computeFileDiff: compute and tokenize file diff
FilesChangedScreen->>useChangesetListItems: build virtualized review items
useChangesetListItems->>HunkSegmentCell: render diff segments
HunkSegmentCell->>LineCommentSheet: open anchored comment composer
FinishReviewSheet->>DiffAPI: submit composed review prompt
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
# Conflicts: # apps/mobile/app/(authenticated)/_layout.tsx # apps/mobile/screens/(authenticated)/(home)/home/HomeScreen.tsx # apps/mobile/screens/(authenticated)/(home)/home/components/NewChatWidget/NewChatWidget.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/WorkspaceScreen/WorkspaceScreen.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/commits/CommitsSheet.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/diff/WorkspaceDiffScreen.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/file/FileViewerScreen.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/files-changed/FilesChangedScreen.tsx # apps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/computeFileDiff/computeFileDiff.ts # apps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/computeFileDiff/index.ts # apps/mobile/screens/(authenticated)/workspace/[id]/hooks/useWorkspacePullRequest/useWorkspacePullRequest.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/mobile/screens/(authenticated)/workspace/[id]/files-changed/FilesChangedScreen.tsx (1)
195-252: 🩺 Stability & Availability | 🔵 TrivialThread the
signalthrough the diff queries. The staged pipeline only advances once the current batch settles, so a few slow host requests can keep later files on the loading placeholder. Passing the TanStack Querysignalintogit.getDiff.querywill let cancellations bound that wait.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/files-changed/FilesChangedScreen.tsx around lines 195 - 252, Update the queryFn in the diffQueries useQueries configuration to accept TanStack Query’s AbortSignal and pass it through to git.getDiff.query. Preserve the existing staged pipeline and diff-processing behavior while ensuring canceled requests receive the signal.apps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx (1)
183-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
TargetRowinto its own component folder.
TargetRowis a second component defined in this file. As per coding guidelines, "Use one folder per component withComponentName/ComponentName.tsxandindex.ts; use one component per file" and single-use items should be "nested under the parent." Move it to something likefinish-review/components/TargetRow/TargetRow.tsx(+index.ts) since it's only used byFinishReviewSheet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx around lines 183 - 222, Extract TargetRow from FinishReviewSheet.tsx into finish-review/components/TargetRow/TargetRow.tsx with a matching index.ts re-export, preserving its props and rendered behavior. Update FinishReviewSheet to import TargetRow from the new component entry point and remove the local definition.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx:
- Around line 70-90: Update submit in the target === "new" branch to await the
startWorkspaceChat mutation before clearing the draft and navigating away.
Remove the mutate-call onSuccess callback, call clearWorkspace(workspaceId) only
after the mutation succeeds, then invoke router.back() so cleanup is not skipped
when the sheet unmounts.
---
Nitpick comments:
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/files-changed/FilesChangedScreen.tsx:
- Around line 195-252: Update the queryFn in the diffQueries useQueries
configuration to accept TanStack Query’s AbortSignal and pass it through to
git.getDiff.query. Preserve the existing staged pipeline and diff-processing
behavior while ensuring canceled requests receive the signal.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx:
- Around line 183-222: Extract TargetRow from FinishReviewSheet.tsx into
finish-review/components/TargetRow/TargetRow.tsx with a matching index.ts
re-export, preserving its props and rendered behavior. Update FinishReviewSheet
to import TargetRow from the new component entry point and remove the local
definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8e634a0-5466-4197-94a4-42e2e9b39994
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
apps/mobile/app/(authenticated)/_layout.tsxapps/mobile/app/(authenticated)/workspace/[id]/finish-review.tsxapps/mobile/app/(authenticated)/workspace/[id]/jump-to-file.tsxapps/mobile/app/(authenticated)/workspace/[id]/line-comment.tsxapps/mobile/components/ai-elements/code-block.tsxapps/mobile/package.jsonapps/mobile/screens/(authenticated)/(home)/home/components/NewChatWidget/NewChatWidget.tsxapps/mobile/screens/(authenticated)/components/PressableScale/PressableScale.tsxapps/mobile/screens/(authenticated)/workspace/[id]/WorkspaceScreen/WorkspaceScreen.tsxapps/mobile/screens/(authenticated)/workspace/[id]/commits/CommitsSheet.tsxapps/mobile/screens/(authenticated)/workspace/[id]/components/FileStatusBadge/FileStatusBadge.tsxapps/mobile/screens/(authenticated)/workspace/[id]/components/FileStatusBadge/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/diff/WorkspaceDiffScreen.tsxapps/mobile/screens/(authenticated)/workspace/[id]/file/FileViewerScreen.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/FilesChangedScreen.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/AnimatedCellContainer/AnimatedCellContainer.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/AnimatedCellContainer/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/CommentCardRow/CommentCardRow.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/CommentCardRow/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/ExpanderRow/ExpanderRow.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/ExpanderRow/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/FileHeaderRow/FileHeaderRow.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/FileHeaderRow/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/HunkSegmentCell/HunkSegmentCell.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/HunkSegmentCell/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/ReviewOverlay/ReviewOverlay.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/components/ReviewOverlay/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/hooks/useChangesetListItems/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/hooks/useChangesetListItems/useChangesetListItems.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/buildDisplayRows/buildDisplayRows.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/buildDisplayRows/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/buildListItems/buildListItems.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/buildListItems/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/computeFileDiff/computeFileDiff.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/computeFileDiff/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/diffMetrics/CharWidthProbe.tsxapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/diffMetrics/diffMetrics.tsapps/mobile/screens/(authenticated)/workspace/[id]/files-changed/utils/diffMetrics/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsxapps/mobile/screens/(authenticated)/workspace/[id]/finish-review/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/jump-to-file/JumpToFileSheet.tsxapps/mobile/screens/(authenticated)/workspace/[id]/jump-to-file/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/line-comment/LineCommentSheet.tsxapps/mobile/screens/(authenticated)/workspace/[id]/line-comment/components/AnchorLineRow/AnchorLineRow.tsxapps/mobile/screens/(authenticated)/workspace/[id]/line-comment/components/AnchorLineRow/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/line-comment/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/commentComposerStore/commentComposerStore.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/commentComposerStore/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/diffViewStore/diffViewStore.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/diffViewStore/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/draftCommentsStore/draftCommentsStore.tsapps/mobile/screens/(authenticated)/workspace/[id]/stores/draftCommentsStore/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/utils/languageForPath/index.tsapps/mobile/screens/(authenticated)/workspace/[id]/utils/languageForPath/languageForPath.ts
| const submit = async () => { | ||
| if (!workspace || !host || comments.length === 0 || sending) return; | ||
| const prompt = composeReviewPrompt(message, comments); | ||
| setSending(true); | ||
| try { | ||
| if (target === "new") { | ||
| startWorkspaceChat.mutate( | ||
| { | ||
| target: { | ||
| workspaceId: workspace.id, | ||
| workspaceName: workspace.name, | ||
| branch: workspace.branch, | ||
| hostId: workspace.hostId, | ||
| }, | ||
| message: { text: prompt, attachments: [] }, | ||
| }, | ||
| { onSuccess: () => clearWorkspace(workspaceId) }, | ||
| ); | ||
| router.back(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
TanStack Query v5 mutate callback onSuccess passed to mutate vs useMutation both called
💡 Result:
In TanStack Query v5, both the onSuccess callback provided to useMutation and the one passed directly to the mutate function are designed to execute, but they serve different purposes and operate with different behaviors [1][2][3]. When both are defined, the library executes them in sequence: first the onSuccess callback from useMutation, and then the onSuccess callback from the mutate call [3]. Key distinctions to keep in mind: 1. Lifecycle and Execution: The onSuccess callback in useMutation is considered global to that mutation instance and will always fire when the mutation succeeds [4][1][2]. Conversely, the onSuccess callback passed to mutate is intended for component-specific side effects. Because these callbacks rely on the component remaining mounted, they will not execute if the component unmounts before the mutation completes [1][2][5]. 2. Consecutive Mutations: If you trigger multiple mutations rapidly, callbacks passed directly to mutate exhibit different behavior than those defined in useMutation [1][2]. The useMutation callbacks execute for every mutation call [1][2]. However, callbacks passed to mutate are bound to the specific mutation request; if a new mutation is triggered before a previous one finishes, the "observer" for the previous request may be removed, which can cause the mutate-specific onSuccess callback to be skipped [4][1][6]. 3. Recommended Usage: Due to the potential for component-specific callbacks to be skipped during unmounting or rapid consecutive calls, it is generally recommended to place core mutation logic (like query invalidation or cache updates) inside the useMutation hook [4][6]. For component-specific side effects (like navigation or UI alerts), if you need them to be more reliable, consider using mutateAsync, which returns a promise you can await, rather than relying on the callback passed to mutate [4][5]. Example of execution order in source code: await latestConfig.onSuccess?.(data, variables!) // useMutation onSuccess await mutateConfig.onSuccess?.(data, variables!) // mutate onSuccess [3]
Citations:
- 1: https://tanstack.com/query/latest/docs/framework/react/guides/mutations?from=reactQueryV3
- 2: https://tanstack.com/query/v5/docs/framework/react/guides/mutations.md
- 3: https://github.com/TanStack/query/blob/6e2e37ea5fe180a6d5ee3dedf76bd72160406b2a/src/react/useMutation.ts
- 4: The onSuccess callback has confusing behavior when specified on useMutation vs. on mutate TanStack/query#3040
- 5: https://tanstack.com/query/v5/docs/framework/react/guides/mutations
- 6: onSuccess options of mutate is not called. TanStack/query#5604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed component and the related mutation hook.
printf '\n== FinishReviewSheet.tsx ==\n'
ast-grep outline apps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx --view expanded || true
printf '\n--- file excerpt ---\n'
sed -n '1,220p' 'apps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx'
printf '\n== useStartWorkspaceChat references ==\n'
rg -n "useStartWorkspaceChat|startWorkspaceChat" apps/mobile -g '!**/node_modules/**'Repository: superset-sh/superset
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='apps/mobile/screens/(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx'
printf '\n== outline ==\n'
ast-grep outline "$file" --view expanded || true
printf '\n== file excerpt ==\n'
sed -n '1,240p' "$file"
printf '\n== hook references ==\n'
rg -n "useStartWorkspaceChat|startWorkspaceChat" apps/mobile -g '!**/node_modules/**'Repository: superset-sh/superset
Length of output: 8607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='apps/mobile/screens/(authenticated)/(home)/home/components/NewChatWidget/hooks/useStartWorkspaceChat/useStartWorkspaceChat.ts'
printf '\n== outline ==\n'
ast-grep outline "$file" --view expanded || true
printf '\n== file excerpt ==\n'
sed -n '1,240p' "$file"Repository: superset-sh/superset
Length of output: 2319
Avoid relying on the mutate-call onSuccess here.
router.back() unmounts this sheet immediately, so clearWorkspace(workspaceId) can be skipped if the mutation settles after unmount. That leaves the draft attached and makes it easy to resend duplicates later. Await the mutation first, then clear the draft and navigate away.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/mobile/screens/`(authenticated)/workspace/[id]/finish-review/FinishReviewSheet.tsx
around lines 70 - 90, Update submit in the target === "new" branch to await the
startWorkspaceChat mutation before clearing the draft and navigating away.
Remove the mutate-call onSuccess callback, call clearWorkspace(workspaceId) only
after the mutation succeeds, then invoke router.back() so cleanup is not skipped
when the sheet unmounts.
Summary
Second wave of the mobile workspace UX work (follow-up to #5635), centered on turning the files-changed screen into a full review surface:
FileStatusBadge).TimelineRow), author avatars (org member → GitHub noreply → monogram), vertical commit glyph.Test plan
bun run lintand turbo typecheck green (mobile, host-client, host-service)Summary by cubic
Turns the mobile files‑changed view into a full review surface with inline comments, a native finish‑review toolbar, and a rebuilt FlashList‑based diff renderer. Also adds a consolidated workspace screen, ACP‑backed chat lists, a commits sheet, and native per‑file headers and menus.
New Features
@shopify/flash-listv2 with exact row heights and per‑file item cache; syntax‑highlighted lines, context expansion, and pipelined loading; horizontal pan with edge‑only back gesture.useHostAcpSessions; host‑clientlistSessionsinput is now optional. Dependencies: added@shopify/flash-listanddiff.Bug Fixes
Written for commit 58f5b6a. Summary will update on new commits.
Summary by CodeRabbit