Added Places Search for API and Fixed Editor Popover Heights#20
Added Places Search for API and Fixed Editor Popover Heights#20fortune710 merged 17 commits intomainfrom
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/components/capture/music/audio-preview-popover.tsx (1)
2-2: Remove unused importuseSharedValue.After removing the dynamic height animation,
useSharedValueis no longer used in this file.🔎 Proposed fix
-import Animated, { SlideInDown, SlideOutDown, useAnimatedStyle, useSharedValue } from 'react-native-reanimated'; +import Animated, { SlideInDown, SlideOutDown } from 'react-native-reanimated';frontend/app/capture/details.tsx (1)
242-251: Bug: Duplicate error toast shown on validation failure.Line 249 shows
"Cannot save entry"unconditionally after the if-else block, causing two toasts to appear when validation fails. The second toast should be removed.🔎 Proposed fix
const handleSave = async () => { if (!capture || !user || !hasSelectedSharing()) { if (!hasSelectedSharing()) { toast('Please select who to share this entry with', 'error'); } else { toast('Cannot save entry', 'error'); } - toast('Cannot save entry', 'error'); return; }
🤖 Fix all issues with AI agents
In @frontend/app/capture/details.tsx:
- Line 2: Remove the unused import symbol Platform from the import list in the
top-level import statement (the line that imports View, Text, TouchableOpacity,
StyleSheet, ScrollView, KeyboardAvoidingView, Platform, Pressable); update that
import to exclude Platform so there are no unused imports remaining.
- Around line 230-234: The code uses logical OR when calling updateTextItem with
currentItem.text || pendingTextValue which treats an empty string as falsy and
restores pendingTextValue when the user clears the text; change this to use the
nullish coalescing operator (currentItem.text ?? pendingTextValue) so only
null/undefined fallback to pendingTextValue, preserving intentional empty
strings when calling updateTextItem(pendingTextItemId, currentItem.text ??
pendingTextValue, updatedStyle as { color: string; fontFamily?: string;
backgroundColor?: string }).
In @frontend/components/capture/canvas/text-canvas-item.tsx:
- Line 11: Remove the stray debug console.log({ textStyle }) in
text-canvas-item.tsx to avoid noisy production logs; locate the console.log call
near the top of the TextCanvasItem component (or where textStyle is used) and
delete it (or replace with a gated debug/logger call that only runs in
development, e.g., using process.env.NODE_ENV === 'development' if you need
retained dev-only logging).
In @frontend/components/capture/editor/location-tab.tsx:
- Line 29: Remove the debug console.log calls in the LocationTab
component—specifically the console.log('currentLocation', currentLocation) and
the other console.log at the later occurrence—by either deleting them or
replacing them with the app's proper logging utility (e.g., logger.debug) if
persistent debug output is needed; locate these inside the location-tab.tsx
component where currentLocation is referenced and update accordingly so no raw
console.log remains in production code.
In @frontend/components/capture/editor/text-tab.tsx:
- Around line 46-54: The useEffect in the TextTab component schedules a
setTimeout to focus textInputRef but doesn't clean it up; store the timeout id
(e.g., const timer = setTimeout(...)) and return a cleanup function that calls
clearTimeout(timer) so the timeout is canceled if activeInternalTab changes or
the component unmounts; ensure the timeout callback still checks
textInputRef.current before calling focus; update any TypeScript typing for the
timer id (number vs NodeJS.Timeout) as needed.
In @frontend/hooks/use-device-location.ts:
- Around line 161-220: The reverse geocoding loop (geocodedLocations.slice(0,
20).map(...) calling Location.reverseGeocodeAsync) risks rate limits and slow
UX; reduce batch size (e.g., 5), process locations in small batches using
Promise.allSettled per batch, and insert a short delay between batches (await
sleep(ms)) to throttle requests; also add a simple in-memory cache (keyed by
`${latitude},${longitude}`, e.g., reverseGeocodeCache) checked before calling
Location.reverseGeocodeAsync and populated with the formatted address to avoid
duplicate requests; update the placesPromises logic to use these
batched/cache/throttled calls and fall back to searchQuery on errors as before.
- Line 107: Remove the stray debug log by deleting the console.log({ address })
statement inside the useDeviceLocation hook (function useDeviceLocation) so no
debug output remains in production; locate the console.log in
frontend/hooks/use-device-location.ts and remove that single line, leaving any
surrounding logic intact.
In @frontend/hooks/use-media-canvas.ts:
- Around line 27-29: In removeElement, change the loose inequality to strict by
using !== when filtering IDs: update the predicate inside setItems(prevItems =>
prevItems.filter(item => item.id !== id)) so it uses strict comparison between
item.id and id to avoid type coercion issues; keep the same function and
setItems usage.
In @frontend/hooks/use-places-search.ts:
- Around line 4-7: Remove the duplicate type definition PlacesSearchCoordinates
from use-places-search.ts and instead import and re-use the canonical export
from the places-search-service (the exported PlacesSearchCoordinates type in
frontend/services/places-search-service.ts); update any local references in
usePlacesSearch (and its exports) to use the imported PlacesSearchCoordinates
type so there is a single source of truth.
In @frontend/services/device-storage.ts:
- Around line 116-118: The getFriends method declares a return type of
FriendWithProfile[] | null but calls getItem with the generic any[]; update the
getItem call to use the correct generic type by changing getItem<any[]> to
getItem<FriendWithProfile[]> (keeping the method name getFriends and the return
signature unchanged) so the inferred types align and callers get proper typing.
In @frontend/services/places-search-service.ts:
- Line 74: The PlacesSearchService is logging sensitive user data at info level;
change the logger.info calls that include the user "query" and the full
"response.features" to use logger.debug (or remove/redact sensitive fields) so
these searches aren’t recorded at info level in production; update the
logger.info(...) calls in PlacesSearchService to logger.debug(...) for the
entries that log query and response.features, or redact PII from the payload
before logging.
- Around line 74-88: The axios GET in PlacesSearchService (call to
axios.get<MapboxSearchResponse>) has no timeout and can hang; add a timeout
property to the request config (e.g., timeout: 5000) inside the options object
passed to axios.get (alongside params) so the call to `${this.BASE_URL}` using
this.ACCESS_TOKEN and params (including proximity from options.coordinates) will
fail-fast; update any callers or error handling around the response to expect a
possible timeout error.
🧹 Nitpick comments (14)
frontend/services/device-storage.ts (1)
112-114: Consider updatingsetFriendsparameter type for consistency.For type consistency with
getFriends, thefriendsparameter should be typed asFriendWithProfile[]instead ofany[].🔎 Suggested refactor
-async setFriends(userId: string, friends: any[]): Promise<void> { +async setFriends(userId: string, friends: FriendWithProfile[]): Promise<void> { await this.setItem(`friends_${userId}`, friends, 60); // Cache for 1 hour }frontend/hooks/use-device-location.ts (1)
233-236: Stale closure in clearLocation callback.The
clearLocationcallback referencesdata?.regionanddata?.cityin the query key, but ifdatabecomesnullafter clearing, subsequent calls won't match the original query key that held actual values. Consider invalidating the query instead:🔎 Proposed fix
const clearLocation = useCallback(() => { queryClient.setQueryData(['device-location'], null); - queryClient.setQueryData(['places-in-state', data?.region, data?.city], []); - }, [queryClient, data?.region, data?.city]); + queryClient.invalidateQueries({ queryKey: ['places-in-state'] }); + }, [queryClient]);frontend/components/capture/entry-attachment-list.tsx (1)
10-14: Consider adding accessibility props for better UX.The
attachmentTypesarray and component structure look good. However, theTouchableOpacityelements lack accessibility props (accessibilityLabel,accessibilityRole).🔎 Proposed accessibility enhancement
<TouchableOpacity key={attachment.type} style={styles.attachmentOption} onPress={() => onSelectAttachment(attachment.type)} + accessibilityLabel={`Add ${attachment.label}`} + accessibilityRole="button" >frontend/hooks/use-places-search.ts (1)
9-30: Minor inconsistent indentation.The hook logic is sound - proper query key composition, conditional enabling, and safe fallback. However, line 13 has inconsistent indentation compared to the rest of the function body.
🔎 Fix indentation
export function usePlacesSearch( query: string, options: { coordinates?: PlacesSearchCoordinates } = {} ) { - const coordinatesKey = options.coordinates ? `${options.coordinates.latitude},${options.coordinates.longitude}` : 'N/A'; - const { + const coordinatesKey = options.coordinates + ? `${options.coordinates.latitude},${options.coordinates.longitude}` + : 'N/A'; + const {frontend/hooks/use-media-canvas.ts (1)
15-25: Consider applying functional state updates consistently.
addText,removeElement, andupdateTextItemnow use functional state updates (prevItems => ...), which is good for avoiding stale closure issues. However,addSticker,addMusic, andaddLocationstill referenceitemsdirectly, which could lead to stale state if called in rapid succession.🔎 Proposed fix for consistency
const addSticker = (sticker: any) => { - setItems([...items, { id: Date.now(), type: "sticker", sticker }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "sticker", sticker }]); } const addMusic = (music: MusicTag) => { - setItems([...items, { id: Date.now(), type: "music", music_tag: music }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "music", music_tag: music }]); } const addLocation = (location: string) => { - setItems([...items, { id: Date.now(), type: "location", location }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "location", location }]); }frontend/components/capture/editor/location-tab.tsx (3)
27-27: Remove unused variables and dead code.Several items are defined but never used:
placesInStateandisLoadingPlacesfromuseDeviceLocation(line 27)handleSelectCurrentLocationfunction (lines 58-67)showResultsvariable (line 95)🔎 Proposed cleanup
-const { location: currentLocation, isLoading: isLoadingCurrent, placesInState, isLoadingPlaces } = useDeviceLocation(); +const { location: currentLocation, isLoading: isLoadingCurrent } = useDeviceLocation();Remove
handleSelectCurrentLocationfunction (lines 58-67) andshowResultsvariable (line 95) if not needed.Also applies to: 58-67, 95-95
74-91: Remove unnecessary dependency from useMemo.
debouncedQueryis included in the dependency array but is not used within the memo computation. This could cause unnecessary re-computations.🔎 Proposed fix
return results; -}, [currentLocation, mapboxPlacesResults, debouncedQuery]); +}, [currentLocation, mapboxPlacesResults]);
15-20: Local interface shadows exported type.The local
LocationSearchResultinterface shadows the one exported fromplaces-search-service.ts. Consider importing and extending that type to avoid confusion and ensure consistency.🔎 Proposed approach
+import { LocationSearchResult as BaseLocationSearchResult } from '@/services/places-search-service'; -interface LocationSearchResult { - id: string; - name: string; - formattedAddress: string; - isCurrentLocation?: boolean; -} +interface LocationSearchResult extends BaseLocationSearchResult { + isCurrentLocation?: boolean; +}frontend/components/capture/editor/text-tab.tsx (2)
1-1: Remove unused imports and variables.
Platform,Keyboardare imported but not used. Theheightvariable fromDimensions.get("window")is also declared but never referenced.🔎 Proposed fix
-import { View, StyleSheet, TextInput, Text, TouchableOpacity, ScrollView, Platform, Keyboard, Dimensions } from "react-native"; +import { View, StyleSheet, TextInput, Text, TouchableOpacity, ScrollView } from "react-native"; // ... -const { height } = Dimensions.get("window");Also applies to: 21-21
177-184: Font list contains duplicates.The fonts array contains duplicate entries (e.g., "Garamond", "Georgia", "Helvetica", "Impact", "Lucida Console", "New York", "Symbol", "Tahoma", "Times New Roman", "Verdana"). Consider deduplicating and potentially moving this to a constants file.
🔎 Proposed fix
+// Consider moving to a constants file +const AVAILABLE_FONTS = [ + "Arial", "Arial Black", "Arial Narrow", "Arial Rounded MT Bold", "Arial Unicode MS", + "Book Antiqua", "Bookman", "Calibri", "Cambria", "Candara", "Century Gothic", + "Comic Sans MS", "Consolas", "Corbel", "Courier", "Courier New", "Curlz MT", + "Franklin Gothic Medium", "Garamond", "Georgia", "Helvetica", "Impact", + "Lucida Console", "Lucida Fax", "Lucida Sans", "Lucida Sans Unicode", + "Microsoft Sans Serif", "Myriad Pro", "New York", "Palatino", "Palatino Linotype", + "Rockwell", "Segoe UI", "Segoe UI Symbol", "Symbol", "Tahoma", "Times New Roman", + "Trebuchet MS", "Verdana", "Webdings", "Wingdings", "Wingdings 2", "Wingdings 3" +]; const renderFontTab = () => ( <View style={styles.tabContent}> <FontStyleSelector - fonts={["Arial", "Helvetica", ...]} + fonts={AVAILABLE_FONTS} onSelect={onFontChange} /> </View> );frontend/components/capture/editor-popover.tsx (3)
66-70: Refine initialText sync effect condition.The condition
initialText !== undefinedis always true sinceinitialTexthas a default value of"". This means the effect runs on every render whereinitialTextchanges (including from""to""). Consider using a ref to track if this is the initial mount or ifinitialTextgenuinely changed.🔎 Proposed fix
+import { useRef } from "react"; +// ... +const prevInitialTextRef = useRef(initialText); + useEffect(() => { - if (initialText !== undefined) { + if (initialText !== prevInitialTextRef.current) { setTextInput(initialText); + prevInitialTextRef.current = initialText; } }, [initialText]);Alternatively, if the intent is to sync only when editing an existing item:
useEffect(() => { - if (initialText !== undefined) { - setTextInput(initialText); - } + setTextInput(initialText); }, [initialText]);
103-105: Remove dead code comment.This comment describes functionality that was intentionally removed. Consider removing the comment to avoid confusion, or if the confirm function might be needed later, track it as a TODO with context.
🔎 Proposed fix
- // Text is now updated in real-time, so we don't need a confirm function - // This is kept for potential future use but not currently called
28-39: Remove unusedaddTextprop fromEditorPopover.The
addTextprop is defined in the interface and passed from the call site indetails.tsx, but it's never invoked within the component. Text updates now flow through real-time callbacks (onTextChangeandonStyleChange), as indicated by the code comment at lines 103-104. RemoveaddTextfrom the interface definition (line 31), function parameters (line 44), and the instantiation indetails.tsxto eliminate unused code.frontend/app/capture/details.tsx (1)
2-4: Remove unused imports.
KeyboardAvoidingView(line 2) andUserPlus(line 4) are imported but never used in this file.🔎 Proposed fix
-import { View, Text, TouchableOpacity, StyleSheet, ScrollView, KeyboardAvoidingView, Platform, Pressable } from 'react-native'; +import { View, Text, TouchableOpacity, StyleSheet, ScrollView, Platform, Pressable } from 'react-native';-import { X, Sticker, UserPlus, UserPlus2 } from 'lucide-react-native'; +import { X, Sticker, UserPlus2 } from 'lucide-react-native';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
frontend/app/capture/details.tsxfrontend/components/capture/canvas/text-canvas-item.tsxfrontend/components/capture/editor-popover.tsxfrontend/components/capture/editor/font-style-selector.tsxfrontend/components/capture/editor/location-tab.tsxfrontend/components/capture/editor/music-tab.tsxfrontend/components/capture/editor/text-tab.tsxfrontend/components/capture/entry-attachment-list.tsxfrontend/components/capture/music/audio-preview-popover.tsxfrontend/components/capture/music/music-list-item.tsxfrontend/hooks/use-device-location.tsfrontend/hooks/use-media-canvas.tsfrontend/hooks/use-places-search.tsfrontend/services/device-storage.tsfrontend/services/places-search-service.tsfrontend/types/capture.ts
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/components/capture/editor/music-tab.tsx (1)
frontend/components/capture/canvas/music-canvas-item.tsx (2)
MusicCanvasItem(10-17)MusicCanvasItemProps(5-8)
frontend/components/capture/entry-attachment-list.tsx (1)
frontend/types/capture.ts (1)
MediaCanvasItemType(1-1)
frontend/services/places-search-service.ts (2)
frontend/hooks/use-places-search.ts (1)
PlacesSearchCoordinates(4-7)frontend/lib/logger.ts (2)
logger(18-36)error(32-35)
frontend/hooks/use-device-location.ts (2)
frontend/lib/logger.ts (1)
error(32-35)frontend/hooks/use-privacy-settings.ts (1)
UsePrivacySettingsResult(15-21)
frontend/components/capture/editor/location-tab.tsx (3)
frontend/services/places-search-service.ts (1)
LocationSearchResult(44-49)frontend/hooks/use-device-location.ts (1)
useDeviceLocation(32-246)frontend/hooks/use-places-search.ts (1)
usePlacesSearch(9-30)
frontend/components/capture/music/audio-preview-popover.tsx (1)
frontend/components/profile/profile-update-popover.tsx (4)
ProfileUpdatePopover(28-116)paddingBottom(38-40)ProfileUpdatePopoverProps(19-26)event(44-48)
frontend/hooks/use-media-canvas.ts (1)
frontend/components/capture/canvas/vault-canvas.tsx (5)
VaultCanvasItem(68-111)VaultCanvasProps(11-17)VaultCanvasItemProps(63-66)VaultCanvas(19-61)item(51-57)
frontend/hooks/use-places-search.ts (1)
frontend/services/places-search-service.ts (2)
PlacesSearchCoordinates(51-54)PlacesSearchService(56-109)
frontend/components/capture/editor-popover.tsx (7)
frontend/types/capture.ts (1)
MediaCanvasItemType(1-1)frontend/hooks/use-debounce.ts (1)
useDebounce(3-17)frontend/hooks/use-music-tag.ts (1)
useMusicTag(4-20)frontend/components/capture/editor/text-tab.tsx (1)
TextTab(31-249)frontend/components/capture/editor/sticker-tab.tsx (1)
StickerTab(17-56)frontend/components/capture/editor/music-tab.tsx (1)
MusicTab(17-51)frontend/components/capture/editor/location-tab.tsx (1)
LocationTab(22-190)
frontend/app/capture/details.tsx (5)
frontend/types/capture.ts (1)
MediaCanvasItemType(1-1)frontend/hooks/use-media-canvas.ts (1)
useMediaCanvas(6-94)frontend/components/capture/media-canvas.tsx (1)
MediaCanvas(28-77)frontend/components/capture/entry-attachment-list.tsx (1)
EntryAttachmentList(16-49)frontend/components/capture/editor-popover.tsx (1)
EditorPopover(41-185)
🔇 Additional comments (16)
frontend/services/device-storage.ts (1)
1-1: LGTM—Import addition supports type improvements.The addition of
FriendWithProfileto the imports is appropriate for the stricter typing introduced in thegetFriendsmethod.frontend/types/capture.ts (1)
10-10: LGTM!The addition of
backgroundColorto the style interface aligns well with the text styling enhancements across the editor components.frontend/components/capture/canvas/text-canvas-item.tsx (1)
13-14: Consider whether defaulting to black background is intentional.The default
backgroundColor: "#000000"means text items without an explicit background will render with a solid black background. IftextStyleis undefined orbackgroundColoris not set, this could be unexpected behavior. Consider using"transparent"as the default if no background styling was intended.frontend/components/capture/music/music-list-item.tsx (1)
37-56: LGTM!The UI sizing adjustments (larger album art, increased canvas radius, and added padding) appear intentional for visual refinement.
frontend/components/capture/editor/music-tab.tsx (2)
4-4: LGTM!The icon change from
AlbumtoMusicis a reasonable choice for the empty state context.
88-88: Significant margin increase - verify visual intent.The
marginVerticalincreased from 36 to 120 (3.3x larger). This pushes the empty state content significantly lower. Ensure this is the intended visual design.frontend/components/capture/music/audio-preview-popover.tsx (1)
43-45: LGTM!Simplifying to static styles while retaining the enter/exit animations is a cleaner approach.
frontend/components/capture/editor/font-style-selector.tsx (2)
59-63: LGTM!Good use of
adjustsFontSizeToFitandnumberOfLinesto handle varying font name lengths gracefully. The logic to showpreviewCharwhen provided (for actual text preview) or fall back to the font name is sensible.
71-71: Good addition of keyboard handling.
keyboardShouldPersistTaps="handled"ensures taps on font options work correctly even when the keyboard is visible.frontend/components/capture/entry-attachment-list.tsx (1)
16-49: LGTM!The component is well-structured with clear separation of concerns. The use of
mapwith proper key extraction, consistent styling, and the callback pattern foronSelectAttachmentare all appropriate.frontend/hooks/use-media-canvas.ts (1)
31-37: LGTM!The new
updateTextItemfunction correctly uses functional state update, proper type guard (item.type === "text"), and immutable update pattern with spread operator.frontend/services/places-search-service.ts (1)
96-101: LGTM on the result mapping logic.The mapping from Mapbox features to
LocationSearchResulthandles missing IDs gracefully with the fallbackplace-${index}pattern, and properly extracts address information.frontend/components/capture/editor/text-tab.tsx (1)
188-249: LGTM on the tab navigation structure.The internal tab system with icon-based navigation is well-implemented. Using
keyboardShouldPersistTaps="handled"ensures smooth UX when interacting with tabs while the keyboard is open.frontend/components/capture/editor-popover.tsx (2)
124-184: LGTM on the popover structure and animations.The animated popover with dynamic height based on active tab, backdrop handling, and conditional tab content rendering is well-implemented. The spring animation configuration provides smooth transitions.
91-95: Remove the suggestion to addpopoverHeightto the dependency array.The dependency array is correct as-is.
popoverHeightis a Reanimated shared value (ref), not a reactive value, so it should not be included in the dependency array. The effect correctly depends only onactiveTab.However, there is a separate issue:
useSharedValue()is being called in the component body on every render. It should be wrapped inuseMemoor moved outside the component to ensure the shared value persists across renders:const popoverHeight = useMemo( () => useSharedValue(activeTab === "text" ? TEXT_TAB_HEIGHT : DEFAULT_TAB_HEIGHT), [] );Likely an incorrect or invalid review comment.
frontend/app/capture/details.tsx (1)
436-447: LGTM!The EditorPopover integration is well-structured with proper handling of the new props for real-time text updates, style changes, and the pending text item lifecycle.
| @@ -1,7 +1,7 @@ | |||
| import React, { useRef, useState } from 'react'; | |||
| import { useRef, useState } from 'react'; | |||
| import { View, Text, TouchableOpacity, StyleSheet, ScrollView, KeyboardAvoidingView, Platform, Pressable } from 'react-native'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\bPlatform\b' frontend/app/capture/details.tsxRepository: fortune710/keepsafe
Length of output: 192
Remove unused Platform import.
Platform is imported on line 2 but never used in the file.
🤖 Prompt for AI Agents
In @frontend/app/capture/details.tsx at line 2, Remove the unused import symbol
Platform from the import list in the top-level import statement (the line that
imports View, Text, TouchableOpacity, StyleSheet, ScrollView,
KeyboardAvoidingView, Platform, Pressable); update that import to exclude
Platform so there are no unused imports remaining.
| export function TextCanvasItem({ text, textStyle }: TextCanvasItemProps) { | ||
| if (!text) return null; | ||
|
|
||
| console.log({ textStyle }) |
There was a problem hiding this comment.
Remove debug console.log before merging.
This debug statement will pollute the console in production.
🔎 Proposed fix
- console.log({ textStyle })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log({ textStyle }) |
🤖 Prompt for AI Agents
In @frontend/components/capture/canvas/text-canvas-item.tsx at line 11, Remove
the stray debug console.log({ textStyle }) in text-canvas-item.tsx to avoid
noisy production logs; locate the console.log call near the top of the
TextCanvasItem component (or where textStyle is used) and delete it (or replace
with a gated debug/logger call that only runs in development, e.g., using
process.env.NODE_ENV === 'development' if you need retained dev-only logging).
| latitude: location.latitude, | ||
| longitude: location.longitude, | ||
| }); | ||
| console.log('currentLocation', currentLocation); |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
These debug statements should be removed or converted to proper logging for production code.
🔎 Proposed fix
- console.log('currentLocation', currentLocation);
// ... code ...
- console.log('allResults', allResults);Also applies to: 103-103
🤖 Prompt for AI Agents
In @frontend/components/capture/editor/location-tab.tsx at line 29, Remove the
debug console.log calls in the LocationTab component—specifically the
console.log('currentLocation', currentLocation) and the other console.log at the
later occurrence—by either deleting them or replacing them with the app's proper
logging utility (e.g., logger.debug) if persistent debug output is needed;
locate these inside the location-tab.tsx component where currentLocation is
referenced and update accordingly so no raw console.log remains in production
code.
| export interface MapboxFeature { | ||
| id: string; | ||
| type: string; | ||
| place_type: string[]; | ||
| relevance: number; | ||
| properties: { | ||
| accuracy?: string; | ||
| name?: string; | ||
| address?: string; | ||
| full_address?: string; | ||
| place_formatted?: string; | ||
| name_preferred?: string; | ||
| context?: { | ||
| country?: { name: string }; | ||
| region?: { name: string }; | ||
| postcode?: { name: string }; | ||
| place?: { name: string }; | ||
| }; | ||
| }; | ||
| text: string; | ||
| place_name: string; | ||
| center: [number, number]; // [longitude, latitude] | ||
| geometry: { | ||
| type: string; | ||
| coordinates: [number, number]; | ||
| }; | ||
| context?: Array<{ | ||
| id: string; | ||
| text: string; | ||
| [key: string]: any; | ||
| }>; | ||
| } | ||
|
|
||
| export interface MapboxSearchResponse { | ||
| type: string; | ||
| query: string[]; | ||
| features: MapboxFeature[]; | ||
| attribution: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Mapbox Search Searchbox API v1 response structure forward endpoint
💡 Result:
The Search Box API v1 /forward endpoint returns a GeoJSON FeatureCollection. Key structure (top-level and per-feature):
-
Top-level
- type: "FeatureCollection"
- features: [ Feature, ... ]
- attribution: string. [1]
-
Feature (each element of features)
- type: "Feature"
- geometry: { type: "Point", coordinates: [longitude, latitude] }
- properties: {
name,
mapbox_id,
feature_type, // e.g., "address", "poi", etc.
address,
full_address,
place_formatted,
context: { // hierarchical place context
country: { id?, name, country_code, country_code_alpha_3 },
region: { id?, name, region_code, region_code_full },
postcode: { name },
place: { name },
neighborhood: { name },
street: { name }
},
coordinates: { // duplicate coords + routable points
latitude,
longitude,
routable_points: [{ name, latitude, longitude }, ...]
},
maki,
poi_category, // array of category names
poi_category_ids,
external_ids,
metadata,
...other metadata fields
}
Example response and full field details in Mapbox docs. [1]
Source: Mapbox Search (Search Box API) documentation — /forward (Text Search) and example responses. [1]
References
[1] Mapbox Search Box API docs — Search Box API (forward, examples).
Update interfaces to match Mapbox Search Searchbox API v1 GeoJSON structure.
The current MapboxFeature interface conflicts with the actual Searchbox API v1 response. The API returns GeoJSON Features where type is always "Feature", and all location data resides in a properties object. The interfaces mix in fields from the legacy Geocoding API:
- The code has
text,place_name,place_type,relevance, andcenterat the Feature level, which don't exist in Searchbox API v1 - The actual API provides
mapbox_idandfeature_type(not in the code) - The
contextstructure is different: the API provides typed objects (country, region, postcode, place, neighborhood, street) with specific fields; the code uses a generic array - The
coordinatesin the actual API are nested withinpropertiesas a structured object withlatitude,longitude, androutable_points MapboxSearchResponseshould havetype: "FeatureCollection"(notstring)
Align these interfaces with the official Mapbox Search Searchbox API v1 response structure to ensure correct deserialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/components/capture/editor/text-tab.tsx:
- Around line 334-343: The colorGridItem style mixes width: '17.5%', height: 40,
and aspectRatio: 1 which conflicts because aspectRatio will override one
dimension (making width fixed to height), so pick one approach and adjust the
style: either remove height and keep width: '17.5%' with aspectRatio: 1 so the
item stays square by percentage, or remove aspectRatio and keep height: 40 with
width calculated (e.g., fixed px or flexBasis) so the fixed height is respected;
update the colorGridItem style object (colorGridItem) accordingly and keep
borderRadius, borderWidth, borderColor, alignItems, justifyContent unchanged.
In @frontend/services/places-search-service.ts:
- Around line 97-102: The map callback that builds the LocationSearchResult
array uses a fallback for id (feature.id || `place-${index}`) but assigns
placeId directly from feature.id, which can be undefined; update the mapping in
the response.data.features.map callback so placeId uses the same fallback as id
(e.g., use feature.id || `place-${index}`) to guarantee a string per the
LocationSearchResult interface, referencing the same feature.id and index values
used for id.
🧹 Nitpick comments (6)
frontend/services/device-storage.ts (1)
112-113: Consider updatingsetFriendsparameter type for consistency.The
setFriendsmethod still acceptsany[]whilegetFriendsnow returnsFriendWithProfile[] | null. For better type safety and consistency across the storage layer, consider updating the parameter type.🔎 Proposed fix
-async setFriends(userId: string, friends: any[]): Promise<void> { +async setFriends(userId: string, friends: FriendWithProfile[]): Promise<void> { await this.setItem(`friends_${userId}`, friends, 60); // Cache for 1 hour }frontend/hooks/use-media-canvas.ts (1)
15-25: Inconsistent state updater pattern may cause stale state issues.
addSticker,addMusic, andaddLocationstill use[...items, ...]which capturesitemsfrom the closure at render time. This can lead to lost updates if multiple items are added in quick succession, sinceaddText,removeElement, andupdateTextItemnow use functional updaters.Consider updating these functions for consistency:
🔎 Proposed fix
const addSticker = (sticker: any) => { - setItems([...items, { id: Date.now(), type: "sticker", sticker }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "sticker", sticker }]); } const addMusic = (music: MusicTag) => { - setItems([...items, { id: Date.now(), type: "music", music_tag: music }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "music", music_tag: music }]); } const addLocation = (location: string) => { - setItems([...items, { id: Date.now(), type: "location", location }]); + setItems(prevItems => [...prevItems, { id: Date.now(), type: "location", location }]); }frontend/components/capture/editor/text-tab.tsx (2)
21-22: Unusedheightvariable.
heightis destructured fromDimensions.get("window")but never used in the component. Consider removing it to clean up dead code.🔎 Proposed fix
-const { height } = Dimensions.get("window");
187-194: Font array contains duplicates and could be extracted.The fonts array in
renderFontTabcontains duplicates (e.g., "Georgia", "Garamond", "Helvetica", "Impact", "Lucida Console", "New York", "Palatino", "Symbol", "Tahoma", "Times New Roman", "Verdana" appear twice). Consider deduplicating and extracting this to a constant for maintainability.🔎 Proposed fix
+// Add near POPULAR_COLORS constant +const FONT_OPTIONS = [ + "Arial", "Helvetica", "Times New Roman", "Georgia", "Verdana", + "Tahoma", "Courier New", "Comic Sans MS", "Impact", "Lucida Console", + "Palatino", "Garamond", "Bookman", "New York", "Rockwell", "Symbol", + "Arial Black", "Arial Narrow", "Arial Rounded MT Bold", "Arial Unicode MS", + "Book Antiqua", "Calibri", "Cambria", "Candara", "Century Gothic", + "Consolas", "Corbel", "Courier", "Curlz MT", "Franklin Gothic Medium", + "Lucida Fax", "Lucida Sans", "Lucida Sans Unicode", "Microsoft Sans Serif", + "Myriad Pro", "Palatino Linotype", "Segoe UI", "Segoe UI Symbol", + "Trebuchet MS", "Webdings", "Wingdings", "Wingdings 2", "Wingdings 3" +]; const renderFontTab = () => ( <View style={styles.tabContent}> <FontStyleSelector - fonts={["Arial", "Helvetica", ...]} + fonts={FONT_OPTIONS} onSelect={onFontChange} /> </View> );frontend/hooks/use-device-location.ts (2)
5-9: Module-level cache may grow unbounded.The
reverseGeocodeCacheMap is never pruned and will accumulate entries indefinitely as users search different locations. For long-running sessions or apps processing many locations, consider adding a max size or LRU eviction strategy.🔎 Example: Simple size-limited cache
// In-memory cache for reverse geocoding results to avoid duplicate requests -const reverseGeocodeCache = new Map<string, string>(); +const MAX_CACHE_SIZE = 100; +const reverseGeocodeCache = new Map<string, string>(); + +const setCacheEntry = (key: string, value: string): void => { + if (reverseGeocodeCache.size >= MAX_CACHE_SIZE) { + // Remove oldest entry (first key in Map iteration order) + const firstKey = reverseGeocodeCache.keys().next().value; + if (firstKey) reverseGeocodeCache.delete(firstKey); + } + reverseGeocodeCache.set(key, value); +};Then replace
reverseGeocodeCache.set(...)calls withsetCacheEntry(...).
288-291:clearLocationmay not clear the intended query if called after data changes.The callback captures
data?.regionanddata?.cityin its dependencies. IfclearLocationis called whendataisnullor has different region/city values than when the query was populated, it will set data on a mismatched query key rather than clearing the original.Consider using
queryClient.removeQuerieswith a partial key match to clear all places-in-state queries regardless of the specific region/city:🔎 Proposed fix
const clearLocation = useCallback(() => { queryClient.setQueryData(['device-location'], null); - queryClient.setQueryData(['places-in-state', data?.region, data?.city], []); -}, [queryClient, data?.region, data?.city]); + queryClient.removeQueries({ queryKey: ['places-in-state'] }); +}, [queryClient]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/capture/details.tsxfrontend/components/capture/editor/text-tab.tsxfrontend/hooks/use-device-location.tsfrontend/hooks/use-media-canvas.tsfrontend/hooks/use-places-search.tsfrontend/services/device-storage.tsfrontend/services/places-search-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/hooks/use-places-search.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/services/device-storage.ts (1)
frontend/types/friends.ts (1)
FriendWithProfile(39-41)
🔇 Additional comments (12)
frontend/services/device-storage.ts (2)
1-1: LGTM!The import correctly adds
FriendWithProfileto support the improved type safety in thegetFriendsmethod.
116-118: Excellent fix!The type parameter mismatch identified in the previous review has been correctly resolved. Both the return type and the
getItemcall now useFriendWithProfile[], providing proper type safety for callers.frontend/hooks/use-media-canvas.ts (2)
10-12: Good use of functional state updater and returning the generated ID.The
addTextfunction now correctly uses a functional updater pattern and returns the generated ID, which enables proper tracking of pending text items.
27-37: LGTM!Both
removeElementandupdateTextItemcorrectly use functional state updaters with proper type checks. The strict equality issue from the previous review has been addressed.frontend/components/capture/editor/text-tab.tsx (1)
46-64: Good implementation of timeout cleanup.The useEffect now properly stores the timer reference and cleans it up when
activeInternalTabchanges or the component unmounts. This addresses the memory leak concern from the previous review.frontend/app/capture/details.tsx (4)
152-161: Good cleanup logic for pending text items.The
handleAddTextfunction properly removes any existing pending text item before adding a new one, preventing orphaned items in the canvas.
191-203: Consider trimming comparison value in close handler.The check
textValue === "Enter text"is case-sensitive and exact. If a user adds a space or changes casing, the item won't be removed. This is likely fine for the intended UX, but worth noting.
230-234: Good use of nullish coalescing.Using
??instead of||ensures that intentionally empty text strings are preserved rather than falling back topendingTextValue. This addresses the concern from the previous review.
436-447: EditorPopover integration looks correct.The EditorPopover is properly wired with all the new props including
defaultTab,onTextChange,onStyleChange, andinitialText. The conditionalinitialTextassignment correctly checks for a pending text item.frontend/services/places-search-service.ts (1)
73-119: LGTM on the request handling and error recovery.The implementation correctly handles:
- Input validation with early returns
- Request timeout (5s) to prevent hanging
- Debug-level logging (addressing prior concern)
- Comprehensive axios error differentiation (timeout vs other errors)
- Graceful degradation returning empty array on failure
frontend/hooks/use-device-location.ts (2)
144-286: Well-structured batching and throttling implementation.The places-in-state query addresses prior rate-limiting concerns effectively:
- Batches of 5 with 200ms delays between batches
- Per-location caching to avoid duplicate API calls
Promise.allSettledfor resilient batch processing- Graceful fallbacks on individual failures
The
enabledflag correctly gates execution until location data is available.
11-27: LGTM on interface extensions.The additions of
postalCode,latitude, andlongitudetoLocationDataand the newPlaceResultinterface are well-defined and align with the functional requirements for places-in-state support.
| colorGridItem: { | ||
| width: '17.5%', | ||
| height: 40, | ||
| aspectRatio: 1, | ||
| borderRadius: 12, | ||
| alignItems: "center", | ||
| borderWidth: 2, | ||
| borderColor: '#E2E8F0', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, |
There was a problem hiding this comment.
Conflicting style properties for colorGridItem.
The style combines width: '17.5%', height: 40, and aspectRatio: 1. When aspectRatio is set, it overrides one dimension based on the other. With height: 40 and aspectRatio: 1, the width will be forced to 40, making width: '17.5%' ineffective on most devices.
Consider using either percentage-based sizing with aspectRatio or fixed dimensions:
🔎 Proposed fix using percentage width
colorGridItem: {
width: '17.5%',
- height: 40,
aspectRatio: 1,
borderRadius: 12,
...
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| colorGridItem: { | |
| width: '17.5%', | |
| height: 40, | |
| aspectRatio: 1, | |
| borderRadius: 12, | |
| alignItems: "center", | |
| borderWidth: 2, | |
| borderColor: '#E2E8F0', | |
| alignItems: 'center', | |
| justifyContent: 'center', | |
| }, | |
| colorGridItem: { | |
| width: '17.5%', | |
| aspectRatio: 1, | |
| borderRadius: 12, | |
| borderWidth: 2, | |
| borderColor: '#E2E8F0', | |
| alignItems: 'center', | |
| justifyContent: 'center', | |
| }, |
🤖 Prompt for AI Agents
In @frontend/components/capture/editor/text-tab.tsx around lines 334 - 343, The
colorGridItem style mixes width: '17.5%', height: 40, and aspectRatio: 1 which
conflicts because aspectRatio will override one dimension (making width fixed to
height), so pick one approach and adjust the style: either remove height and
keep width: '17.5%' with aspectRatio: 1 so the item stays square by percentage,
or remove aspectRatio and keep height: 40 with width calculated (e.g., fixed px
or flexBasis) so the fixed height is respected; update the colorGridItem style
object (colorGridItem) accordingly and keep borderRadius, borderWidth,
borderColor, alignItems, justifyContent unchanged.
| const results: LocationSearchResult[] = response.data.features.map((feature, index) => ({ | ||
| id: feature.id || `place-${index}`, | ||
| formattedAddress: feature.properties.place_formatted || feature.properties.full_address || '', | ||
| placeId: feature.id, | ||
| name: feature.properties.name, | ||
| })); |
There was a problem hiding this comment.
Inconsistent fallback handling for placeId.
Line 98 provides a fallback (feature.id || \place-${index}`) for id, but line 100 assigns feature.iddirectly toplaceIdwithout a fallback. Iffeature.idis undefined,placeIdwill beundefinedwhile theLocationSearchResultinterface requiresstring`.
🔎 Proposed fix
const results: LocationSearchResult[] = response.data.features.map((feature, index) => ({
id: feature.id || `place-${index}`,
formattedAddress: feature.properties.place_formatted || feature.properties.full_address || '',
- placeId: feature.id,
+ placeId: feature.id || `place-${index}`,
name: feature.properties.name,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const results: LocationSearchResult[] = response.data.features.map((feature, index) => ({ | |
| id: feature.id || `place-${index}`, | |
| formattedAddress: feature.properties.place_formatted || feature.properties.full_address || '', | |
| placeId: feature.id, | |
| name: feature.properties.name, | |
| })); | |
| const results: LocationSearchResult[] = response.data.features.map((feature, index) => ({ | |
| id: feature.id || `place-${index}`, | |
| formattedAddress: feature.properties.place_formatted || feature.properties.full_address || '', | |
| placeId: feature.id || `place-${index}`, | |
| name: feature.properties.name, | |
| })); |
🤖 Prompt for AI Agents
In @frontend/services/places-search-service.ts around lines 97 - 102, The map
callback that builds the LocationSearchResult array uses a fallback for id
(feature.id || `place-${index}`) but assigns placeId directly from feature.id,
which can be undefined; update the mapping in the response.data.features.map
callback so placeId uses the same fallback as id (e.g., use feature.id ||
`place-${index}`) to guarantee a string per the LocationSearchResult interface,
referencing the same feature.id and index values used for id.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @fortune710. * #20 (comment) The following files were modified: * `frontend/app/capture/details.tsx` * `frontend/components/capture/canvas/text-canvas-item.tsx` * `frontend/components/capture/editor-popover.tsx` * `frontend/components/capture/editor/location-tab.tsx` * `frontend/components/capture/editor/music-tab.tsx` * `frontend/components/capture/editor/text-tab.tsx` * `frontend/components/capture/entry-attachment-list.tsx` * `frontend/components/capture/music/audio-preview-popover.tsx` * `frontend/components/capture/music/music-list-item.tsx` * `frontend/hooks/use-device-location.ts` * `frontend/hooks/use-media-canvas.ts` * `frontend/hooks/use-places-search.ts`
1 participant
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.