Skip to content

Implement System-Wide Concurrency and Performance Improvements #345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
google-labs-jules wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implement System-Wide Concurrency and Performance Improvements #345

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb3c708
feat: Implement system-wide concurrency and performance improvements
google-labs-jules[bot] Oct 20, 2025
fb825a1
fix: Resolve runtime TypeError and improve concurrency code robustness
google-labs-jules[bot] Oct 20, 2025
f039453
fix: Implement comprehensive concurrency, performance, and stability …
google-labs-jules[bot] Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
420 changes: 217 additions & 203 deletions app/actions.tsx
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion app/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,17 @@ import { MapDataProvider } from '@/components/map/map-data-context'

export default function Page() {
const id = nanoid()
const initialAIState = {
conversations: [
{
id: nanoid(),
chatId: id,
messages: []
}
]
}

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Type the initial AI state to prevent schema drift.

Strongly type this object so refactors don’t silently break runtime.

-  const initialAIState = {
+  const initialAIState = {
     conversations: [
       {
         id: nanoid(),
         chatId: id,
         messages: []
       }
     ]
-  }
+  } satisfies { conversations: Array<{ id: string; chatId: string; messages: any[] }> }

Also applies to: 21-21

🤖 Prompt for AI Agents 
In app/page.tsx around lines 11–19 (and also at line 21), the untyped
initialAIState object can suffer schema drift; declare and use a concrete
TypeScript type (e.g. import or define an AIState/InitialAIState interface
matching conversations: Array<{id: string; chatId: string; messages:
MessageType[]}> and any nested MessageType) and annotate the constant as that
type (const initialAIState: AIState = {...}); ensure any helper types are
imported/defined nearby and update the other occurrence at line 21 to use the
same typed constant so future refactors produce compile errors instead of silent
runtime breaks.

return (
<AI initialAIState={{ chatId: id, messages: [] }}>
<AI initialAIState={initialAIState}>
<MapDataProvider>
<Chat id={id} />
</MapDataProvider>
Expand Down
20 changes: 12 additions & 8 deletions app/search/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { nanoid } from 'nanoid';
import { notFound, redirect } from 'next/navigation';
import { Chat } from '@/components/chat';
import { getChat, getChatMessages } from '@/lib/actions/chat'; // Added getChatMessages
Expand Down Expand Up @@ -59,15 +60,18 @@ export default async function SearchPage({ params }: SearchPageProps) {
};
});

return (
<AI
initialAIState={{
const initialAIState = {
conversations: [
{
id: nanoid(),
chatId: chat.id,
messages: initialMessages, // Use the transformed messages from the database
// isSharePage: true, // This was in PR#533, but share functionality is removed.
// If needed for styling or other logic, it can be set.
}}
>
messages: initialMessages,
}
]
};

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Type the new conversation-based initialAIState to guard against regressions.

Enforce structure using satisfies (no runtime cost).

-  const initialAIState = {
+  const initialAIState = {
     conversations: [
       {
         id: nanoid(),
         chatId: chat.id,
         messages: initialMessages,
       }
     ]
-  };
+  } satisfies { conversations: Array<{ id: string; chatId: string; messages: AIMessage[] }> };

Also confirm dbMsg.role covers all AIMessage['role'] values (e.g., tool/system) or add a mapper/validator.

Also applies to: 73-79

🏁 Script executed:

#!/bin/bash
# Find AIMessage type definition and related types
rg -n "AIMessage|dbMsg" app/search/\[id\]/page.tsx -A 2 -B 2

Length of output: 989

🏁 Script executed:

#!/bin/bash
# Search for AIMessage type in the codebase
rg -n "type AIMessage|interface AIMessage" --type ts --type tsx -A 5

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Find initialMessages definition
rg -n "initialMessages" app/search/\[id\]/page.tsx -B 5 -A 2

Length of output: 464

🏁 Script executed:

#!/bin/bash
# View the context around lines 63-79 to understand both locations
sed -n '55,85p' app/search/\[id\]/page.tsx

Length of output: 704

🏁 Script executed:

#!/bin/bash
# Search for role mapping patterns
rg -n "\.role|AIMessage\['role'\]" app/search/\[id\]/page.tsx -B 2 -A 2

Length of output: 273

🏁 Script executed:

#!/bin/bash
# Find where dbMsg comes from
rg -n "dbMsg|const.*Msg" app/search/\[id\]/page.tsx -B 3 -A 1

Length of output: 558

🏁 Script executed:

#!/bin/bash
# Find AIMessage type definition
rg -n "type AIMessage|interface AIMessage" lib/types -A 10

Length of output: 529

🏁 Script executed:

#!/bin/bash
# Find DrizzleMessage type definition
rg -n "type.*Message|interface.*Message" lib/actions/chat-db -A 5

Length of output: 59

🏁 Script executed:

#!/bin/bash
# Get lines 70-85 to see the second location mentioned (73-79)
sed -n '70,85p' app/search/\[id\]/page.tsx

Length of output: 154

🏁 Script executed:

#!/bin/bash
# Check TypeScript version in package.json
cat package.json | grep -A 2 '"typescript"'

Length of output: 32

🏁 Script executed:

#!/bin/bash
# Search for Message type import in page.tsx
rg -n "Message|import" app/search/\[id\]/page.tsx | head -20

Length of output: 968

🏁 Script executed:

#!/bin/bash
# Find chat-db file with different paths
fd -n "chat-db" --type f

Length of output: 230

🏁 Script executed:

#!/bin/bash
# Search for DrizzleMessage type definition broadly
rg -n "type.*Message|interface.*Message|export.*Message" lib/actions -A 3

Length of output: 2907

🏁 Script executed:

#!/bin/bash
# Search entire page.tsx for all initialAIState occurrences
rg -n "initialAIState" app/search/\[id\]/page.tsx -B 2 -A 5

Length of output: 337

🏁 Script executed:

#!/bin/bash
# Get entire file line count and check around 73-79
wc -l app/search/\[id\]/page.tsx

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Find database schema definition for messages table
fd -type f -name "*.ts" -o -name "*.sql" | xargs rg -l "messages.*schema\|messages.*table\|role.*enum" | head -5

Length of output: 170

🏁 Script executed:

#!/bin/bash
# Search for messages table definition in schema files
rg -n "export const messages|messages = table\|messages: table" --type ts -A 15

Length of output: 2095

🏁 Script executed:

#!/bin/bash
# Find where role enum/type is defined
rg -n "role.*enum\|role:.*varchar\|role:.*text\|role.*values" --type ts -A 2

Length of output: 0

Type initialAIState and add role validation to prevent casting unsafe database values.

The database schema stores role as an unconstrained varchar(50), but the code unsafely casts it to AIMessage['role'] at line 52. Use satisfies to type initialAIState at lines 63–71:

-  const initialAIState = {
+  const initialAIState = {
     conversations: [
       {
         id: nanoid(),
         chatId: chat.id,
         messages: initialMessages,
       }
     ]
-  };
+  } satisfies { conversations: Array<{ id: string; chatId: string; messages: AIMessage[] }> };

Additionally, replace the unsafe cast at line 52 with a validator/mapper that ensures dbMsg.role is one of the allowed AIMessage['role'] values ('user' | 'assistant' | 'system' | 'function' | 'data' | 'tool'), rather than silently allowing any string from the database.

📝 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
const initialAIState = {
conversations: [
{
id: nanoid(),
chatId: chat.id,
messages: initialMessages, // Use the transformed messages from the database
// isSharePage: true, // This was in PR#533, but share functionality is removed.
// If needed for styling or other logic, it can be set.
}}
>
messages: initialMessages,
}
]
};
const initialAIState = {
conversations: [
{
id: nanoid(),
chatId: chat.id,
messages: initialMessages,
}
]
} satisfies { conversations: Array<{ id: string; chatId: string; messages: AIMessage[] }> };
🤖 Prompt for AI Agents 
In app/search/[id]/page.tsx around lines 63–71 and the earlier cast at line 52,
the initialAIState object is untyped and the code unsafely casts dbMsg.role to
AIMessage['role']; instead, annotate initialAIState using TypeScript's satisfies
operator so it conforms to the expected shape (e.g., const initialAIState = {
... } as const satisfies AIStateShape), and replace the unsafe cast at line 52
with a small validator/mapper function that checks dbMsg.role against the
allowed set ('user' | 'assistant' | 'system' | 'function' | 'data' | 'tool') and
returns a safe role (or a default/fallback) before creating the AIMessage;
ensure any invalid roles are handled explicitly (e.g., map to 'user' or
throw/log) so no unchecked database string is assigned to AIMessage['role'].


return (
<AI initialAIState={initialAIState}>
<MapDataProvider>
<Chat id={id} />
</MapDataProvider>
Expand Down
5 changes: 0 additions & 5 deletions bun.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 24 additions & 13 deletions components/chat-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export interface ChatPanelRef {

export const ChatPanel = forwardRef<ChatPanelRef, ChatPanelProps>(({ messages, input, setInput }, ref) => {
const [, setMessages] = useUIState<typeof AI>()
const { submit, clearChat } = useActions()
const { submit } = useActions()
// Removed mcp instance as it's no longer passed to submit
const [isMobile, setIsMobile] = useState(false)
const [selectedFile, setSelectedFile] = useState<File | null>(null)
Expand Down Expand Up @@ -69,7 +69,10 @@ export const ChatPanel = forwardRef<ChatPanelRef, ChatPanelProps>(({ messages, i
}
}

const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
const handleSubmit = async (
e: React.FormEvent<HTMLFormElement>,
newChat?: boolean
) => {
e.preventDefault()
if (!input && !selectedFile) {
return
Expand All @@ -86,18 +89,23 @@ export const ChatPanel = forwardRef<ChatPanelRef, ChatPanelProps>(({ messages, i
})
}

setMessages(currentMessages => [
...currentMessages,
{
id: nanoid(),
component: <UserMessage content={content} />
}
])
if (!newChat) {
setMessages(currentMessages => [
...currentMessages,
{
id: nanoid(),
component: <UserMessage content={content} />
}
])
}

const formData = new FormData(e.currentTarget)
if (selectedFile) {
formData.append('file', selectedFile)
}
if (newChat) {
formData.append('newChat', 'true')
}

setInput('')
clearAttachment()
Expand All @@ -106,10 +114,13 @@ export const ChatPanel = forwardRef<ChatPanelRef, ChatPanelProps>(({ messages, i
setMessages(currentMessages => [...currentMessages, responseMessage as any])
}

const handleClear = async () => {
const handleNewConversation = async () => {
setMessages([])
clearAttachment()
await clearChat()
const formData = new FormData()
formData.append('newChat', 'true')
const responseMessage = await submit(formData)
setMessages(currentMessages => [...currentMessages, responseMessage as any])
}
Comment on lines +117 to 123

@charliecreates charliecreates Bot Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting a new conversation currently adds a server response to the local UI state even though the server returns no UI content for a pure newChat request. This yields a blank message item in the timeline after clearing, which is confusing and unnecessary.

Suggestion

Avoid appending the server response for a newChat-only submit. You can still notify the server to create the conversation without pushing a UI item:

const handleNewConversation = async () => {
setMessages([]);
clearAttachment();
const formData = new FormData();
formData.append('newChat', 'true');
await submit(formData); // no UI append here
};

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.

Comment on lines +117 to 123

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid clearing local UI state to prevent flicker when starting a new conversation

setMessages([]) wipes the UI until the server-rendered state returns, causing a perceptible flash. Prefer leaving the existing UI and simply appending the server response for the new conversation.

Apply this diff:

-  const handleNewConversation = async () => {
-    setMessages([])
-    clearAttachment()
-    const formData = new FormData()
-    formData.append('newChat', 'true')
-    const responseMessage = await submit(formData)
-    setMessages(currentMessages => [...currentMessages, responseMessage as any])
-  }
+  const handleNewConversation = async () => {
+    clearAttachment()
+    const formData = new FormData()
+    formData.append('newChat', 'true')
+    const responseMessage = await submit(formData)
+    setMessages(currentMessages => [...currentMessages, responseMessage as any])
+  }

If you need a visual separator immediately, the server will add one via onGetUIState; no client wipe needed.

📝 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
const handleNewConversation = async () => {
setMessages([])
clearAttachment()
await clearChat()
const formData = new FormData()
formData.append('newChat', 'true')
const responseMessage = await submit(formData)
setMessages(currentMessages => [...currentMessages, responseMessage as any])
}
const handleNewConversation = async () => {
clearAttachment()
const formData = new FormData()
formData.append('newChat', 'true')
const responseMessage = await submit(formData)
setMessages(currentMessages => [...currentMessages, responseMessage as any])
}
🤖 Prompt for AI Agents 
In components/chat-panel.tsx around lines 117 to 124, remove the client-side
wipe that causes UI flicker by deleting the setMessages([]) call; instead, keep
the existing messages and append the server responseMessage to the
currentMessages (as you're already doing), so the UI is not cleared while
waiting for server-rendered state. Leave clearAttachment() only if you still
want attachments cleared, otherwise remove it too; ensure the final behavior is
to call submit(formData) and then setMessages(current => [...current,
responseMessage as any]) without first setting messages to an empty array.


useEffect(() => {
Expand All @@ -129,10 +140,10 @@ export const ChatPanel = forwardRef<ChatPanelRef, ChatPanelProps>(({ messages, i
type="button"
variant={'secondary'}
className="rounded-full bg-secondary/80 group transition-all hover:scale-105 pointer-events-auto"
onClick={() => handleClear()}
onClick={handleNewConversation}
>
<span className="text-sm mr-2 group-hover:block hidden animate-in fade-in duration-300">
New
New Conversation
</span>
<Plus size={18} className="group-hover:rotate-90 transition-all" />
</Button>
Expand Down
2 changes: 1 addition & 1 deletion components/map-toggle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function MapToggle() {
<DropdownMenuItem onClick={() => {setMapType(MapToggleEnum.FreeMode)}}>
My Maps
</DropdownMenuItem>
<DropdownMenuItem onClick={() => {setMapType(MapToggleEnum.DrawingMode)}}>
<DropdownMenuItem data-testid="drawing-mode-button" onClick={() => {setMapType(MapToggleEnum.DrawingMode)}}>
<Pencil className="h-[1rem] w-[1rem] mr-2" />
Draw & Measure
</DropdownMenuItem>
Expand Down
145 changes: 60 additions & 85 deletions components/map/mapbox-map.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useMapToggle, MapToggleEnum } from '../map-toggle-context'
import { useMapData } from './map-data-context'; // Add this import
import { useMapLoading } from '../map-loading-context'; // Import useMapLoading
import { useMap } from './map-context'
import { useWorker } from '@/hooks/useWorker'

Comment on lines +14 to 15

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Good move using a worker; remove unused turf import to shrink bundle.

With calcs offloaded, import * as turf from '@turf/turf' is likely unused here; removing it will reduce JS size.

🤖 Prompt for AI Agents 
In components/map/mapbox-map.tsx around lines 15 to 16, the file still imports
turf but the worker now handles geospatial calculations so the turf import is
unused; remove the unused "import * as turf from '@turf/turf'" line and any
related references, run TypeScript/ESLint checks to ensure no remaining
references, and confirm bundle size is reduced by rebuilding.

mapboxgl.accessToken = process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN as string;

Expand All @@ -38,6 +39,8 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
// Refs for long-press functionality
const longPressTimerRef = useRef<NodeJS.Timeout | null>(null);
const isMouseDownRef = useRef<boolean>(false);
const turfWorker = useWorker<any[]>(new URL('/workers/turf.worker.ts', import.meta.url));

@charliecreates charliecreates Bot Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Web Worker URL is created with a leading slash and without memoization. new URL('/workers/turf.worker.ts', import.meta.url) is resolved as an absolute URL, which is often incorrect in bundlers and Next.js; moreover, creating a new URL every render tears down and recreates the worker due to the useEffect dependency in useWorker, causing performance issues and flicker.

Suggestion

Memoize a correct relative URL and pass the memoized reference into useWorker to avoid worker churn and path resolution issues:

import { useMemo } from 'react';
...
const workerUrl = useMemo(() => new URL('../../workers/turf.worker.ts', import.meta.url), []);
const turfWorker = useWorkerany[](workerUrl);

This ensures a stable identity and a bundler-friendly path. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Worker URL should be relative to this module, not absolute.

new URL('/workers/…', import.meta.url) is treated as absolute and can fail bundling. Use a relative path from components/map/ to workers/.

-  const turfWorker = useWorker<any[]>(new URL('/workers/turf.worker.ts', import.meta.url));
+  const turfWorker = useWorker<any[]>(new URL('../../workers/turf.worker.ts', import.meta.url));
📝 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
const turfWorker = useWorker<any[]>(new URL('/workers/turf.worker.ts', import.meta.url));
const turfWorker = useWorker<any[]>(new URL('../../workers/turf.worker.ts', import.meta.url));
🤖 Prompt for AI Agents 
components/map/mapbox-map.tsx around lines 42 to 43, the worker URL is
constructed as an absolute path new URL('/workers/…', import.meta.url) which can
break bundling; change it to a relative path from components/map to the workers
folder (e.g. new URL('<correct-relative-path>/turf.worker.ts', import.meta.url))
so the bundler resolves it, update the number of ../ segments to match your repo
layout, and verify the worker file exists and the build runs successfully.


// const [isMapLoaded, setIsMapLoaded] = useState(false); // Removed local state

Expand Down Expand Up @@ -71,98 +74,70 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
lineLabelsRef.current = {}

const features = drawRef.current.getAll().features
const currentDrawnFeatures: Array<{ id: string; type: 'Polygon' | 'LineString'; measurement: string; geometry: any }> = []

features.forEach(feature => {
const id = feature.id as string
let featureType: 'Polygon' | 'LineString' | null = null;
let measurement = '';

if (feature.geometry.type === 'Polygon') {
featureType = 'Polygon';
// Calculate area for polygons
const area = turf.area(feature)
const formattedArea = formatMeasurement(area, true)
measurement = formattedArea;

// Get centroid for label placement
const centroid = turf.centroid(feature)
const coordinates = centroid.geometry.coordinates

// Create a label
const el = document.createElement('div')
el.className = 'area-label'
el.style.background = 'rgba(255, 255, 255, 0.8)'
el.style.padding = '4px 8px'
el.style.borderRadius = '4px'
el.style.fontSize = '12px'
el.style.fontWeight = 'bold'
el.style.color = '#333333' // Added darker color
el.style.boxShadow = '0 2px 4px rgba(0,0,0,0.2)'
el.style.pointerEvents = 'none'
el.textContent = formattedArea

// Add marker for the label

turfWorker.postMessage({ features });

}, [turfWorker])

Comment on lines 75 to 80

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Stabilize event handler dependencies to avoid listener churn.

Depending on the whole turfWorker object causes updateMeasurementLabels identity to change frequently. Depend on the memoized function instead.

-  const updateMeasurementLabels = useCallback(() => {
+  const updateMeasurementLabels = useCallback(() => {
     if (!map.current || !drawRef.current) return
     // ...
-    turfWorker.postMessage({ features });
-  }, [turfWorker])
+    turfWorker.postMessage({ features });
+  }, [turfWorker.postMessage])

Requires the useWorker change to memoize postMessage (see hook refactor).

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In components/map/mapbox-map.tsx around lines 76 to 80, the effect depends on
the entire turfWorker object which causes the handler identity to change
frequently; change the dependency to a memoized postMessage function instead —
i.e. update the useWorker hook (as noted) to return a stable postMessage
reference and then replace the dependency [turfWorker] with the stable
postMessage (e.g. [turfWorkerPostMessage]) so updateMeasurementLabels won’t be
recreated on every turfWorker mutation.

useEffect(() => {
if (turfWorker.data && map.current && drawRef.current) {
const features = drawRef.current.getAll().features;
const currentDrawnFeatures: Array<{ id: string; type: 'Polygon' | 'LineString'; measurement: string; geometry: any }> = [];

turfWorker.data.forEach(result => {
const { id, calculation } = result;
if (!calculation) return;

const feature = features.find(f => f.id === id);
if (!feature) return;

let featureType: 'Polygon' | 'LineString' | null = null;
let measurement = '';
let coordinates: [number, number] | undefined;

if (calculation.type === 'Polygon') {
featureType = 'Polygon';
measurement = formatMeasurement(calculation.area, true);
coordinates = calculation.center;
} else if (calculation.type === 'LineString') {
featureType = 'LineString';
measurement = formatMeasurement(calculation.length, false);
coordinates = calculation.center;
}

if (featureType && measurement && coordinates && map.current) {
const el = document.createElement('div');
el.className = `${featureType.toLowerCase()}-label`;
el.style.background = 'rgba(255, 255, 255, 0.8)';
el.style.padding = '4px 8px';
el.style.borderRadius = '4px';
el.style.fontSize = '12px';
el.style.fontWeight = 'bold';
el.style.color = '#333333';
el.style.boxShadow = '0 2px 4px rgba(0,0,0,0.2)';
el.style.pointerEvents = 'none';
el.textContent = measurement;

if (map.current) {
const marker = new mapboxgl.Marker({ element: el })
.setLngLat(coordinates as [number, number])
.addTo(map.current)

polygonLabelsRef.current[id] = marker
}
}
else if (feature.geometry.type === 'LineString') {
featureType = 'LineString';
// Calculate length for lines
const length = turf.length(feature, { units: 'kilometers' }) * 1000 // Convert to meters
const formattedLength = formatMeasurement(length, false)
measurement = formattedLength;

// Get midpoint for label placement
const line = feature.geometry.coordinates
const midIndex = Math.floor(line.length / 2) - 1
const midpoint = midIndex >= 0 ? line[midIndex] : line[0]

// Create a label
const el = document.createElement('div')
el.className = 'distance-label'
el.style.background = 'rgba(255, 255, 255, 0.8)'
el.style.padding = '4px 8px'
el.style.borderRadius = '4px'
el.style.fontSize = '12px'
el.style.fontWeight = 'bold'
el.style.color = '#333333' // Added darker color
el.style.boxShadow = '0 2px 4px rgba(0,0,0,0.2)'
el.style.pointerEvents = 'none'
el.textContent = formattedLength

// Add marker for the label
if (map.current) {
const marker = new mapboxgl.Marker({ element: el })
.setLngLat(midpoint as [number, number])
.addTo(map.current)

lineLabelsRef.current[id] = marker
}
}
.setLngLat(coordinates)
.addTo(map.current);

if (featureType && id && measurement && feature.geometry) {
currentDrawnFeatures.push({
id,
type: featureType,
measurement,
geometry: feature.geometry,
});
}
})
if (featureType === 'Polygon') {
polygonLabelsRef.current[id] = marker;
} else {
lineLabelsRef.current[id] = marker;
}

setMapData(prevData => ({ ...prevData, drawnFeatures: currentDrawnFeatures }))
}, [formatMeasurement, setMapData])
currentDrawnFeatures.push({
id,
type: featureType,
measurement,
geometry: feature.geometry,
});
}
});
setMapData(prevData => ({ ...prevData, drawnFeatures: currentDrawnFeatures }));
}
}, [turfWorker.data, formatMeasurement, setMapData])
Comment on lines +82 to +140

@charliecreates charliecreates Bot Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worker results are processed asynchronously without staleness protection. Rapid edits can cause out-of-order results to repaint labels for older geometries. Although you guard against deleted features, measurements can still be stale relative to the current geometry.

Suggestion

Introduce a monotonically increasing requestId and echo it through the worker so late results can be ignored:

  • In the component:
const calcRequestId = useRef(0)
const updateMeasurementLabels = useCallback(() => {
  // ... remove existing labels
  const requestId = ++calcRequestId.current
  const features = drawRef.current!.getAll().features
  turfWorker.postMessage({ requestId, features })
}, [turfWorker])

useEffect(() => {
  if (!turfWorker.data || !map.current || !drawRef.current) return
  const { requestId, results } = turfWorker.data
  if (requestId !== calcRequestId.current) return // ignore stale
  // process results as before...
}, [turfWorker.data])
  • Update the worker to echo requestId back (see worker comment).

Reply with "@CharlieHelps yes please" if you'd like me to add these correlated request changes across the hook, worker, and map component.


// Handle map rotation
const rotateMap = useCallback(() => {
Expand Down
50 changes: 50 additions & 0 deletions hooks/useWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { useState, useEffect, useRef } from 'react';

type UseWorkerReturnType<T> = {
postMessage: (data: any) => void;
data: T | null;
error: string | null;
isLoading: boolean;
};

export function useWorker<T>(workerUrl: URL): UseWorkerReturnType<T> {
const [data, setData] = useState<T | null>(null);
const [error, setError] = useState<string | null>(null);
const [isLoading, setIsLoading] = useState<boolean>(false);
const workerRef = useRef<Worker | null>(null);

useEffect(() => {
// Create a new worker instance
const worker = new Worker(workerUrl, { type: 'module' });
workerRef.current = worker;

worker.onmessage = (event: MessageEvent<T>) => {
setData(event.data);
setIsLoading(false);
};

worker.onerror = (err: ErrorEvent) => {
setError(err.message);
setIsLoading(false);
};
Comment on lines +25 to +28

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle structured-clone errors too.

Add onmessageerror to capture deserialization failures.

     worker.onerror = (err: ErrorEvent) => {
       setError(err.message);
       setIsLoading(false);
     };
+
+    worker.onmessageerror = (err: MessageEvent) => {
+      setError('Worker message deserialization failed');
+      setIsLoading(false);
+    };
📝 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
worker.onerror = (err: ErrorEvent) => {
setError(err.message);
setIsLoading(false);
};
worker.onerror = (err: ErrorEvent) => {
setError(err.message);
setIsLoading(false);
};
worker.onmessageerror = (err: MessageEvent) => {
setError('Worker message deserialization failed');
setIsLoading(false);
};
🤖 Prompt for AI Agents 
In hooks/useWorker.ts around lines 26 to 29, the worker only handles runtime
errors via worker.onerror but misses structured-clone (deserialization)
failures; add a worker.onmessageerror handler that sets the same error state and
loading flag (e.g., setError with a descriptive message or err.message if
available, and setIsLoading(false)) so message deserialization failures are
captured the same way as other worker errors.


// Cleanup worker on component unmount
return () => {
if (workerRef.current) {
workerRef.current.terminate();
workerRef.current = null;
}
};
}, [workerUrl]);
Comment on lines +16 to +41

@charliecreates charliecreates Bot Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useWorker provides a nice abstraction but doesn’t support correlating responses or ignoring out-of-order results, which is important for UI correctness when requests are fired quickly. Adding an optional requestId passthrough lets consumers discard stale results.

Suggestion

Augment the hook to allow structured messages with a requestId, and type onmessage accordingly:

type WorkerEnvelope<T> = { requestId?: number; results: T }

// onmessage
worker.onmessage = (event: MessageEvent<WorkerEnvelope<T>>) => {
  setData(event.data as any)
  setIsLoading(false)
}

// postMessage stays generic; caller provides requestId

I can wire this up and update the map/worker to use it. Reply with "@CharlieHelps yes please" to have me implement it.


Comment on lines +16 to +42

@charliecreates charliecreates Bot Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useWorker tears down and re-creates the worker whenever a new URL object instance is passed (even if it points to the same href), because the effect depends on the URL object identity. This is brittle and can cause unnecessary churn in any caller that constructs the URL inline.

Suggestion

Reduce sensitivity to object identity by depending on the string form of the URL (or change the hook signature to accept a string):

useEffect(() => {
const worker = new Worker(workerUrl, { type: 'module' });
...
return () => { workerRef.current?.terminate(); workerRef.current = null; };
// Depend on the href string, not the URL object identity
}, [workerUrl.toString()]);

This prevents unnecessary terminations when callers re-create equivalent URL instances. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.

const postMessage = (messageData: any) => {
if (workerRef.current) {
setIsLoading(true);
setError(null);
setData(null);
workerRef.current.postMessage(messageData);
}
};

@coderabbitai coderabbitai Bot Oct 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Stabilize postMessage and returned object to prevent listener leaks and unnecessary re-subscribes.

Use useCallback and useMemo so callers can depend on postMessage (not the whole hook object).

-  const postMessage = (messageData: any) => {
+  const postMessage = useCallback((messageData: any, transfer?: Transferable[]) => {
     if (workerRef.current) {
       setIsLoading(true);
       setError(null);
       setData(null);
-      workerRef.current.postMessage(messageData);
+      // pass transferables when provided
+      transfer ? workerRef.current.postMessage(messageData, transfer)
+               : workerRef.current.postMessage(messageData);
     }
-  };
+  }, []);
 
-  return { postMessage, data, error, isLoading };
+  return useMemo(() => ({ postMessage, data, error, isLoading }), [postMessage, data, error, isLoading]);

Also applies to: 49-50

🤖 Prompt for AI Agents 
In hooks/useWorker.ts around lines 40 to 47 (and similarly for lines 49-50) the
postMessage function and the returned hook object are recreated on every render
which forces consumers to re-subscribe and may leak listeners; wrap postMessage
in useCallback with proper dependencies (workerRef.current and setters) to
stabilize its identity, and wrap the returned object in useMemo so only stable
references are returned (include postMessage and the state values in the memo
deps). Ensure you do not include functions/objects that change every render in
the deps so callers can safely depend on postMessage without causing unnecessary
re-subscribes or leaks.


return { postMessage, data, error, isLoading };
}
Loading