-
Notifications
You must be signed in to change notification settings - Fork 6
Develop #268
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.
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
Develop #268
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| diff --git a/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt b/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt | ||
| index 8cda49e..05cbf87 100644 | ||
| --- a/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt | ||
| +++ b/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt | ||
| @@ -73,17 +73,50 @@ fun LocationEngine.requestLocationUpdatesV11(callback: LocationEngineCallback, l | ||
| } | ||
| } | ||
| val observer = LocationObserverAdapter(callback) | ||
| + // Publish into `observers` *before* registering with the provider, and pin the provider we | ||
| + // register on. A concurrent `removeLocationUpdates` (same two threads described below) that | ||
| + // lands between the two steps would otherwise never see this observer and would leave it | ||
| + // registered — feeding location updates to a callback the caller already tore down, on a | ||
| + // provider instance the request refresh above may have replaced. Registration itself stays | ||
| + // outside the monitor: no Mapbox SDK call is ever made while holding it. | ||
| + val provider = locationProvider | ||
| + synchronized(observers) { | ||
| + observers.add(observer) | ||
| + } | ||
| if (looper != null) { | ||
| - locationProvider.addLocationObserver(observer, looper) | ||
| + provider.addLocationObserver(observer, looper) | ||
| } else { | ||
| - locationProvider.addLocationObserver(observer) | ||
| + provider.addLocationObserver(observer) | ||
| + } | ||
| + // Gone from the list means a removal ran while we were registering, and its | ||
| + // `removeLocationObserver` hit a provider that did not know this observer yet. Undo here. | ||
| + val canceled = synchronized(observers) { !observers.contains(observer) } | ||
| + if (canceled) { | ||
| + provider.removeLocationObserver(observer) | ||
| } | ||
| - observers.add(observer) | ||
| } | ||
|
|
||
| +// `observers` is reached from more than one thread: LocationManager.enable() runs on the | ||
| +// main thread when the activity resumes, and again on the React native-modules thread when | ||
| +// RNMBXLocationModule.start()/setMinDisplacement() land. Unsynchronized, the two overlap | ||
| +// inside Kotlin's `removeAll { }` (filterInPlace), whose trailing `removeAt(readIndex)` | ||
| +// walks indices captured before the other thread shrank the list — IndexOutOfBoundsException | ||
| +// "Index 0 out of bounds for length 0", crashing the app on foreground. | ||
| +// | ||
| +// Guard both mutations, and match on identity via a plain collection so `filterInPlace` | ||
| +// is not involved at all. `removeLocationObserver` runs outside the lock: it calls into the | ||
| +// Mapbox SDK and must not be holding our monitor while it does. | ||
| +// | ||
| +// Dropping an observer from the list is also the cancel signal for a registration still | ||
| +// in flight on the other thread — see `requestLocationUpdatesV11`, which re-checks | ||
| +// membership after registering and unregisters itself if it was pulled out meanwhile. | ||
| fun LocationEngine.removeLocationUpdates(callback: LocationEngineCallback) { | ||
| - observers.filter { it.callback == callback }.forEach { locationProvider.removeLocationObserver(it) } | ||
| - observers.removeAll { it.callback == callback } | ||
| + val stale = synchronized(observers) { | ||
| + val matched = observers.filter { it.callback == callback } | ||
| + observers.removeAll(matched.toSet()) | ||
| + matched | ||
| + } | ||
| + stale.forEach { locationProvider.removeLocationObserver(it) } | ||
| } | ||
|
|
||
| fun LocationEngine.getLastLocation(callback: LocationEngineCallback) { | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| diff --git a/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m b/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m | ||
| index aa90548..0c99c73 100644 | ||
| --- a/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m | ||
| +++ b/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m | ||
| @@ -1,7 +1,15 @@ | ||
| #import "RNCWebViewDecisionManager.h" | ||
|
|
||
| - | ||
| - | ||
| +/** | ||
| + * Thread-safe singleton that manages navigation decision handlers for WKWebView. | ||
| + * | ||
| + * This class bridges async navigation decisions between: | ||
| + * - WKWebView delegate (main thread) - stores decision handlers | ||
| + * - React Native bridge (background thread) - resolves decisions from JS | ||
| + * | ||
| + * All public methods use @synchronized for thread safety since they access | ||
| + * shared state (nextLockIdentifier and decisionHandlers) from different threads. | ||
| + */ | ||
| @implementation RNCWebViewDecisionManager | ||
|
|
||
| @synthesize nextLockIdentifier; | ||
| @@ -16,22 +24,39 @@ | ||
| return lockManager; | ||
| } | ||
|
|
||
| +/** | ||
| + * Stores a decision handler and returns a unique identifier. | ||
| + * Called from the main thread (WKNavigationDelegate). | ||
| + * @synchronized ensures atomic increment + insertion. | ||
| + */ | ||
| - (int)setDecisionHandler:(DecisionBlock)decisionHandler { | ||
| - int lockIdentifier = self.nextLockIdentifier++; | ||
| - | ||
| - [self.decisionHandlers setObject:decisionHandler forKey:@(lockIdentifier)]; | ||
| - return lockIdentifier; | ||
| + @synchronized (self) { | ||
| + int lockIdentifier = self.nextLockIdentifier++; | ||
| + [self.decisionHandlers setObject:decisionHandler forKey:@(lockIdentifier)]; | ||
| + return lockIdentifier; | ||
| + } | ||
| } | ||
|
|
||
| +/** | ||
| + * Resolves a pending navigation decision. | ||
| + * Called from the RN bridge thread (background) when JS responds. | ||
| + * | ||
| + * The handler is invoked OUTSIDE the @synchronized block to prevent deadlocks, | ||
| + * since the handler dispatches to the main queue and could potentially | ||
| + * trigger another navigation that re-enters this class. | ||
| + */ | ||
| - (void) setResult:(BOOL)shouldStart | ||
| forLockIdentifier:(int)lockIdentifier { | ||
| - DecisionBlock handler = [self.decisionHandlers objectForKey:@(lockIdentifier)]; | ||
| - if (handler == nil) { | ||
| - RCTLogWarn(@"Lock not found"); | ||
| - return; | ||
| + DecisionBlock handler; | ||
| + @synchronized (self) { | ||
| + handler = [self.decisionHandlers objectForKey:@(lockIdentifier)]; | ||
| + if (handler == nil) { | ||
| + RCTLogWarn(@"Lock not found"); | ||
| + return; | ||
| + } | ||
| + [self.decisionHandlers removeObjectForKey:@(lockIdentifier)]; | ||
| } | ||
| handler(shouldStart); | ||
| - [self.decisionHandlers removeObjectForKey:@(lockIdentifier)]; | ||
| } | ||
|
|
||
| - (id)init { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { readFileSync } from 'fs'; | ||
| import { join } from 'path'; | ||
|
|
||
| /** | ||
| * Guards `patches/react-native-webview+13.16.1.patch`. | ||
| * | ||
| * `RNCWebViewDecisionManager` is a process-wide singleton whose `decisionHandlers` | ||
| * dictionary is written from the main thread (`WKNavigationDelegate` storing a handler) | ||
| * and read from a bridge worker thread (`shouldStartLoadWithLockIdentifier` resolving one | ||
| * from JS). Unsynchronized, a lookup could probe buckets while the dictionary rehashed and | ||
| * send `isEqual:` to a freed key — `EXC_BAD_ACCESS ... KERN_INVALID_ADDRESS`, which crashed | ||
| * the app when a WebView screen was torn down mid-navigation. | ||
| * | ||
| * Upstream fixed this in 14.0.1, but Expo SDK 56 pins react-native-webview to exactly | ||
| * 13.16.1, so the fix is backported verbatim rather than taken by upgrade. | ||
| * Losing the patch — to a `yarn install`, or to an Expo bump that lands a version still | ||
| * carrying the race — brings the crash straight back. | ||
| */ | ||
| describe('react-native-webview decision manager patch', () => { | ||
| const source = readFileSync(join(process.cwd(), 'node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m'), 'utf8'); | ||
|
|
||
| it('serialises access to the shared decision handler map', () => { | ||
| expect(source).toContain('@synchronized (self)'); | ||
| // Both entry points must hold the lock, not just the reader. | ||
| expect(source.match(/@synchronized \(self\)/g)).toHaveLength(2); | ||
| }); | ||
|
|
||
| it('invokes the handler outside the lock so a re-entrant navigation cannot deadlock', () => { | ||
| // The entry is removed inside the lock, then the closing brace, then the call. | ||
| expect(source).toMatch(/removeObjectForKey:@\(lockIdentifier\)\];\s*\}\s*handler\(shouldStart\);/); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { VStack } from '@/components/ui/vstack'; | |
| import { logger } from '@/lib/logging'; | ||
| import { getAvatarUrl } from '@/lib/utils'; | ||
| import { type RecipientsResultData } from '@/models/v4/messages/recipientsResultData'; | ||
| import useAuthStore from '@/stores/auth/store'; | ||
| import { useToastStore } from '@/stores/toast/store'; | ||
|
|
||
| interface NewConversationSheetProps { | ||
|
|
@@ -38,8 +39,14 @@ function isPersonRecipient(recipient: RecipientsResultData): boolean { | |
| return type === 'personnel' || type === 'person' || type === 'user' || type === 'p' || type === ''; | ||
| } | ||
|
|
||
| /** The server rejects self-DMs, so the current user never belongs in the picker. */ | ||
| function isSelfRecipient(recipient: RecipientsResultData, currentUserId: string | null): boolean { | ||
| return !!currentUserId && recipientUserId(recipient).toLowerCase() === currentUserId.toLowerCase(); | ||
| } | ||
|
Comment on lines
+42
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 5 '\buserId\b|isAuthenticated|hydrate|persist' src/stores/auth/store.tsx
rg -n -C 5 'NewConversationSheet' srcRepository: Resgrid/Unit Length of output: 10259 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- new conversation sheet ---'
sed -n '1,150p' src/components/chat/new-conversation-sheet.tsx
printf '%s\n' '--- chat screen opening logic ---'
sed -n '150,235p' 'src/app/(app)/chat.tsx'
printf '%s\n' '--- auth store hydration and state contract ---'
sed -n '1,40p' src/stores/auth/store.tsx
sed -n '245,380p' src/stores/auth/store.tsx
printf '%s\n' '--- auth gating and hydrate callers ---'
rg -n -C 5 'hydrate\(\)|useAuthStore|status === .signedIn.|isAuthenticated\(' src/app src/components src/hooks src/lib | head -n 240Repository: Resgrid/Unit Length of output: 32700 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- route/auth gate ---'
fd -t f '_layout.tsx' src/app --exec sh -c 'echo "--- $1"; rg -n -C 8 "status|userId|signedIn|redirect|router|hydrate" "$1"' sh {}
printf '%s\n' '--- chat component state and imports ---'
sed -n '1,90p' 'src/app/(app)/chat.tsx'
printf '%s\n' '--- complete auth hook contract ---'
sed -n '1,80p' src/lib/auth/index.tsx
rg -n -C 8 'set\(\{[^}]*userId|userId:' src/stores/auth/store.tsx
printf '%s\n' '--- all auth-store initialization calls ---'
rg -n -C 6 'useAuthStore\.getState\(\)\.hydrate|\.hydrate\(\)' srcRepository: Resgrid/Unit Length of output: 21881 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
auth = Path('src/stores/auth/store.tsx').read_text()
layout = Path('src/app/(app)/_layout.tsx').read_text()
sheet = Path('src/components/chat/new-conversation-sheet.tsx').read_text()
signed_in_blocks = re.findall(r'set\(\{([^}]*)\}\)', auth, re.S)
transitions = []
for block in signed_in_blocks:
if re.search(r"status:\s*['\"]signedIn['\"]", block):
transitions.append({
'setsStatusSignedIn': True,
'setsUserId': bool(re.search(r'\buserId\s*:', block)),
'setsProfile': bool(re.search(r'\bprofile\s*:', block)),
})
print('auth signedIn transitions:', transitions)
print('signedIn route requires userId:', bool(re.search(r"if\s*\(status === ['\"]signedIn['\"]", layout) and re.search(r'if\s*\(status !== [\'\"]signedIn[\'\"]', layout)))
print('route guard references userId:', bool(re.search(r'userId', layout[layout.find('if (isFirstTime)'):layout.find('const content')])))
effect = re.search(r'useEffect\(\(\) => \{(.*?)\n \}, \[isOpen, currentUserId\]\);', sheet, re.S)
print('recipient effect depends on currentUserId:', bool(effect))
print('recipient effect gates on currentUserId:', bool(effect and re.search(r'currentUserId', effect.group(1))))
PYRepository: Resgrid/Unit Length of output: 632 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 18 "status: 'signedIn'" src/stores/auth/store.tsxRepository: Resgrid/Unit Length of output: 7156 Keep the picker closed until
🤖 Prompt for AI Agents |
||
|
|
||
| export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewConversationSheetProps) { | ||
| const { t } = useTranslation(); | ||
| const currentUserId = useAuthStore((s) => s.userId); | ||
| const [recipients, setRecipients] = useState<RecipientsResultData[]>([]); | ||
| const [loading, setLoading] = useState(false); | ||
| const [loadError, setLoadError] = useState(false); | ||
|
|
@@ -59,7 +66,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| getRecipients(true, false) | ||
| .then((result) => { | ||
| if (cancelled) return; | ||
| setRecipients((result.Data ?? []).filter(isPersonRecipient)); | ||
| setRecipients((result.Data ?? []).filter((r) => isPersonRecipient(r) && !isSelfRecipient(r, currentUserId))); | ||
| }) | ||
| .catch((error) => { | ||
| if (cancelled) return; | ||
|
|
@@ -73,7 +80,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [isOpen]); | ||
| }, [isOpen, currentUserId]); | ||
|
|
||
| const filtered = useMemo(() => { | ||
| const q = query.trim().toLowerCase(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import mapboxPackage from '@rnmapbox/maps/package.json'; | ||
| import { readFileSync } from 'fs'; | ||
| import { join } from 'path'; | ||
|
|
||
| describe('@rnmapbox/maps version floor', () => { | ||
| /** | ||
| * 10.2.x `AnimatedPoint` assigned `this._listeners = {}` in its constructor. It extends | ||
| * React Native's `AnimatedWithChildren`, and `AnimatedNode` owns that field — on RN 0.85 | ||
| * it is a `Map`, so the plain object broke `AnimatedNode.__callListeners`, which calls | ||
| * `this._listeners.forEach(...)`. | ||
| * | ||
| * `Mapbox.UserLocation` defaults to `animated`, so every location update on a screen with | ||
| * a map ran `AnimatedPoint.timing().start()` and the first frame threw | ||
| * `TypeError: undefined is not a function`, taking the app down (seen in Responder, | ||
| * reproduced on the call detail screen). 10.3.0 guards the assignment; dropping below that | ||
| * brings the crash straight back. | ||
| */ | ||
| it('is at least 10.3.0, where the AnimatedPoint listener clobber was fixed', () => { | ||
| const [major, minor] = mapboxPackage.version.split('.').map(Number); | ||
|
|
||
| expect(major).toBeGreaterThanOrEqual(10); | ||
| expect(major > 10 || minor >= 3).toBe(true); | ||
| }); | ||
|
|
||
| /** | ||
| * The JS bindings are generated against a specific native SDK. Pinning an older one in the | ||
| * Expo plugin leaves style props the bindings emit (`symbolZOffset`) unimplemented | ||
| * natively, which traps in `RNMBXStyle.symbolLayer` on iOS. | ||
| */ | ||
| it('pins the same native Mapbox SDK the installed bindings target', () => { | ||
| const appConfig = readFileSync(join(process.cwd(), 'app.config.ts'), 'utf8'); | ||
| const pinned = /RNMapboxMapsVersion:\s*'([^']+)'/.exec(appConfig)?.[1]; | ||
|
|
||
| expect(pinned).toBe(mapboxPackage.mapbox.android); | ||
| }); | ||
|
|
||
| /** | ||
| * Guards `patches/@rnmapbox+maps+10.3.5.patch`. Upstream's `LocationEngine.observers` is a | ||
| * plain list mutated from both the main thread (activity resume) and the React | ||
| * native-modules thread (`RNMBXLocationModule.start`). The overlap lands inside Kotlin's | ||
| * `removeAll { }` and throws `IndexOutOfBoundsException: Index 0 out of bounds for | ||
| * length 0`, killing the app as it foregrounds. A `yarn install` | ||
| * that drops the patch brings the crash back, so assert on the installed source. | ||
| */ | ||
| it('keeps the LocationEngine observer list guarded against concurrent mutation', () => { | ||
| const locationKt = readFileSync(join(process.cwd(), 'node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt'), 'utf8'); | ||
|
|
||
| expect(locationKt).toContain('synchronized(observers)'); | ||
| expect(locationKt).not.toMatch(/observers\.removeAll\s*\{/); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.