[codex] Add Android mobile support#3579
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| {Platform.OS === "android" ? ( | ||
| <AndroidScreenHeader | ||
| title="Choose project" | ||
| onBack={layout.usesSplitView ? () => router.back() : undefined} |
There was a problem hiding this comment.
🟠 High new/index.tsx:118
On Android, the native stack header is hidden (headerShown: false), so the only navigation chrome is AndroidScreenHeader. When layout.usesSplitView is false (normal phone layout), onBack is undefined, which means AndroidScreenHeader renders no back button — leaving the user with no visible way to navigate back from this screen. The fix is to always pass onBack on Android so the back chevron is always rendered.
| onBack={layout.usesSplitView ? () => router.back() : undefined} | |
| onBack={() => router.back()} |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/app/new/index.tsx around line 118:
On Android, the native stack header is hidden (`headerShown: false`), so the only navigation chrome is `AndroidScreenHeader`. When `layout.usesSplitView` is `false` (normal phone layout), `onBack` is `undefined`, which means `AndroidScreenHeader` renders no back button — leaving the user with no visible way to navigate back from this screen. The fix is to always pass `onBack` on Android so the back chevron is always rendered.
There was a problem hiding this comment.
🟡 Medium
In split layout, viewportWidth starts at 0 and is only corrected after onLayout fires. This makes userBubbleMaxWidth and reviewCommentBubbleWidth start at 0, and since those values are passed only through the renderItem closure — not included in extraData (listAppearanceData) — LegendList does not repaint the already-visible rows when the widths are corrected. The initial rows stay rendered with width 0, collapsing user bubbles and review comments until some unrelated list update triggers a repaint. Consider adding userBubbleMaxWidth and reviewCommentBubbleWidth to listAppearanceData so visible rows re-render when the viewport width is resolved.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1254:
In split layout, `viewportWidth` starts at `0` and is only corrected after `onLayout` fires. This makes `userBubbleMaxWidth` and `reviewCommentBubbleWidth` start at `0`, and since those values are passed only through the `renderItem` closure — not included in `extraData` (`listAppearanceData`) — LegendList does not repaint the already-visible rows when the widths are corrected. The initial rows stay rendered with width `0`, collapsing user bubbles and review comments until some unrelated list update triggers a repaint. Consider adding `userBubbleMaxWidth` and `reviewCommentBubbleWidth` to `listAppearanceData` so visible rows re-render when the viewport width is resolved.
| } | ||
|
|
||
| let cancelled = false; | ||
| let frame: number | null = null; |
There was a problem hiding this comment.
🟡 Medium diffs/nativeReviewDiffSurface.ts:187
useNativeReviewDiffPayload stops retrying after 4 animation frames when nativeRef.current or its command is still missing. Because rowsJson/tokensJson/tokensPatchJson are only delivered via this imperative command (not passed as props to the native view), a slow device that takes longer than 4 frames to attach the native tag silently drops the initial payload, leaving the diff surface blank until the JSON changes again and the effect re-runs. Consider removing the 4-attempt cap so dispatch keeps retrying each frame until the ref is ready or the effect is cleaned up.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts around line 187:
`useNativeReviewDiffPayload` stops retrying after 4 animation frames when `nativeRef.current` or its command is still missing. Because `rowsJson`/`tokensJson`/`tokensPatchJson` are only delivered via this imperative command (not passed as props to the native view), a slow device that takes longer than 4 frames to attach the native tag silently drops the initial payload, leaving the diff surface blank until the JSON changes again and the effect re-runs. Consider removing the 4-attempt cap so `dispatch` keeps retrying each frame until the ref is ready or the effect is cleaned up.
| const sfSymbol = typeof props.name === "string" ? props.name : props.name.ios; | ||
| const AndroidIcon = sfSymbol ? ANDROID_ICON_BY_SF_SYMBOL[sfSymbol] : undefined; | ||
|
|
||
| if (!AndroidIcon) { |
There was a problem hiding this comment.
🟡 Medium components/AppSymbol.tsx:128
When props.name resolves to an SF Symbol not present in ANDROID_ICON_BY_SF_SYMBOL (e.g. "sidebar.left", "arrow.up.left.and.arrow.down.right"), this branch returns props.fallback ?? null, silently dropping the icon on Android. Several screens already use unmapped symbol names, so those controls render with no icon at all on Android. The missing symbols need corresponding entries in ANDROID_ICON_BY_SF_SYMBOL, or a generic placeholder icon should be rendered here instead of null when no fallback is provided.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/components/AppSymbol.tsx around line 128:
When `props.name` resolves to an SF Symbol not present in `ANDROID_ICON_BY_SF_SYMBOL` (e.g. `"sidebar.left"`, `"arrow.up.left.and.arrow.down.right"`), this branch returns `props.fallback ?? null`, silently dropping the icon on Android. Several screens already use unmapped symbol names, so those controls render with no icon at all on Android. The missing symbols need corresponding entries in `ANDROID_ICON_BY_SF_SYMBOL`, or a generic placeholder icon should be rendered here instead of `null` when no fallback is provided.
| if (Platform.OS !== "android") return; | ||
| onExpandedChange?.(isExpanded); | ||
| }, [isExpanded, onExpandedChange]); |
There was a problem hiding this comment.
🟡 Medium threads/ThreadComposer.tsx:221
On Android, isExpanded stays true when the editor has content but is blurred, so the useEffect calls onExpandedChange(true). The parent (ThreadDetailScreen) then sets composerBottomInset to 0, removing the bottom safe-area padding even though the keyboard is dismissed — the blurred composer can overlap the system gesture/navigation area. Consider reporting isFocused to onExpandedChange instead of isExpanded, since the parent uses this signal to decide whether safe-area padding is needed.
if (Platform.OS !== "android") return;
- onExpandedChange?.(isExpanded);
- }, [isExpanded, onExpandedChange]);
+ onExpandedChange?.(isFocused);
+ }, [isFocused, onExpandedChange]);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadComposer.tsx around lines 221-223:
On Android, `isExpanded` stays `true` when the editor has content but is blurred, so the `useEffect` calls `onExpandedChange(true)`. The parent (`ThreadDetailScreen`) then sets `composerBottomInset` to `0`, removing the bottom safe-area padding even though the keyboard is dismissed — the blurred composer can overlap the system gesture/navigation area. Consider reporting `isFocused` to `onExpandedChange` instead of `isExpanded`, since the parent uses this signal to decide whether safe-area padding is needed.
| public func scrollViewWillBeginDragging(_ scrollView: UIScrollView) { | ||
| // A direct gesture takes ownership from any interrupted programmatic jump. | ||
| // Resume visible-file events immediately so the inspector follows the finger. | ||
| isProgrammaticScrollActive = false | ||
| contentView.isVerticalScrollActive = true | ||
| } |
There was a problem hiding this comment.
🟡 Medium ios/T3ReviewDiffView.swift:394
scrollViewWillBeginDragging clears isProgrammaticScrollActive but leaves pendingScrollFileId and pendingScrollAnimated set. If scrollToFile was queued before the target header existed (e.g. rows still decoding), a later setRowsJson calls applyPendingScrollIfNeeded() and jumps to the old target, overriding the user's manual scroll. Clear the pending scroll request when the user takes control.
| public func scrollViewWillBeginDragging(_ scrollView: UIScrollView) { | |
| // A direct gesture takes ownership from any interrupted programmatic jump. | |
| // Resume visible-file events immediately so the inspector follows the finger. | |
| isProgrammaticScrollActive = false | |
| contentView.isVerticalScrollActive = true | |
| } | |
| public func scrollViewWillBeginDragging(_ scrollView: UIScrollView) { | |
| // A direct gesture takes ownership from any interrupted programmatic jump. | |
| // Resume visible-file events immediately so the inspector follows the finger. | |
| isProgrammaticScrollActive = false | |
| pendingScrollFileId = nil | |
| pendingScrollAnimated = false | |
| contentView.isVerticalScrollActive = true | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift around lines 394-399:
`scrollViewWillBeginDragging` clears `isProgrammaticScrollActive` but leaves `pendingScrollFileId` and `pendingScrollAnimated` set. If `scrollToFile` was queued before the target header existed (e.g. rows still decoding), a later `setRowsJson` calls `applyPendingScrollIfNeeded()` and jumps to the old target, overriding the user's manual scroll. Clear the pending scroll request when the user takes control.
There was a problem hiding this comment.
🟡 Medium
t3code/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift
Lines 384 to 385 in 927d43b
scrollToFile stores the target file id in pendingScrollFileId and calls applyPendingScrollIfNeeded(), but that helper returns early when bounds.height == 0. layoutSubviews never retries the pending scroll after layout completes, so a scrollToFile call made before the view's first layout silently does nothing — pendingScrollFileId stays set but is never consumed. Consider calling applyPendingScrollIfNeeded() from layoutSubviews so the deferred scroll fires once bounds are available.
updateContentMetrics()
+ applyPendingScrollIfNeeded()
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift around lines 384-385:
`scrollToFile` stores the target file id in `pendingScrollFileId` and calls `applyPendingScrollIfNeeded()`, but that helper returns early when `bounds.height == 0`. `layoutSubviews` never retries the pending scroll after layout completes, so a `scrollToFile` call made before the view's first layout silently does nothing — `pendingScrollFileId` stays set but is never consumed. Consider calling `applyPendingScrollIfNeeded()` from `layoutSubviews` so the deferred scroll fires once bounds are available.
| } | ||
| } |
There was a problem hiding this comment.
🟠 High plugins/withIosSceneLifecycle.cjs:27
scene(_:willConnectTo:options:) only forwards connectionOptions.urlContexts and ignores connectionOptions.userActivities. On a cold start, iOS delivers universal links to willConnectTo via userActivities (not via scene(_:continue:), which is only called when the app is already running). As a result, launching the app from a universal link drops the initial activity, breaking Linking.getInitialURL() and similar callbacks on first launch. Consider forwarding connectionOptions.userActivities to appDelegate.application(_:continue:restorationHandler:) inside willConnectTo.
+ }
+
+ if let userActivity = connectionOptions.userActivities.first {
+ _ = appDelegate.application(
+ UIApplication.shared,
+ continue: userActivity,
+ restorationHandler: { _ in })
+ }
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/plugins/withIosSceneLifecycle.cjs around lines 27-28:
`scene(_:willConnectTo:options:)` only forwards `connectionOptions.urlContexts` and ignores `connectionOptions.userActivities`. On a cold start, iOS delivers universal links to `willConnectTo` via `userActivities` (not via `scene(_:continue:)`, which is only called when the app is already running). As a result, launching the app from a universal link drops the initial activity, breaking `Linking.getInitialURL()` and similar callbacks on first launch. Consider forwarding `connectionOptions.userActivities` to `appDelegate.application(_:continue:restorationHandler:)` inside `willConnectTo`.
| paddingTop: 8, | ||
| paddingBottom: target ? 0 : Math.max(insets.bottom, 18), | ||
| paddingTop: isAndroid ? insets.top + 8 : 8, | ||
| paddingBottom: target ? (isAndroid ? 72 : 0) : Math.max(insets.bottom, 18), |
There was a problem hiding this comment.
🟡 Medium review/ReviewCommentComposerSheet.tsx:174
The hard-coded 72 px paddingBottom on Android assumes the KeyboardStickyView footer is always 72 px tall, but the footer height is actually 44 (button) + 8 (pt-2) + Math.max(insets.bottom, 10). On devices where insets.bottom exceeds ~20 (e.g. iPhones with a home indicator, where it can be ~34), the footer is taller than 72 px and overlaps the bottom of the comment input and attachment strip, hiding them from view and making them untappable. The padding should be computed from the actual footer height instead of hard-coded.
- paddingBottom: target ? (isAndroid ? 72 : 0) : Math.max(insets.bottom, 18),
+ paddingBottom: target ? (isAndroid ? Math.max(insets.bottom, 10) + 52 : 0) : Math.max(insets.bottom, 18),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/review/ReviewCommentComposerSheet.tsx around line 174:
The hard-coded `72` px `paddingBottom` on Android assumes the `KeyboardStickyView` footer is always 72 px tall, but the footer height is actually `44` (button) + `8` (`pt-2`) + `Math.max(insets.bottom, 10)`. On devices where `insets.bottom` exceeds ~20 (e.g. iPhones with a home indicator, where it can be ~34), the footer is taller than 72 px and overlaps the bottom of the comment input and attachment strip, hiding them from view and making them untappable. The padding should be computed from the actual footer height instead of hard-coded.
juliusmarminge
force-pushed
the
android-dev-pr-3514
branch
from
927d43b to
212ac28
Compare
juliusmarminge
changed the base branch from
main
to
t3code/ipad-responsive-mobile-layout
| </View> | ||
| ) : null} | ||
| </View> | ||
| <View ref={composerOverlayRef} onLayout={onComposerLayout} style={{ paddingTop: 8 }}> |
There was a problem hiding this comment.
🟡 Medium threads/ThreadDetailScreen.tsx:404
In split layout on tablets or wide windows, the chat pane no longer constrains content width — the removal of contentMaxWidth and the CHAT_CONTENT_MAX_WIDTH logic means ThreadFeed now uses horizontalPadding directly and the floating composer wrapper with maxWidth: contentMaxWidth was deleted. As a result, message bubbles, review comment cards, and the composer stretch across the full pane width instead of staying centered and readable. If removing the max-width constraint was intentional, consider documenting the rationale; otherwise, re-introduce the contentMaxWidth prop and the centered wrapper for split layout.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadDetailScreen.tsx around line 404:
In split layout on tablets or wide windows, the chat pane no longer constrains content width — the removal of `contentMaxWidth` and the `CHAT_CONTENT_MAX_WIDTH` logic means `ThreadFeed` now uses `horizontalPadding` directly and the floating composer wrapper with `maxWidth: contentMaxWidth` was deleted. As a result, message bubbles, review comment cards, and the composer stretch across the full pane width instead of staying centered and readable. If removing the max-width constraint was intentional, consider documenting the rationale; otherwise, re-introduce the `contentMaxWidth` prop and the centered wrapper for split layout.
| backgroundColor: drawerBg, | ||
| paddingTop: insets.top + 10, | ||
| paddingBottom: Math.max(insets.bottom, 18), | ||
| paddingTop: insets.top, |
There was a problem hiding this comment.
🟡 Medium threads/ThreadNavigationDrawer.tsx:134
The PR removed paddingBottom: Math.max(insets.bottom, 18) from the drawer container, leaving the ScrollView's contentInset as the only source of bottom spacing. contentInset is iOS-only on React Native's ScrollView, so on Android the last thread rows render with no bottom padding and extend into the bottom safe area, making them partially obscured and harder to tap. Consider restoring paddingBottom on the container so the inset applies cross-platform.
| paddingTop: insets.top, | |
| paddingTop: insets.top, | |
| paddingBottom: Math.max(insets.bottom, 18), |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadNavigationDrawer.tsx around line 134:
The PR removed `paddingBottom: Math.max(insets.bottom, 18)` from the drawer container, leaving the `ScrollView`'s `contentInset` as the only source of bottom spacing. `contentInset` is iOS-only on React Native's `ScrollView`, so on Android the last thread rows render with no bottom padding and extend into the bottom safe area, making them partially obscured and harder to tap. Consider restoring `paddingBottom` on the container so the inset applies cross-platform.
| private fun JSONObject.floatDp(key: String, fallbackPx: Float, density: Float): Float = | ||
| if (has(key)) optDouble(key).toFloat() * density else fallbackPx | ||
|
|
||
| private fun JSONObject.floatSp(key: String, fallbackPx: Float, density: Float): Float = |
There was a problem hiding this comment.
🟡 Medium t3reviewdiff/T3ReviewDiffView.kt:1022
floatSp multiplies the JSON value by density (raw screen density), but sp units must use scaledDensity (which incorporates the user's font-scale accessibility setting). As a result, custom codeFontSize and lineNumberFontSize values from styleJson render at the wrong size whenever the user's font scale is not 1.0, ignoring Android's text-scaling accessibility feature. Consider passing Resources.displayMetrics.scaledDensity to floatSp instead of density.
Also found in 1 other location(s)
apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorModule.kt:29
contentInsetVerticalis received as aDoublefrom JS but this prop truncates it withtoInt()before passing it tosetContentInsetVertical. Fractional dp values are therefore rounded down on Android (0.5becomes0), unlike iOS where the prop is kept as a floating-point inset. Any non-integer inset from shared code will render with the wrong padding on Android.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt around line 1022:
`floatSp` multiplies the JSON value by `density` (raw screen density), but `sp` units must use `scaledDensity` (which incorporates the user's font-scale accessibility setting). As a result, custom `codeFontSize` and `lineNumberFontSize` values from `styleJson` render at the wrong size whenever the user's font scale is not 1.0, ignoring Android's text-scaling accessibility feature. Consider passing `Resources.displayMetrics.scaledDensity` to `floatSp` instead of `density`.
Also found in 1 other location(s):
- apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorModule.kt:29 -- `contentInsetVertical` is received as a `Double` from JS but this prop truncates it with `toInt()` before passing it to `setContentInsetVertical`. Fractional dp values are therefore rounded down on Android (`0.5` becomes `0`), unlike iOS where the prop is kept as a floating-point inset. Any non-integer inset from shared code will render with the wrong padding on Android.
| distanceX: Float, | ||
| distanceY: Float, | ||
| ): Boolean { | ||
| scrollRemainder += distanceY / cellHeightPx |
There was a problem hiding this comment.
🟡 Medium t3terminal/TerminalCanvasView.kt:237
onScroll passes Android's distanceY directly to onScrollRows without negating it. On Android, distanceY is lastY - currentY, so dragging the finger downward produces a negative value. That negative delta is forwarded to ghostty_terminal_scroll_view, which treats negative deltas as scrolling up. The result is inverted scroll direction: dragging down scrolls the terminal viewport up. Negating distanceY before converting to row deltas fixes this.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-terminal/android/src/main/java/expo/modules/t3terminal/TerminalCanvasView.kt around line 237:
`onScroll` passes Android's `distanceY` directly to `onScrollRows` without negating it. On Android, `distanceY` is `lastY - currentY`, so dragging the finger downward produces a negative value. That negative delta is forwarded to `ghostty_terminal_scroll_view`, which treats negative deltas as scrolling up. The result is inverted scroll direction: dragging down scrolls the terminal viewport up. Negating `distanceY` before converting to row deltas fixes this.
| accessibilityLabel: `Archive ${props.thread.title}`, | ||
| icon: "archivebox", | ||
| label: "Archive", | ||
| onPress: props.onArchive, |
There was a problem hiding this comment.
🟡 Medium home/HomeScreen.tsx:299
The Archive swipe action passes props.onArchive directly as onPress, so tapping it does not close the swipeable row first. The Delete action calls swipeableMethods.close() before deleting, but Archive skips that step — the row stays open with action buttons visible, and openSwipeableRef in HomeScreen is never cleared via onSwipeableClose. Wrap the archive handler to close the swipeable first, matching the Delete behavior.
| onPress: props.onArchive, | |
| onPress: () => { | |
| swipeableRef.current?.close(); | |
| props.onArchive(); | |
| }, |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/home/HomeScreen.tsx around line 299:
The Archive swipe action passes `props.onArchive` directly as `onPress`, so tapping it does not close the swipeable row first. The Delete action calls `swipeableMethods.close()` before deleting, but Archive skips that step — the row stays open with action buttons visible, and `openSwipeableRef` in `HomeScreen` is never cleared via `onSwipeableClose`. Wrap the archive handler to close the swipeable first, matching the Delete behavior.
| canvas.drawRect(left, top, right, bottom, paint) | ||
| val index = currentFrame.cursorY * currentFrame.cols + currentFrame.cursorX | ||
| val text = currentFrame.cellText[index] | ||
| if (text.isNotEmpty()) { |
There was a problem hiding this comment.
🟠 High t3terminal/TerminalCanvasView.kt:203
In drawCursor, the block-cursor branch redraws the cell text with only text.isNotEmpty() as the guard, unlike onDraw which also checks flags and FLAG_INVISIBLE == 0. As a result, any character marked invisible by the terminal is drawn in currentFrame.background whenever the block cursor sits on that cell, making hidden text visible. Consider adding the same FLAG_INVISIBLE check before redrawing the text.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-terminal/android/src/main/java/expo/modules/t3terminal/TerminalCanvasView.kt around line 203:
In `drawCursor`, the block-cursor branch redraws the cell text with only `text.isNotEmpty()` as the guard, unlike `onDraw` which also checks `flags and FLAG_INVISIBLE == 0`. As a result, any character marked invisible by the terminal is drawn in `currentFrame.background` whenever the block cursor sits on that cell, making hidden text visible. Consider adding the same `FLAG_INVISIBLE` check before redrawing the text.
| } | ||
| } | ||
| } | ||
| if (palette.isNotEmpty()) { |
There was a problem hiding this comment.
🟡 Medium t3terminal/T3TerminalView.kt:335
parseThemeConfig fills unspecified palette slots with foregroundColorValue. When the config only defines a high-index entry (e.g. palette=7=...), the resulting IntArray covers 0..lastIndex and every lower slot is set to the foreground color. The JNI treats every element in that array as an explicit palette override, so those unspecified indices replace the terminal's default colors with the foreground color instead of being left unchanged. Only explicitly defined palette indices should override defaults; consider skipping the array entirely when the palette is sparse or passing a structure that distinguishes set from unset entries.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-terminal/android/src/main/java/expo/modules/t3terminal/T3TerminalView.kt around line 335:
`parseThemeConfig` fills unspecified palette slots with `foregroundColorValue`. When the config only defines a high-index entry (e.g. `palette=7=...`), the resulting `IntArray` covers `0..lastIndex` and every lower slot is set to the foreground color. The JNI treats every element in that array as an explicit palette override, so those unspecified indices replace the terminal's default colors with the foreground color instead of being left unchanged. Only explicitly defined palette indices should override defaults; consider skipping the array entirely when the palette is sparse or passing a structure that distinguishes set from unset entries.
| fun setContentResetKey(value: String) { | ||
| if (contentResetKey == value) return | ||
| contentResetKey = value | ||
| tokensDecodeGeneration += 1 |
There was a problem hiding this comment.
🟡 Medium t3reviewdiff/T3ReviewDiffView.kt:84
setContentResetKey() increments tokensDecodeGeneration but not rowsDecodeGeneration, so any in-flight setRowsJson() decode that started before the reset still passes the generation guard on line 145 and overwrites rows/visibleRows with the previous section's data. Consider incrementing rowsDecodeGeneration here as well so stale decode results are discarded.
tokensDecodeGeneration += 1
+ rowsDecodeGeneration += 1🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt around line 84:
`setContentResetKey()` increments `tokensDecodeGeneration` but not `rowsDecodeGeneration`, so any in-flight `setRowsJson()` decode that started before the reset still passes the generation guard on line 145 and overwrites `rows`/`visibleRows` with the previous section's data. Consider incrementing `rowsDecodeGeneration` here as well so stale decode results are discarded.
| view.setSpellCheck(spellCheck) | ||
| } | ||
|
|
||
| Events( |
There was a problem hiding this comment.
🟡 Medium t3composereditor/T3ComposerEditorModule.kt:48
The Events(...) declaration omits onComposerSubmit, and T3ComposerEditorView defines no corresponding EventDispatcher or hardware-keyboard handler. As a result, the onSubmit prop exposed by ComposerEditorProps is never invoked on Android — hardware-keyboard submit that works on iOS silently does nothing here. Consider adding "onComposerSubmit" to the Events list, wiring up an onComposerSubmit dispatcher in the view, and detecting the submit key combination (e.g., Enter without Shift) to fire it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorModule.kt around line 48:
The `Events(...)` declaration omits `onComposerSubmit`, and `T3ComposerEditorView` defines no corresponding `EventDispatcher` or hardware-keyboard handler. As a result, the `onSubmit` prop exposed by `ComposerEditorProps` is never invoked on Android — hardware-keyboard submit that works on iOS silently does nothing here. Consider adding `"onComposerSubmit"` to the `Events` list, wiring up an `onComposerSubmit` dispatcher in the view, and detecting the submit key combination (e.g., Enter without Shift) to fire it.
| backgroundColorValue, | ||
| cursorColorValue, | ||
| paletteColors, | ||
| ) |
There was a problem hiding this comment.
🟡 Medium t3terminal/T3TerminalView.kt:252
createTerminal() assigns the result of GhosttyBridge.nativeCreate() directly to terminalHandle without checking for a 0 return value, which indicates native session creation failed. When this happens, terminalHandle stays 0L and emitResize() proceeds to fire onResize and call feedPendingBuffer()/renderSnapshot() as if the terminal were live, so the JS layer never learns creation failed and the view stays blank. Consider checking the return value and emitting an error event when it is 0.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-terminal/android/src/main/java/expo/modules/t3terminal/T3TerminalView.kt around line 252:
`createTerminal()` assigns the result of `GhosttyBridge.nativeCreate()` directly to `terminalHandle` without checking for a `0` return value, which indicates native session creation failed. When this happens, `terminalHandle` stays `0L` and `emitResize()` proceeds to fire `onResize` and call `feedPendingBuffer()`/`renderSnapshot()` as if the terminal were live, so the JS layer never learns creation failed and the view stays blank. Consider checking the return value and emitting an error event when it is `0`.
| ``` | ||
|
|
||
| The script downloads Zig 0.15.2 when needed, checks out the pinned upstream Ghostty revision, and | ||
| rebuilds all four Android ABIs with 16 KB page-size support. |
There was a problem hiding this comment.
🟡 Medium t3-terminal/README.md:50
The new README section states the script rebuilds "all four Android ABIs with 16 KB page-size support," but build-libghostty-android.sh never passes -Wl,-z,max-page-size=16384 and -Wl,-z,common-page-size=16384 to the linker. A developer following these instructions will rebuild and vendor .so files that are not 16 KB-page compatible while the docs claim otherwise. Add the required linker flags to the script so the rebuilt libraries match the documented behavior.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-terminal/README.md around line 50:
The new README section states the script rebuilds "all four Android ABIs with 16 KB page-size support," but `build-libghostty-android.sh` never passes `-Wl,-z,max-page-size=16384` and `-Wl,-z,common-page-size=16384` to the linker. A developer following these instructions will rebuild and vendor `.so` files that are not 16 KB-page compatible while the docs claim otherwise. Add the required linker flags to the script so the rebuilt libraries match the documented behavior.
| canvasView.setHorizontalOffset(0) | ||
| applyPendingInitialScroll() | ||
| } |
There was a problem hiding this comment.
🟡 Medium t3reviewdiff/T3ReviewDiffView.kt:89
setContentResetKey() calls applyPendingInitialScroll() while the old visibleRows are still present. This consumes the pendingInitialScroll flag and posts a scroll to initialRowIndex against the stale rows. When new rows later arrive, rebuildVisibleRows() calls applyPendingInitialScroll() again but the flag is already false, so the scroll is never reapplied to the new content — the view stays at whatever position the stale scroll produced. Removing the direct call lets the pending flag survive until rebuildVisibleRows() applies it against the fresh rows.
canvasView.setHorizontalOffset(0)
- applyPendingInitialScroll()
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt around lines 89-91:
`setContentResetKey()` calls `applyPendingInitialScroll()` while the old `visibleRows` are still present. This consumes the `pendingInitialScroll` flag and posts a scroll to `initialRowIndex` against the stale rows. When new rows later arrive, `rebuildVisibleRows()` calls `applyPendingInitialScroll()` again but the flag is already `false`, so the scroll is never reapplied to the new content — the view stays at whatever position the stale scroll produced. Removing the direct call lets the pending flag survive until `rebuildVisibleRows()` applies it against the fresh rows.
| var x = codeX | ||
| tokens.forEach { token -> | ||
| textPaint.textSize = style.codeFontSizePx | ||
| textPaint.color = token.color ?: theme.text |
There was a problem hiding this comment.
🟡 Medium t3reviewdiff/T3ReviewDiffView.kt:837
In drawLineRow, when a line has syntax tokens, each token's color falls back to theme.text when token.color is null. Added/deleted lines without tokens correctly use theme.addText/theme.deleteText, but the token branch ignores the change type — so added/deleted lines containing null-color tokens are rendered in the neutral context color instead of the diff green/red. The fix is to use the same change-type-based fallback in the token branch.
var x = codeX
+ val tokenFallback = when (row.change) {
+ "add" -> theme.addText
+ "delete" -> theme.deleteText
+ else -> theme.text
+ }
tokens.forEach { token ->
textPaint.textSize = style.codeFontSizePx
- textPaint.color = token.color ?: theme.text
+ textPaint.color = token.color ?: tokenFallback🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt around lines 837-840:
In `drawLineRow`, when a line has syntax tokens, each token's color falls back to `theme.text` when `token.color` is `null`. Added/deleted lines without tokens correctly use `theme.addText`/`theme.deleteText`, but the token branch ignores the change type — so added/deleted lines containing `null`-color tokens are rendered in the neutral context color instead of the diff green/red. The fix is to use the same change-type-based fallback in the token branch.
| textPaint.typeface = when { | ||
| token.fontStyle and 1 != 0 -> Typeface.create(Typeface.MONOSPACE, Typeface.ITALIC) | ||
| token.fontStyle and 2 != 0 -> Typeface.create(Typeface.MONOSPACE, Typeface.BOLD) |
There was a problem hiding this comment.
🟡 Medium t3reviewdiff/T3ReviewDiffView.kt:841
In drawLineRow, the when block checks the italic bit (1) before the bold bit (2). When token.fontStyle is 3 (bold+italic), the first branch matches and the typeface is created as Typeface.ITALIC only, so the bold weight is silently dropped. Android supports Typeface.BOLD_ITALIC for the combined case — consider checking for both bits first.
textPaint.typeface = when {
+ (token.fontStyle and 3) == 3 -> Typeface.create(Typeface.MONOSPACE, Typeface.BOLD_ITALIC)
token.fontStyle and 1 != 0 -> Typeface.create(Typeface.MONOSPACE, Typeface.ITALIC)
token.fontStyle and 2 != 0 -> Typeface.create(Typeface.MONOSPACE, Typeface.BOLD)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt around lines 841-843:
In `drawLineRow`, the `when` block checks the italic bit (1) before the bold bit (2). When `token.fontStyle` is `3` (bold+italic), the first branch matches and the typeface is created as `Typeface.ITALIC` only, so the bold weight is silently dropped. Android supports `Typeface.BOLD_ITALIC` for the combined case — consider checking for both bits first.
| Prop("editable") { view: T3ComposerEditorView, editable: Boolean -> | ||
| view.setEditable(editable) | ||
| } | ||
| Prop("scrollEnabled") { view: T3ComposerEditorView, scrollEnabled: Boolean -> |
There was a problem hiding this comment.
🟡 Medium t3composereditor/T3ComposerEditorModule.kt:35
The scrollEnabled prop does not actually disable scrolling. setScrollEnabled only toggles editor.isVerticalScrollBarEnabled, which controls whether the scrollbar is drawn — not whether the view scrolls. When scrollEnabled={false} is passed from JS, the editor remains scrollable and only the scrollbar disappears, so the prop silently does not work. Consider disabling touch interception or overriding onTouchEvent to actually prevent scrolling when scrollEnabled is false.
Also found in 1 other location(s)
apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt:199
setScrollEnabled()at line199only assignseditor.isVerticalScrollBarEnabled. Android'ssetVerticalScrollBarEnabled()controls whether the scrollbar is drawn, not whether anEditTextcan actually scroll. When callers passscrollEnabled=false, long composer contents can still be vertically scrolled; only the scrollbar disappears, so the exposed prop does not work on Android.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorModule.kt around line 35:
The `scrollEnabled` prop does not actually disable scrolling. `setScrollEnabled` only toggles `editor.isVerticalScrollBarEnabled`, which controls whether the scrollbar is drawn — not whether the view scrolls. When `scrollEnabled={false}` is passed from JS, the editor remains scrollable and only the scrollbar disappears, so the prop silently does not work. Consider disabling touch interception or overriding `onTouchEvent` to actually prevent scrolling when `scrollEnabled` is false.
Also found in 1 other location(s):
- apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt:199 -- `setScrollEnabled()` at line `199` only assigns `editor.isVerticalScrollBarEnabled`. Android's `setVerticalScrollBarEnabled()` controls whether the scrollbar is drawn, not whether an `EditText` can actually scroll. When callers pass `scrollEnabled=false`, long composer contents can still be vertically scrolled; only the scrollbar disappears, so the exposed prop does not work on Android.
| </> | ||
| )} | ||
|
|
||
| <GitActionProgressOverlay progress={gitActionProgress} onDismiss={dismissGitActionResult} /> |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:725
GitActionProgressOverlay positions itself at insets.top + 48, which was calibrated for the native iOS header. On Android, the new AndroidScreenHeader is Math.max(insets.top, 12) + 58 tall, so the overlay renders ~10px inside the header and overlaps the title and action buttons while a git action is running or after it completes. Consider passing a platform-aware top offset to GitActionProgressOverlay so it clears the Android header.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 725:
`GitActionProgressOverlay` positions itself at `insets.top + 48`, which was calibrated for the native iOS header. On Android, the new `AndroidScreenHeader` is `Math.max(insets.top, 12) + 58` tall, so the overlay renders ~10px inside the header and overlaps the title and action buttons while a git action is running or after it completes. Consider passing a platform-aware top offset to `GitActionProgressOverlay` so it clears the Android header.
| editor.hint = placeholder | ||
| } | ||
|
|
||
| fun setFontFamily(fontFamily: String) { |
There was a problem hiding this comment.
🟡 Medium t3composereditor/T3ComposerEditorView.kt:166
setFontFamily only checks whether fontFamily contains "Mono" and otherwise assigns Typeface.DEFAULT, so every non-monospace font name (e.g. "DMSans_400Regular") is silently ignored and the editor always renders the system default. Consider resolving the requested font name to an actual Typeface (e.g. via the expo font registry or ResourcesCompat) so the configured app font is applied.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt around line 166:
`setFontFamily` only checks whether `fontFamily` contains `"Mono"` and otherwise assigns `Typeface.DEFAULT`, so every non-monospace font name (e.g. `"DMSans_400Regular"`) is silently ignored and the editor always renders the system default. Consider resolving the requested font name to an actual `Typeface` (e.g. via the expo font registry or `ResourcesCompat`) so the configured app font is applied.
There was a problem hiding this comment.
🟡 Medium review/ReviewSheet.tsx:490
handleVisibleFileChange unconditionally calls setVisibleFile with the native event's fileId, overwriting whatever the user just selected. On Android, the native diff view never emits null for the top-of-diff position, so after the user taps the "All files" row and the diff scrolls to the top, the next visibility event reports the first file's id and the navigator selection jumps back to that file. This makes "All files" impossible to keep selected on Android. Consider tracking the last user-initiated selection and skipping the visibility update when the user chose "All files" (fileId === null) and the incoming event carries a non-null fileId.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/review/ReviewSheet.tsx around line 490:
`handleVisibleFileChange` unconditionally calls `setVisibleFile` with the native event's `fileId`, overwriting whatever the user just selected. On Android, the native diff view never emits `null` for the top-of-diff position, so after the user taps the "All files" row and the diff scrolls to the top, the next visibility event reports the first file's id and the navigator selection jumps back to that file. This makes "All files" impossible to keep selected on Android. Consider tracking the last user-initiated selection and skipping the visibility update when the user chose "All files" (`fileId === null`) and the incoming event carries a non-null `fileId`.
There was a problem hiding this comment.
🟡 Medium threads/ThreadNavigationSidebar.tsx:243
The filter menu subactions set MenuAction.state to "on"/"off" to indicate the active environment, sort order, and grouping, but @react-native-menu/menu only supports state on iOS 14+. On Android, none of these selections show a checked/active indicator, so users cannot tell which option is currently selected. Consider baking the checkmark into the title (e.g. prefixing with ✓ ) so the active state is visible cross-platform.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadNavigationSidebar.tsx around line 243:
The filter menu subactions set `MenuAction.state` to `"on"`/`"off"` to indicate the active environment, sort order, and grouping, but `@react-native-menu/menu` only supports `state` on iOS 14+. On Android, none of these selections show a checked/active indicator, so users cannot tell which option is currently selected. Consider baking the checkmark into the `title` (e.g. prefixing with `✓ `) so the active state is visible cross-platform.
| {Platform.OS === "android" ? ( | ||
| <AndroidScreenHeader | ||
| title={showScanner ? "Scan QR Code" : "Add Environment"} | ||
| onBack={() => router.back()} |
There was a problem hiding this comment.
🟡 Medium connections/new.tsx:137
The Android header's onBack calls router.back() directly instead of the already-imported dismissRoute(router). When there is no back history (e.g. the route was loaded directly), router.back() is a no-op and the user is stuck on the screen, whereas dismissRoute falls back to router.replace("/"). Consider using dismissRoute(router) here to match the behavior the native stack header provided.
| onBack={() => router.back()} | |
| onBack={() => dismissRoute(router)} |
Also found in 1 other location(s)
apps/mobile/src/app/settings/index.tsx:50
The new
Close settingsbutton at line50always callsrouter.back(). When/settingsis the first route in the stack (for example after a deep link, state restoration, or opening the screen without a prior in-app page),back()is a no-op, so the visible close control does nothing and leaves the user stuck on the settings screen. Other routes in this codebase use a fallbackreplace(...)/dismissRoute(...)for this case.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/app/connections/new.tsx around line 137:
The Android header's `onBack` calls `router.back()` directly instead of the already-imported `dismissRoute(router)`. When there is no back history (e.g. the route was loaded directly), `router.back()` is a no-op and the user is stuck on the screen, whereas `dismissRoute` falls back to `router.replace("/")`. Consider using `dismissRoute(router)` here to match the behavior the native stack header provided.
Also found in 1 other location(s):
- apps/mobile/src/app/settings/index.tsx:50 -- The new `Close settings` button at line `50` always calls `router.back()`. When `/settings` is the first route in the stack (for example after a deep link, state restoration, or opening the screen without a prior in-app page), `back()` is a no-op, so the visible close control does nothing and leaves the user stuck on the settings screen. Other routes in this codebase use a fallback `replace(...)`/`dismissRoute(...)` for this case.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 6 potential issues.
Bugbot Autofix prepared fixes for all 6 issues found in the latest run.
- ✅ Fixed: iOS associated domains removed
- Restored the associatedDomains array with applinks and webcredentials entries for clerk.t3.codes, which was inadvertently dropped when relyingParty was removed from the variant config.
- ✅ Fixed: Composer scroll flag ignored
- Added a scrollEnabled property to SelectionAwareEditText with a scrollTo override that blocks scrolling when disabled, so setScrollEnabled now controls actual scroll behavior, not just the scroll bar.
- ✅ Fixed: Collapsed files skew highlight indices
- Added a visibleToOriginalIndex mapping array built during rebuildVisibleRows, and the onVisibleRowsChanged callback now translates filtered indices back to original row indices before emitting the visible-range event.
- ✅ Fixed: Stale rows after content reset
- setContentResetKey now increments rowsDecodeGeneration and clears rows, visibleRows, visibleToOriginalIndex, and canvasView.rows so in-flight decodes from the previous section are rejected and stale content is immediately cleared.
- ✅ Fixed: Top scroll never clears file
- emitVisibleFile now checks if verticalOffset is at zero and emits onVisibleFileChange with a null fileId in that case, matching the iOS behavior that selects the "All files" navigator destination.
- ✅ Fixed: Collapsed comments stay tall
- Added collapsedCommentIds to DiffCanvasView and updated rowHeight to return 44dp for collapsed comments (matching iOS's 44pt) instead of the full expanded height.
Or push these changes by commenting:
@cursor push a991bbde89
Preview (a991bbde89)
diff --git a/apps/mobile/app.config.ts b/apps/mobile/app.config.ts
--- a/apps/mobile/app.config.ts
+++ b/apps/mobile/app.config.ts
@@ -107,6 +107,7 @@
icon: variant.iosIcon,
supportsTablet: true,
bundleIdentifier: iosBundleIdentifier,
+ associatedDomains: ["applinks:clerk.t3.codes", "webcredentials:clerk.t3.codes"],
infoPlist: {
NSAppTransportSecurity: {
NSAllowsArbitraryLoads: true,
diff --git a/apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt b/apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt
--- a/apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt
+++ b/apps/mobile/modules/t3-composer-editor/android/src/main/java/expo/modules/t3composereditor/T3ComposerEditorView.kt
@@ -197,6 +197,7 @@
fun setScrollEnabled(scrollEnabled: Boolean) {
editor.isVerticalScrollBarEnabled = scrollEnabled
+ editor.scrollEnabled = scrollEnabled
}
fun setAutoFocus(autoFocus: Boolean) {
@@ -424,7 +425,12 @@
private class SelectionAwareEditText(context: Context) : EditText(context) {
var selectionListener: ((Int, Int) -> Unit)? = null
var pasteImagesListener: ((List<String>) -> Unit)? = null
+ var scrollEnabled = true
+ override fun scrollTo(x: Int, y: Int) {
+ if (scrollEnabled) super.scrollTo(x, y)
+ }
+
override fun onSelectionChanged(selStart: Int, selEnd: Int) {
super.onSelectionChanged(selStart, selEnd)
selectionListener?.invoke(selStart, selEnd)
diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt
--- a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt
+++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt
@@ -32,6 +32,7 @@
private val onToggleComment by EventDispatcher()
private var rows: List<DiffRow> = emptyList()
private var visibleRows: List<DiffRow> = emptyList()
+ private var visibleToOriginalIndex: IntArray = intArrayOf()
private var collapsedFileIds: Set<String> = emptySet()
private var viewedFileIds: Set<String> = emptySet()
private var selectedRowIds: Set<String> = emptySet()
@@ -56,11 +57,13 @@
init {
canvasView.onRowTap = { row, gesture -> handleRowTap(row, gesture) }
canvasView.onVisibleRowsChanged = { first, last ->
+ val originalFirst = if (first in visibleToOriginalIndex.indices) visibleToOriginalIndex[first] else first
+ val originalLast = if (last in visibleToOriginalIndex.indices) visibleToOriginalIndex[last] else last
onDebug(
mapOf(
"message" to "visible-range",
- "firstRowIndex" to first,
- "lastRowIndex" to last,
+ "firstRowIndex" to originalFirst,
+ "lastRowIndex" to originalLast,
),
)
emitVisibleFile(first)
@@ -81,7 +84,12 @@
fun setContentResetKey(value: String) {
if (contentResetKey == value) return
contentResetKey = value
+ rowsDecodeGeneration += 1
tokensDecodeGeneration += 1
+ rows = emptyList()
+ visibleRows = emptyList()
+ visibleToOriginalIndex = intArrayOf()
+ canvasView.rows = emptyList()
canvasView.tokensByRowId = emptyMap()
lastVisibleFileId = null
pendingInitialScroll = true
@@ -314,21 +322,27 @@
private fun rebuildVisibleRows() {
val filtered = ArrayList<DiffRow>(rows.size)
+ val indexMapping = ArrayList<Int>(rows.size)
var currentFileCollapsed = false
- rows.forEach { row ->
+ rows.forEachIndexed { originalIndex, row ->
if (row.kind == "file") {
currentFileCollapsed = collapsedFileIds.contains(row.resolvedFileId)
filtered.add(row)
+ indexMapping.add(originalIndex)
} else if (!currentFileCollapsed) {
if (row.kind != "comment" || !collapsedCommentIds.contains(row.id)) {
filtered.add(row)
+ indexMapping.add(originalIndex)
} else {
filtered.add(row.copy(commentText = "Comment collapsed"))
+ indexMapping.add(originalIndex)
}
}
}
visibleRows = filtered
+ visibleToOriginalIndex = indexMapping.toIntArray()
canvasView.rows = filtered
+ canvasView.collapsedCommentIds = collapsedCommentIds
canvasView.viewedFileIds = viewedFileIds
canvasView.selectedRowIds = selectedRowIds
applyPendingInitialScroll()
@@ -360,6 +374,13 @@
private fun emitVisibleFile(firstVisibleIndex: Int) {
if (visibleRows.isEmpty()) return
+ if (canvasView.verticalOffset() <= 0) {
+ if (lastVisibleFileId != null) {
+ lastVisibleFileId = null
+ onVisibleFileChange(mapOf("fileId" to null))
+ }
+ return
+ }
val start = firstVisibleIndex.coerceIn(0, visibleRows.lastIndex)
val fileId = (start downTo 0)
.asSequence()
@@ -590,6 +611,11 @@
field = value
invalidate()
}
+ var collapsedCommentIds: Set<String> = emptySet()
+ set(value) {
+ field = value
+ rebuildOffsets()
+ }
var theme: DiffTheme = DiffTheme.fallback("light")
set(value) {
field = value
@@ -691,7 +717,11 @@
private fun rowHeight(row: DiffRow): Int = when (row.kind) {
"file" -> style.fileHeaderHeightPx.toInt()
- "comment" -> max((style.rowHeightPx * 3.2f).toInt(), (56 * density).toInt())
+ "comment" -> if (collapsedCommentIds.contains(row.id)) {
+ (44 * density).toInt()
+ } else {
+ max((style.rowHeightPx * 3.2f).toInt(), (56 * density).toInt())
+ }
else -> style.rowHeightPx.toInt()
}.coerceAtLeast(1)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
| "expo-asset", | ||
| "expo-font", | ||
| "expo-secure-store", | ||
| ["@clerk/expo", { theme: "./clerk-theme.json" }], |
There was a problem hiding this comment.
iOS associated domains removed
High Severity
Removing relyingParty from the variant config inadvertently dropped the associatedDomains block from the iOS configuration. This prevents standard iOS builds from declaring Universal Links and webcredentials for the Clerk relying party.
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
|
|
||
| fun setScrollEnabled(scrollEnabled: Boolean) { | ||
| editor.isVerticalScrollBarEnabled = scrollEnabled | ||
| } |
There was a problem hiding this comment.
Composer scroll flag ignored
Medium Severity
On Android, setScrollEnabled only toggles the vertical scroll bar visibility, not the actual scrolling behavior of the EditText. This allows the editor to scroll even when scrollEnabled={false} is set, unlike iOS.
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
| } | ||
| } | ||
| visibleRows = filtered | ||
| canvasView.rows = filtered |
There was a problem hiding this comment.
Collapsed files skew highlight indices
High Severity
On Android, collapsed files drop non-header rows from visibleRows, but onDebug visible-range events use indices into that filtered list. JavaScript highlighting treats those numbers as indexes into the full data.rows array, so syntax tokens attach to the wrong lines whenever any file is collapsed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
| canvasView.setVerticalOffset(0) | ||
| canvasView.setHorizontalOffset(0) | ||
| applyPendingInitialScroll() | ||
| } |
There was a problem hiding this comment.
Stale rows after content reset
Medium Severity
When contentResetKey changes, the view clears tokens and scroll but does not bump rowsDecodeGeneration or clear rows. An in-flight setRowsJson decode from the previous review section can still post after the reset and repopulate the canvas with the old diff.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
| if (fileId == lastVisibleFileId) return | ||
| lastVisibleFileId = fileId | ||
| onVisibleFileChange(mapOf("fileId" to fileId)) | ||
| } |
There was a problem hiding this comment.
Top scroll never clears file
Medium Severity
Android never emits onVisibleFileChange with a null fileId when the diff is scrolled to the top. The navigator keeps highlighting the first file instead of the shared “all files” destination that iOS reports at offset zero.
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
| "file" -> style.fileHeaderHeightPx.toInt() | ||
| "comment" -> max((style.rowHeightPx * 3.2f).toInt(), (56 * density).toInt()) | ||
| else -> style.rowHeightPx.toInt() | ||
| }.coerceAtLeast(1) |
There was a problem hiding this comment.
Collapsed comments stay tall
Medium Severity
On Android, collapsing a review comment only swaps the label to “Comment collapsed” while rowHeight still allocates the full multi-line comment height, unlike iOS where collapsed comments use a short row. Scroll offsets, visible-row indexing, and tap targets diverge from iOS and from the collapsed state users expect.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 276b453. Configure here.
ApprovabilityVerdict: Needs human review 36 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Summary
Why
The mobile app inherited iOS-only native modules and navigation assumptions. Android could build only after filling those native module gaps, and several shared screens rendered with incorrect insets, missing icons, inaccessible controls, or desktop/iOS-oriented interaction patterns.
The review diff also scrolled its entire native canvas horizontally, which moved line gutters and file headers with the code. It now owns horizontal code offset internally so persistent chrome stays fixed.
Impact
Android now has a usable end-to-end thread, file, Git review, composer, and connection flow. The iOS implementation keeps using its existing native toolbar and form-sheet behavior through platform-specific branches.
Validation
vp checkvp run typecheckvp run lint:mobile./gradlew :t3tools-mobile-review-diff-native:compileDebugKotlin./gradlew :app:assembleDebugNote
Add Android support to the mobile app
AndroidScreenHeader,AndroidHeaderIconButton) across thread, settings, connections, files, home, and review screens, replacing the default Expo Router stack header.SymbolViewin AppSymbol.tsx that maps SF Symbols to Tabler icons on Android with graceful fallback.TerminalCanvasViewcanvas, feeds I/O through JNI to the vendoredlibghostty-vtshared library, and emits resize/response events.MarkdownCodeBlockanduseMarkdownCodeHighlight, with async cached highlighting backed by a Jotai atom family.libghostty-vtfor all four Android ABIs using Zig..sothat must be rebuilt if the upstream Ghostty revision changes.Macroscope summarized 276b453.
Note
High Risk
Large new native surface on Android and vendored terminal binaries; iOS Personal Team mode changes auth and entitlements when enabled.
Overview
Adds Android implementations for the existing Expo native modules (
T3ComposerEditor,T3NativeControls,T3ReviewDiffSurface) so Kotlin views expose the same props, events, and async APIs as iOS. The review diff Android view decodes rows/tokens off the main thread, keeps file headers sticky, and handles vertical/horizontal scrolling and taps separately from the gutter.Android terminal is documented and backed by vendored libghostty-vt (pinned revision, LICENSE, headers, rebuild script notes in module README).
iOS local builds gain an opt-in Personal Team path via
T3CODE_IOS_PERSONAL_TEAMand a custom bundle ID: widgets plugin is skipped, Clerk Sign in with Apple is disabled, associated domains are removed from the shown config diff, andwithoutIosPersonalTeamCapabilitiesstrips push, Apple Sign In, and app group entitlements. README addsvp run ios:releaseand Personal Team examples.Tooling: Metro blocks the repo
.t3path from the bundle;app.configaddsexpo-assetand refactors the widgets plugin definition.Reviewed by Cursor Bugbot for commit 276b453. Bugbot is set up for automated code reviews on this repo. Configure here.