feat(mobile): auto-hiding reader toolbar with customizable actions#2527
Open
esimkowitz wants to merge 12 commits intokarakeep-app:mainfrom
Open
feat(mobile): auto-hiding reader toolbar with customizable actions#2527esimkowitz wants to merge 12 commits intokarakeep-app:mainfrom
esimkowitz wants to merge 12 commits intokarakeep-app:mainfrom
Conversation
…s and overflow menu Redesign the bookmark reader bottom bar with configurable toolbar actions, an overflow ellipsis menu using native MenuView, and auto-hide on scroll. Add toolbar settings page for reordering visible/overflow actions. Support iOS 26 glass pill styling and transparent headers with edge-to-edge content.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a configurable toolbar: new Toolbar Buttons settings page and settings fields; implements a data-driven toolbar registry and BottomActions refactor; threads scroll-offset and barsVisible through bookmark previews; adds useScrollDirection hook, iOS26 header/footer handling, and animated footer/banner behaviors. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile SummaryImplements auto-hiding reader toolbar with customizable actions, iOS 26 glass pill styling, and overflow menu with SF Symbol icons. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 497d21f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/mobile/app/dashboard/settings/toolbar-settings.tsx (2)
25-34: Potential stale closure insavecallback.The
savecallback capturessettingsin its closure and spreads it when saving. Ifsettingschanges between renders but beforesaveis called, you may inadvertently overwrite other settings that changed concurrently.Consider only updating the specific fields:
♻️ Suggested refactor
const save = useCallback( (nextVisible: ToolbarActionId[], nextOverflow: ToolbarActionId[]) => { - setSettings({ - ...settings, + setSettings((prev) => ({ + ...prev, toolbarActions: nextVisible, overflowActions: nextOverflow, - }); + })); }, - [settings, setSettings], + [setSettings], );This requires
setSettingsto support a functional update pattern. If it doesn't currently, the existing approach is acceptable but should be noted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/dashboard/settings/toolbar-settings.tsx` around lines 25 - 34, The save callback currently closes over settings and spreads it when calling setSettings, which can cause stale-overwrite; change save to use the functional updater form of setSettings so you only update toolbarActions and overflowActions based on the previous state (e.g., call setSettings(prev => ({ ...prev, toolbarActions: nextVisible, overflowActions: nextOverflow }))). Ensure setSettings supports a functional update; if not, note the limitation or convert the setter to accept a functional updater.
74-77: Consider adding accessibility labels for drag handles.The long-press interaction for dragging is not accessible to screen reader users. Consider adding accessibility hints to indicate the drag functionality:
<TouchableOpacity activeOpacity={0.7} onLongPress={drag} disabled={isActive} className="flex flex-row items-center gap-3 bg-card px-4 py-3" + accessibilityLabel={`${meta.label}, drag to reorder`} + accessibilityHint="Long press and drag to reorder" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/dashboard/settings/toolbar-settings.tsx` around lines 74 - 77, The TouchableOpacity acting as the drag handle (the component with onLongPress={drag} and disabled={isActive}) needs accessibility props so screen readers know it supports reordering; add accessible={true}, an accessibilityRole (e.g., "button" or "adjustable"), an accessibilityLabel like "Reorder item" and an accessibilityHint such as "Long press and drag to reorder this item", and ensure the disabled state updates accessibilityState accordingly so VoiceOver/TalkBack convey when dragging is unavailable.apps/mobile/app/dashboard/bookmarks/[slug]/index.tsx (2)
98-102: Consider extracting the magic number 44.The value
44represents the standard iOS navigation bar height. While this is a well-known constant, consider extracting it for clarity:const IOS_NAV_BAR_HEIGHT = 44; const contentInsetTop = isIOS26 ? insets.top + IOS_NAV_BAR_HEIGHT : 0;This same value appears on line 130 in
BookmarkLinkPreview.tsx, suggesting a shared constant would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/dashboard/bookmarks/`[slug]/index.tsx around lines 98 - 102, Extract the magic number 44 into a clearly named constant (e.g., IOS_NAV_BAR_HEIGHT) and use it when computing contentInsetTop (where isIOS26 is used) so the line const contentInsetTop = isIOS26 ? insets.top + 44 : 0 becomes insets.top + IOS_NAV_BAR_HEIGHT when applicable; also replace the same literal in BookmarkLinkPreview (the other occurrence) or move the constant to a shared module and import it to keep a single source of truth.
29-30: Platform.Version type handling may be inconsistent.On iOS,
Platform.Versionis typically a string (e.g.,"26.0"), but the type definition allows bothstringandnumber. TheparseIntcall handles strings correctly, but theas stringcast could mask type issues if React Native's behavior changes.This works correctly for iOS, but consider using a more defensive approach:
const isIOS26 = - Platform.OS === "ios" && parseInt(Platform.Version as string, 10) >= 26; + Platform.OS === "ios" && parseInt(String(Platform.Version), 10) >= 26;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/dashboard/bookmarks/`[slug]/index.tsx around lines 29 - 30, The isIOS26 check uses a forced cast (Platform.Version as string) which can mask runtime types; update the check to defensively handle both string and number values for Platform.Version (e.g., use typeof Platform.Version === "number" ? Platform.Version : parseInt(String(Platform.Version), 10) or Number/Math.floor on the string) so isIOS26 correctly evaluates on iOS regardless of whether Platform.Version is a number or a string; replace the current isIOS26 assignment with this type-guarded conversion referencing Platform.Version and isIOS26.apps/mobile/components/bookmarks/BottomActions.tsx (2)
36-37: DuplicateisIOS26constant across files.The
isIOS26check is duplicated inapps/mobile/app/dashboard/bookmarks/[slug]/index.tsx(line 29-30) and here. Consider extracting this to a shared utility:// apps/mobile/lib/platform.ts export const isIOS26 = Platform.OS === "ios" && parseInt(String(Platform.Version), 10) >= 26;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BottomActions.tsx` around lines 36 - 37, The isIOS26 platform check is duplicated; create a shared constant (e.g., export const isIOS26 in a new apps/mobile/lib/platform.ts) implementing Platform.OS === "ios" && parseInt(String(Platform.Version), 10) >= 26, then replace the local const isIOS26 in BottomActions.tsx (and the duplicate in dashboard/bookmarks/[slug]/index.tsx) with an import from that shared utility and remove the duplicated declaration so both files use the single exported isIOS26.
203-227: Hardcoded file extension may cause issues with non-JPEG images.The temp file is always saved as
temp_image.jpgregardless of the actual image format (PNG, WebP, etc.). This could cause issues with sharing certain image types.Consider deriving the extension from the asset metadata or content type:
♻️ Suggested improvement
if ( bookmark.content.assetType === "image" && (await Sharing.isAvailableAsync()) ) { const assetUrl = `${settings.address}/api/assets/${bookmark.content.assetId}`; - const fileUri = `${FileSystem.documentDirectory}temp_image.jpg`; + const fileName = bookmark.content.fileName || "temp_image"; + const fileUri = `${FileSystem.documentDirectory}${fileName}`; const downloadResult = await FileSystem.downloadAsync(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BottomActions.tsx` around lines 203 - 227, The code saves images to a hardcoded temp filename ("temp_image.jpg") which breaks for non-JPEGs; change the logic in the BookmarkTypes.ASSET branch (around fileUri, FileSystem.downloadAsync and subsequent delete) to derive the file extension from the asset metadata or response content-type before creating fileUri: use bookmark.content.fileName or assetId metadata if available, otherwise perform a lightweight HEAD/fetch to read the Content-Type header and map it to an extension (e.g., image/png -> .png) with a safe fallback like .jpg; then construct fileUri using that extension, call FileSystem.downloadAsync into that path, and keep the existing shareAsync and deleteAsync flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Around line 320-327: The onClick handler is using the stale
bookmark.favourited value instead of the optimistic isFavourited state, causing
incorrect toggles on rapid clicks; update the onClick in BottomActions (the
handler that calls Haptics.selectionAsync and favouriteBookmark) to compute the
new value from isFavourited (i.e. pass favourited: !isFavourited to
favouriteBookmark) so the optimistic state drives the network call and UI
remains consistent with rapid toggles.
- Line 380: The constant name useGlassPill wrongly suggests a React hook; rename
the symbol (e.g., to isGlassPill or glassPillEnabled) and update all references
in this module to the new name (replace usages of useGlassPill in
JSX/conditionals such as the places currently checking it). Keep the
initialization logic the same: const isGlassPill = isIOS26 &&
isGlassEffectAPIAvailable(); and update any imports/exports or local references
(including the two occurrences in this file) to use the new identifier.
In `@apps/mobile/package.json`:
- Line 40: The project currently pulls react-native-reanimated@~4.1.1 via
react-native-draggable-flatlist@4.0.3 which requires the React Native New
Architecture but the repo config does not enable it; either enable New
Architecture flags in your app configuration (set newArchitecture: true for both
ios and android in app.config.js and ensure build profiles in eas.json account
for it) so reanimated v4 can run, or downgrade react-native-reanimated to a 3.x
compatible version and update package.json and lockfile accordingly to remove
the v4 dependency from react-native-draggable-flatlist resolution.
---
Nitpick comments:
In `@apps/mobile/app/dashboard/bookmarks/`[slug]/index.tsx:
- Around line 98-102: Extract the magic number 44 into a clearly named constant
(e.g., IOS_NAV_BAR_HEIGHT) and use it when computing contentInsetTop (where
isIOS26 is used) so the line const contentInsetTop = isIOS26 ? insets.top + 44 :
0 becomes insets.top + IOS_NAV_BAR_HEIGHT when applicable; also replace the same
literal in BookmarkLinkPreview (the other occurrence) or move the constant to a
shared module and import it to keep a single source of truth.
- Around line 29-30: The isIOS26 check uses a forced cast (Platform.Version as
string) which can mask runtime types; update the check to defensively handle
both string and number values for Platform.Version (e.g., use typeof
Platform.Version === "number" ? Platform.Version :
parseInt(String(Platform.Version), 10) or Number/Math.floor on the string) so
isIOS26 correctly evaluates on iOS regardless of whether Platform.Version is a
number or a string; replace the current isIOS26 assignment with this
type-guarded conversion referencing Platform.Version and isIOS26.
In `@apps/mobile/app/dashboard/settings/toolbar-settings.tsx`:
- Around line 25-34: The save callback currently closes over settings and
spreads it when calling setSettings, which can cause stale-overwrite; change
save to use the functional updater form of setSettings so you only update
toolbarActions and overflowActions based on the previous state (e.g., call
setSettings(prev => ({ ...prev, toolbarActions: nextVisible, overflowActions:
nextOverflow }))). Ensure setSettings supports a functional update; if not, note
the limitation or convert the setter to accept a functional updater.
- Around line 74-77: The TouchableOpacity acting as the drag handle (the
component with onLongPress={drag} and disabled={isActive}) needs accessibility
props so screen readers know it supports reordering; add accessible={true}, an
accessibilityRole (e.g., "button" or "adjustable"), an accessibilityLabel like
"Reorder item" and an accessibilityHint such as "Long press and drag to reorder
this item", and ensure the disabled state updates accessibilityState accordingly
so VoiceOver/TalkBack convey when dragging is unavailable.
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Around line 36-37: The isIOS26 platform check is duplicated; create a shared
constant (e.g., export const isIOS26 in a new apps/mobile/lib/platform.ts)
implementing Platform.OS === "ios" && parseInt(String(Platform.Version), 10) >=
26, then replace the local const isIOS26 in BottomActions.tsx (and the duplicate
in dashboard/bookmarks/[slug]/index.tsx) with an import from that shared utility
and remove the duplicated declaration so both files use the single exported
isIOS26.
- Around line 203-227: The code saves images to a hardcoded temp filename
("temp_image.jpg") which breaks for non-JPEGs; change the logic in the
BookmarkTypes.ASSET branch (around fileUri, FileSystem.downloadAsync and
subsequent delete) to derive the file extension from the asset metadata or
response content-type before creating fileUri: use bookmark.content.fileName or
assetId metadata if available, otherwise perform a lightweight HEAD/fetch to
read the Content-Type header and map it to an extension (e.g., image/png ->
.png) with a safe fallback like .jpg; then construct fileUri using that
extension, call FileSystem.downloadAsync into that path, and keep the existing
shareAsync and deleteAsync flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/mobile/app/dashboard/(tabs)/(settings)/index.tsxapps/mobile/app/dashboard/_layout.tsxapps/mobile/app/dashboard/bookmarks/[slug]/index.tsxapps/mobile/app/dashboard/settings/toolbar-settings.tsxapps/mobile/components/bookmarks/BookmarkHtmlHighlighterDom.tsxapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/mobile/components/bookmarks/BookmarkLinkView.tsxapps/mobile/components/bookmarks/BookmarkTextView.tsxapps/mobile/components/bookmarks/BottomActions.tsxapps/mobile/lib/settings.tsapps/mobile/lib/useScrollDirection.tsapps/mobile/package.json
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mobile/components/bookmarks/BottomActions.tsx (1)
379-379: 🛠️ Refactor suggestion | 🟠 MajorMisleading naming:
useGlassPillis not a hook.The constant
useGlassPillstarts withusewhich suggests it's a React hook, but it's actually a module-level constant. This violates the Rules of Hooks naming convention.-const useGlassPill = isIOS26 && isGlassEffectAPIAvailable(); +const shouldUseGlassPill = isIOS26 && isGlassEffectAPIAvailable();Then update usages on lines 388 and 426.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BottomActions.tsx` at line 379, The name useGlassPill incorrectly looks like a React hook; rename the module-level constant to a non-hook boolean name (for example isGlassPillEnabled or showGlassPill) and update every usage of useGlassPill (the boolean checks/conditionals and props that currently read useGlassPill) to the new name; keep the value assignment the same (e.g., const isGlassPillEnabled = isIOS26 && isGlassEffectAPIAvailable()) and ensure all references (the two places that currently read useGlassPill for rendering/logic) are updated accordingly.
🧹 Nitpick comments (1)
apps/mobile/components/bookmarks/BottomActions.tsx (1)
209-227: Potential race condition with static temp filename.Using a hardcoded
temp_image.jpgfilename could cause issues if multiple share operations occur concurrently. Consider using a unique filename (e.g., with timestamp or bookmark ID).♻️ Suggested fix with unique filename
- const fileUri = `${FileSystem.documentDirectory}temp_image.jpg`; + const fileUri = `${FileSystem.documentDirectory}share_${bookmark.id}_${Date.now()}.jpg`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BottomActions.tsx` around lines 209 - 227, The code uses a hardcoded temp filename (`temp_image.jpg`) when building fileUri in the BottomActions component (see variable fileUri and the FileSystem.downloadAsync / Sharing.shareAsync / FileSystem.deleteAsync calls), which can cause race conditions for concurrent share operations; change fileUri to use a unique filename (e.g., include Date.now(), bookmark id, or a UUID) so each downloadAsync target is distinct, then continue to share and delete that unique path (ensure the same unique identifier is used for deleteAsync).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Around line 320-326: The favouriteBookmark mutation call is missing the
required bookmarkId argument which will cause the API call to fail; update the
onClick handler where favouriteBookmark is invoked (near Haptics.selectionAsync
and isFavourited) to pass the bookmarkId the same way archiveBookmark does
(include the bookmarkId property along with favourited: !isFavourited) so the
mutation receives both fields.
---
Duplicate comments:
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Line 379: The name useGlassPill incorrectly looks like a React hook; rename
the module-level constant to a non-hook boolean name (for example
isGlassPillEnabled or showGlassPill) and update every usage of useGlassPill (the
boolean checks/conditionals and props that currently read useGlassPill) to the
new name; keep the value assignment the same (e.g., const isGlassPillEnabled =
isIOS26 && isGlassEffectAPIAvailable()) and ensure all references (the two
places that currently read useGlassPill for rendering/logic) are updated
accordingly.
---
Nitpick comments:
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Around line 209-227: The code uses a hardcoded temp filename
(`temp_image.jpg`) when building fileUri in the BottomActions component (see
variable fileUri and the FileSystem.downloadAsync / Sharing.shareAsync /
FileSystem.deleteAsync calls), which can cause race conditions for concurrent
share operations; change fileUri to use a unique filename (e.g., include
Date.now(), bookmark id, or a UUID) so each downloadAsync target is distinct,
then continue to share and delete that unique path (ensure the same unique
identifier is used for deleteAsync).
- Fix silent fall-through when sharing non-image/non-pdf assets - Pass contentInsetTop to browser/archive WebViews for iOS 26 - Auto-inject missing action IDs into overflow on settings load - Extract NAV_BAR_HEIGHT constant to replace magic number 44 - Remove unused ownerOnly/linkOnly from ToolbarActionMeta - Extract triggerHaptic() helper to handle async catch - Deduplicate image/PDF share logic with finally cleanup - Add console.warn for unknown menu action events - Simplify headerPlatformOptions to plain isIOS26 ternary - Fix inaccurate comment about top/bottom padding
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx (1)
34-35: DuplicateNAV_BAR_HEIGHTconstant.This constant is also defined in
apps/mobile/app/dashboard/bookmarks/[slug]/index.tsxat line 33. Consider extracting it to a shared constants file to maintain a single source of truth.♻️ Extract to shared constants
Create a shared constants file (e.g.,
apps/mobile/lib/constants.ts):// Standard iOS navigation bar height (points) export const NAV_BAR_HEIGHT = 44;Then import it in both files:
-// Standard iOS navigation bar height (points) -const NAV_BAR_HEIGHT = 44; +import { NAV_BAR_HEIGHT } from "@/lib/constants";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx` around lines 34 - 35, Duplicate NAV_BAR_HEIGHT constant: remove the local constant from BookmarkLinkPreview and the other file, create a single shared constants module that exports NAV_BAR_HEIGHT, and update both BookmarkLinkPreview (component) and the dashboard bookmarks page (index component) to import NAV_BAR_HEIGHT from that module so both use the same source of truth.apps/mobile/components/bookmarks/BottomActions.tsx (1)
175-201: Consider preserving original image file extension.The temporary file for image sharing uses a hardcoded
.jpgextension regardless of the actual asset type. While this works for most images, it could cause issues with PNGs or other formats when sharing to apps that respect MIME types based on extension.🔧 Optional: Derive extension from asset metadata
const fileUri = bookmark.content.assetType === "pdf" ? `${FileSystem.documentDirectory}${bookmark.content.fileName || "document.pdf"}` - : `${FileSystem.documentDirectory}temp_image.jpg`; + : `${FileSystem.documentDirectory}temp_image.${bookmark.content.fileName?.split('.').pop() || 'jpg'}`;Alternatively, you could use a MIME type mapping if the asset metadata includes content type information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BottomActions.tsx` around lines 175 - 201, The temp image filename currently hardcodes ".jpg" which can mis-label PNGs/etc; update the logic that builds fileUri in the BottomActions component to derive the extension and MIME from available metadata (use bookmark.content.fileName's extension if present, or map bookmark.content.assetType/contentType to an extension like .png/.jpg/.webp) and use that extension in fileUri, and pass the correct mimeType/UTI into Sharing.shareAsync (references: fileUri, bookmark.content.assetType, bookmark.content.fileName, FileSystem.downloadAsync, Sharing.shareAsync, downloadResult.uri).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/mobile/components/bookmarks/BookmarkLinkPreview.tsx`:
- Around line 34-35: Duplicate NAV_BAR_HEIGHT constant: remove the local
constant from BookmarkLinkPreview and the other file, create a single shared
constants module that exports NAV_BAR_HEIGHT, and update both
BookmarkLinkPreview (component) and the dashboard bookmarks page (index
component) to import NAV_BAR_HEIGHT from that module so both use the same source
of truth.
In `@apps/mobile/components/bookmarks/BottomActions.tsx`:
- Around line 175-201: The temp image filename currently hardcodes ".jpg" which
can mis-label PNGs/etc; update the logic that builds fileUri in the
BottomActions component to derive the extension and MIME from available metadata
(use bookmark.content.fileName's extension if present, or map
bookmark.content.assetType/contentType to an extension like .png/.jpg/.webp) and
use that extension in fileUri, and pass the correct mimeType/UTI into
Sharing.shareAsync (references: fileUri, bookmark.content.assetType,
bookmark.content.fileName, FileSystem.downloadAsync, Sharing.shareAsync,
downloadResult.uri).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/mobile/app/dashboard/bookmarks/[slug]/index.tsxapps/mobile/components/bookmarks/BookmarkLinkPreview.tsxapps/mobile/components/bookmarks/BookmarkLinkView.tsxapps/mobile/components/bookmarks/BottomActions.tsxapps/mobile/lib/settings.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/mobile/components/bookmarks/BookmarkTextView.tsx (1)
105-111: Avoid wiring scroll events when no listener is provided.
ScrollViewcurrently emits high-frequency scroll events even whenonScrollOffsetChangeis absent. MakeonScrollandscrollEventThrottleconditional to reduce unnecessary work.♻️ Proposed refactor
<ScrollView className="flex-1 rounded-lg border border-border bg-card p-2" contentInset={{ top: contentInsetTop }} contentOffset={{ x: 0, y: -contentInsetTop }} - scrollEventThrottle={16} - onScroll={(e) => onScrollOffsetChange?.(e.nativeEvent.contentOffset.y)} + scrollEventThrottle={onScrollOffsetChange ? 16 : undefined} + onScroll={ + onScrollOffsetChange + ? (e) => onScrollOffsetChange(e.nativeEvent.contentOffset.y) + : undefined + } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/components/bookmarks/BookmarkTextView.tsx` around lines 105 - 111, The ScrollView in BookmarkTextView.tsx is always wired to high-frequency scroll events; change it so that onScroll and scrollEventThrottle are only set when onScrollOffsetChange is provided: check the onScrollOffsetChange prop and, if present, pass onScroll={(e) => onScrollOffsetChange?.(e.nativeEvent.contentOffset.y)} and scrollEventThrottle={16}, otherwise omit both props (or set scrollEventThrottle={0}/undefined) to avoid unnecessary work. Update the JSX where ScrollView is rendered and reference the onScrollOffsetChange prop to gate these props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/mobile/components/bookmarks/BookmarkTextView.tsx`:
- Around line 105-111: The ScrollView in BookmarkTextView.tsx is always wired to
high-frequency scroll events; change it so that onScroll and scrollEventThrottle
are only set when onScrollOffsetChange is provided: check the
onScrollOffsetChange prop and, if present, pass onScroll={(e) =>
onScrollOffsetChange?.(e.nativeEvent.contentOffset.y)} and
scrollEventThrottle={16}, otherwise omit both props (or set
scrollEventThrottle={0}/undefined) to avoid unnecessary work. Update the JSX
where ScrollView is rendered and reference the onScrollOffsetChange prop to gate
these props.
esimkowitz
force-pushed
the
mobile-reader-action-bar
branch
from
2ccece0 to
f493cc6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #2261
Resolves #2262
Resolves #2263
Summary
Auto-hiding reader bars (#2263)
When scrolling down in the bookmark reader, the top navigation bar and bottom toolbar now automatically hide to maximize reading space. Scrolling back up reveals them again. This applies to all reader view types (browser, reader, archive, and text).
useScrollDirectionhook detects scroll direction and toggles bar visibilitynavigation.setOptions({ headerShown }), footer animates out viareact-native-reanimatedtranslateYBookmarkLinkView,BookmarkTextView, and all link preview variants)Customizable bottom toolbar (#2261)
The bottom action bar is now fully customizable. Users can choose which actions appear directly on the toolbar (up to 6) and which go into an overflow menu accessible via an ellipsis button.
MenuViewwith SF Symbol icons, plus an "Edit Toolbar..." shortcut at the bottomRedesigned "Continue reading" banner
Shortened from "Continue where you left off" to "Continue reading", made the banner a pill shape where the whole banner is a touch target. Renders as Liquid Glass on iOS 26. Uses
expo-blurto apply a blur effect on iOS <26.iOS 26 polish
expo-glass-effectwhen available on iOS 26Test plan