Conversation
📝 WalkthroughWalkthroughThis PR replaces multiple WebView usages with a new cross-platform HtmlRenderer, adds style prop support to several web UI primitives, updates tests/mocks for HtmlRenderer, and hardens map initialization, camera and marker rendering with size observation and coordinate validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 11
🤖 Fix all issues with AI agents
In `@src/app/call/`[id].tsx:
- Line 312: The HtmlRenderer call should guard against null/undefined values so
it doesn't render "null"/"undefined" as text; update the jsx that uses
HtmlRenderer (e.g., HtmlRenderer html={call.Note ...} and the similar usage for
call.Nature) to pass a safe default like html={call.Note ?? ''} (and
html={call.Nature ?? ''}) so the component always receives a string; preserve
the existing style prop (StyleSheet.flatten([styles.container, { height: 200
}])) when making this change.
- Around line 463-468: The visibility checks for the map incorrectly use
truthiness on coordinates (coordinates.latitude && coordinates.longitude and
!coordinates.latitude || !coordinates.longitude) which fails for valid 0 values;
update both checks to explicit null checks (e.g., ensure coordinates.latitude
!== null && coordinates.longitude !== null when showing the StaticMap and use
!== null checks when gating behavior elsewhere) so 0 is treated as a valid
coordinate — look for references to coordinates.latitude, coordinates.longitude
and the StaticMap usage to apply the change.
In `@src/components/calls/call-card.tsx`:
- Around line 1-5: Remove the stray blank line and reorder the import groups so
external packages and framework imports are grouped before local/project
imports: keep imports for 'lucide-react-native', 'react', and 'react-native'
together, then import '@/components/ui/html-renderer' after them; after
adjusting the ordering, run the project's linter autofix (npx eslint --fix) to
ensure the import sorting (simple-import-sort/imports) passes CI.
In `@src/components/maps/map-view.web.tsx`:
- Around line 243-251: The current map error handler in the newMap.on('error',
...) callback relies on a fragile minified symbol check ('r[3]'); remove that
check and stop matching on minified internals — instead only special-case the
stable "Invalid LngLat" message (if you still want to suppress that silently)
and for all other errors log them at warn level without rethrowing. Update the
handler inside MapView.web.tsx (the newMap.on('error' ...) callback) to drop the
msg.includes('r[3]') branch, keep or add a single stable string check for
"Invalid LngLat", and change the console warning code to consistently log
remaining mapbox-gl errors at warn level.
- Around line 386-421: The effect currently unconditionally skips the first
centerCoordinate update via hasInitialized, which prevents maps without an
initialCenter prop from being moved on first render; change the logic in the
useEffect inside map-view.web.tsx so you only return early when an initialCenter
was actually provided to the MapView. Concretely, detect whether initialCenter
(or a boolean like initialCenterProvided) exists, then update the block around
hasInitialized.current to set hasInitialized.current = true and only return if
initialCenter was provided; otherwise allow the subsequent camera flyTo/easeTo
to run on the first render. Reference: hasInitialized, useEffect, map,
centerCoordinate, initialCenter, animationMode, animationDuration, flyTo,
easeTo.
In `@src/components/maps/static-map.tsx`:
- Around line 30-36: The current truthiness check in the StaticMap component
(variables latitude and longitude) uses "!latitude || !longitude", which treats
0 as falsy and hides valid coordinates; change the guard to explicitly check for
null/undefined (e.g., latitude == null || longitude == null or typeof latitude
=== 'undefined' || latitude === null || ... ) so that 0 is accepted as a valid
coordinate and only missing values trigger the "no_location" fallback.
- Line 40: Remove the non-API props initialCenter and initialZoom from the
Mapbox.MapView invocation (currently passing initialCenter={[longitude,
latitude]} and initialZoom={zoom}); these props are not supported by
`@rnmapbox/maps` and cause inconsistency with the existing Mapbox.Camera usage—use
Mapbox.Camera alone to control position/zoom across native and web; keep the
existing mapStyle, styles.map/height, logoEnabled, attributionEnabled,
compassEnabled, zoomEnabled, and rotateEnabled props intact and ensure Camera is
configured with longitude, latitude and zoom.
In `@src/components/notes/note-details-sheet.tsx`:
- Around line 66-72: Guard against null/undefined selectedNote.Body before
passing it to HtmlRenderer by using a safe fallback (e.g. selectedNote?.Body ??
'' or String(selectedNote?.Body || '')) so HtmlRenderer never receives
undefined/null; update the HtmlRenderer prop in the Note details sheet component
(where HtmlRenderer is rendered with html={selectedNote.Body}) to use the
fallback expression instead.
In `@src/components/protocols/protocol-details-sheet.tsx`:
- Around line 87-95: HtmlRenderer is being passed selectedProtocol.ProtocolText
which can be null/undefined and will cause the iframe body to render the literal
"undefined"/"null"; fix by ensuring the prop is a safe string (e.g., pass an
empty string when missing) or conditionally render the HtmlRenderer only when
ProtocolText is present. Update the call site around HtmlRenderer (the component
using selectedProtocol.ProtocolText and selectedProtocolId) to either use a
default like selectedProtocol.ProtocolText || '' or wrap the HtmlRenderer render
in a guard that verifies selectedProtocol.ProtocolText before rendering.
In `@src/components/ui/html-renderer/index.web.tsx`:
- Around line 109-120: The message handler handleMessage must validate the
message origin/source before acting; update handleMessage to only accept link
messages where event.data?.type === 'html-renderer-link' && event.data.url and
event.source === iframeRef.current?.contentWindow (or additionally check
event.origin if you need origin verification) so only the trusted iframe
triggers onLinkPress or Linking.openURL; reference iframeRef, onLinkPress, and
Linking.openURL in the updated logic and keep the rest of the callback
unchanged.
- Line 135: The inline ESLint disable comment "// eslint-disable-next-line
react-native/no-inline-styles" in the iframe render of the HtmlRenderer web
component is referencing a rule that isn't configured and breaks CI; remove that
invalid disable comment in src/components/ui/html-renderer/index.web.tsx (the
iframe element inside the HtmlRenderer/HtmlRendererWeb render function) and
either move the inline style into a named constant (e.g. iframeStyle) or, if you
must suppress a linter, replace it with a valid web-specific rule disable that
exists in our config; ensure the change targets the iframe render location and
that no invalid eslint-disable lines remain.
🧹 Nitpick comments (13)
src/components/ui/shared-tabs.tsx (1)
17-20: Shared global zustand store will cause cross-instance state conflicts.
useTabStoreis a module-level singleton. If two or moreSharedTabsinstances on the same screen both use theonChangeprop, they'll read and write the sameactiveIndex, causing tabs to sync incorrectly across unrelated tab groups. This is pre-existing, but since this file is being touched, it's worth noting.Consider either removing the global store in favor of local state only, or creating a store per instance (e.g., via
useRef(() => create(...))).Also applies to: 58-59, 65-65
src/components/maps/map-view.web.tsx (2)
306-316:minHeightfallback silently overrides intentional zero-height.
style?.height || style?.minHeight || 100uses falsy coercion, so a deliberateheight: 0from a parent would still produceminHeight: 100. This is likely acceptable for the map use-case (zero-height map is never useful), butstyle?.height ?? style?.minHeight ?? 100would be more precise if numeric zero ever needs to propagate.
593-619: Dual cleanup returns inUserLocationeffect are correct but fragile.The
if (map.loaded())branch falls through to the cleanup at line 613, while theelsebranch returns its own cleanup at line 603. Both paths correctly remove the geolocate control. However, this structure is easy to misread and could be broken by future edits.A single cleanup return at the end would be clearer:
Suggested simplification
if (map.loaded()) { geolocate.trigger(); } else { - const onMapLoad = () => { - geolocate.trigger(); - }; + const onMapLoad = () => geolocate.trigger(); map.on('load', onMapLoad); - - return () => { - try { - map.off('load', onMapLoad); - map.removeControl(geolocate); - } catch { - // map may already be destroyed during route transitions - } - }; } return () => { try { + map.off?.('load', undefined); // no-op if no listener map.removeControl(geolocate); } catch { // map may already be destroyed during route transitions } };Alternatively, store
onMapLoadin a ref or local variable and conditionally clean it up in a single return block.src/components/contacts/__tests__/contact-details-sheet.test.tsx (1)
7-14: Stale testID"mock-webview"in the HtmlRenderer mock.The mock renders with
testID="mock-webview"which is a leftover from the WebView era. While no test in this file queries it, it's inconsistent with the migration and could mislead future test authors. Consider renaming to"html-renderer"for consistency with other test files in this PR.Proposed fix
- HtmlRenderer: (props: any) => <View testID="mock-webview" {...props} />, + HtmlRenderer: (props: any) => <View testID="html-renderer" {...props} />,src/components/contacts/contact-notes-list.tsx (2)
85-152: Consider memoizing thecustomCSSstring.The large
customCSStemplate literal is recreated on every render ofContactNoteCard. Since it only depends oncolorScheme, wrapping it inuseMemowould avoid unnecessary string allocations, especially when rendering multiple cards in a list.
90-90: RedundantonLinkPress— HtmlRenderer already defaults toLinking.openURL.The HtmlRenderer component defaults its link handler to
Linking.openURLwhenonLinkPressis not provided (seeindex.tsxline ~93). Passing(url) => Linking.openURL(url)is functionally identical but creates a new closure each render and enablesonShouldStartLoadWithRequest/navigation interception logic unnecessarily.You can either omit
onLinkPressentirely (to use the default) or keep it if you want the explicit link-interception behavior on native.src/components/protocols/__tests__/protocol-details-sheet.test.tsx (1)
28-63: Overly detailed mock couples tests to HtmlRenderer internals.This mock replicates the full HTML document structure (DOCTYPE, viewport meta, CSS styles) that the real
HtmlRendererproduces. Tests in the "WebView Content" describe block (lines 281-314) then assert on those mock-generated strings — effectively testing the mock, not the component under test.A simpler mock (like the one in
contact-notes-list.test.tsx) that just exposes thehtmlprop would make these tests less brittle while still verifying the correct content is passed through.src/components/ui/card/index.web.tsx (1)
9-11:StyleSheet.flattenalready handles non-array inputs — theArray.isArrayguard is redundant.
StyleSheet.flattenaccepts both single styles and arrays, so you can simplify:Simplification
- const flatStyle = Array.isArray(style) ? StyleSheet.flatten(style) : style; + const flatStyle = StyleSheet.flatten(style);src/components/ui/html-renderer/index.web.tsx (1)
55-65:linkScriptis computed on every render but referenced insideuseMemodeps — consider memoizing it.
linkScriptis derived only fromonLinkPresstruthiness, yet it's a string that's recreated every render, causingfullHtmlto recompute even when nothing meaningful changed. You could fold the script directly into theuseMemobody (keying on!!onLinkPressinstead oflinkScript) to avoid unnecessary recalculations.Suggested simplification
- const linkScript = onLinkPress - ? `<script>...</script>` - : ''; - const fullHtml = useMemo( - () => `...${html}${linkScript}</body>...`, - [html, scrollEnabled, showsVerticalScrollIndicator, resolvedTextColor, resolvedBgColor, customCSS, linkScript] + () => { + const script = onLinkPress + ? `<script> + document.addEventListener('click', function(e) { + var anchor = e.target.closest('a'); + if (anchor && anchor.href) { + e.preventDefault(); + window.parent.postMessage({ type: 'html-renderer-link', url: anchor.href }, '*'); + } + }); + </script>` + : ''; + return `...${html}${script}</body>...`; + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [html, scrollEnabled, showsVerticalScrollIndicator, resolvedTextColor, resolvedBgColor, customCSS, !!onLinkPress] );Also applies to: 102-102
src/components/ui/html-renderer/index.tsx (4)
65-88:fullHtmlis not memoized — inconsistent with the web variant and rebuilt every render.The web variant wraps this template in
useMemo. The native variant should do the same to avoid handing a newsourceobject toWebViewon every render, which can cause unnecessary re-renders or flickers.Proposed fix
+ import React, { useMemo } from 'react'; ... - const fullHtml = ` + const fullHtml = useMemo(() => ` <!DOCTYPE html> <html> ... <body>${html}</body> </html> - `; + `, [html, resolvedTextColor, resolvedBgColor, customCSS]);
90-118: Inline anonymous functions are recreated every render — wrap withuseCallback.
handleLinkPress,onShouldStartLoadWithRequest, andonNavigationStateChangeare all recreated on each render. Wrapping them inuseCallback(or extracting them) would avoid unnecessary WebView prop changes. As per coding guidelines: "Avoid anonymous functions in … event handlers to prevent re-renders."Proposed fix sketch
+ import { useCallback } from 'react'; ... - const handleLinkPress = onLinkPress ?? ((url: string) => Linking.openURL(url)); + const handleLinkPress = useCallback( + (url: string) => { + if (onLinkPress) { onLinkPress(url); } + else { Linking.openURL(url); } + }, + [onLinkPress] + ); + + const handleShouldStartLoad = useCallback( + (request: { url: string }) => { + if (request.url.startsWith('about:') || request.url.startsWith('data:')) return true; + handleLinkPress(request.url); + return false; + }, + [handleLinkPress] + ); + + const handleNavStateChange = useCallback( + (navState: { url: string }) => { + if (navState.url && !navState.url.startsWith('about:') && !navState.url.startsWith('data:')) { + handleLinkPress(navState.url); + } + }, + [handleLinkPress] + );
6-10:THEME_COLORSandHtmlRendererPropsare duplicated between the native and web variants.Both
index.tsxandindex.web.tsxdefine identicalTHEME_COLORSandHtmlRendererProps. Consider extracting them into a shared module (e.g.src/components/ui/html-renderer/types.tsandsrc/components/ui/html-renderer/constants.ts) so changes only need to happen in one place.Also applies to: 12-36
86-86: Consider adding HTML sanitization for defense-in-depth security.While the current implementation mitigates XSS risk by disabling JavaScript by default and using only trusted backend data sources, the HTML string is interpolated directly without sanitization. To follow defense-in-depth principles and protect against potential data corruption at the API layer, consider using a sanitization library (e.g., DOMPurify via
isomorphic-dompurify) on thehtmlprop before interpolation. Alternatively, document that thehtmlprop accepts only trusted content from vetted backend sources.
src/components/maps/map-view.web.tsx
Outdated
| useEffect(() => { | ||
| if (!map) return; | ||
|
|
||
| if (centerCoordinate) { | ||
| map.easeTo( | ||
| { | ||
| center: centerCoordinate, | ||
| zoom: zoomLevel, | ||
| bearing: heading, | ||
| pitch: pitch, | ||
| duration: animationDuration, | ||
| }, | ||
| { _programmatic: true } | ||
| ); | ||
| if ( | ||
| centerCoordinate && | ||
| centerCoordinate.length === 2 && | ||
| isFinite(centerCoordinate[0]) && | ||
| isFinite(centerCoordinate[1]) | ||
| ) { | ||
| // Skip the first render — the MapView already initialized at the correct | ||
| // position via initialCenter/initialZoom, so no programmatic move needed. | ||
| if (!hasInitialized.current) { | ||
| hasInitialized.current = true; | ||
| return; | ||
| } | ||
|
|
||
| // For subsequent coordinate/zoom changes, animate to the new position. | ||
| const cameraOptions = { | ||
| center: centerCoordinate as [number, number], | ||
| zoom: zoomLevel, | ||
| bearing: heading, | ||
| pitch: pitch, | ||
| duration: animationDuration, | ||
| }; | ||
|
|
||
| try { | ||
| if (animationMode === 'flyTo') { | ||
| map.flyTo(cameraOptions, { _programmatic: true }); | ||
| } else { | ||
| map.easeTo(cameraOptions, { _programmatic: true }); | ||
| } | ||
| } catch { | ||
| // Suppress projection-matrix errors during resize/transition | ||
| } | ||
| } | ||
| }, [map, centerCoordinate, zoomLevel, heading, pitch, animationDuration]); | ||
| }, [map, centerCoordinate?.[0], centerCoordinate?.[1], zoomLevel, heading, pitch, animationDuration, animationMode]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.tsx" -o -name "*.ts" \) | head -20Repository: Resgrid/Unit
Length of output: 753
🏁 Script executed:
rg -n 'initialCenter' --type=tsx --type=ts -C2Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
rg -n '<MapView' --type=tsx --type=ts -A15Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
rg -n '<Camera' --type=tsx --type=ts -B3 -A10Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
fd -e tsx -e ts | head -30Repository: Resgrid/Unit
Length of output: 981
🏁 Script executed:
rg 'initialCenter' -C2Repository: Resgrid/Unit
Length of output: 2663
🏁 Script executed:
rg '<MapView' -A15Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
rg '<Camera' -B3 -A10Repository: Resgrid/Unit
Length of output: 1331
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | sed -n '380,425p'Repository: Resgrid/Unit
Length of output: 1686
🏁 Script executed:
rg 'map-view.web' -A5 -B2Repository: Resgrid/Unit
Length of output: 50369
🏁 Script executed:
rg 'from.*map-view' -A5Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
rg 'MapView' src/components -B2 -A15 | head -120Repository: Resgrid/Unit
Length of output: 9290
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | sed -n '1,100p'Repository: Resgrid/Unit
Length of output: 3849
🏁 Script executed:
rg '<MapView' -B2 -A20 | head -200Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
rg 'import.*MapView' -B1 -A5Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
fd -name "*.tsx" -exec grep -l "MapView\|Camera" {} \; | head -20Repository: Resgrid/Unit
Length of output: 286
🏁 Script executed:
rg 'Mapbox\.MapView' -B2 -A15Repository: Resgrid/Unit
Length of output: 16333
🏁 Script executed:
rg 'Mapbox\.Camera' -B2 -A10Repository: Resgrid/Unit
Length of output: 6790
🏁 Script executed:
rg 'from.*mapbox'Repository: Resgrid/Unit
Length of output: 709
🏁 Script executed:
fd -name "*.tsx" -exec grep -H "MapView\|Camera" {} \;Repository: Resgrid/Unit
Length of output: 286
🏁 Script executed:
cat -n src/app/\(app\)/index.tsx | sed -n '1,150p'Repository: Resgrid/Unit
Length of output: 6982
🏁 Script executed:
cat -n src/app/\(app\)/index.tsx | sed -n '150,250p'Repository: Resgrid/Unit
Length of output: 3904
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | sed -n '200,280p'Repository: Resgrid/Unit
Length of output: 3894
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | sed -n '280,360p'Repository: Resgrid/Unit
Length of output: 2951
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | sed -n '340,430p'Repository: Resgrid/Unit
Length of output: 3530
Camera effect skips first render when MapView lacks initialCenter—map may render at default US location.
The hasInitialized flag skips the first centerCoordinate update unconditionally, assuming MapView initialized via initialCenter. However, in location-picker.tsx, full-screen-location-picker.tsx, and the main map screen (src/app/(app)/index.tsx), MapView is used without initialCenter while Camera provides centerCoordinate. On first render, the Camera effect returns early (line 399) and never moves the map, leaving it at the default US center [-98.5795, 39.8283] until a subsequent update triggers the camera move.
Either pass initialCenter to all MapView instances that use Camera, or track whether initialCenter was provided and skip the first render only when it was explicitly set.
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 386 - 421, The effect
currently unconditionally skips the first centerCoordinate update via
hasInitialized, which prevents maps without an initialCenter prop from being
moved on first render; change the logic in the useEffect inside map-view.web.tsx
so you only return early when an initialCenter was actually provided to the
MapView. Concretely, detect whether initialCenter (or a boolean like
initialCenterProvided) exists, then update the block around
hasInitialized.current to set hasInitialized.current = true and only return if
initialCenter was provided; otherwise allow the subsequent camera flyTo/easeTo
to run on the first render. Reference: hasInitialized, useEffect, map,
centerCoordinate, initialCenter, animationMode, animationDuration, flyTo,
easeTo.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/maps/map-view.web.tsx`:
- Around line 304-316: The minHeight fallback currently uses truthiness
(style?.height || style?.minHeight || 100) which treats 0 as falsy and will
override an intentional height: 0; change the fallback to use nullish coalescing
so only null/undefined are replaced (e.g., use style?.height ?? style?.minHeight
?? 100) in the JSX where mapContainer/testID/style are used to ensure explicit
zero heights are preserved.
🧹 Nitpick comments (8)
src/components/ui/vstack/index.web.tsx (1)
7-7: Minor inconsistency:ComponentPropsvsComponentPropsWithoutRef.The other sibling components (HStack, Center, Card) use
React.ComponentPropsWithoutRef<'div'>, but VStack usesReact.ComponentProps<'div'>. SinceforwardRefhandlesrefseparately,ComponentPropsWithoutRefis the more precise choice and avoids a duplicaterefin the props type.Suggested fix
-type IVStackProps = React.ComponentProps<'div'> & VariantProps<typeof vstackStyle> & { style?: StyleProp<ViewStyle> }; +type IVStackProps = React.ComponentPropsWithoutRef<'div'> & VariantProps<typeof vstackStyle> & { style?: StyleProp<ViewStyle> };src/components/maps/static-map.tsx (1)
51-51: Use ternary operator for conditional rendering instead of&&.Per coding guidelines, prefer
? :over&&for conditional rendering.Proposed fix
- {showUserLocation && <Mapbox.UserLocation visible={true} showsUserHeadingIndicator={true} />} + {showUserLocation ? <Mapbox.UserLocation visible={true} showsUserHeadingIndicator={true} /> : null}As per coding guidelines, "Use ternary operator
? :for conditional rendering instead of&&".src/components/maps/map-view.web.tsx (3)
178-240: Monkey-patching mapbox-gl internals is fragile but pragmatic.The patches to
unproject,easeTo,flyTo, and_renderare reasonable workarounds for real mapbox-gl edge cases. However, these are tightly coupled to mapbox-gl v3 internals (e.g.,_render, canvas size checks). Any mapbox-gl upgrade could break these patches silently.Consider adding a comment near the top of the init block documenting which mapbox-gl version these patches target (
3.18.1), so future upgraders know to re-verify.
122-267: Map initialization effect depends only on[hasSize]— prop changes tostyleURL,initialCenter, etc. won't reinitialize.The dependency array is
[hasSize], meaning changes tostyleURL,initialCenter,initialZoom, or callback props after mount won't recreate the map.styleURLis handled separately (line 298-302), butinitialCenter/initialZoomare only read during construction. This is probably fine since Camera handles subsequent moves, but it's worth noting thatinitialCenter/initialZoomare truly "initial-only" props — document this in the interface JSDoc or prop names (e.g.,defaultCenter).
18-18: Multipleanytypes throughout the file.
MapContextis typed asany,mapref isany, and several function parameters useany. The coding guidelines say to avoidanyand strive for precise types. While a full typing overhaul is out of scope for this bug-fix PR, consider creating atype MapInstance = mapboxgl.Mapalias and using it for the context and ref to incrementally improve type safety.As per coding guidelines, "Avoid using
any; strive for precise types".Also applies to: 86-86
src/components/ui/html-renderer/index.web.tsx (1)
55-106:linkScriptdefeatsuseMemo— it is a new string reference every render.
linkScriptis recreated on every render whenonLinkPressis truthy (it's a plain template literal, not memoized). Because it appears in theuseMemodependency array forfullHtml(line 105),fullHtmlwill be recomputed every render even though the string content is identical.Move the script construction inside the
useMemocallback or memoizelinkScriptseparately withuseMemo.♻️ Suggested fix — move linkScript inside fullHtml's useMemo
- const linkScript = onLinkPress - ? `<script> - document.addEventListener('click', function(e) { - var anchor = e.target.closest('a'); - if (anchor && anchor.href) { - e.preventDefault(); - window.parent.postMessage({ type: 'html-renderer-link', url: anchor.href }, '*'); - } - }); - </script>` - : ''; - const fullHtml = useMemo( - () => ` + () => { + const linkScript = onLinkPress + ? `<script> + document.addEventListener('click', function(e) { + var anchor = e.target.closest('a'); + if (anchor && anchor.href) { + e.preventDefault(); + window.parent.postMessage({ type: 'html-renderer-link', url: anchor.href }, '*'); + } + }); + </script>` + : ''; + return ` <!DOCTYPE html> ... <body>${html}${linkScript}</body> </html> - `, - [html, scrollEnabled, showsVerticalScrollIndicator, resolvedTextColor, resolvedBgColor, customCSS, linkScript] + `; + }, + [html, scrollEnabled, showsVerticalScrollIndicator, resolvedTextColor, resolvedBgColor, customCSS, onLinkPress] );src/components/calls/call-card.tsx (1)
102-106: Use ternary operator for conditional rendering instead of&&.Per coding guidelines, prefer
? :over&&for conditional rendering.♻️ Suggested fix
- {call.Nature && ( + {call.Nature ? ( <Box className="mt-4 rounded-lg bg-white/50 p-3"> <HtmlRenderer html={call.Nature} style={StyleSheet.flatten([styles.container, { height: 80 }])} textColor={textColor} /> </Box> - )} + ) : null}As per coding guidelines, "Use ternary operator
? :for conditional rendering instead of&&".src/components/contacts/contact-notes-list.tsx (1)
84-151: Consider extracting theonLinkPresshandler and thecustomCSSblock.
The anonymous arrow function
(url) => Linking.openURL(url)on line 89 creates a new reference each render, which will causeHtmlRendererto re-render (and on web, re-attach the message listener). Extract it to a stable callback at the component level.The large
customCSStemplate string (60+ lines) dominates the JSX and could be extracted to a module-level constant or auseMemo(since it depends oncolorScheme).♻️ Suggested extraction
const ContactNoteCard: React.FC<ContactNoteCardProps> = ({ note }) => { const { t } = useTranslation(); ... const { colorScheme } = useColorScheme(); + + const handleLinkPress = React.useCallback((url: string) => { + Linking.openURL(url); + }, []); + + const noteCustomCSS = React.useMemo(() => ` + html, body { width: 100%; height: auto; min-height: 100%; ... } + a { color: ${colorScheme === 'dark' ? '#60A5FA' : '#3B82F6'}; ... } + ... + `, [colorScheme]); ... <HtmlRenderer html={noteContent} style={styles.webView} scrollEnabled={true} showsVerticalScrollIndicator={true} - onLinkPress={(url) => Linking.openURL(url)} - customCSS={`...`} + onLinkPress={handleLinkPress} + customCSS={noteCustomCSS} />
| return ( | ||
| <div ref={mapContainer} data-testid={testID} style={{ width: '100%', height: '100%', ...style }}> | ||
| <div | ||
| ref={mapContainer} | ||
| data-testid={testID} | ||
| style={{ | ||
| width: '100%', | ||
| height: '100%', | ||
| position: 'relative', | ||
| ...style, | ||
| // Ensure the container is never zero-height on web | ||
| minHeight: style?.height || style?.minHeight || 100, | ||
| }} | ||
| > |
There was a problem hiding this comment.
minHeight fallback uses truthiness — height: 0 silently becomes 100.
style?.height || style?.minHeight || 100 treats 0 as falsy. If a consumer intentionally passes height: 0 (e.g., to collapse the map), it will be overridden to 100. While this is likely rare and the intent here is to prevent the zero-size crash, a nullish coalesce would be more precise:
Proposed fix
- minHeight: style?.height || style?.minHeight || 100,
+ minHeight: style?.height ?? style?.minHeight ?? 100,📝 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.
| return ( | |
| <div ref={mapContainer} data-testid={testID} style={{ width: '100%', height: '100%', ...style }}> | |
| <div | |
| ref={mapContainer} | |
| data-testid={testID} | |
| style={{ | |
| width: '100%', | |
| height: '100%', | |
| position: 'relative', | |
| ...style, | |
| // Ensure the container is never zero-height on web | |
| minHeight: style?.height || style?.minHeight || 100, | |
| }} | |
| > | |
| return ( | |
| <div | |
| ref={mapContainer} | |
| data-testid={testID} | |
| style={{ | |
| width: '100%', | |
| height: '100%', | |
| position: 'relative', | |
| ...style, | |
| // Ensure the container is never zero-height on web | |
| minHeight: style?.height ?? style?.minHeight ?? 100, | |
| }} | |
| > |
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 304 - 316, The minHeight
fallback currently uses truthiness (style?.height || style?.minHeight || 100)
which treats 0 as falsy and will override an intentional height: 0; change the
fallback to use nullish coalescing so only null/undefined are replaced (e.g.,
use style?.height ?? style?.minHeight ?? 100) in the JSX where
mapContainer/testID/style are used to ensure explicit zero heights are
preserved.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes & Improvements