Conversation
📝 WalkthroughWalkthroughBumps RN Mapbox to v10.2.10, adds multiple patch scripts and a postinstall hook to comment out deprecated Mapbox setters; changes call update endpoint/payload and edit form behavior; removes a call layout and its test; adds concurrent-playback guard to audio service; plus assorted UI and formatting tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const stripHtml = (html: string | undefined | null): string => { | ||
| if (!html) return ''; | ||
| // Remove HTML tags | ||
| let text = html.replace(/<[^>]*>/g, ''); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
In general, the best fix is to stop relying on regex-based HTML/tag stripping and manual entity decoding, and instead delegate this to a battle-tested HTML-to-text or sanitization library. This eliminates the multi-character replacement pitfalls and ensures that complex cases (nested tags, comments, malformed HTML, scripts, etc.) are handled correctly.
For this specific case, the function is named stripHtml and returns a plain string. The least intrusive change is to implement stripHtml using a well-known library such as html-to-text, which parses the HTML and produces plain text. That way:
- All tags, including
script, are safely removed. - Entity decoding is handled comprehensively.
- The function still returns trimmed text, preserving existing semantics for normal input.
Concretely, in src/app/call/[id]/edit.tsx:
- Add an import for
html-to-textnear the other imports:import { htmlToText } from 'html-to-text';
- Replace the current implementation of
stripHtml(lines 61–77) with a simplified version that:- Returns
''for falsy input (as before). - Uses
htmlToText(html, { wordwrap: false })to convert HTML to text. - Trims the result before returning.
- Returns
No other files or code paths need to be modified to resolve the CodeQL finding, and existing callers of stripHtml continue to receive plain text.
| @@ -11,6 +11,7 @@ | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { ScrollView, View } from 'react-native'; | ||
| import * as z from 'zod'; | ||
| import { htmlToText } from 'html-to-text'; | ||
|
|
||
| import { DispatchSelectionModal } from '@/components/calls/dispatch-selection-modal'; | ||
| import { Loading } from '@/components/common/loading'; | ||
| @@ -61,18 +62,11 @@ | ||
| // Helper function to strip HTML tags from rich text content | ||
| const stripHtml = (html: string | undefined | null): string => { | ||
| if (!html) return ''; | ||
| // Remove HTML tags | ||
| let text = html.replace(/<[^>]*>/g, ''); | ||
| // Decode common HTML entities | ||
| text = text | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, "'") | ||
| .replace(/'/g, "'"); | ||
| // Trim whitespace | ||
|
|
||
| const text = htmlToText(html, { | ||
| wordwrap: false, | ||
| }); | ||
|
|
||
| return text.trim(); | ||
| }; | ||
|
|
| @@ -164,7 +164,8 @@ | ||
| "sanitize-html": "^2.17.0", | ||
| "tailwind-variants": "^3.1.0", | ||
| "zod": "~3.23.8", | ||
| "zustand": "~4.5.5" | ||
| "zustand": "~4.5.5", | ||
| "html-to-text": "^9.0.5" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "~7.26.0", |
| Package | Version | Security advisories |
| html-to-text (npm) | 9.0.5 | None |
| text = text | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') |
Check failure
Code scanning / CodeQL
Double escaping or unescaping High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
In general, to avoid double unescaping when manually decoding HTML entities, ensure that the escape character & is decoded after all other entities, or better, use a well‑tested HTML entity decoding library. The current implementation decodes & before decoding <, >, quotes, and apostrophes, so it can convert &lt; into < and then immediately decode that into <.
The minimal, behavior‑preserving fix is to change the order of the .replace calls so that the replacement of & happens last. That way, sequences like &lt; first become &<? No—they’ll remain &lt; until the final step, and since < is never present as a plain substring prior to that last step, we avoid double decoding. Concretely, within stripHtml in src/app/call/[id]/edit.tsx, we should reorder the replacements on lines 67–74 so that .replace(/&/g, '&') is the final one in the chain, keeping the same set of entities and trimming behavior. No new imports or helper methods are needed for this small change.
| @@ -66,12 +66,12 @@ | ||
| // Decode common HTML entities | ||
| text = text | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, "'") | ||
| .replace(/'/g, "'"); | ||
| .replace(/'/g, "'") | ||
| .replace(/&/g, '&'); | ||
| // Trim whitespace | ||
| return text.trim(); | ||
| }; |
![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 `@scripts/patch-mapbox-final.js`:
- Around line 137-141: The script overwrites filePath without creating a
.original backup; before calling fs.writeFileSync(filePath, result.join('\n')),
ensure you create a backup if one doesn't exist by reading the current file
(e.g., with fs.readFileSync(filePath, 'utf8') into originalContent) and writing
it to a backup path (e.g., originalFilePath = filePath + '.original') only when
fs.existsSync(originalFilePath) is false; then proceed to write the patched
content as now done so you can restore the original on future runs and avoid
data loss.
In `@scripts/patch-mapbox.js`:
- Around line 1-4: The script uses the __dirname global to build filePath which
ESLint flags as undefined; fix by enabling Node globals or deriving __dirname:
add an ESLint environment directive (e.g. add "/* eslint-env node */" at the top
of scripts/patch-mapbox.js) or alternatively compute __dirname via
path.dirname(new URL(import.meta.url).pathname) if converting to ESM; update the
code that declares filePath (and mirror the same fix used in
patch-mapbox-final.js) so ESLint no longer reports __dirname as undefined.
In `@scripts/patch-rnmapbox-full.pl`:
- Around line 12-17: The script reads from "$file.original" if it exists but
never creates that backup, so on first run the original will be overwritten;
modify the logic around "$file.original", "$file" and `@lines` so that if
"$file.original" does not exist you create it by copying the current "$file"
contents into "$file.original" (e.g., open "$file" for read, read into `@lines`,
then write those contents to "$file.original"), and keep the existing restore
path intact; ensure you handle open/write errors with die or a logged error to
avoid silent failures.
In `@scripts/patch-rnmapbox-sed.sh`:
- Around line 24-35: The perl one-liner uses a fixed "$comment_next = 10"
heuristic which can leave trailing statements active and break Kotlin when
blocks; update the logic in the perl script so that after matching the
deprecated case label (the regex that matches the long list of case strings) it
comments lines until it reaches the next Kotlin case/else label or the end of
the when block instead of counting 10 lines — for example, replace the
"$comment_next = 10" approach with a loop/flag that continues commenting until a
line matches /^\s*(case|else)\b/ or a closing brace of the when block, ensuring
you still skip already-commented lines and use the same $FILE variable and the
existing matching regex to locate the start of the deprecated case.
In `@scripts/patch-rnmapbox-v2.sh`:
- Around line 1-56: The script can clobber the Kotlin file and fails on non-GNU
awk because it relies on gensub; to fix, first check that FILE exists and exit
with an error if not, then require GNU awk (detect gawk via command -v and abort
with a helpful message if missing) or explicitly set AWK_CMD to gawk; write awk
output to a safe temporary file (use mktemp) instead of redirecting straight to
$FILE, and after successful processing atomically replace the original (mv tmp
-> $FILE) while keeping the existing backup created by cp "$FILE" "$FILE.bak2";
update any gensub usages only within the AWK script invoked by AWK_CMD so you
guarantee compatibility.
In `@scripts/patch-rnmapbox.py`:
- Around line 38-44: The backup file is being written after `content` is
modified so the `.bak` contains the patched content; fix this by saving the
original content before any `re.sub` modifications (e.g., store
`original_content = content` immediately after reading the file) and write
`original_content` to `file_path + '.bak'` (or move the backup write to before
the loop that alters `content`); ensure subsequent writes to `file_path` still
use the modified `content`.
In `@src/api/calls/calls.ts`:
- Around line 167-168: Remove the debug console.log that prints the request
payload in src/api/calls/calls.ts: locate the console.log('Sending updateCall
request with data:', JSON.stringify(data, null, 2)); and delete it (or replace
with a non-sensitive logger call if required by project policy), ensuring no
sensitive data is written to stdout before merging; reference the surrounding
updateCall/request-sending logic to find the exact spot.
In `@src/app/call/`[id]/edit.tsx:
- Around line 61-77: The stripHtml function is unsafe: replace the custom regex
+ sequential replacements with a proven sanitizer and robust decoder—update
stripHtml to first sanitize the input using a library like DOMPurify (e.g.,
DOMPurify.sanitize(html, {ALLOWED_TAGS: []}) or similar) to eliminate malformed
tags and script content, then decode HTML entities using a reliable decoder such
as html-entities' decode (or, if you cannot add a dependency, iteratively decode
until stable and use a safer tag-stripping regex like /<[^>]*>?/g); ensure you
reference and modify the stripHtml function to use DOMPurify and decode so
malformed tags and double-escaped entities are handled securely.
- Around line 868-891: The inline anonymous Button onPress handler is
overcomplicated and fails to await async errors because onSubmit(data) isn't
awaited; extract the logic into a named async callback (e.g., handleSave) using
useCallback, call await handleSubmit(onSubmit)() or await handleSubmit(async
(data) => await onSubmit(data))() inside that callback, replace the inline
onPress with onPress={handleSave}, and remove the console-heavy inline
diagnostics (you can keep minimal logging inside handleSave if needed); ensure
handleSave includes try/catch so errors from onSubmit are caught.
- Around line 46-47: The edit form's Zod schema currently makes priority
optional; change the schema's priority from optional to required using
z.string().min(1, 'Priority is required') in the edit form definition to match
the UpdateCallRequest API contract and the new-call form; also update the form
submission handler (the onSubmit/handleSubmit logic that currently allows
priority to be null) to always coerce/parse priority to a number before sending
to UpdateCallRequest so null/empty values are never sent.
In `@src/components/staffing/staffing-bottom-sheet.tsx`:
- Around line 268-270: The label "optional" is always shown but should be
conditional when the note is required (selectedStaffing?.Note === 2); update the
Text rendering that uses safeT('common.optional') so it toggles between
safeT('common.optional') and a required indicator (or omits the optional text)
based on the same rule used by canProceedFromCurrentStep() /
selectedStaffing?.Note === 2, locating the UI in staffing-bottom-sheet.tsx where
safeT('home.staffing.note') is rendered and adjust the conditional logic to
mirror canProceedFromCurrentStep()'s note-required check.
🧹 Nitpick comments (14)
src/services/audio.service.ts (1)
212-216: Hardcoded 500ms timeout may not match actual sound duration.If a sound is longer than 500ms, the guard clears prematurely, allowing overlapping playback. Conversely, for very short sounds, this adds unnecessary blocking.
Consider using
setOnPlaybackStatusUpdateto detect when playback actually finishes:♻️ Suggested approach using playback status callback
- // Remove from playing set after a short delay - // Most UI sounds are < 500ms, so this is safe - setTimeout(() => { - this.isPlayingSound.delete(soundName); - }, 500); + // Use playback status update to detect actual completion + sound.setOnPlaybackStatusUpdate((status) => { + if (status.isLoaded && status.didJustFinish) { + this.isPlayingSound.delete(soundName); + sound.setOnPlaybackStatusUpdate(null); + } + });src/services/__tests__/audio.service.test.ts (2)
154-159: UseclearMockCalls()for consistency and completeness.This test manually clears mocks but doesn't clear
isPlayingSound, whichclearMockCalls()handles. Use the helper for consistency:♻️ Suggested fix
it('should play start transmitting sound successfully', async () => { - mockSound.getStatusAsync.mockClear(); - mockSound.setPositionAsync.mockClear(); - mockSound.playAsync.mockClear(); - (logger.error as jest.MockedFunction<any>).mockClear(); - (logger.warn as jest.MockedFunction<any>).mockClear(); - (logger.debug as jest.MockedFunction<any>).mockClear(); + clearMockCalls(); await audioService.playStartTransmittingSound();
180-208: Consider adding test forgetStatusAsyncfailure.The implementation handles
getStatusAsyncthrowing an error (lines 194-201 in audio.service.ts), but there's no dedicated test for this scenario. Consider adding a test case:💡 Suggested test case
it('should skip playback if getStatusAsync fails', async () => { clearMockCalls(); mockSound.getStatusAsync.mockRejectedValueOnce(new Error('Status check failed')); await audioService.playStartTransmittingSound(); expect(mockSound.setPositionAsync).not.toHaveBeenCalled(); expect(mockSound.playAsync).not.toHaveBeenCalled(); expect(logger.warn).toHaveBeenCalledWith({ message: 'Failed to get sound status: startTransmitting', context: { error: expect.any(Error) }, }); });src/components/staffing/staffing-bottom-sheet.tsx (1)
255-288: Use ternary for conditional rendering in the add-note block.Project guidance prefers
? :over&&for conditional rendering; this line was modified, so please align with the standard. As per coding guidelines, use ternary for conditional rendering.♻️ Proposed change
-{currentStep === 'add-note' && ( +{currentStep === 'add-note' ? ( <KeyboardAwareScrollView keyboardShouldPersistTaps={Platform.OS === 'android' ? 'handled' : 'always'} showsVerticalScrollIndicator={false} bottomOffset={20} style={{ flexGrow: 0, width: '100%' }} > {/* ...existing content... */} </KeyboardAwareScrollView> -)} +) : null}src/components/calls/call-images-modal.tsx (1)
459-459: Avoid inline onPress handler inside renderItem.Line 459 creates a new closure per render; prefer a memoized handler/factory so list items remain stable.
♻️ Suggested refactor
const handleImagePress = useCallback((imageSource: { uri: string }, itemName?: string) => { setFullScreenImage({ source: imageSource, name: itemName }); }, []); + +const getImagePressHandler = useCallback( + (imageSource: { uri: string }, itemName?: string) => () => handleImagePress(imageSource, itemName), + [handleImagePress] +);- <TouchableOpacity onPress={() => handleImagePress(imageSource, item.Name)} testID={`image-${item.Id}-touchable`} activeOpacity={0.7} style={{ width: '100%' }} delayPressIn={0} delayPressOut={0}> + <TouchableOpacity onPress={getImagePressHandler(imageSource, item.Name)} testID={`image-${item.Id}-touchable`} activeOpacity={0.7} style={{ width: '100%' }} delayPressIn={0} delayPressOut={0}>Based on learnings, please avoid anonymous handlers in renderItem.
scripts/patch-rnmapbox-complete.pl (1)
12-18: Create a.originalbackup when missing to keep rollback safe.Right now the script only restores if the backup already exists. Creating it on first run makes patches safer and idempotent.
♻️ Suggested refactor
# Backup (restore from original if exists) if (-f "$file.original") { open(my $orig, '<', "$file.original") or die "Cannot open original: $!"; `@lines` = <$orig>; close($orig); +} else { + open(my $orig, '>', "$file.original") or die "Cannot create original: $!"; + print $orig `@lines`; + close($orig); }scripts/patch-rnmapbox.sh (1)
19-29: Range-based sed can stop at the first nested}and leave functions partially uncommented.If any targeted setter has nested blocks, the range
/fun .../,/^\s*}$/will terminate early. Please verify these functions are single-block or switch to a brace-counting patcher (awk/perl) to avoid partial edits.scripts/patch-rnmapbox.py (1)
7-8: Missing error handling for file operations.If the target file doesn't exist (e.g.,
node_modulesnot installed), the script will crash with an unhandled exception. Consider adding a check or try/except block with a helpful error message.Suggested improvement
+import os + +if not os.path.exists(file_path): + print(f"Error: File not found: {file_path}") + print("Make sure to run 'npm install' first.") + exit(1) + # Read the file with open(file_path, 'r') as f: content = f.read()scripts/patch-rnmapbox-full.pl (1)
41-44: Confusing push-then-replace pattern.The code pushes
$lineto@output, then modifies$line, then overwrites the last element. This works but is confusing. A cleaner approach would be to modify$linefirst, then push.Suggested improvement
if ($line =~ /^\s+$func\(/ && $line !~ /fun\s+$func/) { # This is a function call, comment it and any associated logger line - push `@output`, $line; $line =~ s/^(\s*)(.+)$/$1\/\/ $2/; - $output[-1] = $line; + push `@output`, $line; $is_call = 1; last; }scripts/patch-mapbox.js (1)
1-54: Consider consolidating duplicate patch scripts.This repository now contains multiple patch scripts targeting the same file (
RNMBXStyleFactory.kt) with similar logic:
patch-mapbox.jspatch-mapbox-final.jspatch-rnmapbox.pypatch-rnmapbox.plpatch-rnmapbox-full.plConsider consolidating into a single, well-tested script to reduce maintenance burden and avoid inconsistencies in the deprecated function lists.
scripts/patch-rnmapbox.pl (1)
12-15: Inconsistent backup file extension.This script uses
.backupextension, while other scripts use.originalor.bak. This inconsistency could cause issues if scripts are run in different orders (one script's backup might not be found by another).Consider standardizing on a single backup extension across all patch scripts (e.g.,
.original).src/app/call/[id]/edit.tsx (2)
206-216: Remove debug console.log statements before merging.Multiple
console.logcalls throughout the file expose internal state and data structures. These should be removed or replaced with the project'sloggerutility (used in index.tsx) for appropriate log levels.♻️ Suggested cleanup
- console.log('Raw call data:', JSON.stringify(call, null, 2)); - console.log('Loading call data:', { Priority: call.Priority, Type: call.Type }); - console.log('Available call types:', callTypes); - console.log('Available call priorities:', callPriorities); - const priority = callPriorities.find((p) => p.Id === call.Priority); const type = callTypes.find((t) => t.Id === call.Type); - - console.log('Found priority:', priority); - console.log('Found type:', type);Similar cleanup needed at lines 289-312, 361, and 873-889.
147-147:getValuesis only used for debug logging.Once the debug
console.logstatements are removed,getValueswill be unused and should be removed from the destructuring.src/app/call/[id]/index.tsx (1)
580-585: Consider extracting repeated header configuration.The same
headerLeftandheaderRightconfiguration is repeated 5 times. Consider extracting to a shared constant or function to reduce duplication.♻️ Optional refactor
const getHeaderOptions = useCallback(() => ({ title: t('call_detail.title'), headerShown: true, headerLeft: () => ( <Pressable onPress={handleBack} className="p-2" testID="back-button"> <ArrowLeftIcon size={24} className="text-gray-700 dark:text-gray-300" /> </Pressable> ), headerRight: () => <HeaderRightMenu />, }), [t, handleBack, HeaderRightMenu]); // Then use: <Stack.Screen options={getHeaderOptions()} />
| priority: z.string().optional(), | ||
| type: z.string().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for new/create call forms and their schema definitions
fd -e tsx -e ts | xargs grep -l "new.*call\|create.*call" | head -20Repository: Resgrid/Responder
Length of output: 918
🏁 Script executed:
# Search more specifically for the edit and new/create forms
fd "edit\\.tsx|new\\.tsx|create\\.tsx" src/app/callRepository: Resgrid/Responder
Length of output: 87
🏁 Script executed:
# Check the schema definition in the edit form more thoroughly
cat -n src/app/call/\[id\]/edit.tsx | head -80Repository: Resgrid/Responder
Length of output: 3915
🏁 Script executed:
# Look for API definitions related to calls
fd -e ts -e tsx src/api -type f | xargs ls -lhRepository: Resgrid/Responder
Length of output: 2245
🏁 Script executed:
# Search for priority and type field definitions in API
rg "priority|Priority" src/api -A 3 -B 1 -g "*.ts" | head -60Repository: Resgrid/Responder
Length of output: 1249
🏁 Script executed:
# Check the new call form schema
cat -n src/app/call/new/index.tsx | head -100Repository: Resgrid/Responder
Length of output: 4595
🏁 Script executed:
# Look for the form submission and data transformation in edit.tsx
cat -n src/app/call/\[id\]/edit.tsx | grep -A 20 "onSubmit\|handleSubmit\|axios\|api.calls"Repository: Resgrid/Responder
Length of output: 5304
🏁 Script executed:
# Get the full API calls file to see the request structure
cat -n src/api/calls/calls.tsRepository: Resgrid/Responder
Length of output: 6658
Priority field must be required in edit form to match API contract and new form behavior.
The edit form makes priority optional (line 46), but the UpdateCallRequest API interface requires priority: number (src/api/calls/calls.ts line 62). Additionally, the new call form requires both priority and type (src/app/call/new/index.tsx lines 61-62), creating inconsistency. The form submission logic allows priority to be null (line 308), which will fail when sent to the API. Change priority: z.string().optional() to priority: z.string().min(1, 'Priority is required') to enforce the API contract and maintain consistency with the create flow.
🤖 Prompt for AI Agents
In `@src/app/call/`[id]/edit.tsx around lines 46 - 47, The edit form's Zod schema
currently makes priority optional; change the schema's priority from optional to
required using z.string().min(1, 'Priority is required') in the edit form
definition to match the UpdateCallRequest API contract and the new-call form;
also update the form submission handler (the onSubmit/handleSubmit logic that
currently allows priority to be null) to always coerce/parse priority to a
number before sending to UpdateCallRequest so null/empty values are never sent.
| // Helper function to strip HTML tags from rich text content | ||
| const stripHtml = (html: string | undefined | null): string => { | ||
| if (!html) return ''; | ||
| // Remove HTML tags | ||
| let text = html.replace(/<[^>]*>/g, ''); | ||
| // Decode common HTML entities | ||
| text = text | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, "'") | ||
| .replace(/'/g, "'"); | ||
| // Trim whitespace | ||
| return text.trim(); | ||
| }; |
There was a problem hiding this comment.
Incomplete HTML sanitization - potential XSS/injection vulnerability.
The stripHtml function has security issues flagged by static analysis:
- Incomplete tag removal: The regex
/<[^>]*>/gdoesn't handle malformed tags like<scriptwithout closing>, leaving partial tags that could enable injection. - Double-unescaping risk: Sequential entity replacements can produce new entities (e.g.,
&lt;→<→<).
Consider using a proper HTML sanitization library like DOMPurify or at minimum, iterate the entity decoding until stable.
🔒 Safer implementation
-// Helper function to strip HTML tags from rich text content
-const stripHtml = (html: string | undefined | null): string => {
- if (!html) return '';
- // Remove HTML tags
- let text = html.replace(/<[^>]*>/g, '');
- // Decode common HTML entities
- text = text
- .replace(/ /g, ' ')
- .replace(/&/g, '&')
- .replace(/</g, '<')
- .replace(/>/g, '>')
- .replace(/"/g, '"')
- .replace(/'/g, "'")
- .replace(/'/g, "'");
- // Trim whitespace
- return text.trim();
-};
+// Helper function to strip HTML tags from rich text content
+const stripHtml = (html: string | undefined | null): string => {
+ if (!html) return '';
+ // Remove all HTML tags including malformed ones
+ let text = html.replace(/<[^>]*>?/g, '');
+ // Decode HTML entities - loop until stable to handle double-encoding
+ let prev = '';
+ while (prev !== text) {
+ prev = text;
+ text = text
+ .replace(/ /g, ' ')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, "'")
+ .replace(/'/g, "'")
+ .replace(/&/g, '&'); // & must be last to avoid double-decode
+ }
+ return text.trim();
+};Alternatively, consider using a library like html-entities for robust decoding:
import { decode } from 'html-entities';
const stripHtml = (html: string | undefined | null): string => {
if (!html) return '';
return decode(html.replace(/<[^>]*>?/g, '')).trim();
};🧰 Tools
🤖 Prompt for AI Agents
In `@src/app/call/`[id]/edit.tsx around lines 61 - 77, The stripHtml function is
unsafe: replace the custom regex + sequential replacements with a proven
sanitizer and robust decoder—update stripHtml to first sanitize the input using
a library like DOMPurify (e.g., DOMPurify.sanitize(html, {ALLOWED_TAGS: []}) or
similar) to eliminate malformed tags and script content, then decode HTML
entities using a reliable decoder such as html-entities' decode (or, if you
cannot add a dependency, iteratively decode until stable and use a safer
tag-stripping regex like /<[^>]*>?/g); ensure you reference and modify the
stripHtml function to use DOMPurify and decode so malformed tags and
double-escaped entities are handled securely.
| <Button | ||
| className="ml-10 flex-1" | ||
| variant="solid" | ||
| action="primary" | ||
| onPress={async () => { | ||
| console.log('Save button pressed'); | ||
| console.log('Form errors:', errors); | ||
| console.log('Current form values:', getValues()); | ||
| try { | ||
| console.log('Calling handleSubmit...'); | ||
| await handleSubmit( | ||
| (data) => { | ||
| console.log('Validation passed, calling onSubmit'); | ||
| onSubmit(data); | ||
| }, | ||
| (errors) => { | ||
| console.log('Validation failed with errors:', errors); | ||
| } | ||
| )(); | ||
| console.log('handleSubmit completed'); | ||
| } catch (error) { | ||
| console.error('Error in handleSubmit:', error); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Missing await on onSubmit call and overly complex inline handler.
The onSubmit(data) call on line 881 is not awaited, so any async errors won't be caught by the outer try/catch block. Additionally, per coding guidelines, avoid anonymous functions in event handlers.
🐛 Proposed fix
onPress={async () => {
- console.log('Save button pressed');
- console.log('Form errors:', errors);
- console.log('Current form values:', getValues());
try {
- console.log('Calling handleSubmit...');
await handleSubmit(
- (data) => {
- console.log('Validation passed, calling onSubmit');
- onSubmit(data);
- },
- (errors) => {
- console.log('Validation failed with errors:', errors);
- }
+ async (data) => {
+ await onSubmit(data);
+ }
)();
- console.log('handleSubmit completed');
} catch (error) {
console.error('Error in handleSubmit:', error);
}
}}Or extract to a named callback:
const handleSave = useCallback(async () => {
await handleSubmit(onSubmit)();
}, [handleSubmit, onSubmit]);🤖 Prompt for AI Agents
In `@src/app/call/`[id]/edit.tsx around lines 868 - 891, The inline anonymous
Button onPress handler is overcomplicated and fails to await async errors
because onSubmit(data) isn't awaited; extract the logic into a named async
callback (e.g., handleSave) using useCallback, call await
handleSubmit(onSubmit)() or await handleSubmit(async (data) => await
onSubmit(data))() inside that callback, replace the inline onPress with
onPress={handleSave}, and remove the console-heavy inline diagnostics (you can
keep minimal logging inside handleSave if needed); ensure handleSave includes
try/catch so errors from onSubmit are caught.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/calls/calls.ts (1)
152-165: PlusCode is missing from the updateCall payload (data loss risk).The
UpdateCallRequestinterface includesplusCode, and thecreateCallfunction correctly includesPlusCode: callData.plusCode || ''in its payload. However,updateCallomits this field entirely. When users edit a call and change the PlusCode, it will be silently dropped from the request to the backend, causing data loss.Fix
const data = { Id: callData.callId, Name: callData.name, Nature: callData.nature, Note: callData.note || '', Address: callData.address || '', Geolocation: `${callData.latitude?.toString() || ''},${callData.longitude?.toString() || ''}`, Priority: callData.priority, Type: callData.type || '', ContactName: callData.contactName || '', ContactInfo: callData.contactInfo || '', What3Words: callData.what3words || '', + PlusCode: callData.plusCode || '', DispatchList: dispatchList, };src/app/call/[id]/edit.tsx (1)
716-741: Align form validation schema with new call form.The edit form has priority and type as optional fields, but the new call form requires them. Either:
- Update the validation schema for priority and type to use
.min(1)(matching the new form), or- If these fields should remain optional during edits, the error rendering is correctly omitted.
Currently, the schema defines priority and type as
.optional(), so the suggested error UI would never trigger. To add validation feedback like the new form, the schema must change first to make these fields required.
🤖 Fix all issues with AI agents
In `@scripts/patch-mapbox-final.js`:
- Around line 35-41: The current deprecated-case check uses a regex that only
matches when `->` is at end-of-line, so change the test in the loop that uses
line.match(new RegExp(`"${prop}"\\s*->\\s*$`)) to a regex that also matches `->`
followed by inline code or `{` (e.g. new
RegExp(`"${prop}"\\s*->\\s*(?:$|\\{|[^\\n]+)`)), so `"prop" -> {` and `"prop" ->
setX(...)` are detected; keep the existing
result.push(line.replace(/^(\s*)(.*)$/, '$1// $2 // Deprecated in Mapbox SDK
11.8+')) behavior after the match. Ensure you update the regex only in the block
referencing deprecatedProps, isDeprecatedCase, and line.match to avoid affecting
unrelated logic.
In `@src/app/call/`[id]/edit.tsx:
- Around line 206-216: Remove or guard the verbose console.log statements that
print full call payloads (the logs referencing `call`, `callTypes`,
`callPriorities`, and the found `priority`/`type`) to avoid leaking PII; either
delete them or wrap them with a development-only guard (e.g., `__DEV__`) and
redact sensitive fields (contact info, identifiers) before logging. Update all
occurrences in this file (the logs around the block that finds `priority` and
`type` and the additional logs noted at 290-363) so only non-PII or sanitized
summaries are emitted in development, and ensure no raw `call` payload is logged
in production.
- Around line 294-350: The code is coercing missing coordinates to 0 by using
truthy checks and defaulting latitude/longitude to 0 in updatePayload; change
the selectedLocation checks to nullish checks (e.g., test
selectedLocation?.latitude != null and selectedLocation?.longitude != null) so
valid 0 is preserved, stop defaulting data.latitude/data.longitude to 0 in the
updatePayload (leave them undefined or omit the fields when absent), and update
the analytics hasCoordinates check to use nullish checks (e.g., data.latitude !=
null && data.longitude != null) so it correctly treats 0 as a real coordinate;
adjust references: selectedLocation, data.latitude, data.longitude,
hasCoordinates, and updatePayload.
♻️ Duplicate comments (4)
scripts/patch-rnmapbox-sed.sh (1)
26-39: Brace stop condition can leave dangling braces in Kotlinwhencases.
Stopping on any}can end commenting inside nested blocks, leaving stray code or unmatched braces once the case label is commented. Track indent (or brace depth) so only thewhen’s closing brace stops the comment pass.🔧 Suggested fix (indent-aware stop)
-perl -i -pe ' -BEGIN { $commenting = 0; } -if (/\/\/ "(?:fillPatternCrossFade|lineElevationReference|lineCrossSlope|linePatternCrossFade|circleElevationReference|fillExtrusionPatternCrossFade|fillExtrusionHeightAlignment|fillExtrusionBaseAlignment|backgroundPitchAlignment)"/) { - $commenting = 1; # Start commenting following lines - next; -} -if ($commenting) { - # Stop commenting if we hit next case/else or closing brace at appropriate indent - if (/^\s*"[^"]+"\s*->/ || /^\s*else\s*->/ || /^\s*\}/) { - $commenting = 0; - } elsif (/\S/ && !/^\s*\/\//) { - # Comment non-empty lines that aren'\''t already commented - s/^(\s*)(\S.*)$/$1\/\/ $2/; - } -} -' "$FILE" +perl -i -pe ' +BEGIN { $commenting = 0; $case_indent = 0; } +if (/^(\s*)\/\/ "(?:fillPatternCrossFade|lineElevationReference|lineCrossSlope|linePatternCrossFade|circleElevationReference|fillExtrusionPatternCrossFade|fillExtrusionHeightAlignment|fillExtrusionBaseAlignment|backgroundPitchAlignment)"\s*->/) { + $commenting = 1; # Start commenting following lines + $case_indent = length($1); + next; +} +if ($commenting) { + # Stop commenting if we hit next case/else or the when's closing brace (same indent) + if (/^\s*"[^"]+"\s*->/ || /^\s*else\s*->/ || (/^(\s*)\}/ && length($1) <= $case_indent)) { + $commenting = 0; + } elsif (/\S/ && !/^\s*\/\//) { + # Comment non-empty lines that aren'\''t already commented + s/^(\s*)(\S.*)$/$1\/\/ $2/; + } +} +' "$FILE"src/app/call/[id]/edit.tsx (3)
46-47: Priority should remain required to meet the API contract.Line 46 makes
priorityoptional, butUpdateCallRequestrequires a number and the submit logic falls back to0, which can be invalid. Please enforce priority as required.✅ Suggested change
- priority: z.string().optional(), + priority: z.string().min(1, 'Priority is required'),
61-77:stripHtmlis unsafe; use a robust sanitizer/decoder.Line 61-77 uses regex + sequential replacements that don’t handle malformed tags or double-encoded entities (CodeQL flags). This can still leak unsafe markup into the UI. Consider a safer regex and iterative decode (or a sanitizer like DOMPurify).
🔒 Safer in-place alternative
- // Remove HTML tags - let text = html.replace(/<[^>]*>/g, ''); - // Decode common HTML entities - text = text - .replace(/ /g, ' ') - .replace(/&/g, '&') - .replace(/</g, '<') - .replace(/>/g, '>') - .replace(/"/g, '"') - .replace(/'/g, "'") - .replace(/'/g, "'"); + // Remove HTML tags (including malformed ones) + let text = html.replace(/<[^>]*>?/g, ''); + // Decode entities until stable to avoid double-encoding issues + let prev = ''; + while (prev !== text) { + prev = text; + text = text + .replace(/ /g, ' ') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/'/g, "'") + .replace(/&/g, '&'); + }Also applies to: 255-258
871-895: Extract the save handler and awaitonSubmit.The inline handler doesn’t await
onSubmit, so async errors may bypass the try/catch, and it violates the “no anonymous handlers” guidance. Use a memoizedhandleSaveandawait handleSubmit(onSubmit)(). Based on learnings, avoid anonymous functions in event handlers.♻️ Suggested change (onPress)
- onPress={async () => { - console.log('Save button pressed'); - console.log('Form errors:', errors); - console.log('Current form values:', getValues()); - try { - console.log('Calling handleSubmit...'); - await handleSubmit( - (data) => { - console.log('Validation passed, calling onSubmit'); - onSubmit(data); - }, - (errors) => { - console.log('Validation failed with errors:', errors); - } - )(); - console.log('handleSubmit completed'); - } catch (error) { - console.error('Error in handleSubmit:', error); - } - }} + onPress={handleSave}Add a memoized handler near
onSubmit:const handleSave = useCallback(async () => { try { await handleSubmit(onSubmit)(); } catch (error) { console.error('Error in handleSubmit:', error); } }, [handleSubmit, onSubmit]);
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.