Conversation
Contributor
📝 WalkthroughWalkthroughAdds three new web components for Call Detail, Edit Call, and New Call plus a small loading animation change and a runtime dependency addition. Features include maps, geocoding, zod/react-hook-form forms, dispatch selection modals, keyboard shortcuts, and responsive two-column layouts. (49 words) Changes
Sequence DiagramssequenceDiagram
participant User
participant FormUI as "New/Edit Call UI"
participant Validator as "Zod Validation"
participant Store as "Store / Loader"
participant API as "Backend API"
participant Toast as "Toast Service"
participant Nav as "Navigation"
User->>FormUI: Fill fields and submit (Ctrl/Cmd+Enter or Ctrl/Cmd+S)
FormUI->>Validator: Validate form
alt valid
FormUI->>Store: Ensure priorities/types loaded
Store-->>FormUI: priorities/types
FormUI->>API: createCall / updateCall (payload)
alt API success
API-->>FormUI: success response
FormUI->>Toast: show success
FormUI->>Nav: redirect to calls list / detail
else API error
API-->>FormUI: error
FormUI->>Toast: show error
end
else invalid
Validator-->>FormUI: validation errors
FormUI->>Toast: display validation errors
end
sequenceDiagram
participant User
participant DetailUI as "Call Detail UI"
participant Store as "Store / Loader"
participant Maps as "Map Service (StaticMap / External)"
participant Modal as "Modal Service"
User->>DetailUI: Open call detail page
DetailUI->>Store: fetch call data
Store-->>DetailUI: call data + metadata
DetailUI->>DetailUI: render tabs & map
User->>DetailUI: request directions / open map
DetailUI->>Maps: render StaticMap / open external maps link
Maps-->>DetailUI: map rendered / link opened
User->>DetailUI: open notes/images/files
DetailUI->>Modal: open corresponding modal
Modal-->>DetailUI: modal result (close/update)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/app/call/`[id].web.tsx:
- Around line 226-228: The code renders user-controlled HTML via
dangerouslySetInnerHTML using call.Note in three places (the div inside the Note
View and the other two occurrences), creating an XSS risk; fix by installing and
importing DOMPurify, sanitize call.Note (e.g., const safeNote =
DOMPurify.sanitize(call.Note || '<em>No notes</em>')) before passing it to
dangerouslySetInnerHTML (or alternatively render as plain text if HTML is not
required), and apply the same sanitization for the other two usages to ensure
all call.Note outputs are cleaned (update imports and replace raw call.Note with
the sanitized variable wherever dangerouslySetInnerHTML is used).
- Around line 128-147: The keyboard shortcut useEffect registers handleKeyDown
which closes over handleEditCall, router, setActiveTab and TabKey but has an
empty dependency array causing stale closures; update the effect to include its
dependencies (at minimum handleEditCall, router, setActiveTab) and ensure
handleEditCall is memoized with useCallback (so it updates when callId or
routing context changes) and then reference the memoized handleEditCall inside
useEffect so Ctrl+E navigates to the correct edit page and the tab number
handling via setActiveTab remains fresh.
In `@src/app/call/`[id]/edit.web.tsx:
- Around line 296-318: The useEffect that registers keyboard shortcuts
references handleSubmit and onSubmit but doesn't declare them as dependencies;
wrap your onSubmit handler in useCallback (including any state/props it uses)
and then add both onSubmit and handleSubmit to the useEffect dependency array so
the effect updates when the submit handler changes; ensure the existing state
setters (setShowLocationPicker, setShowAddressSelection, setShowDispatchModal)
and router usage remain correct and that handleKeyDown is referencing the
memoized onSubmit inside the effect.
- Around line 332-333: The code is using a non-null assertion on callId when
invoking useCallDetailStore.getState().updateCall (callId!), which can be
undefined because id from useLocalSearchParams may be missing; add a guard
clause that checks the callId (or id) is present before calling updateCall and
bail out (return or show error) if it’s undefined so you never call updateCall
with an undefined callId; locate the call to
useCallDetailStore.getState().updateCall and the variable that holds the id
(from useLocalSearchParams) and wrap the updateCall invocation in an if
(!callId) { /* handle missing id */ } block.
In `@src/app/call/new/index.web.tsx`:
- Around line 282-306: The keyboard shortcut effect is missing dependencies
which can cause stale closures; update the dependency array for the useEffect
that defines handleKeyDown to include handleSubmit and onSubmit (and router if
router.back is used inside) in addition to showLocationPicker,
showAddressSelection, and showDispatchModal so the effect re-registers when
those handlers change; locate the useEffect block in index.web.tsx that
references handleKeyDown/handleSubmit/onSubmit/router.back and add handleSubmit,
onSubmit (and router) to the dependencies.
🧹 Nitpick comments (6)
src/components/common/loading.tsx (1)
53-56: Consider restoring staggered animation for the loading dots.Removing the
animationDelaystyle causes all three dots to pulse simultaneously rather than in a wave pattern. Staggered animation typically provides better visual feedback that something is loading.💡 Suggested staggered animation
{[1, 2, 3].map((i) => ( - <Box key={i} className={`bg-primary rounded-full ${size === 'small' ? 'size-2' : size === 'large' ? 'size-3' : 'size-4'} animate-pulse`} /> + <Box + key={i} + className={`bg-primary rounded-full ${size === 'small' ? 'size-2' : size === 'large' ? 'size-3' : 'size-4'} animate-pulse`} + style={{ animationDelay: `${(i - 1) * 150}ms` }} + /> ))}src/app/call/[id].web.tsx (2)
107-116: Add validation for parsed coordinates.
parseFloatreturnsNaNfor malformed input, and theGeolocationstring split doesn't trim whitespace. Invalid coordinates could cause map rendering issues.🛡️ Defensive parsing
useEffect(() => { if (call) { if (call.Latitude && call.Longitude) { - setCoordinates({ latitude: parseFloat(call.Latitude), longitude: parseFloat(call.Longitude) }); + const lat = parseFloat(call.Latitude); + const lng = parseFloat(call.Longitude); + if (!isNaN(lat) && !isNaN(lng) && lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180) { + setCoordinates({ latitude: lat, longitude: lng }); + } } else if (call.Geolocation) { - const [lat, lng] = call.Geolocation.split(','); - setCoordinates({ latitude: parseFloat(lat), longitude: parseFloat(lng) }); + const parts = call.Geolocation.split(',').map(s => s.trim()); + if (parts.length === 2) { + const lat = parseFloat(parts[0]); + const lng = parseFloat(parts[1]); + if (!isNaN(lat) && !isNaN(lng) && lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180) { + setCoordinates({ latitude: lat, longitude: lng }); + } + } } } }, [call]);
42-42: Unusedheightvariable.
heightis destructured fromuseWindowDimensions()but never used.-const { width, height } = useWindowDimensions(); +const { width } = useWindowDimensions();src/app/call/[id]/edit.web.tsx (1)
63-131: Extract sharedWebInputcomponent to avoid duplication.
WebInputis nearly identical inedit.web.tsxandnew/index.web.tsx. Extract it to a shared component file to improve maintainability.Consider creating
src/components/common/web-input.tsx:// src/components/common/web-input.tsx export const WebInput: React.FC<WebInputProps> = ({ ... }) => { ... }; export const WebSelect: React.FC<WebSelectProps> = ({ ... }) => { ... };Then import in both files:
import { WebInput, WebSelect } from '@/components/common/web-input';src/app/call/new/index.web.tsx (2)
228-228: Remove unusednameInputRef.
nameInputRefis declared but never used in the component.-const nameInputRef = useRef<HTMLInputElement>(null);
18-23: Remove unused imports.
Button,ButtonText,HStack, andVStackare imported but not used in the component.-import { Button, ButtonText } from '@/components/ui/button'; import { Card } from '@/components/ui/card'; import { FocusAwareStatusBar } from '@/components/ui/focus-aware-status-bar'; -import { HStack } from '@/components/ui/hstack'; import { Text } from '@/components/ui/text'; -import { VStack } from '@/components/ui/vstack';
Comment on lines
+296
to
+318
| // Keyboard shortcuts | ||
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if ((e.ctrlKey || e.metaKey) && e.key === 's') { | ||
| e.preventDefault(); | ||
| handleSubmit(onSubmit)(); | ||
| } | ||
| if (e.key === 'Escape') { | ||
| if (showLocationPicker) { | ||
| setShowLocationPicker(false); | ||
| } else if (showAddressSelection) { | ||
| setShowAddressSelection(false); | ||
| } else if (showDispatchModal) { | ||
| setShowDispatchModal(false); | ||
| } else { | ||
| router.back(); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('keydown', handleKeyDown); | ||
| return () => window.removeEventListener('keydown', handleKeyDown); | ||
| }, [showLocationPicker, showAddressSelection, showDispatchModal]); |
Contributor
There was a problem hiding this comment.
Missing dependencies in keyboard shortcut useEffect.
handleSubmit and onSubmit are used in the effect but not listed as dependencies. While react-hook-form's handleSubmit is typically stable, onSubmit captures local state.
🔧 Add missing dependencies
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if ((e.ctrlKey || e.metaKey) && e.key === 's') {
e.preventDefault();
handleSubmit(onSubmit)();
}
// ... rest of handler
};
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
- }, [showLocationPicker, showAddressSelection, showDispatchModal]);
+ }, [showLocationPicker, showAddressSelection, showDispatchModal, handleSubmit, onSubmit]);Wrap onSubmit in useCallback with appropriate dependencies.
📝 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.
Suggested change
| // Keyboard shortcuts | |
| useEffect(() => { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| if ((e.ctrlKey || e.metaKey) && e.key === 's') { | |
| e.preventDefault(); | |
| handleSubmit(onSubmit)(); | |
| } | |
| if (e.key === 'Escape') { | |
| if (showLocationPicker) { | |
| setShowLocationPicker(false); | |
| } else if (showAddressSelection) { | |
| setShowAddressSelection(false); | |
| } else if (showDispatchModal) { | |
| setShowDispatchModal(false); | |
| } else { | |
| router.back(); | |
| } | |
| } | |
| }; | |
| window.addEventListener('keydown', handleKeyDown); | |
| return () => window.removeEventListener('keydown', handleKeyDown); | |
| }, [showLocationPicker, showAddressSelection, showDispatchModal]); | |
| // Keyboard shortcuts | |
| useEffect(() => { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| if ((e.ctrlKey || e.metaKey) && e.key === 's') { | |
| e.preventDefault(); | |
| handleSubmit(onSubmit)(); | |
| } | |
| if (e.key === 'Escape') { | |
| if (showLocationPicker) { | |
| setShowLocationPicker(false); | |
| } else if (showAddressSelection) { | |
| setShowAddressSelection(false); | |
| } else if (showDispatchModal) { | |
| setShowDispatchModal(false); | |
| } else { | |
| router.back(); | |
| } | |
| } | |
| }; | |
| window.addEventListener('keydown', handleKeyDown); | |
| return () => window.removeEventListener('keydown', handleKeyDown); | |
| }, [showLocationPicker, showAddressSelection, showDispatchModal, handleSubmit, onSubmit]); |
🤖 Prompt for AI Agents
In `@src/app/call/`[id]/edit.web.tsx around lines 296 - 318, The useEffect that
registers keyboard shortcuts references handleSubmit and onSubmit but doesn't
declare them as dependencies; wrap your onSubmit handler in useCallback
(including any state/props it uses) and then add both onSubmit and handleSubmit
to the useEffect dependency array so the effect updates when the submit handler
changes; ensure the existing state setters (setShowLocationPicker,
setShowAddressSelection, setShowDispatchModal) and router usage remain correct
and that handleKeyDown is referencing the memoized onSubmit inside the effect.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 114: Remove the deprecated `@types/dompurify` entry from devDependencies in
package.json and rely on the built-in TypeScript types shipped by the dompurify
package (keep "dompurify": "^3.3.1"); after removing the `@types` package, run
your package manager install to update lockfiles, and adjust any imports/type
references (if present) to import types directly from "dompurify" (e.g., replace
any use of `@types/dompurify` with DOMPurify types from the dompurify package).
In `@src/app/call/new/index.web.tsx`:
- Around line 282-308: The onSubmit function is referenced in the useEffect
dependency array before it is declared; make onSubmit a stable callback and
define it before the useEffect: convert the current const onSubmit = async
(data: FormValues) => { ... } into a useCallback hook (e.g., const onSubmit =
useCallback(async (data: FormValues) => { ... }, [/* include real dependencies
used inside onSubmit */])) and move that declaration above the useEffect that
registers handleKeyDown; then update the useEffect dependency array to include
this new onSubmit callback (and ensure handleSubmit is still included). Also
verify any values used inside onSubmit are listed in its dependency array to
avoid stale closures.
♻️ Duplicate comments (4)
src/app/call/[id].web.tsx (2)
228-229: XSS protection properly implemented with DOMPurify.The previous review flagged XSS vulnerabilities with
dangerouslySetInnerHTML. This has been properly addressed by usingDOMPurify.sanitize()for all HTML content rendering (call.Note,protocol.ProtocolText,call.Nature).Also applies to: 251-251, 339-339
129-148: Keyboard shortcut dependencies properly fixed.The previous review flagged missing dependencies in the keyboard shortcut
useEffect. This has been addressed by:
- Wrapping
handleEditCallinuseCallback(line 78)- Including
handleEditCall,router, andsetActiveTabin the dependency arraysrc/app/call/[id]/edit.web.tsx (2)
320-331: Guard clause for missing callId properly implemented.The previous review flagged the non-null assertion on
callId. This has been addressed with a proper guard clause that shows an error toast and returns early ifcallIdis undefined.
296-318: Keyboard shortcutuseEffectstill missing some dependencies.The previous review flagged that
handleSubmitandonSubmitshould be in the dependency array. While the modal state dependencies were added,handleSubmitandonSubmitare still missing.Since
onSubmitcaptures form state through thedataparameter and uses external state likeselectedLocation, this could lead to stale closures in edge cases.♻️ Suggested fix
+ const onSubmit = useCallback(async (data: FormValues) => { + // ... existing implementation + }, [callId, callPriorities, callTypes, selectedLocation, toast, t, router]); // Keyboard shortcuts useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if ((e.ctrlKey || e.metaKey) && e.key === 's') { e.preventDefault(); handleSubmit(onSubmit)(); } // ... }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); - }, [showLocationPicker, showAddressSelection, showDispatchModal]); + }, [showLocationPicker, showAddressSelection, showDispatchModal, handleSubmit, onSubmit]);
🧹 Nitpick comments (3)
src/app/call/[id]/edit.web.tsx (1)
341-349: Consider handling unmatched priority/type more explicitly.When mapping priority/type names back to IDs, the code uses fallback values (
0for priority,''for type) if the lookup fails. This could result in submitting invalid data if the lookup fails unexpectedly (e.g., stale store data).💡 Optional improvement
const priority = callPriorities.find((p) => p.Name === data.priority); const type = callTypes.find((t) => t.Name === data.type); + if (!priority || !type) { + toast.show({ + placement: 'top', + render: () => ( + <Box className="rounded-lg bg-red-500 p-4 shadow-lg"> + <Text className="text-white">{t('call_detail.invalid_priority_or_type')}</Text> + </Box> + ), + }); + return; + } await useCallDetailStore.getState().updateCall({ callId: callId, // ... - priority: priority?.Id || 0, - type: type?.Id || '', + priority: priority.Id, + type: type.Id, // ... });src/app/call/new/index.web.tsx (2)
439-474: What3words search validation and transformation could be improved.The what3words validation (line 446) checks against lowercase, but if the check passes, the original
what3wordsvalue is sent to the API without transformation. This could fail if the user entered mixed case that happened to pass the test.💡 Minor improvement
const handleWhat3WordsSearch = async (what3words: string) => { if (!what3words.trim()) { toast.warning(t('calls.what3words_required')); return; } - const w3wRegex = /^[a-z]+\.[a-z]+\.[a-z]+$/; - if (!w3wRegex.test(what3words.trim().toLowerCase())) { + const normalizedW3w = what3words.trim().toLowerCase(); + const w3wRegex = /^[a-z]+\.[a-z]+\.[a-z]+$/; + if (!w3wRegex.test(normalizedW3w)) { toast.warning(t('calls.what3words_invalid_format')); return; } setIsGeocodingWhat3Words(true); try { const apiKey = config?.W3WKey; if (!apiKey) throw new Error('what3words API key not configured'); - const response = await axios.get<What3WordsResponse>(`https://api.what3words.com/v3/convert-to-coordinates?words=${encodeURIComponent(what3words)}&key=${apiKey}`); + const response = await axios.get<What3WordsResponse>(`https://api.what3words.com/v3/convert-to-coordinates?words=${encodeURIComponent(normalizedW3w)}&key=${apiKey}`);
89-195: Consider extracting shared WebInput and WebSelect components.The
WebInputandWebSelectcomponents are nearly identical to those inedit.web.tsx. Consider extracting these into a shared component file (e.g.,@/components/ui/web-input.tsx) to reduce duplication and ensure consistency.
Comment on lines
+282
to
+308
| // Keyboard shortcuts | ||
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| // Ctrl/Cmd + Enter to submit | ||
| if ((e.ctrlKey || e.metaKey) && e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| handleSubmit(onSubmit)(); | ||
| } | ||
| // Escape to cancel | ||
| if (e.key === 'Escape') { | ||
| if (showLocationPicker) { | ||
| setShowLocationPicker(false); | ||
| } else if (showAddressSelection) { | ||
| setShowAddressSelection(false); | ||
| } else if (showDispatchModal) { | ||
| setShowDispatchModal(false); | ||
| } else { | ||
| router.back(); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('keydown', handleKeyDown); | ||
| return () => window.removeEventListener('keydown', handleKeyDown); | ||
| }, [showLocationPicker, showAddressSelection, showDispatchModal, handleSubmit, onSubmit, router]); | ||
|
|
||
| const onSubmit = async (data: FormValues) => { |
Contributor
There was a problem hiding this comment.
Critical: onSubmit is referenced before declaration.
The static analysis correctly identifies that onSubmit is used in the dependency array (line 306) before it's declared (line 308). Function expressions assigned to const are not hoisted, so this will cause issues.
🐛 Proposed fix - wrap onSubmit in useCallback and move before useEffect
+ const onSubmit = useCallback(async (data: FormValues) => {
+ try {
+ setIsSubmitting(true);
+
+ if (selectedLocation?.latitude && selectedLocation?.longitude) {
+ data.latitude = selectedLocation.latitude;
+ data.longitude = selectedLocation.longitude;
+ }
+
+ const priority = callPriorities.find((p) => p.Name === data.priority);
+ const type = callTypes.find((t) => t.Name === data.type);
+
+ if (!priority) {
+ toast.error(t('calls.invalid_priority'));
+ return;
+ }
+
+ if (!type) {
+ toast.error(t('calls.invalid_type'));
+ return;
+ }
+
+ await createCall({
+ name: data.name,
+ nature: data.nature,
+ priority: priority.Id,
+ type: type.Id,
+ note: data.note,
+ address: data.address,
+ latitude: data.latitude,
+ longitude: data.longitude,
+ what3words: data.what3words,
+ plusCode: data.plusCode,
+ dispatchUsers: data.dispatchSelection?.users,
+ dispatchGroups: data.dispatchSelection?.groups,
+ dispatchRoles: data.dispatchSelection?.roles,
+ dispatchUnits: data.dispatchSelection?.units,
+ dispatchEveryone: data.dispatchSelection?.everyone,
+ });
+
+ toast.success(t('calls.create_success'));
+ router.push('/calls' as Href);
+ } catch (err) {
+ console.error('Error creating call:', err);
+ toast.error(t('calls.create_error'));
+ } finally {
+ setIsSubmitting(false);
+ }
+ }, [selectedLocation, callPriorities, callTypes, toast, t, router]);
// Keyboard shortcuts
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
// Ctrl/Cmd + Enter to submit
if ((e.ctrlKey || e.metaKey) && e.key === 'Enter') {
e.preventDefault();
handleSubmit(onSubmit)();
}
// ...
};
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
}, [showLocationPicker, showAddressSelection, showDispatchModal, handleSubmit, onSubmit, router]);
- const onSubmit = async (data: FormValues) => {
- // ... move this above the useEffect
- };🧰 Tools
🪛 Biome (2.1.2)
[error] 306-306: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🤖 Prompt for AI Agents
In `@src/app/call/new/index.web.tsx` around lines 282 - 308, The onSubmit function
is referenced in the useEffect dependency array before it is declared; make
onSubmit a stable callback and define it before the useEffect: convert the
current const onSubmit = async (data: FormValues) => { ... } into a useCallback
hook (e.g., const onSubmit = useCallback(async (data: FormValues) => { ... },
[/* include real dependencies used inside onSubmit */])) and move that
declaration above the useEffect that registers handleKeyDown; then update the
useEffect dependency array to include this new onSubmit callback (and ensure
handleSubmit is still included). Also verify any values used inside onSubmit are
listed in its dependency array to avoid stale closures.
Member
Author
|
Approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.