Develop #58
Develop #58
Conversation
WalkthroughBroad update introducing Aptabase analytics across many screens/components, PII-based UI gating, Bluetooth/audio UX and telemetry, new status/offline queue stores, env/config changes, and CI/CD Android release automation. Significant Android native files were removed. Extensive tests and docs added; some mocks and libraries were replaced or deleted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Screen
participant Focus as useFocusEffect
participant Hook as useAnalytics
participant Service as aptabaseService
Screen->>Focus: onFocus callback
Focus-->>Screen: invoked on focus
Screen->>Hook: const { trackEvent }
Screen->>Service: trackEvent('event_name', {timestamp, ...})
Service-->>Screen: fire-and-forget (no blocking)
sequenceDiagram
autonumber
participant UI as DeviceSelectionSheet
participant Store as BluetoothAudioStore
participant Svc as bluetooth-audio.service
participant Pref as usePreferredBluetoothDevice
participant Analytics as aptabaseService
UI->>Analytics: bluetooth_device_selection_sheet_viewed
UI->>Svc: startScanning(10000)
UI->>Pref: setPreferredDevice(null)
alt connected device exists
UI->>Svc: disconnectDevice(prevId)
end
UI->>Pref: setPreferredDevice(newDevice)
UI->>Svc: connectToDevice(newDevice.id)
UI->>Analytics: bluetooth_device_selection_completed {...}
UI-->>UI: close sheet
sequenceDiagram
autonumber
participant App as App Root
participant Init as appInitializationService
participant Offline as offlineQueueService
participant Logger as logger
App->>Init: initialize()
Init-->>App: ok/fail (logged)
App->>Offline: initialize()
Offline-->>App: ok/fail (logged)
App-->>Logger: "App initialization completed"
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limits.
🔭 Outside diff range comments (10)
src/components/status/__tests__/personnel-status-bottom-sheet.test.tsx (2)
654-654: Fix false negative: query the exact label including colonEarlier assertions use the label with a trailing colon (
custom_responding_to:). This check will pass even if the label is still rendered, because it omits the colon.- expect(screen.queryByText('personnel.status.custom_responding_to')).toBeNull(); + expect(screen.queryByText('personnel.status.custom_responding_to:')).toBeNull();
672-672: Fix false negative: query the exact “note” label including colonSame issue as above for the “note” label.
- expect(screen.queryByText('personnel.status.note')).toBeNull(); + expect(screen.queryByText('personnel.status.note:')).toBeNull();src/components/notifications/__tests__/NotificationInbox.test.tsx (1)
342-354: Test name doesn't match its implementation.The test is named "shows success toast on successful delete" but doesn't actually verify that a toast is shown. It only directly calls the delete API without going through the component's delete flow.
Either rename the test to match what it actually does, or implement the proper toast verification:
- it('shows success toast on successful delete', async () => { + it('calls delete API successfully', async () => { mockDeleteMessage.mockResolvedValue(undefined); - const { getByText } = render( - <NotificationInbox isOpen={true} onClose={mockOnClose} /> - ); - await act(async () => { await deleteMessage('1'); }); expect(mockDeleteMessage).toHaveBeenCalledWith('1'); });Or implement proper toast verification through the component's delete flow.
app.config.ts (1)
176-180: Fix typos and product name inconsistencies in user-facing strings
- “Resgird” → “Resgrid”
- “Resgrid Unit” → “Resgrid Responder” (align with the rest of the app’s wording)
Apply this diff:
- locationWhenInUsePermission: 'Allow Resgird Responder to show current location on map.', + locationWhenInUsePermission: 'Allow Resgrid Responder to show current location on map.', @@ - NSBluetoothAlwaysUsageDescription: 'Allow Resgrid Unit to connect to bluetooth devices for PTT.', + NSBluetoothAlwaysUsageDescription: 'Allow Resgrid Responder to connect to Bluetooth devices for PTT.', @@ - { - microphonePermission: 'Allow Resgrid Unit to access the microphone for audio input used in PTT and calls.', - }, + { + microphonePermission: 'Allow Resgrid Responder to access the microphone for audio input used in PTT and calls.', + },Also applies to: 51-52, 256-259
src/app/call/[id].tsx (1)
492-495: Typo in property name: renameImgagesCount→ImagesCountacross model, UI, and testsThe model, UI component, and tests currently use the misspelled
ImgagesCount. This will never pick up the real image count (e.g. if the API JSON key isImagesCount). Please update all occurrences:• Model
File:src/models/v4/calls/callResultData.tsexport class CallResultData { - public ImgagesCount: number = 0; + public ImagesCount: number = 0; // …other fields }• UI component
File:src/app/call/[id].tsx(around lines 492–495)- {call?.ImgagesCount ? ( + {call?.ImagesCount ? ( <Box className="absolute -right-1 -top-1 h-5 min-w-[20px] items-center justify-center rounded-full bg-red-500 px-1"> - <Text className="text-xs font-medium text-white">{call.ImgagesCount}</Text> + <Text className="text-xs font-medium text-white">{call.ImagesCount}</Text> </Box> ) : null}• Tests
File:src/app/call/__tests__/[id].test.tsx- ImgagesCount: 1, + ImagesCount: 1,Please run a global search/replace for
ImgagesCount→ImagesCountto ensure no leftovers.env.js (1)
9-9: Align import convention for Env
Update the top‐of‐file docs inenv.jsto match the named‐import style used throughout the codebase.• File: env.js (around line 9)
Change the example from a default to a named import:- * To access the client environment variables in your `src` folder, you can import them from `@env`. For example: `import Env from '@env'`. + * To access the client environment variables in your `src` folder, you can import them from `@env`. For example: `import { Env } from '@env'`.src/services/push-notification.ts (3)
146-211: Harden registration: redact token in logs, simplify token extraction, and add unitId guard
- Don’t log raw push tokens. They’re sensitive and can be abused.
getDevicePushTokenAsyncyields the string token indatafor both platforms; the ternary is redundant.- If
unitIdordepartmentCodeis empty, short-circuit to avoid bad backend registrations.Apply:
public async registerForPushNotifications(unitId: string, departmentCode: string): Promise<string | null> { + if (!unitId || !departmentCode) { + logger.warn({ + message: 'Missing unitId or departmentCode; skipping push registration', + context: { hasUnitId: !!unitId, hasDepartmentCode: !!departmentCode }, + }); + return null; + } @@ - // The token format depends on the platform - const token = Platform.OS === 'ios' ? devicePushToken.data : devicePushToken.data; + // Unified token extraction for both platforms + const token = devicePushToken.data as string; @@ - logger.info({ + const redacted = typeof this.pushToken === 'string' + ? `${this.pushToken.slice(0, 6)}...${this.pushToken.slice(-4)}` + : 'unknown'; + logger.info({ message: 'Push notification token obtained', context: { - token: this.pushToken, + token: redacted, unitId, platform: Platform.OS, }, });
283-309: TheusePushNotificationshook is registering with the user’s auth ID instead of the current unit’s ID—this will mis-associate device tokens. You’ll need to pull in your app’s active-unit selector (e.g.useCoreStore((s) => s.activeUnitId)) and switch everyuserIdtounitId.• File to update:
src/services/push-notification.ts
• Lines 283–309: replaceconst userId = …withconst unitId = useCoreStore(…), adjust the effect’s guard toif (rights?.DepartmentCode && unitId && unitId !== previousUnitIdRef.current), call.registerForPushNotifications(unitId, rights.DepartmentCode), log{ unitId }, and setpreviousUnitIdRef.current = unitId.Diff sketch:
-import { useAuthStore } from '@/stores/auth'; +import { useAuthStore } from '@/stores/auth'; import { securityStore } from '@/stores/security'; +import { useCoreStore } from '@/stores/core'; … -export const usePushNotifications = () => { - const userId = useAuthStore((state) => state.userId); +export const usePushNotifications = () => { + const unitId = useCoreStore((state) => state.activeUnitId); const rights = securityStore((state) => state.rights); - const previousUnitIdRef = useRef<string | null>(null); + const previousUnitIdRef = useRef<string | null>(null); useEffect(() => { - if (rights && userId && userId !== previousUnitIdRef.current) { - pushNotificationService - .registerForPushNotifications(userId, rights.DepartmentCode) + if (rights?.DepartmentCode && unitId && unitId !== previousUnitIdRef.current) { + pushNotificationService + .registerForPushNotifications(unitId, rights.DepartmentCode) .then((token) => { if (token) { logger.info({ message: 'Successfully registered for push notifications', - context: { userId: userId }, + context: { unitId }, }); } }) @@ - previousUnitIdRef.current = userId; + previousUnitIdRef.current = unitId; } }, [rights, unitId]);Run this to find your core‐store selector key if it’s named differently:
rg -n -C3 'activeUnitId' src/stores
22-30: Remove unsupported NotificationBehavior properties for [email protected]The project’s installed version (expo-notifications 0.29.14) only defines these keys in
NotificationBehavior:
- shouldShowAlert
- shouldPlaySound
- shouldSetBadge
Properties
shouldShowBannerandshouldShowListaren’t present until a much newer SDK (v53+) and will be ignored (or cause type errors). Either remove them or upgrade yourexpo-notificationsdependency.Please update
src/services/push-notification.ts:Notifications.setNotificationHandler({ handleNotification: async () => ({ shouldShowAlert: true, shouldPlaySound: true, shouldSetBadge: true, - shouldShowBanner: true, - shouldShowList: true, }), });If you need banner/list presentation on iOS, consider bumping to
expo-notifications>= 0.53.src/stores/app/__tests__/core-store.test.ts (1)
18-21: Mock path is outdated; tests won’t stub getConfigYou now import getConfig from '@/api/config' (Line 74) but still mock '@/api/config/config'. This leaves getConfig unmocked and will break tests.
Apply this diff:
-jest.mock('@/api/config/config', () => ({ - getConfig: jest.fn(), -})); +jest.mock('@/api/config', () => ({ + getConfig: jest.fn(), +}));
| <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/> | ||
| <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/> | ||
| <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/> | ||
| <uses-permission android:name="android.permission.BIND_TELECOM_CONNECTION_SERVICE"/> |
There was a problem hiding this comment.
Remove BIND_TELECOM_CONNECTION_SERVICE from uses-permission
This is a signature-level permission; third-party apps cannot be granted it. Keep it only as the service’s required permission.
- <uses-permission android:name="android.permission.BIND_TELECOM_CONNECTION_SERVICE"/>📝 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.
| <uses-permission android:name="android.permission.BIND_TELECOM_CONNECTION_SERVICE"/> |
🤖 Prompt for AI Agents
In android/app/src/main/AndroidManifest.xml around line 8, the manifest
incorrectly declares android.permission.BIND_TELECOM_CONNECTION_SERVICE as a
uses-permission (a signature-level permission third-party apps cannot hold);
remove that <uses-permission> entry and ensure the permission remains only as
the service’s requiredPermission attribute on the corresponding <service>
element (i.e., keep
requiredPermission="android.permission.BIND_TELECOM_CONNECTION_SERVICE" on the
telecom connection service declaration).
| version: packageJSON.version, | ||
| versionCode: parseInt(packageJSON.versionCode), | ||
| adaptiveIcon: { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against NaN for Android versionCode
parseInt(packageJSON.versionCode) will produce NaN if versionCode is missing or not a numeric string. That will break EAS/Gradle config.
Apply this diff to harden parsing and set a sane fallback:
- versionCode: parseInt(packageJSON.versionCode),
+ versionCode: Number.isInteger(Number(packageJSON.versionCode))
+ ? parseInt(String(packageJSON.versionCode), 10)
+ : 1,🤖 Prompt for AI Agents
In app.config.ts around lines 64-66, the versionCode currently uses
parseInt(packageJSON.versionCode) which can produce NaN if versionCode is
missing or non-numeric; update it to parse with an explicit radix (parseInt(...,
10)) and guard the result with Number.isFinite/Number.isNaN (or isNaN) to
provide a sane numeric fallback (e.g., 1) so versionCode always resolves to a
valid number for EAS/Gradle.
| "react-native-ble-manager": "^12.1.5", | ||
| "react-native-calendars": "^1.1313.0", | ||
| "react-native-callkeep": "github:Irfanwani/react-native-callkeep#957193d0716f1c2dfdc18e627cbff0f8a0800971", | ||
| "react-native-edge-to-edge": "~1.1.2", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
BLE stack migration: ensure no remaining ble-plx references and that BleManager is correctly imported
The dependency switch is correct. Make sure the codebase and mocks no longer import react-native-ble-plx, and that all BLE code now uses react-native-ble-manager patterns.
Run:
🏁 Script executed:
#!/bin/bash
# Confirm no residual `react-native-ble-plx` references
rg -n -C2 -P "react-native-ble-plx|BleManager" --type ts --type tsx --type js
# If iOS/Android native setup exists, ensure podspec/gradle steps aren't referencing ble-plx names
rg -n -C2 -P "ble-plx" -g "ios/**" -g "android/**"Length of output: 307
I’ve refined the search to avoid unrecognized types and to target both JS/TS code and native config files. Please re-run:
🏁 Script executed:
#!/bin/bash
# Search for any remaining imports of react-native-ble-plx
rg -n -C2 "react-native-ble-plx" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx"
# Search for BleManager references (likely from ble-plx)
rg -n -C2 "BleManager" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx"
# Check iOS native files (Podfile and podspecs) for ble-plx mentions
rg -n -C2 "ble-plx" -g "ios/Podfile" -g "ios/**/*.podspec"
# Check Android Gradle config for ble-plx mentions
rg -n -C2 "ble-plx" -g "android/**/*.gradle" -g "android/**/build.gradle"Length of output: 9683
Update outdated BLE comment reference
Our search confirms all react-native-ble-plx imports have been removed and no native (iOS/Android) configs reference “ble-plx.” However, there’s still one outdated comment:
- src/stores/app/bluetooth-audio-store.ts, line 7
Change
// Bluetooth state enum to match react-native-ble-plx API
to reference react-native-ble-manager instead of react-native-ble-plx.
🤖 Prompt for AI Agents
In src/stores/app/bluetooth-audio-store.ts around line 7, update the comment
that currently reads "// Bluetooth state enum to match react-native-ble-plx API"
to reference the correct library: replace "react-native-ble-plx" with
"react-native-ble-manager" so the comment becomes something like "// Bluetooth
state enum to match react-native-ble-manager API"; keep the rest of the comment
intact.
| // Keep processing in background for a short time | ||
| setTimeout(() => { | ||
| if (AppState.currentState === 'background') { | ||
| this.stopProcessing(); | ||
| } | ||
| }, 30000); // 30 seconds |
There was a problem hiding this comment.
Potential memory leak with background timeout.
The timeout created when the app goes to background might not be cleared if the app state changes before the timeout fires, potentially causing a memory leak or unexpected behavior.
+ private backgroundTimeoutId: NodeJS.Timeout | null = null;
+
private handleAppStateChange = (nextAppState: AppStateStatus): void => {
logger.info({
message: 'Offline event manager handling app state change',
context: { nextAppState },
});
+ // Clear any existing background timeout
+ if (this.backgroundTimeoutId) {
+ clearTimeout(this.backgroundTimeoutId);
+ this.backgroundTimeoutId = null;
+ }
+
if (nextAppState === 'active') {
this.startProcessing();
} else if (nextAppState === 'background') {
// Keep processing in background for a short time
- setTimeout(() => {
+ this.backgroundTimeoutId = setTimeout(() => {
if (AppState.currentState === 'background') {
this.stopProcessing();
}
+ this.backgroundTimeoutId = null;
}, 30000); // 30 seconds
} else if (nextAppState === 'inactive') {
this.stopProcessing();
}
};Also update the cleanup method:
public cleanup(): void {
this.stopProcessing();
+ if (this.backgroundTimeoutId) {
+ clearTimeout(this.backgroundTimeoutId);
+ this.backgroundTimeoutId = null;
+ }
+
if (this.appStateSubscription) {
this.appStateSubscription.remove();
this.appStateSubscription = null;
}🤖 Prompt for AI Agents
In src/services/offline-event-manager.service.ts around lines 346 to 351, the
setTimeout created when the app enters background is never tracked or cleared
which can leak if app state changes before it fires; store the timeout id (typed
appropriately for browser/Node), replace the inline setTimeout with assignment
to a class property, clear it with clearTimeout when the app state changes or
when stopping processing, and update the cleanup method to clear and null out
that timeout id to avoid lingering timers.
| await TaskManager.defineTask(QUEUE_PROCESSOR_TASK_NAME, async () => { | ||
| await this.processQueue(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix duplicate task definition.
The task is being defined twice - once in startBackgroundProcessing (lines 120-122) and already at the module level (lines 14-28). This duplicate definition is unnecessary and may cause unexpected behavior.
// Register background task if not already registered
const isTaskRegistered = await TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME);
if (!isTaskRegistered) {
- await TaskManager.defineTask(QUEUE_PROCESSOR_TASK_NAME, async () => {
- await this.processQueue();
- });
-
logger.info({
- message: 'Background queue processor task registered',
+ message: 'Background queue processor task was not registered - this should not happen',
});
}📝 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.
| await TaskManager.defineTask(QUEUE_PROCESSOR_TASK_NAME, async () => { | |
| await this.processQueue(); | |
| }); | |
| // Register background task if not already registered | |
| const isTaskRegistered = await TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME); | |
| if (!isTaskRegistered) { | |
| logger.info({ | |
| message: 'Background queue processor task was not registered - this should not happen', | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/services/offline-queue-processor.ts around lines 120 to 123, there is a
duplicate TaskManager.defineTask call that defines the same
QUEUE_PROCESSOR_TASK_NAME already defined at module level (lines 14-28); remove
the second definition inside startBackgroundProcessing (or replace it with a
guard that checks TaskManager.hasTask/exists before defining) so the task is
defined only once and startBackgroundProcessing only starts/enables the existing
task rather than redefining it.
| TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME).then((isRegistered) => { | ||
| if (isRegistered) { | ||
| TaskManager.unregisterTaskAsync(QUEUE_PROCESSOR_TASK_NAME); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for background task unregistration.
The background task unregistration lacks error handling. If unregistration fails, it could leave the task registered and potentially cause issues on the next app launch.
// Unregister background task
- TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME).then((isRegistered) => {
- if (isRegistered) {
- TaskManager.unregisterTaskAsync(QUEUE_PROCESSOR_TASK_NAME);
- }
- });
+ TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME)
+ .then(async (isRegistered) => {
+ if (isRegistered) {
+ try {
+ await TaskManager.unregisterTaskAsync(QUEUE_PROCESSOR_TASK_NAME);
+ logger.info({
+ message: 'Background task unregistered successfully',
+ });
+ } catch (error) {
+ logger.error({
+ message: 'Failed to unregister background task',
+ context: { error },
+ });
+ }
+ }
+ })
+ .catch((error) => {
+ logger.error({
+ message: 'Failed to check if background task is registered',
+ context: { error },
+ });
+ });🤖 Prompt for AI Agents
In src/services/offline-queue-processor.ts around lines 335 to 339, the call
that checks and unregisters the background task runs without error handling;
change it to an awaited async flow (or wrap in an immediately-invoked async
function) that first awaits TaskManager.isTaskRegisteredAsync, then if true
awaits TaskManager.unregisterTaskAsync inside a try/catch, and on error logs the
failure (include task name and error) to the module logger/telemetry and
optionally surface or rethrow the error depending on caller expectations; ensure
any Promise rejections are handled so unregistration failures do not go
unobserved.
| export class OfflineQueueService { | ||
| private static instance: OfflineQueueService; | ||
|
|
||
| private constructor() {} | ||
|
|
||
| public static getInstance(): OfflineQueueService { | ||
| if (!OfflineQueueService.instance) { | ||
| OfflineQueueService.instance = new OfflineQueueService(); | ||
| } | ||
| return OfflineQueueService.instance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make initialization idempotent
If initialize() is called multiple times (e.g., layout re-mounts), you may start multiple processors or re-wire listeners.
Apply this diff to add a guard flag:
export class OfflineQueueService {
private static instance: OfflineQueueService;
+ private initialized = false;
private constructor() {}📝 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.
| export class OfflineQueueService { | |
| private static instance: OfflineQueueService; | |
| private constructor() {} | |
| public static getInstance(): OfflineQueueService { | |
| if (!OfflineQueueService.instance) { | |
| OfflineQueueService.instance = new OfflineQueueService(); | |
| } | |
| return OfflineQueueService.instance; | |
| } | |
| export class OfflineQueueService { | |
| private static instance: OfflineQueueService; | |
| private initialized = false; | |
| private constructor() {} | |
| public static getInstance(): OfflineQueueService { | |
| if (!OfflineQueueService.instance) { | |
| OfflineQueueService.instance = new OfflineQueueService(); | |
| } | |
| return OfflineQueueService.instance; | |
| } | |
| // Example initialize method using the guard: | |
| public initialize(): void { | |
| if (this.initialized) { | |
| return; | |
| } | |
| this.initialized = true; | |
| // ... set up processors, listeners, etc. | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/services/offline-queue.service.ts around lines 6 to 16, the class lacks
an idempotent initialize guard so repeated initialize() calls can start
duplicate processors or listeners; add a private boolean flag (e.g., initialized
= false) to the class, have initialize() check the flag and return immediately
if true, set the flag to true after successful initialization, and optionally
reset it to false in any teardown/destroy method so re-initialization after
cleanup remains possible.
| public async initialize(): Promise<void> { | ||
| try { | ||
| logger.info({ | ||
| message: 'Initializing offline queue service', | ||
| }); | ||
|
|
||
| // Initialize the network listener in the store | ||
| const queueStore = useOfflineQueueStore.getState(); | ||
| queueStore.initializeNetworkListener(); | ||
|
|
||
| // Start the queue processor | ||
| await offlineQueueProcessor.startProcessing(); | ||
|
|
||
| logger.info({ | ||
| message: 'Offline queue service initialized successfully', | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Failed to initialize offline queue service', | ||
| context: { error }, | ||
| }); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard repeated initialize calls and await potential async store setup
Add an early return when already initialized, and set the flag upon success. Also safely await initializeNetworkListener if it becomes async.
Apply this diff:
public async initialize(): Promise<void> {
try {
+ if (this.initialized) {
+ logger.info({ message: 'Offline queue service already initialized, skipping' });
+ return;
+ }
logger.info({
message: 'Initializing offline queue service',
});
// Initialize the network listener in the store
const queueStore = useOfflineQueueStore.getState();
- queueStore.initializeNetworkListener();
+ await Promise.resolve(queueStore.initializeNetworkListener?.());
// Start the queue processor
await offlineQueueProcessor.startProcessing();
logger.info({
message: 'Offline queue service initialized successfully',
});
+ this.initialized = true;
} catch (error) {
logger.error({
message: 'Failed to initialize offline queue service',
context: { error },
});
+ this.initialized = false;
throw error;
}
}📝 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.
| public async initialize(): Promise<void> { | |
| try { | |
| logger.info({ | |
| message: 'Initializing offline queue service', | |
| }); | |
| // Initialize the network listener in the store | |
| const queueStore = useOfflineQueueStore.getState(); | |
| queueStore.initializeNetworkListener(); | |
| // Start the queue processor | |
| await offlineQueueProcessor.startProcessing(); | |
| logger.info({ | |
| message: 'Offline queue service initialized successfully', | |
| }); | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Failed to initialize offline queue service', | |
| context: { error }, | |
| }); | |
| throw error; | |
| } | |
| } | |
| public async initialize(): Promise<void> { | |
| try { | |
| if (this.initialized) { | |
| logger.info({ message: 'Offline queue service already initialized, skipping' }); | |
| return; | |
| } | |
| logger.info({ | |
| message: 'Initializing offline queue service', | |
| }); | |
| // Initialize the network listener in the store | |
| const queueStore = useOfflineQueueStore.getState(); | |
| await Promise.resolve(queueStore.initializeNetworkListener?.()); | |
| // Start the queue processor | |
| await offlineQueueProcessor.startProcessing(); | |
| logger.info({ | |
| message: 'Offline queue service initialized successfully', | |
| }); | |
| this.initialized = true; | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Failed to initialize offline queue service', | |
| context: { error }, | |
| }); | |
| this.initialized = false; | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/services/offline-queue.service.ts around lines 18 to 41, the initialize
method needs a guard against repeated calls and should await the store's network
listener if it becomes async; add an early return when an "initialized" flag is
already true, set that flag to true only after successful startup, and change
the call to queueStore.initializeNetworkListener() to await it (i.e., await
queueStore.initializeNetworkListener()) so the method supports async setup;
ensure the flag is set after offlineQueueProcessor.startProcessing() completes
and clear or handle it on failure so repeated attempts behave correctly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| run: | | ||
| set -eo pipefail | ||
| # Grab lines after "## Release Notes" until the next header | ||
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | ||
| | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | ||
| # Use a unique delimiter to write multiline into GITHUB_ENV | ||
| delimiter="EOF_$(date +%s)_$RANDOM" | ||
| { | ||
| echo "RELEASE_NOTES<<$delimiter" | ||
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | ||
| echo "$delimiter" | ||
| } >> "$GITHUB_ENV" |
Check failure
Code scanning / CodeQL
Environment variable built from user-controlled sources Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we must ensure that user-controlled data (the PR body) cannot inject new environment variables or break out of the intended assignment in the $GITHUB_ENV file. The best way to do this is:
- Check that the chosen delimiter does not appear in the extracted release notes. If it does, generate a new delimiter and try again, or sanitize the release notes to remove or escape the delimiter.
- Alternatively, use a cryptographically strong random delimiter (e.g., using
uuidgenif available) to make delimiter collision practically impossible. - Optionally, sanitize the release notes to remove any lines that could be interpreted as environment variable assignments, or to remove newlines if a single-line variable is sufficient.
In this case, the best fix is to use uuidgen to generate a strong, unique delimiter, and to check that the delimiter does not appear in the release notes before writing to $GITHUB_ENV. If uuidgen is not available, fall back to the current method, but still check for delimiter collisions.
Required changes:
- In the step at lines 279-295, update the delimiter generation to use
uuidgenif available. - Add a check to ensure the delimiter does not appear in the release notes. If it does, regenerate the delimiter.
- Optionally, add a maximum number of attempts to avoid infinite loops.
| @@ -285,14 +285,29 @@ | ||
| # Grab lines after "## Release Notes" until the next header | ||
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | ||
| | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | ||
| # Use a unique delimiter to write multiline into GITHUB_ENV | ||
| delimiter="EOF_$(date +%s)_$RANDOM" | ||
| # Use a cryptographically strong unique delimiter and check for collision | ||
| max_attempts=5 | ||
| attempt=0 | ||
| while [ $attempt -lt $max_attempts ]; do | ||
| if command -v uuidgen >/dev/null 2>&1; then | ||
| delimiter="EOF_$(uuidgen)" | ||
| else | ||
| delimiter="EOF_$(date +%s)_$RANDOM" | ||
| fi | ||
| if ! printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | grep -q "$delimiter"; then | ||
| break | ||
| fi | ||
| attempt=$((attempt+1)) | ||
| done | ||
| if [ $attempt -eq $max_attempts ]; then | ||
| echo "Failed to generate a safe delimiter for RELEASE_NOTES" >&2 | ||
| exit 1 | ||
| fi | ||
| { | ||
| echo "RELEASE_NOTES<<$delimiter" | ||
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | ||
| echo "$delimiter" | ||
| } >> "$GITHUB_ENV" | ||
|
|
||
| - name: 📋 Prepare Release Notes file | ||
| if: ${{ matrix.platform == 'android' }} | ||
| run: | |
| // Mock analytics hook | ||
| jest.mock('@/hooks/use-analytics'); | ||
| const mockUseAnalytics = useAnalytics as jest.MockedFunction<typeof useAnalytics>; | ||
|
|
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the incomplete multi-character sanitization, we should ensure that the mock stripHtmlTags function removes all HTML tags robustly, including script tags and edge cases. The best way is to use a well-tested library such as sanitize-html for sanitization, even in mocks. If adding a dependency is not desirable in test code, we can apply the regular expression replacement repeatedly until no more replacements are made, as recommended in the background. This ensures that all tags are removed, even if nested or malformed.
Specifically, in src/components/protocols/__tests__/protocol-details-sheet.test.tsx, update the mock implementation of stripHtmlTags (line 17) to repeatedly apply the replacement until the string no longer changes. No new imports are needed, as this is a simple function.
| @@ -14,7 +14,15 @@ | ||
| jest.mock('@/lib/utils', () => ({ | ||
| formatDateForDisplay: jest.fn((date) => date ? '2023-01-01 12:00 UTC' : ''), | ||
| parseDateISOString: jest.fn((dateString) => dateString ? new Date(dateString) : null), | ||
| stripHtmlTags: jest.fn((html) => html ? html.replace(/<[^>]*>/g, '') : ''), | ||
| stripHtmlTags: jest.fn((html) => { | ||
| if (!html) return ''; | ||
| let previous; | ||
| do { | ||
| previous = html; | ||
| html = html.replace(/<[^>]*>/g, ''); | ||
| } while (html !== previous); | ||
| return html; | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock('nativewind', () => ({ |
|
|
||
| try { | ||
| // Generate a new UUID for this call | ||
| currentCallUUID = this.generateUUID(); |
Check failure
Code scanning / CodeQL
Insecure randomness High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we should ensure that the fallback UUID generation does not use Math.random(), but instead uses a cryptographically secure random number generator. In a React Native environment, the best approach is to use the react-native-get-random-values polyfill (if available) or Node's crypto module for secure random values. Since we cannot assume the presence of external libraries not shown in the code, and we are limited to well-known APIs, we should use the Web Crypto API (crypto.getRandomValues) if available, or Node's crypto.randomBytes if running in a Node environment. The fallback should generate random bytes and use them to construct the UUID, following the RFC4122 version 4 specification.
The changes should be made in the generateUUID() method in src/services/callkeep.service.ios.ts. We need to:
- Add a secure fallback for UUID generation using
crypto.getRandomValues(browser/React Native) orrequire('crypto').randomBytes(Node). - Avoid using
Math.random()for any part of the UUID generation.
| @@ -295,12 +295,44 @@ | ||
| // RN 0.76 typically provides global crypto.randomUUID via Hermes/JSI | ||
| const rndUUID = (global as any)?.crypto?.randomUUID?.(); | ||
| if (typeof rndUUID === 'string') return rndUUID; | ||
| // Fallback | ||
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { | ||
| const r = (Math.random() * 16) | 0; | ||
| const v = c === 'x' ? r : (r & 0x3) | 0x8; | ||
| return v.toString(16); | ||
| }); | ||
| // Fallback: Use cryptographically secure random values | ||
| let getRandomValues: ((arr: Uint8Array) => Uint8Array) | null = null; | ||
| if (typeof global !== 'undefined' && global.crypto && typeof global.crypto.getRandomValues === 'function') { | ||
| getRandomValues = global.crypto.getRandomValues.bind(global.crypto); | ||
| } else if (typeof require === 'function') { | ||
| try { | ||
| // Node.js environment | ||
| const nodeCrypto = require('crypto'); | ||
| getRandomValues = (arr: Uint8Array) => { | ||
| const buf = nodeCrypto.randomBytes(arr.length); | ||
| arr.set(buf); | ||
| return arr; | ||
| }; | ||
| } catch (e) { | ||
| // No secure random available | ||
| getRandomValues = null; | ||
| } | ||
| } | ||
| if (getRandomValues) { | ||
| // Generate RFC4122 version 4 UUID | ||
| const bytes = getRandomValues(new Uint8Array(16)); | ||
| // Set version bits (4) and variant bits (RFC4122) | ||
| bytes[6] = (bytes[6] & 0x0f) | 0x40; | ||
| bytes[8] = (bytes[8] & 0x3f) | 0x80; | ||
| const hex: string[] = []; | ||
| for (let i = 0; i < bytes.length; i++) { | ||
| hex.push(bytes[i].toString(16).padStart(2, '0')); | ||
| } | ||
| return ( | ||
| hex.slice(0, 4).join('') + '-' + | ||
| hex.slice(4, 6).join('') + '-' + | ||
| hex.slice(6, 8).join('') + '-' + | ||
| hex.slice(8, 10).join('') + '-' + | ||
| hex.slice(10, 16).join('') | ||
| ); | ||
| } | ||
| // If no secure random available, throw error | ||
| throw new Error('No cryptographically secure random number generator available for UUID generation'); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Actionable comments posted: 90
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
docs/compose-message-sheet-analytics-implementation.md (1)
137-153: Remove PII: raw recipient IDs must not be sentWe’ve confirmed that all 8 analytics events are implemented and exercised by tests. However, the payload for the
compose_message_recipient_toggledevent still includes a rawrecipientId, which exposes user‐identifiable information. Please remove or anonymize this field.• In
src/components/messages/compose-message-sheet.tsx, lines 227–231 (inside thetrackEvent('compose_message_recipient_toggled', { … })call), remove or replace:trackEvent('compose_message_recipient_toggled', { timestamp: new Date().toISOString(), - recipientId: id, + // recipientId: id, ← remove or replace with hashed/pseudonymous ID or count });Consider sending a hashed/pseudonymous identifier or simply the total count of selected recipients instead.
src/app/(app)/shifts.tsx (2)
54-57: Prevent crash when filtering by code: guard against undefined/nullCode
shift.Code.toLowerCase()will throw ifCodeis null/undefined. Harden the filter to avoid runtime crashes with real backend data.Apply this diff:
- if (!searchQuery.trim()) return shifts; - const query = searchQuery.trim().toLowerCase(); - return shifts.filter((shift) => shift.Name.toLowerCase().includes(query) || shift.Code.toLowerCase().includes(query)); + const q = searchQuery.trim().toLowerCase(); + if (!q) return shifts; + return shifts.filter((shift) => { + const name = (shift.Name || '').toLowerCase(); + const code = (shift.Code || '').toLowerCase(); + return name.includes(q) || code.includes(q); + });
59-63: Defensive filter for today’s shifts: guardShiftNameaccess
shiftDay.ShiftName.toLowerCase()will throw ifShiftNameis null/undefined. Add a safe fallback.Apply this diff:
- if (!searchQuery.trim()) return todaysShiftDays; - const query = searchQuery.trim().toLowerCase(); - return todaysShiftDays.filter((shiftDay) => shiftDay.ShiftName.toLowerCase().includes(query)); + const q = searchQuery.trim().toLowerCase(); + if (!q) return todaysShiftDays; + return todaysShiftDays.filter((shiftDay) => (shiftDay.ShiftName || '').toLowerCase().includes(q));src/app/(app)/__tests__/shifts.test.tsx (1)
143-155: Incorrect mock target: mockingzero-state-exampleinstead ofzero-stateProduction imports
@/components/common/zero-state, but the test mockszero-state-example. This likely breaks zero-state assertions (getByTestId('zero-state')) or makes tests rely on the real component.Apply this diff:
-jest.mock('@/components/common/zero-state-example', () => { +jest.mock('@/components/common/zero-state', () => { const React = require('react'); const { View, Text } = require('react-native'); return { __esModule: true, default: ({ title }: any) => React.createElement( View, { testID: 'zero-state' }, React.createElement(Text, null, title) ), }; });src/app/onboarding.tsx (2)
66-72: Prevent analytics failures from crashing the UI (wrap trackEvent in a safe helper).If
trackEventthrows (network, provider init, etc.), the screen can crash (your tests even simulate this). Add a small safe wrapper and reuse it for all emissions.Apply this diff to introduce a safe wrapper and strengthen types on refs:
- const { trackEvent } = useAnalytics(); - const router = useRouter(); - const [currentIndex, setCurrentIndex] = useState(0); - const flatListRef = useRef<FlatList>(null); - const buttonOpacity = useSharedValue(0); - const { colorScheme } = useColorScheme(); + const { trackEvent } = useAnalytics(); + const safeTrack = useCallback( + (name: string, props: Record<string, any>) => { + try { + trackEvent(name, props); + } catch (_err) { + // Swallow analytics errors to avoid affecting the UX + } + }, + [trackEvent] + ); + const router = useRouter(); + const [currentIndex, setCurrentIndex] = useState(0); + const flatListRef = useRef<FlatList<OnboardingItemProps>>(null); + const lastIndexRef = useRef(0); + const buttonOpacity = useSharedValue(0); + const { colorScheme } = useColorScheme();
88-109: Deduplicate slide-change analytics; current state-based guard can double-fire during fast scroll.Using
currentIndexinside the scroll handler can emit multiple identical events before state updates. Track the last sent index via a ref and only emit when the index actually changes. Also, use the proper RN scroll event type.- const handleScroll = (event: { nativeEvent: { contentOffset: { x: number } } }) => { - const index = Math.round(event.nativeEvent.contentOffset.x / width); - const wasLastIndex = currentIndex; - setCurrentIndex(index); - - // Analytics: Track slide changes - if (index !== wasLastIndex) { - trackEvent('onboarding_slide_changed', { - timestamp: new Date().toISOString(), - fromSlide: wasLastIndex, - toSlide: index, - slideTitle: onboardingData[index]?.title || 'Unknown', - }); - } + const handleScroll = (event: NativeSyntheticEvent<NativeScrollEvent>) => { + const index = Math.round(event.nativeEvent.contentOffset.x / width); + const from = lastIndexRef.current; + if (index !== from) { + lastIndexRef.current = index; + setCurrentIndex(index); + // Analytics: Track slide changes + safeTrack('onboarding_slide_changed', { + timestamp: new Date().toISOString(), + fromSlide: from, + toSlide: index, + slideTitle: onboardingData[index]?.title || 'Unknown', + }); + } // Show button with animation when on the last slide if (index === onboardingData.length - 1) { buttonOpacity.value = withTiming(1, { duration: 500 }); } else { buttonOpacity.value = withTiming(0, { duration: 300 }); } };Additionally, update the import to use proper types (outside these lines):
// at top-level imports import { Dimensions, FlatList, Image, NativeScrollEvent, NativeSyntheticEvent } from 'react-native';.github/workflows/react-native-cicd.yml (1)
191-198: Quote and safely write GRADLE_PROPERTIES to preserve content and avoid word-splitting.Unquoted variable expansion with echo may mangle whitespace or special characters. Use printf with quotes.
mkdir -p ~/.gradle/ - echo ${GRADLE_PROPERTIES} > ~/.gradle/gradle.properties + printf '%s' "${GRADLE_PROPERTIES}" > ~/.gradle/gradle.propertiesapp.config.ts (1)
74-83: Missing Android 12+ Bluetooth permissions; remove unsupported CAPTURE_AUDIO_OUTPUT.
- BLE on Android 12+ requires BLUETOOTH_CONNECT and BLUETOOTH_SCAN (and optionally BLUETOOTH_ADVERTISE). Without these, scanning/connecting will fail at runtime.
- android.permission.CAPTURE_AUDIO_OUTPUT is a signature-level permission and cannot be granted to third-party apps; keeping it may cause Play Console warnings/rejections.
permissions: [ 'android.permission.WAKE_LOCK', 'android.permission.RECORD_AUDIO', - 'android.permission.CAPTURE_AUDIO_OUTPUT', 'android.permission.POST_NOTIFICATIONS', 'android.permission.FOREGROUND_SERVICE', 'android.permission.FOREGROUND_SERVICE_MICROPHONE', 'android.permission.FOREGROUND_SERVICE_CONNECTED_DEVICE', 'android.permission.FOREGROUND_SERVICE_MEDIA_PLAYBACK', + // BLE permissions (Android 12+) + 'android.permission.BLUETOOTH_CONNECT', + 'android.permission.BLUETOOTH_SCAN', + 'android.permission.BLUETOOTH_ADVERTISE', + // Legacy BLE (pre-Android 12) for broader compatibility + 'android.permission.BLUETOOTH', + 'android.permission.BLUETOOTH_ADMIN', ],If location is also used for BLE scanning on older Android, ACCESS_FINE_LOCATION may be needed, but expo-location likely already adds it via its plugin.
src/app/call/[id]/edit.tsx (1)
210-213: Incorrect truthiness check drops valid 0.0 coordinatesUsing
if (selectedLocation?.latitude && selectedLocation?.longitude)will skip setting coordinates at 0 or 0.0. Use explicit null/undefined checks.Apply this fix:
- if (selectedLocation?.latitude && selectedLocation?.longitude) { - data.latitude = selectedLocation.latitude; - data.longitude = selectedLocation.longitude; - } + if ( + selectedLocation && + typeof selectedLocation.latitude === 'number' && + typeof selectedLocation.longitude === 'number' + ) { + data.latitude = selectedLocation.latitude; + data.longitude = selectedLocation.longitude; + }src/app/login/index.tsx (1)
1-117: PII in analytics events — raw username is being sent (action required)Verified: repo contains production analytics calls that include the raw username and multiple tests that assert that field. This is PII and should not be forwarded to the analytics provider.
Files that need attention
- src/app/login/index.tsx — trackEvent('login_attempted', { username: data.username }) (production)
- src/app/(app)/settings.tsx — trackEvent('settings_login_info_updated', { username: data.username }) (production)
- src/services/aptabase.service.ts — central analytics wrapper (add scrub here if you prefer a single enforcement point)
- Tests that will need updates if payloads change:
- src/app/login/tests/index-analytics-simple.test.tsx
- src/app/login/tests/index-analytics-integration.test.ts
- src/app/login/tests/index.test.tsx
- src/components/settings/tests/login-info-bottom-sheet.test.tsx
- (and other tests that assert username in analytics — search for username in test files)
Recommended fixes (pick one or combine)
- Remove raw username from analytics payloads. Replace with a non-identifying field, e.g. hasUsername: !!data.username or username_present: true.
- OR send only a pseudonymous identifier (non-reversible, per-tenant/per-installation ID) if you must correlate users — do not send cleartext username or email.
- Enforce scrubbing centrally in src/services/aptabase.service.ts (drop known PII keys: username, email, password, token, ssn) before calling the external trackEvent.
- Update affected tests to assert the new payload shape.
If you want, I can:
- open a PR that (A) removes username from the two production call sites and (B) adds a PII-stripper in src/services/aptabase.service.ts and updates tests accordingly.
src/app/call/[id].tsx (1)
170-174: Same coordinate falsy-check bug in route guard.A 0/0.0 coordinate will incorrectly block routing. Use null checks.
- if (!coordinates.latitude || !coordinates.longitude) { + if (coordinates.latitude === null || coordinates.longitude === null) { showToast('error', t('call_detail.no_location_for_routing')); return; }src/app/(app)/map.tsx (2)
65-105: Prevent multiple map_viewed emissions and unintended resets while focusedThe useFocusEffect depends on isMapReady, location.latitude/longitude/isMapLocked/heading, and trackEvent. While focused, any of these changes will re-run the effect, re-emitting map_viewed and re-running “focus-time” reset logic (which can fight the user).
Emit map_viewed only on focus transitions, and decouple the focus reset from dynamic dependencies. Use useIsFocused for the view event and read latest store values at focus time to avoid stale closures.
Apply this diff:
@@ -import { useFocusEffect } from 'expo-router'; +import { useFocusEffect } from 'expo-router'; +import { useIsFocused } from '@react-navigation/native'; @@ const { t } = useTranslation(); const { trackEvent } = useAnalytics(); + const isFocused = useIsFocused(); @@ - // Handle navigation focus - reset map state when user navigates back to map page - useFocusEffect( - useCallback(() => { - // Track analytics when view becomes visible - trackEvent('map_viewed', { - timestamp: new Date().toISOString(), - isMapLocked: location.isMapLocked, - hasLocation: !!(location.latitude && location.longitude), - }); - - // Reset hasUserMovedMap when navigating back to map - setHasUserMovedMap(false); - - // Reset camera to current location when navigating back to map - if (isMapReady && location.latitude && location.longitude) { - const cameraConfig: any = { - centerCoordinate: [location.longitude, location.latitude], - zoomLevel: location.isMapLocked ? 16 : 12, - animationDuration: 1000, - heading: 0, - pitch: 0, - }; - - // Add heading and pitch for navigation mode when locked - if (location.isMapLocked && location.heading !== null && location.heading !== undefined) { - cameraConfig.heading = location.heading; - cameraConfig.pitch = 45; - } - - cameraRef.current?.setCamera(cameraConfig); - - logger.info({ - message: 'Home Map focused, resetting camera to current location', - context: { - latitude: location.latitude, - longitude: location.longitude, - isMapLocked: location.isMapLocked, - }, - }); - } - }, [isMapReady, location.latitude, location.longitude, location.isMapLocked, location.heading, trackEvent]) - ); + // Emit analytics only when the screen becomes focused (avoid dependency-driven re-fires) + useEffect(() => { + if (isFocused) { + const { latitude, longitude, isMapLocked } = useLocationStore.getState(); + trackEvent('map_viewed', { + timestamp: new Date().toISOString(), + isMapLocked, + hasLocation: latitude != null && longitude != null, + }); + } + }, [isFocused, trackEvent]); + + // Handle navigation focus - reset local map state and camera once on focus + useFocusEffect( + useCallback(() => { + // Reset hasUserMovedMap when navigating back to map + setHasUserMovedMap(false); + + // Reset camera to current location when navigating back to map + const { latitude, longitude, isMapLocked, heading } = useLocationStore.getState(); + if (isMapReady && latitude != null && longitude != null) { + const cameraConfig: any = { + centerCoordinate: [longitude, latitude], + zoomLevel: isMapLocked ? 16 : 12, + animationDuration: 1000, + heading: 0, + pitch: 0, + }; + + // Add heading and pitch for navigation mode when locked + if (isMapLocked && heading !== null && heading !== undefined) { + cameraConfig.heading = heading; + cameraConfig.pitch = 45; + } + + cameraRef.current?.setCamera(cameraConfig); + + logger.info({ + message: 'Home Map focused, resetting camera to current location', + context: { + latitude, + longitude, + isMapLocked, + }, + }); + } + return undefined; + }, [isMapReady]) + );
1-489: Use explicit nullish checks for latitude/longitude across the repoSeveral components and analytics calls currently use truthiness checks (
latitude && longitudeor!!(latitude && longitude)) to guard on coordinates. Because0is falsy, valid coordinates on the equator or prime meridian would be treated as “no location.” To avoid this bug, replace these with explicit nullish checks (latitude != null && longitude != nullorNumber.isFinite(latitude) && Number.isFinite(longitude)).Affected locations include (but may not be limited to):
- src/components/sidebar/call-sidebar.tsx:79
- src/api/calls/callNotes.ts:24
- src/api/calls/callFiles.ts:140
- src/app/call/[id].tsx:138, 181, 540
- src/app/call/new/index.tsx:234
- src/app/(app)/map.tsx:71, 78, 134, 173, 229, 307, 345
- Various analytics‐integration tests under src/app/call/**/tests
Action items:
- Update each guard to use
latitude != null && longitude != null(or equivalent).- Ensure analytics payloads (
hasCoordinates,hasLocation, etc.) also use nullish checks.- Add or update unit tests to cover edge cases where latitude or longitude is
0.src/app/(app)/__tests__/map.test.tsx (2)
112-136: Make MapView mock trigger user interaction to deterministically surface the recenter button and eventRight now, MapView is a passive container and never calls onCameraChanged. As a result, hasUserMovedMap never flips to true, so tests that depend on the recenter button being rendered end up conditional or inert. Drive the callback once on mount to simulate a user gesture and make the recenter path testable and deterministic.
Apply this diff to your Mapbox mock:
jest.mock('@rnmapbox/maps', () => ({ __esModule: true, default: { setAccessToken: jest.fn(), StyleURL: { Street: 'mapbox://styles/mapbox/streets-v11', Satellite: 'mapbox://styles/mapbox/satellite-v9', }, - MapView: ({ children, testID }: any) => { - const { View } = require('react-native'); - return <View testID={testID}>{children}</View>; - }, + MapView: (props: any) => { + const React = require('react'); + const { View } = require('react-native'); + // Simulate a single user interaction after mount + React.useEffect(() => { + props.onCameraChanged?.({ properties: { isUserInteraction: true } }); + // Also mark map load complete so isMapReady paths are unblocked + props.onDidFinishLoadingMap?.(); + }, []); + return <View testID={props.testID}>{props.children}</View>; + }, Camera: ({ children }: any) => { const { View } = require('react-native'); return <View testID="map-camera">{children}</View>; }, PointAnnotation: ({ children, testID }: any) => { const { View } = require('react-native'); return <View testID={testID || 'point-annotation'}>{children}</View>; }, UserTrackingMode: { FollowWithHeading: 'follow-with-heading', }, }, }));
372-390: Test name vs. assertion mismatch: either rename or assert presence after simulated movementThe test is titled “shows recenter button when user has moved map and location is available” but only asserts that the button is not visible. After enabling simulated user interaction (see MapView mock refactor), you should assert the button appears.
- it('shows recenter button when user has moved map and location is available', async () => { + it('shows recenter button when user has moved map and location is available', async () => { const mockLocationStore = jest.requireMock('@/stores/app/location-store') as any; mockLocationStore.useLocationStore.mockReturnValue({ latitude: 40.7128, longitude: -74.006, heading: 90, isMapLocked: false, }); render(<HomeMap />); // Simulate map ready await waitFor(() => { expect(screen.getByTestId('home-map-view')).toBeTruthy(); }); - // Initially, recenter button should not be visible - expect(screen.queryByTestId('recenter-button')).toBeNull(); + // After user interaction is simulated by the MapView mock, the recenter button should appear + await waitFor(() => { + expect(screen.getByTestId('recenter-button')).toBeTruthy(); + }); });src/app/(app)/messages.tsx (1)
69-89: messages_viewed will re-fire whenever the message count changes; scope analytics to focus onlyIncluding filteredMessages.length in the useFocusEffect dependency array will retrigger the focus effect (and the messages_viewed event) when the list changes while the screen is focused, inflating your analytics. If the intent is to track a “screen viewed” event, it should fire on focus only. Consider removing filteredMessages.length from the dependencies (and, if desired, emitting a separate “messages_count_changed” event via a plain useEffect).
useFocusEffect( useCallback(() => { // Track analytics when view becomes visible trackEvent('messages_viewed', { timestamp: new Date().toISOString(), currentFilter, messageCount: filteredMessages.length, }); if (currentFilter === 'sent') { fetchSentMessages(); } else if (currentFilter === 'inbox') { fetchInboxMessages(); } else { // For 'all', fetch both inbox and sent messages fetchInboxMessages(); fetchSentMessages(); } - }, [fetchInboxMessages, fetchSentMessages, currentFilter, trackEvent, filteredMessages.length]) + }, [fetchInboxMessages, fetchSentMessages, currentFilter, trackEvent]) );Optionally, if you want to track list changes independently:
useEffect(() => { trackEvent('messages_count_changed', { timestamp: new Date().toISOString(), currentFilter, messageCount: filteredMessages.length, }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [filteredMessages.length]);
♻️ Duplicate comments (2)
.github/workflows/react-native-cicd.yml (1)
157-166: Android versionCode computation and jq update look correct (resolves prior issue).Using a numeric androidVersionCode and a single jq invocation with --arg/--argjson is the right fix. This addresses the previously flagged non-expanded string and type issue.
app.config.ts (1)
66-66: Guard against NaN/invalid Android versionCode (duplicate of prior feedback).parseInt(packageJSON.versionCode) can yield NaN or an unintended value if undefined or non-numeric. This will break EAS/Gradle.
Apply this diff:
- versionCode: parseInt(packageJSON.versionCode), + versionCode: Number.isInteger(Number(packageJSON.versionCode)) + ? parseInt(String(packageJSON.versionCode), 10) + : 1,
| - name: 📋 Extract Release Notes from PR Body | ||
| if: ${{ matrix.platform == 'android' }} | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} | ||
| run: | | ||
| set -eo pipefail | ||
| # Grab lines after "## Release Notes" until the next header | ||
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | ||
| | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | ||
| # Use a unique delimiter to write multiline into GITHUB_ENV | ||
| delimiter="EOF_$(date +%s)_$RANDOM" | ||
| { | ||
| echo "RELEASE_NOTES<<$delimiter" | ||
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | ||
| echo "$delimiter" | ||
| } >> "$GITHUB_ENV" | ||
There was a problem hiding this comment.
Avoid writing PR body into GITHUB_ENV; write directly to a file to eliminate env injection risk.
PR bodies are user-controlled on pull_request and workflow_dispatch. Even with a randomized delimiter, writing arbitrary content into GITHUB_ENV is unnecessary here and flagged by CodeQL. Generate the release notes file directly and drop the environment variable.
Apply this diff to remove the env write step:
- - name: 📋 Extract Release Notes from PR Body
- if: ${{ matrix.platform == 'android' }}
- env:
- PR_BODY: ${{ github.event.pull_request.body }}
- run: |
- set -eo pipefail
- # Grab lines after "## Release Notes" until the next header
- RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \
- | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')"
- # Use a unique delimiter to write multiline into GITHUB_ENV
- delimiter="EOF_$(date +%s)_$RANDOM"
- {
- echo "RELEASE_NOTES<<$delimiter"
- printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}"
- echo "$delimiter"
- } >> "$GITHUB_ENV"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 283-294: Environment variable built from user-controlled sources
Potential environment variable injection in [set -eo pipefail
Grab lines after "## Release Notes" until the next header
RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \
| awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')"
Use a unique delimiter to write multiline into GITHUB_ENV
delimiter="EOF_$(date +%s)_$RANDOM"
{
echo "RELEASE_NOTES<<$delimiter"
printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}"
echo "$delimiter"
} >> "$GITHUB_ENV"](1), which may be controlled by an external user (workflow_dispatch).
🤖 Prompt for AI Agents
.github/workflows/react-native-cicd.yml around lines 279-295: the job currently
extracts the "## Release Notes" section from the PR body and writes it into
GITHUB_ENV using a randomized delimiter, which risks env injection; instead,
write the extracted release notes directly to a file in the workspace (for
example, ./release-notes.txt or ./release-notes.md) and remove the GITHUB_ENV
write and delimiter logic; keep the same extraction logic but redirect output to
a file (with a safe default like "No release notes provided." when empty) and
update any downstream steps to read that file rather than relying on an
environment variable.
| - name: 📋 Prepare Release Notes file | ||
| if: ${{ matrix.platform == 'android' }} | ||
| run: | | ||
| { | ||
| echo "## Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)" | ||
| echo | ||
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | ||
| } > RELEASE_NOTES.md | ||
There was a problem hiding this comment.
Build the RELEASE_NOTES.md directly (no dependency on env state).
Inline the extraction in this step and write the file directly.
- - name: 📋 Prepare Release Notes file
- if: ${{ matrix.platform == 'android' }}
- run: |
- {
- echo "## Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)"
- echo
- printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}"
- } > RELEASE_NOTES.md
+ - name: 📋 Prepare Release Notes file
+ if: ${{ matrix.platform == 'android' }}
+ env:
+ PR_BODY: ${{ github.event.pull_request.body }}
+ run: |
+ set -eo pipefail
+ NOTES="$(printf '%s\n' "$PR_BODY" | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')"
+ {
+ echo "## Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)"
+ echo
+ if [ -n "$NOTES" ]; then
+ printf '%s\n' "$NOTES"
+ else
+ echo "No release notes provided."
+ fi
+ } > RELEASE_NOTES.md📝 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.
| - name: 📋 Prepare Release Notes file | |
| if: ${{ matrix.platform == 'android' }} | |
| run: | | |
| { | |
| echo "## Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)" | |
| echo | |
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | |
| } > RELEASE_NOTES.md | |
| - name: 📋 Prepare Release Notes file | |
| if: ${{ matrix.platform == 'android' }} | |
| env: | |
| PR_BODY: ${{ github.event.pull_request.body }} | |
| run: | | |
| set -eo pipefail | |
| NOTES="$(printf '%s\n' "$PR_BODY" | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | |
| { | |
| echo "## Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)" | |
| echo | |
| if [ -n "$NOTES" ]; then | |
| printf '%s\n' "$NOTES" | |
| else | |
| echo "No release notes provided." | |
| fi | |
| } > RELEASE_NOTES.md |
🤖 Prompt for AI Agents
In .github/workflows/react-native-cicd.yml around lines 296–304, replace the
current step that writes RELEASE_NOTES.md using the RELEASE_NOTES env var with
an inline generation that writes the file directly: create the header ("##
Version 10.${{ github.run_number }} - $(date +%Y-%m-%d)"), then append extracted
release notes obtained inline (prefer github.event.inputs.release_notes or
github.event.pull_request.body if present, otherwise fall back to recent commit
messages via git log), and write the combined content to RELEASE_NOTES.md in
this step (fail the job if extraction fails).
| microphonePermission: 'Allow Resgrid Responder to access the microphone for audio input used in PTT and calls.', | ||
| }, | ||
| ], | ||
| 'react-native-ble-manager', |
There was a problem hiding this comment.
Incorrect Expo config plugin ID for BLE Manager.
'react-native-ble-manager' is a library name, not the Expo config plugin. As-is, no native config changes will be applied (permissions, Gradle settings, etc.). Use the config plugin package.
- 'react-native-ble-manager',
+ '@config-plugins/react-native-ble-manager',If you intentionally rely on manual permissions instead of a config plugin, please confirm and document it.
📝 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.
| 'react-native-ble-manager', | |
| '@config-plugins/react-native-ble-manager', |
🤖 Prompt for AI Agents
In app.config.ts around line 262, the entry 'react-native-ble-manager' is the
npm library name, not an Expo config plugin, so no native changes will be
applied; fix by replacing that string with the library's actual Expo config
plugin package/name as documented by the BLE library (or the plugin path it
exports) so permissions/Gradle/manifest edits are applied automatically; if the
BLE library has no config plugin, remove this incorrect entry and add a clear
comment in the repo (and README) stating you are handling all Android/iOS
permissions and native setup manually, listing the exact manual steps and
required manifest/Gradle edits.
| // Close button | ||
| onPress={() => { | ||
| trackEvent('audio_stream_bottom_sheet_closed', { | ||
| timestamp: new Date().toISOString(), | ||
| hasCurrentStream: !!currentStream, | ||
| isPlaying, | ||
| timeSpent: Date.now(), // Could be improved with actual time tracking | ||
| }); | ||
|
|
||
| setIsBottomSheetVisible(false); | ||
| }} | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: timeSpent example is incorrect (uses absolute epoch time).
timeSpent should be the elapsed duration the bottom sheet was open, not Date.now(). Recommend documenting the correct pattern with an open timestamp and computing the delta on close.
Apply this diff to the example:
- timeSpent: Date.now(), // Could be improved with actual time tracking
+ // Compute actual time spent (ms) since the bottom sheet became visible
+ timeSpent: Date.now() - openedAtTimestamp,And add a short snippet demonstrating how to capture openedAtTimestamp:
// On open
const openedAtRef = React.useRef<number | null>(null);
useEffect(() => {
if (isBottomSheetVisible) {
openedAtRef.current = Date.now();
} else {
openedAtRef.current = null;
}
}, [isBottomSheetVisible]);🤖 Prompt for AI Agents
In docs/audio-stream-analytics-implementation.md around lines 218 to 229, the
example incorrectly sends timeSpent as an absolute epoch (Date.now()); change it
to compute elapsed time by storing an openedAt timestamp when the bottom sheet
opens (e.g., in a React ref) and computing timeSpent = Date.now() - openedAt
(with a safe fallback) when closing; update the onPress trackEvent to use that
delta and add a short snippet showing how to set openedAt on open and clear it
on close.
| - `hasPreferredDevice`: Boolean indicating if user has a preferred device set | ||
| - `preferredDeviceId`: ID of the preferred device (empty string if none) | ||
| - `connectedDeviceId`: ID of currently connected device (empty string if none) | ||
| - `bluetoothState`: Current Bluetooth adapter state |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace raw device IDs with anonymized hashes
These properties may capture persistent hardware identifiers. Use anonymized variants to avoid sending raw IDs.
-- `preferredDeviceId`: ID of the preferred device (empty string if none)
-- `connectedDeviceId`: ID of currently connected device (empty string if none)
+- `preferredDeviceHash`: Anonymized, salted hash of the preferred device identifier (empty string if none)
+- `connectedDeviceHash`: Anonymized, salted hash of the currently connected device identifier (empty string if none)📝 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.
| - `hasPreferredDevice`: Boolean indicating if user has a preferred device set | |
| - `preferredDeviceId`: ID of the preferred device (empty string if none) | |
| - `connectedDeviceId`: ID of currently connected device (empty string if none) | |
| - `bluetoothState`: Current Bluetooth adapter state | |
| - `hasPreferredDevice`: Boolean indicating if user has a preferred device set | |
| - `preferredDeviceHash`: Anonymized, salted hash of the preferred device identifier (empty string if none) | |
| - `connectedDeviceHash`: Anonymized, salted hash of the currently connected device identifier (empty string if none) | |
| - `bluetoothState`: Current Bluetooth adapter state |
🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: There might be a mistake here.
Context: ...ating if user has a preferred device set - preferredDeviceId: ID of the preferred device (empty stri...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ... preferred device (empty string if none) - connectedDeviceId: ID of currently connected device (empt...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... connected device (empty string if none) - bluetoothState: Current Bluetooth adapter state - `has...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...hState: Current Bluetooth adapter state - hasConnectionError`: Boolean indicating if there's a connec...
(QB_NEW_EN)
| @@ -0,0 +1,251 @@ | |||
| import React from 'react'; | |||
| import { render } from '@testing-library/react-native'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import waitFor to avoid race conditions with useEffect-driven tracking.
Analytics is typically emitted in a useEffect; asserting immediately can be flaky. Import waitFor here and use it in the relevant tests.
-import { render } from '@testing-library/react-native';
+import { render, waitFor } from '@testing-library/react-native';📝 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.
| import { render } from '@testing-library/react-native'; | |
| import { render, waitFor } from '@testing-library/react-native'; |
🤖 Prompt for AI Agents
In
src/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsx
around line 2, the test imports only render which can cause flaky assertions for
analytics emitted from useEffect; add an import for waitFor from
'@testing-library/react-native' and wrap the analytics assertions inside waitFor
(or replace immediate expects with await waitFor(() => { /* assertions */ })) so
the test waits for the useEffect-driven tracking to run before asserting.
| it('tracks analytics when sheet becomes visible', () => { | ||
| render( | ||
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | ||
| ); | ||
|
|
||
| expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', { | ||
| itemId: 'test-item-1', | ||
| itemType: 1, | ||
| hasLocation: true, | ||
| hasDescription: true, | ||
| isAllDay: false, | ||
| canSignUp: true, | ||
| isSignedUp: false, | ||
| attendeeCount: 1, | ||
| signupType: 1, | ||
| typeName: 'Meeting', | ||
| timestamp: expect.any(String), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await analytics emission and assert ISO timestamp format.
Guard against effect timing and validate timestamp quality.
-it('tracks analytics when sheet becomes visible', () => {
- render(
- <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} />
- );
-
- expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', {
- itemId: 'test-item-1',
- itemType: 1,
- hasLocation: true,
- hasDescription: true,
- isAllDay: false,
- canSignUp: true,
- isSignedUp: false,
- attendeeCount: 1,
- signupType: 1,
- typeName: 'Meeting',
- timestamp: expect.any(String),
- });
-});
+it('tracks analytics when sheet becomes visible', async () => {
+ render(
+ <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} />
+ );
+
+ await waitFor(() => {
+ expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', expect.objectContaining({
+ itemId: 'test-item-1',
+ itemType: 1,
+ hasLocation: true,
+ hasDescription: true,
+ isAllDay: false,
+ canSignUp: true,
+ isSignedUp: false,
+ attendeeCount: 1,
+ signupType: 1,
+ typeName: 'Meeting',
+ timestamp: expect.any(String),
+ }));
+ });
+
+ const [, payload] = mockTrackEvent.mock.calls[0];
+ expect(payload.timestamp).toMatch(ISO_8601_RE);
+});📝 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.
| it('tracks analytics when sheet becomes visible', () => { | |
| render( | |
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | |
| ); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', { | |
| itemId: 'test-item-1', | |
| itemType: 1, | |
| hasLocation: true, | |
| hasDescription: true, | |
| isAllDay: false, | |
| canSignUp: true, | |
| isSignedUp: false, | |
| attendeeCount: 1, | |
| signupType: 1, | |
| typeName: 'Meeting', | |
| timestamp: expect.any(String), | |
| }); | |
| }); | |
| it('tracks analytics when sheet becomes visible', async () => { | |
| render( | |
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | |
| ); | |
| await waitFor(() => { | |
| expect(mockTrackEvent).toHaveBeenCalledWith( | |
| 'calendar_item_details_viewed', | |
| expect.objectContaining({ | |
| itemId: 'test-item-1', | |
| itemType: 1, | |
| hasLocation: true, | |
| hasDescription: true, | |
| isAllDay: false, | |
| canSignUp: true, | |
| isSignedUp: false, | |
| attendeeCount: 1, | |
| signupType: 1, | |
| typeName: 'Meeting', | |
| timestamp: expect.any(String), | |
| }) | |
| ); | |
| }); | |
| const [, payload] = mockTrackEvent.mock.calls[0]; | |
| expect(payload.timestamp).toMatch(ISO_8601_RE); | |
| }); |
🤖 Prompt for AI Agents
In
src/components/calendar/__tests__/calendar-item-details-sheet-analytics.test.tsx
around lines 156 to 174, the test currently asserts mockTrackEvent immediately
and only checks timestamp is any String; update the test to wait for the
analytics effect to run (e.g., use async test with await waitFor or similar)
before asserting, and replace expect.any(String) with a strict ISO timestamp
check (e.g., match against /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/ or
use Date.parse to assert validity) so the test both waits for emission and
verifies ISO format for the timestamp.
| @@ -0,0 +1,170 @@ | |||
| import React from 'react'; | |||
| import { render } from '@testing-library/react-native'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import waitFor for effect timing.
Mirror the approach in the other test to avoid races.
-import { render } from '@testing-library/react-native';
+import { render, waitFor } from '@testing-library/react-native';📝 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.
| import { render } from '@testing-library/react-native'; | |
| import { render, waitFor } from '@testing-library/react-native'; |
🤖 Prompt for AI Agents
In
src/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsx
around line 2, the test is missing an import of waitFor which can cause
timing/race issues; add waitFor to the import from
'@testing-library/react-native' and update the test to use await waitFor(...)
around the assertions or any expectation that depends on effects/async updates
(mirroring the other test) so effects settle before asserting.
| it('tracks analytics when sheet becomes visible', () => { | ||
| render( | ||
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | ||
| ); | ||
|
|
||
| expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', { | ||
| itemId: 'test-item-1', | ||
| itemType: 1, | ||
| hasLocation: true, | ||
| hasDescription: true, | ||
| isAllDay: false, | ||
| canSignUp: true, | ||
| isSignedUp: false, | ||
| attendeeCount: 0, | ||
| signupType: 1, | ||
| typeName: 'Meeting', | ||
| timestamp: expect.any(String), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await analytics emission and assert ISO timestamp format.
Strengthen the test and reduce flakiness.
-it('tracks analytics when sheet becomes visible', () => {
- render(
- <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} />
- );
-
- expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', {
- itemId: 'test-item-1',
- itemType: 1,
- hasLocation: true,
- hasDescription: true,
- isAllDay: false,
- canSignUp: true,
- isSignedUp: false,
- attendeeCount: 0,
- signupType: 1,
- typeName: 'Meeting',
- timestamp: expect.any(String),
- });
-});
+it('tracks analytics when sheet becomes visible', async () => {
+ render(
+ <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} />
+ );
+ await waitFor(() =>
+ expect(mockTrackEvent).toHaveBeenCalledWith(
+ 'calendar_item_details_viewed',
+ expect.objectContaining({
+ itemId: 'test-item-1',
+ itemType: 1,
+ hasLocation: true,
+ hasDescription: true,
+ isAllDay: false,
+ canSignUp: true,
+ isSignedUp: false,
+ attendeeCount: 0,
+ signupType: 1,
+ typeName: 'Meeting',
+ timestamp: expect.any(String),
+ })
+ )
+ );
+ const [, payload] = (mockTrackEvent as jest.Mock).mock.calls[0];
+ const ISO_8601_RE = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{3,})?Z$/;
+ expect(payload.timestamp).toMatch(ISO_8601_RE);
+});📝 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.
| it('tracks analytics when sheet becomes visible', () => { | |
| render( | |
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | |
| ); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('calendar_item_details_viewed', { | |
| itemId: 'test-item-1', | |
| itemType: 1, | |
| hasLocation: true, | |
| hasDescription: true, | |
| isAllDay: false, | |
| canSignUp: true, | |
| isSignedUp: false, | |
| attendeeCount: 0, | |
| signupType: 1, | |
| typeName: 'Meeting', | |
| timestamp: expect.any(String), | |
| }); | |
| }); | |
| it('tracks analytics when sheet becomes visible', async () => { | |
| render( | |
| <CalendarItemDetailsSheet item={mockCalendarItem} isOpen={true} onClose={mockOnClose} /> | |
| ); | |
| await waitFor(() => | |
| expect(mockTrackEvent).toHaveBeenCalledWith( | |
| 'calendar_item_details_viewed', | |
| expect.objectContaining({ | |
| itemId: 'test-item-1', | |
| itemType: 1, | |
| hasLocation: true, | |
| hasDescription: true, | |
| isAllDay: false, | |
| canSignUp: true, | |
| isSignedUp: false, | |
| attendeeCount: 0, | |
| signupType: 1, | |
| typeName: 'Meeting', | |
| timestamp: expect.any(String), | |
| }) | |
| ) | |
| ); | |
| const [, payload] = (mockTrackEvent as jest.Mock).mock.calls[0]; | |
| const ISO_8601_RE = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{3,})?Z$/; | |
| expect(payload.timestamp).toMatch(ISO_8601_RE); | |
| }); |
🤖 Prompt for AI Agents
In
src/components/calendar/__tests__/calendar-item-details-sheet-minimal.test.tsx
around lines 117 to 135, the test currently asserts analytics immediately and
uses expect.any(String) for timestamp which is flaky; change the test to be
async, wait for the analytics call to occur (e.g. use waitFor or similar) and
replace the loose timestamp assertion with an ISO-8601 format check (use
expect.stringMatching with a regex matching full ISO timestamps, e.g.
YYYY-MM-DDTHH:MM:SS(.sss)?Z) so the test awaits emission and validates the
timestamp format reliably.
| #### 4. Calendar Item Attendance Failed | ||
| **Event Name:** `calendar_item_attendance_failed` | ||
|
|
||
| **Triggered When:** Attendance status change fails | ||
|
|
||
| **Properties:** | ||
| - `itemId` (string): Unique identifier for the calendar item | ||
| - `attending` (boolean): Attempted attendance status | ||
| - `error` (string): Error message | ||
| - `timestamp` (string): ISO timestamp | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid sending raw error messages in analytics payloads (potential PII leak).
The calendar_item_attendance_failed event currently documents error as a free-form string. Raw error messages can include PII or sensitive server details. Prefer a stable error code/category and optionally a sanitized summary.
Proposed doc update:
- - `error` (string): Error message
+ - `errorCode` (string): Stable, non-PII identifier for the error (e.g., "NETWORK_TIMEOUT", "UNAUTHORIZED")
+ - `errorCategory` (string): High-level category (e.g., "network", "auth", "validation")
+ - `errorMessage` (string, optional): Sanitized, non-PII message suitable for analyticsPlease also verify the implementation emits only sanitized values for failures and does not include exception stacks or user input. If you want, I can draft a small error-mapping utility and update the component.
🏁 Script executed:
#!/bin/bash
# Search for analytics event emission in the codebase
rg -nP "calendar_item_attendance_failed" -C3Length of output: 2158
Enforce non-PII error reporting for attendance-failure analytics
The implementation currently sends raw error.message (see src/components/calendar/calendar-item-details-sheet.tsx:138), and tests expect that exact string. To prevent potential PII leaks, we should refactor both the code and docs to use structured error metadata:
• Affected files:
– src/components/calendar/calendar-item-details-sheet.tsx (update trackEvent payload)
– src/components/calendar/tests/calendar-item-details-sheet.test.tsx (update expectations)
– src/components/calendar/README-analytics.md (update schema)
• Doc diff proposal:
--- a/src/components/calendar/README-analytics.md
+++ b/src/components/calendar/README-analytics.md
@@ -60,7 +60,10 @@
- `error` (string): Error message
+ `errorCode` (string): Stable, non-PII identifier (e.g., "NETWORK_TIMEOUT", "UNAUTHORIZED")
+ `errorCategory` (string): High-level category (e.g., "network", "auth", "validation")
+ `errorMessage` (string, optional): Sanitized, non-PII summary suitable for analytics• Code sketch:
const { code, category, message } = mapError(error);
trackEvent('calendar_item_attendance_failed', {
itemId: item.CalendarItemId,
attending,
errorCode: code,
errorCategory: category,
errorMessage: message,
});Please implement an error-mapping utility, update the component to emit only these fields, and adjust the corresponding tests.
🧰 Tools
🪛 LanguageTool
[grammar] ~53-~53: There might be a mistake here.
Context: ... #### 4. Calendar Item Attendance Failed Event Name: `calendar_item_attendance_...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In src/components/calendar/README-analytics.md around lines 53-63 and in the
implementation at src/components/calendar/calendar-item-details-sheet.tsx
(referenced ~line 138) plus tests at
src/components/calendar/__tests__/calendar-item-details-sheet.test.tsx, the code
currently emits raw error.message for attendance failures; replace this with
structured, non-PII fields. Create a small error-mapping utility (e.g.,
mapError(error) -> { code, category, message }) that extracts a safe errorCode,
errorCategory and a sanitized errorMessage (no PII), update the component to
call mapError(error) and send trackEvent('calendar_item_attendance_failed', {
itemId, attending, errorCode, errorCategory, errorMessage, timestamp }), update
the unit test expectations to assert these three fields instead of raw
error.message, and update the README-analytics.md schema to list errorCode
(string), errorCategory (string) and errorMessage (string) instead of a raw
error property.
| ### Data Privacy | ||
| - No sensitive call content is tracked | ||
| - Only metadata and interaction patterns captured | ||
| - Follows existing analytics privacy patterns | ||
|
|
There was a problem hiding this comment.
Privacy claim contradicts tracked payloads (addresses and precise coordinates).
The "Data Privacy" section states no sensitive content is tracked, but the implementation (see src/app/call/new/index.tsx) logs precise latitude/longitude and the full formatted address when selecting a result. That’s potentially PII.
Either:
- Update the docs to explicitly state that precise location/address data is tracked and justify it, or
- Change the implementation to de-identify/anonymize:
- Round coordinates to a coarse precision (e.g., 2–3 decimals) or bucket them.
- Remove
selectedAddressfrom analytics, or hash/truncate to a non-identifying level (e.g., city/locality only).
I can draft the code changes if you choose the anonymization route.
🧰 Tools
🪛 LanguageTool
[grammar] ~122-~122: There might be a mistake here.
Context: ...en call data is loaded ### Data Privacy - No sensitive call content is tracked - O...
(QB_NEW_EN)
| - **Download Failure**: `call_file_download_failed` | ||
| - Properties: `timestamp`, `callId`, `fileId`, `fileName`, `error` | ||
|
|
There was a problem hiding this comment.
Sanitize error details for analytics safety.
Avoid emitting raw error strings; prefer structured codes/names and optionally a truncated summary to prevent PII or secret leakage.
Apply this doc tweak:
- - Properties: `timestamp`, `callId`, `fileId`, `fileName`, `error`
+ - Properties: `timestamp`, `callId`, `fileId`, `fileName`, `errorCode`, `errorName`, `errorMessage` (truncated, sanitized)Also consider the same for call_files_retry_pressed properties below if it currently includes a raw error.
📝 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.
| - **Download Failure**: `call_file_download_failed` | |
| - Properties: `timestamp`, `callId`, `fileId`, `fileName`, `error` | |
| - **Download Failure**: `call_file_download_failed` | |
| - Properties: `timestamp`, `callId`, `fileId`, `fileName`, `errorCode`, `errorName`, `errorMessage` (truncated, sanitized) |
🧰 Tools
🪛 LanguageTool
[grammar] ~24-~24: There might be a mistake here.
Context: ...aredboolean - **Download Failure**:call_file_download_failed - Properties:timestamp, callId, file...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/call-files-modal-analytics-implementation.md around lines 24-26, the
spec currently emits a raw `error` string for `call_file_download_failed`;
update the event properties to use a structured error field (e.g., `errorCode`
or `errorName`) and an optional sanitized/truncated `errorSummary` (max length,
remove PII/secrets via whitelist or regex) instead of the raw message; ensure
the same change is applied to `call_files_retry_pressed` if it also includes a
raw `error`, and add a note describing the sanitization/truncation rules and max
length.
| #### Close Failure Analytics | ||
| - **Event Name**: `close_call_failed` | ||
| - **Triggered**: When the API call to close the call fails | ||
| - **Data Tracked**: | ||
| - `timestamp`: ISO string timestamp | ||
| - `callId`: ID of the call | ||
| - `closeType`: Close type attempted | ||
| - `hasNote`: Whether a note was provided | ||
| - `noteLength`: Length of the note text | ||
| - `error`: Error message from the failed API call | ||
|
|
There was a problem hiding this comment.
Avoid sending raw error messages in analytics payloads.
Raw error strings may leak PII, tokens, or backend internals. Track sanitized error metadata instead (code/name) and optionally a truncated message.
Apply this doc tweak:
- - `error`: Error message from the failed API call
+ - `errorCode`: Stable error code or HTTP status (e.g., 400, 401, 500)
+ - `errorName`: Short error classification (e.g., "validation_error", "network_error")
+ - `errorMessage`: Truncated and sanitized summary (optional, max 200 chars)If useful, I can produce a cross-repo grep to surface other spots sending raw error strings and propose patches.
📝 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.
| #### Close Failure Analytics | |
| - **Event Name**: `close_call_failed` | |
| - **Triggered**: When the API call to close the call fails | |
| - **Data Tracked**: | |
| - `timestamp`: ISO string timestamp | |
| - `callId`: ID of the call | |
| - `closeType`: Close type attempted | |
| - `hasNote`: Whether a note was provided | |
| - `noteLength`: Length of the note text | |
| - `error`: Error message from the failed API call | |
| #### Close Failure Analytics | |
| - **Event Name**: `close_call_failed` | |
| - **Triggered**: When the API call to close the call fails | |
| - **Data Tracked**: | |
| - `timestamp`: ISO string timestamp | |
| - `callId`: ID of the call | |
| - `closeType`: Close type attempted | |
| - `hasNote`: Whether a note was provided | |
| - `noteLength`: Length of the note text | |
| - `errorCode`: Stable error code or HTTP status (e.g., 400, 401, 500) | |
| - `errorName`: Short error classification (e.g., "validation_error", "network_error") | |
| - `errorMessage`: Truncated and sanitized summary (optional, max 200 chars) |
🧰 Tools
🪛 LanguageTool
[grammar] ~52-~52: There might be a mistake here.
Context: ... note text #### Close Failure Analytics - Event Name: close_call_failed - **Tr...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...ose Failure Analytics - Event Name: close_call_failed - Triggered: When the API call to close ...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...hen the API call to close the call fails - Data Tracked: - timestamp: ISO str...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...close the call fails - Data Tracked: - timestamp: ISO string timestamp - callId: ID ...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...*: - timestamp: ISO string timestamp - callId: ID of the call - closeType: Close ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...g timestamp - callId: ID of the call - closeType: Close type attempted - hasNote: Wh...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...ll - closeType: Close type attempted - hasNote: Whether a note was provided - `noteL...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...- hasNote: Whether a note was provided - noteLength: Length of the note text - error: E...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... - noteLength: Length of the note text - error: Error message from the failed API call...
(QB_NEW_EN)
| #### Manual Close Analytics | ||
| - **Event Name**: `close_call_bottom_sheet_closed` | ||
| - **Triggered**: When user manually closes the bottom sheet (cancel button) | ||
| - **Data Tracked**: | ||
| - `timestamp`: ISO string timestamp | ||
| - `callId`: ID of the call | ||
| - `wasManualClose`: Always true for manual closes | ||
| - `hadCloseCallType`: Whether a close type was selected | ||
| - `hadCloseCallNote`: Whether a note was entered | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Generalize the close event to cover gesture/system closes (with wasManualClose flag).
Currently documented as “manual close.” Align with other modal docs by treating this as a general “closed” event with a boolean flag.
Apply this doc tweak:
-#### Manual Close Analytics
-- **Event Name**: `close_call_bottom_sheet_closed`
-- **Triggered**: When user manually closes the bottom sheet (cancel button)
- - `wasManualClose`: Always true for manual closes
+#### Close Analytics
+- **Event Name**: `close_call_bottom_sheet_closed`
+- **Triggered**: When the bottom sheet closes (cancel button, gesture, backdrop, back button)
+ - `wasManualClose`: true for explicit cancel button, false for gesture/back/backdrop🧰 Tools
🪛 LanguageTool
[grammar] ~63-~63: There might be a mistake here.
Context: ...ed API call #### Manual Close Analytics - Event Name: `close_call_bottom_sheet_c...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...anual Close Analytics - Event Name: close_call_bottom_sheet_closed - Triggered: When user manually closes t...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ... closes the bottom sheet (cancel button) - Data Tracked: - timestamp: ISO str...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...heet (cancel button) - Data Tracked: - timestamp: ISO string timestamp - callId: ID ...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...*: - timestamp: ISO string timestamp - callId: ID of the call - wasManualClose: A...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...g timestamp - callId: ID of the call - wasManualClose: Always true for manual closes - `had...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...ualClose: Always true for manual closes - hadCloseCallType`: Whether a close type was selected - ...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...Type: Whether a close type was selected - hadCloseCallNote`: Whether a note was entered ### 3. Err...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/close-call-bottom-sheet-analytics-implementation.md around lines 63 to
72, change the section from “Manual Close Analytics” to a generalized “Close
Analytics”, update the Triggered line to “When the bottom sheet is closed
(manual, gesture, or system)”, and document that the event tracks a boolean
wasManualClose (true only when closed via the cancel button, false for
gesture/system closes); keep the other fields (timestamp, callId,
hadCloseCallType, hadCloseCallNote) unchanged and ensure wording aligns with
other modal docs.
| - `timestamp`: ISO string of when the event occurred | ||
| - `username`: The username attempted (for analytics purposes) | ||
|
|
There was a problem hiding this comment.
Do not send raw usernames to analytics; use a one-way hash or stable pseudonymous ID instead.
Capturing username is PII and increases risk (GDPR/CCPA/HIPAA implications). Replace the raw value with a hashed or mapped identifier.
Apply these diffs to the documentation:
- - `username`: The username attempted (for analytics purposes)
+ - `user_id_hash`: One-way hash of the username (no raw PII) trackEvent('login_attempted', {
timestamp: new Date().toISOString(),
- username: data.username,
+ user_id_hash: await hashUsername(data.username),
});Outside this block, add a helper example to make the approach concrete:
// Example helper (React Native/Expo)
import * as Crypto from 'expo-crypto';
async function hashUsername(username: string) {
return await Crypto.digestStringAsync(
Crypto.CryptoDigestAlgorithm.SHA256,
username.trim().toLowerCase()
);
}Also applies to: 57-61
🤖 Prompt for AI Agents
In docs/login-analytics-implementation.md around lines 19-21 (and similarly
lines 57-61), the doc currently records raw `username` which is PII; replace
`username` with a one-way hashed or stable pseudonymous identifier (e.g.,
`user_id_hash`) and describe that the hash must be computed client-side or
before logging using a strong algorithm (SHA-256), normalizing input (trim and
lowercase) and without reversible salts; update example payload fields
accordingly and, outside the existing block, add a short helper example (React
Native/Expo) that demonstrates computing the SHA-256 hash of the normalized
username using expo-crypto as the concrete implementation reference.
| // Simulate the onSubmit function pattern | ||
| const onSubmit = jest.fn(async (data) => { | ||
| trackEvent('login_attempted', { | ||
| timestamp: new Date().toISOString(), | ||
| username: data.username, | ||
| }); | ||
| await mockLogin({ username: data.username, password: data.password }); | ||
| }); | ||
|
|
||
| const testData = { username: 'testuser', password: 'testpass' }; | ||
| onSubmit(testData); | ||
|
|
||
| expect(mockTrackEvent).toHaveBeenCalledWith('login_attempted', { | ||
| timestamp: expect.any(String), | ||
| username: 'testuser', | ||
| }); | ||
| expect(mockLogin).toHaveBeenCalledWith({ | ||
| username: 'testuser', | ||
| password: 'testpass', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update onSubmit analytics to avoid PII (use hasUsername).
Also adjust the assertion accordingly.
const onSubmit = jest.fn(async (data) => {
- trackEvent('login_attempted', {
- timestamp: new Date().toISOString(),
- username: data.username,
- });
+ trackEvent('login_attempted', {
+ timestamp: new Date().toISOString(),
+ hasUsername: Boolean(data.username),
+ });
await mockLogin({ username: data.username, password: data.password });
});
@@
- expect(mockTrackEvent).toHaveBeenCalledWith('login_attempted', {
- timestamp: expect.any(String),
- username: 'testuser',
- });
+ expect(mockTrackEvent).toHaveBeenCalledWith('login_attempted', {
+ timestamp: expect.any(String),
+ hasUsername: true,
+ });📝 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.
| // Simulate the onSubmit function pattern | |
| const onSubmit = jest.fn(async (data) => { | |
| trackEvent('login_attempted', { | |
| timestamp: new Date().toISOString(), | |
| username: data.username, | |
| }); | |
| await mockLogin({ username: data.username, password: data.password }); | |
| }); | |
| const testData = { username: 'testuser', password: 'testpass' }; | |
| onSubmit(testData); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('login_attempted', { | |
| timestamp: expect.any(String), | |
| username: 'testuser', | |
| }); | |
| expect(mockLogin).toHaveBeenCalledWith({ | |
| username: 'testuser', | |
| password: 'testpass', | |
| }); | |
| }); | |
| // Simulate the onSubmit function pattern | |
| const onSubmit = jest.fn(async (data) => { | |
| trackEvent('login_attempted', { | |
| timestamp: new Date().toISOString(), | |
| hasUsername: Boolean(data.username), | |
| }); | |
| await mockLogin({ username: data.username, password: data.password }); | |
| }); | |
| const testData = { username: 'testuser', password: 'testpass' }; | |
| onSubmit(testData); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('login_attempted', { | |
| timestamp: expect.any(String), | |
| hasUsername: true, | |
| }); | |
| expect(mockLogin).toHaveBeenCalledWith({ | |
| username: 'testuser', | |
| password: 'testpass', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/app/login/__tests__/index.test.tsx around lines 72 to 92, the test
currently asserts that the analytics event includes the raw username (PII);
change the tracked payload to use hasUsername boolean instead of username, and
update the assertion accordingly. Specifically, modify the onSubmit simulation
to call trackEvent('login_attempted', { timestamp: ..., hasUsername:
Boolean(data.username) }) and then update the
expect(mockTrackEvent).toHaveBeenCalledWith(...) to expect hasUsername: true
(keep timestamp as expect.any(String)), while leaving the mockLogin assertion
unchanged.
| it('should validate analytics data structure', () => { | ||
| // Test that all required analytics properties are correctly formatted | ||
| const loginViewedData = { | ||
| timestamp: new Date().toISOString(), | ||
| }; | ||
|
|
||
| const loginAttemptedData = { | ||
| timestamp: new Date().toISOString(), | ||
| username: 'testuser', | ||
| }; | ||
|
|
||
| const loginSuccessData = { | ||
| timestamp: new Date().toISOString(), | ||
| }; | ||
|
|
||
| const loginFailedData = { | ||
| timestamp: new Date().toISOString(), | ||
| error: 'Invalid credentials', | ||
| }; | ||
|
|
||
| // Validate data types | ||
| expect(typeof loginViewedData.timestamp).toBe('string'); | ||
| expect(typeof loginAttemptedData.timestamp).toBe('string'); | ||
| expect(typeof loginAttemptedData.username).toBe('string'); | ||
| expect(typeof loginSuccessData.timestamp).toBe('string'); | ||
| expect(typeof loginFailedData.timestamp).toBe('string'); | ||
| expect(typeof loginFailedData.error).toBe('string'); | ||
|
|
||
| // Validate timestamp format | ||
| expect(Date.parse(loginViewedData.timestamp)).not.toBeNaN(); | ||
| expect(Date.parse(loginAttemptedData.timestamp)).not.toBeNaN(); | ||
| expect(Date.parse(loginSuccessData.timestamp)).not.toBeNaN(); | ||
| expect(Date.parse(loginFailedData.timestamp)).not.toBeNaN(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust data structure validation to reflect non-PII analytics.
Validate hasUsername is a boolean and timestamps remain parseable.
const loginAttemptedData = {
timestamp: new Date().toISOString(),
- username: 'testuser',
+ hasUsername: true,
};
@@
- expect(typeof loginAttemptedData.username).toBe('string');
+ expect(typeof loginAttemptedData.hasUsername).toBe('boolean');
@@
expect(Date.parse(loginAttemptedData.timestamp)).not.toBeNaN();📝 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.
| it('should validate analytics data structure', () => { | |
| // Test that all required analytics properties are correctly formatted | |
| const loginViewedData = { | |
| timestamp: new Date().toISOString(), | |
| }; | |
| const loginAttemptedData = { | |
| timestamp: new Date().toISOString(), | |
| username: 'testuser', | |
| }; | |
| const loginSuccessData = { | |
| timestamp: new Date().toISOString(), | |
| }; | |
| const loginFailedData = { | |
| timestamp: new Date().toISOString(), | |
| error: 'Invalid credentials', | |
| }; | |
| // Validate data types | |
| expect(typeof loginViewedData.timestamp).toBe('string'); | |
| expect(typeof loginAttemptedData.timestamp).toBe('string'); | |
| expect(typeof loginAttemptedData.username).toBe('string'); | |
| expect(typeof loginSuccessData.timestamp).toBe('string'); | |
| expect(typeof loginFailedData.timestamp).toBe('string'); | |
| expect(typeof loginFailedData.error).toBe('string'); | |
| // Validate timestamp format | |
| expect(Date.parse(loginViewedData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginAttemptedData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginSuccessData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginFailedData.timestamp)).not.toBeNaN(); | |
| }); | |
| it('should validate analytics data structure', () => { | |
| // Test that all required analytics properties are correctly formatted | |
| const loginViewedData = { | |
| timestamp: new Date().toISOString(), | |
| }; | |
| const loginAttemptedData = { | |
| timestamp: new Date().toISOString(), | |
| hasUsername: true, | |
| }; | |
| const loginSuccessData = { | |
| timestamp: new Date().toISOString(), | |
| }; | |
| const loginFailedData = { | |
| timestamp: new Date().toISOString(), | |
| error: 'Invalid credentials', | |
| }; | |
| // Validate data types | |
| expect(typeof loginViewedData.timestamp).toBe('string'); | |
| expect(typeof loginAttemptedData.timestamp).toBe('string'); | |
| expect(typeof loginAttemptedData.hasUsername).toBe('boolean'); | |
| expect(typeof loginSuccessData.timestamp).toBe('string'); | |
| expect(typeof loginFailedData.timestamp).toBe('string'); | |
| expect(typeof loginFailedData.error).toBe('string'); | |
| // Validate timestamp format | |
| expect(Date.parse(loginViewedData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginAttemptedData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginSuccessData.timestamp)).not.toBeNaN(); | |
| expect(Date.parse(loginFailedData.timestamp)).not.toBeNaN(); | |
| }); |
🤖 Prompt for AI Agents
In src/app/login/__tests__/index.test.tsx around lines 169 to 202, the analytics
test should use a non-PII flag for username presence rather than sending the
actual username; add a hasUsername boolean property to the relevant analytics
objects (e.g., loginAttemptedData and any other events that previously included
username), assert that hasUsername is a boolean, and keep the existing
assertions that all timestamps are strings and parseable; update/remove any
assertions that expect the raw username string so the test validates non-PII
analytics only.
| // Track login failure | ||
| trackEvent('login_failed', { | ||
| timestamp: new Date().toISOString(), | ||
| error: error || 'Unknown error', | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize error analytics and guard with try/catch.
Raw error strings can include sensitive info. Prefer a bounded, classified error signal.
- // Track login failure
- trackEvent('login_failed', {
- timestamp: new Date().toISOString(),
- error: error || 'Unknown error',
- });
+ // Track login failure (sanitized)
+ try {
+ const safeError =
+ typeof error === 'string'
+ ? error
+ : (error && (error.code || error.name)) || 'Unknown error';
+ // Optionally: limit length
+ const boundedError =
+ typeof safeError === 'string' ? safeError.slice(0, 128) : 'Unknown error';
+ trackEvent('login_failed', {
+ timestamp: new Date().toISOString(),
+ error: boundedError,
+ });
+ } catch (e) {
+ console.warn('Analytics failed (login_failed):', e);
+ }📝 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.
| // Track login failure | |
| trackEvent('login_failed', { | |
| timestamp: new Date().toISOString(), | |
| error: error || 'Unknown error', | |
| }); | |
| // Track login failure (sanitized) | |
| try { | |
| const safeError = | |
| typeof error === 'string' | |
| ? error | |
| : (error && (error.code || error.name)) || 'Unknown error'; | |
| // Optionally: limit length | |
| const boundedError = | |
| typeof safeError === 'string' ? safeError.slice(0, 128) : 'Unknown error'; | |
| trackEvent('login_failed', { | |
| timestamp: new Date().toISOString(), | |
| error: boundedError, | |
| }); | |
| } catch (e) { | |
| console.warn('Analytics failed (login_failed):', e); | |
| } |
🤖 Prompt for AI Agents
In src/app/login/index.tsx around lines 55-60 the code sends the raw error to
analytics; change this to classify and truncate the error and guard the
analytics call with try/catch: derive a safeErrorCategory or errorCode from
known/whitelisted error types (fall back to "unknown_error"), build a minimal
payload with only bounded fields (e.g. { timestamp, errorCode, category,
message: truncatedMessage <=100 chars }) and do NOT include stack traces or full
error strings, then call trackEvent with that payload inside a try/catch so any
analytics failure is swallowed (optionally log a non-sensitive local warning).
| logger.info({ | ||
| message: 'Starting Login (button press)', | ||
| context: { username: data.username }, | ||
| }); |
There was a problem hiding this comment.
Avoid logging raw usernames in application logs.
This is PII in logs. Prefer a boolean signal or a safe hash if necessary.
- logger.info({
- message: 'Starting Login (button press)',
- context: { username: data.username },
- });
+ logger.info({
+ message: 'Starting Login (button press)',
+ // Avoid logging PII
+ context: { hasUsername: Boolean(data.username) },
+ });📝 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.
| logger.info({ | |
| message: 'Starting Login (button press)', | |
| context: { username: data.username }, | |
| }); | |
| logger.info({ | |
| message: 'Starting Login (button press)', | |
| // Avoid logging PII | |
| context: { hasUsername: Boolean(data.username) }, | |
| }); |
🤖 Prompt for AI Agents
In src/app/login/index.tsx around lines 66 to 69, the code currently logs the
raw username which is PII; remove the plain username from logs and replace it
with either a boolean flag (e.g., { hasUsername: !!data.username }) or an
anonymized identifier such as a one-way salted hash (compute a secure hash/HMAC
using an application salt/key from config and log that value) so logs never
contain the raw username; ensure the salt/key is retrieved from secure config
and that the hashing is non-reversible before updating the logger call.
| // Track login attempt | ||
| trackEvent('login_attempted', { | ||
| timestamp: new Date().toISOString(), | ||
| username: data.username, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Do not send raw username to analytics; replace with a non-PII signal and wrap in try/catch.
Sending usernames to analytics is a privacy risk (PII leakage). Track only presence or a non-reversible hash, and guard analytics calls.
- // Track login attempt
- trackEvent('login_attempted', {
- timestamp: new Date().toISOString(),
- username: data.username,
- });
+ // Track login attempt (no PII)
+ try {
+ trackEvent('login_attempted', {
+ timestamp: new Date().toISOString(),
+ hasUsername: Boolean(data.username),
+ });
+ } catch (e) {
+ console.warn('Analytics failed (login_attempted):', e);
+ }📝 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.
| // Track login attempt | |
| trackEvent('login_attempted', { | |
| timestamp: new Date().toISOString(), | |
| username: data.username, | |
| }); | |
| // Track login attempt (no PII) | |
| try { | |
| trackEvent('login_attempted', { | |
| timestamp: new Date().toISOString(), | |
| hasUsername: Boolean(data.username), | |
| }); | |
| } catch (e) { | |
| console.warn('Analytics failed (login_attempted):', e); | |
| } |
🤖 Prompt for AI Agents
In src/app/login/index.tsx around lines 71 to 76, the code sends raw usernames
to analytics; replace that with a non-PII signal (e.g., a boolean "hasUsername"
or a non-reversible hash of the username like SHA-256) and wrap the analytics
call in a try/catch to prevent runtime errors from bubbling up. Compute the hash
or boolean locally, do not include the raw username in the payload, and call
trackEvent inside a try block with errors caught and logged (or ignored) in the
catch.
| it('tracks recenter map action', async () => { | ||
| const mockLocationStore = jest.requireMock('@/stores/app/location-store') as any; | ||
| mockLocationStore.useLocationStore.mockReturnValue({ | ||
| latitude: 40.7128, | ||
| longitude: -74.006, | ||
| heading: 90, | ||
| isMapLocked: false, | ||
| }); | ||
|
|
||
| render(<HomeMap />); | ||
|
|
||
| // Wait for map to be ready and simulate user moving map | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId('home-map-view')).toBeTruthy(); | ||
| }); | ||
|
|
||
| // Clear initial analytics call | ||
| mockTrackEvent.mockClear(); | ||
|
|
||
| // Note: In the actual implementation, the recenter button would only show | ||
| // when hasUserMovedMap is true, but for testing we'll just verify the handler | ||
| const recenterButton = screen.queryByTestId('recenter-button'); | ||
| if (recenterButton) { | ||
| fireEvent.press(recenterButton); | ||
|
|
||
| // Check analytics tracking for recenter action | ||
| expect(mockTrackEvent).toHaveBeenCalledWith('map_recentered', { | ||
| timestamp: expect.any(String), | ||
| isMapLocked: false, | ||
| zoomLevel: 12, | ||
| }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recenter analytics test is currently inert; assert and trigger deterministically
Because the test presses the recenter button only if it exists, it can silently pass without asserting anything. With the MapView mock triggering user interaction, assert the button exists and the analytics event is emitted.
- // Note: In the actual implementation, the recenter button would only show
- // when hasUserMovedMap is true, but for testing we'll just verify the handler
- const recenterButton = screen.queryByTestId('recenter-button');
- if (recenterButton) {
- fireEvent.press(recenterButton);
-
- // Check analytics tracking for recenter action
- expect(mockTrackEvent).toHaveBeenCalledWith('map_recentered', {
- timestamp: expect.any(String),
- isMapLocked: false,
- zoomLevel: 12,
- });
- }
+ // With MapView mock simulating user movement, the recenter button should exist
+ const recenterButton = await screen.findByTestId('recenter-button');
+ fireEvent.press(recenterButton);
+
+ // Check analytics tracking for recenter action
+ expect(mockTrackEvent).toHaveBeenCalledWith('map_recentered', {
+ timestamp: expect.any(String),
+ isMapLocked: false,
+ zoomLevel: 12,
+ });📝 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.
| it('tracks recenter map action', async () => { | |
| const mockLocationStore = jest.requireMock('@/stores/app/location-store') as any; | |
| mockLocationStore.useLocationStore.mockReturnValue({ | |
| latitude: 40.7128, | |
| longitude: -74.006, | |
| heading: 90, | |
| isMapLocked: false, | |
| }); | |
| render(<HomeMap />); | |
| // Wait for map to be ready and simulate user moving map | |
| await waitFor(() => { | |
| expect(screen.getByTestId('home-map-view')).toBeTruthy(); | |
| }); | |
| // Clear initial analytics call | |
| mockTrackEvent.mockClear(); | |
| // Note: In the actual implementation, the recenter button would only show | |
| // when hasUserMovedMap is true, but for testing we'll just verify the handler | |
| const recenterButton = screen.queryByTestId('recenter-button'); | |
| if (recenterButton) { | |
| fireEvent.press(recenterButton); | |
| // Check analytics tracking for recenter action | |
| expect(mockTrackEvent).toHaveBeenCalledWith('map_recentered', { | |
| timestamp: expect.any(String), | |
| isMapLocked: false, | |
| zoomLevel: 12, | |
| }); | |
| } | |
| }); | |
| it('tracks recenter map action', async () => { | |
| const mockLocationStore = jest.requireMock('@/stores/app/location-store') as any; | |
| mockLocationStore.useLocationStore.mockReturnValue({ | |
| latitude: 40.7128, | |
| longitude: -74.006, | |
| heading: 90, | |
| isMapLocked: false, | |
| }); | |
| render(<HomeMap />); | |
| // Wait for map to be ready and simulate user moving map | |
| await waitFor(() => { | |
| expect(screen.getByTestId('home-map-view')).toBeTruthy(); | |
| }); | |
| // Clear initial analytics call | |
| mockTrackEvent.mockClear(); | |
| // With MapView mock simulating user movement, the recenter button should exist | |
| const recenterButton = await screen.findByTestId('recenter-button'); | |
| fireEvent.press(recenterButton); | |
| // Check analytics tracking for recenter action | |
| expect(mockTrackEvent).toHaveBeenCalledWith('map_recentered', { | |
| timestamp: expect.any(String), | |
| isMapLocked: false, | |
| zoomLevel: 12, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/app/(app)/__tests__/map.test.tsx around lines 559 to 592, the test
conditionally presses the recenter button which can make the test pass silently
if the button is not rendered; update the test to deterministically assert the
recenter button exists (e.g. expect(recenterButton).toBeTruthy()), ensure the
component is rendered in a state that shows the recenter button (set the
mock/location or MapView mock so hasUserMovedMap is true or simulate user
movement), then fireEvent.press(recenterButton) and assert mockTrackEvent was
called with the expected payload (including timestamp as any(String) and other
properties).
| it('hides compose FAB when in selection mode even if user can create messages', () => { | ||
| mockedUseMessagesStore.mockReturnValue({ | ||
| ...mockStore, | ||
| selectedForDeletion: new Set(['1']), | ||
| hasSelectedMessages: jest.fn(() => true), | ||
| }); | ||
| mockedUseSecurityStore.mockReturnValue({ | ||
| ...mockSecurityStore, | ||
| canUserCreateMessages: true, | ||
| }); | ||
|
|
||
| // Simulate entering selection mode by triggering long press | ||
| render(<MessagesScreen />); | ||
|
|
||
| // Since we can't easily test the selection mode state change, | ||
| // we'll test that the FAB is hidden when selectedForDeletion has items | ||
| // This is an indirect test, but validates the behavior | ||
| expect(screen.getByTestId('messages-compose-fab')).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Fix selection-mode FAB visibility test to actually enter selection mode and assert hidden
Current test expects the FAB to be present even though the description says it should be hidden. Also, toggling selectedForDeletion alone doesn’t flip the internal isSelectionMode state. Simulate a long-press to enter selection mode, then assert the FAB is not rendered.
- it('hides compose FAB when in selection mode even if user can create messages', () => {
- mockedUseMessagesStore.mockReturnValue({
- ...mockStore,
- selectedForDeletion: new Set(['1']),
- hasSelectedMessages: jest.fn(() => true),
- });
- mockedUseSecurityStore.mockReturnValue({
- ...mockSecurityStore,
- canUserCreateMessages: true,
- });
-
- // Simulate entering selection mode by triggering long press
- render(<MessagesScreen />);
-
- // Since we can't easily test the selection mode state change,
- // we'll test that the FAB is hidden when selectedForDeletion has items
- // This is an indirect test, but validates the behavior
- expect(screen.getByTestId('messages-compose-fab')).toBeTruthy();
- });
+ it('hides compose FAB when in selection mode even if user can create messages', () => {
+ mockedUseSecurityStore.mockReturnValue({
+ ...mockSecurityStore,
+ canUserCreateMessages: true,
+ });
+ render(<MessagesScreen />);
+ // Enter selection mode via long press on a message card
+ const messageCard = screen.getByTestId('message-card-1');
+ fireEvent(messageCard, 'longPress');
+ // FAB should now be hidden
+ expect(screen.queryByTestId('messages-compose-fab')).toBeNull();
+ });Committable suggestion skipped: line range outside the PR's diff.
| describe('Notes Screen Analytics', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
These tests don’t exercise the Notes screen; they only call the mock
This provides little value and wouldn’t catch regressions (e.g., event name mismatches). Render Notes, simulate focus/search/select, and assert analytics calls.
I can provide a test scaffold using @testing-library/react-native to render Notes, mock the store, simulate input, and verify trackEvent invocations.
Do you want me to generate a concrete test that renders the Notes screen and covers viewed/searched/refreshed/note_selected flows?
🤖 Prompt for AI Agents
In src/app/(app)/__tests__/notes.test.tsx around lines 32 to 35, the current
tests only invoke the analytics mock and do not render the Notes screen or
simulate user flows; update the test to render the actual Notes component with
@testing-library/react-native, provide necessary mocked store/state/providers,
then simulate screen focus (or navigation), perform a search input change,
trigger a refresh action, and select a note; finally assert that the analytics
tracking function (trackEvent) was called with the expected event names and
payloads for viewed, searched, refreshed, and note_selected events. Ensure you
mock any network/data hooks and navigation to avoid flakiness and call
jest.clearAllMocks in beforeEach as already present.
| const { useAnalytics } = require('@/hooks/use-analytics'); | ||
| const { useNotesStore } = require('@/stores/notes/store'); | ||
|
|
||
| it('returns all notes when search query is empty', () => { | ||
| expect(filterNotes(testNotes, '')).toHaveLength(4); | ||
| expect(filterNotes(testNotes, ' ')).toHaveLength(4); // Whitespace only | ||
| }); | ||
| const { trackEvent } = useAnalytics(); | ||
| const { notes } = useNotesStore(); | ||
|
|
||
| it('filters notes by title correctly', () => { | ||
| const result = filterNotes(testNotes, 'meeting'); | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].NoteId).toBe('1'); | ||
| // Simulate the analytics tracking that happens in useFocusEffect | ||
| trackEvent('notes_view', { | ||
| noteCount: notes.length, | ||
| hasSearchQuery: false, | ||
| currentCategory: 'All', | ||
| }); | ||
|
|
||
| it('filters notes by body content correctly', () => { | ||
| const result = filterNotes(testNotes, 'birthday'); | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].NoteId).toBe('2'); | ||
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_view', { | ||
| noteCount: 0, | ||
| hasSearchQuery: false, | ||
| currentCategory: 'All', | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Event name mismatch with implementation (“notes_view” vs “notes_viewed”)
The Notes screen emits notes_viewed; this test asserts notes_view. Align the test to the codebase convention.
Apply this diff:
- trackEvent('notes_view', {
+ trackEvent('notes_viewed', {
noteCount: notes.length,
hasSearchQuery: false,
currentCategory: 'All',
});
- expect(mockTrackEvent).toHaveBeenCalledWith('notes_view', {
+ expect(mockTrackEvent).toHaveBeenCalledWith('notes_viewed', {
noteCount: 0,
hasSearchQuery: false,
currentCategory: 'All',
});📝 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.
| it('tracks notes view analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { useNotesStore } = require('@/stores/notes/store'); | |
| it('returns all notes when search query is empty', () => { | |
| expect(filterNotes(testNotes, '')).toHaveLength(4); | |
| expect(filterNotes(testNotes, ' ')).toHaveLength(4); // Whitespace only | |
| }); | |
| const { trackEvent } = useAnalytics(); | |
| const { notes } = useNotesStore(); | |
| it('filters notes by title correctly', () => { | |
| const result = filterNotes(testNotes, 'meeting'); | |
| expect(result).toHaveLength(1); | |
| expect(result[0].NoteId).toBe('1'); | |
| // Simulate the analytics tracking that happens in useFocusEffect | |
| trackEvent('notes_view', { | |
| noteCount: notes.length, | |
| hasSearchQuery: false, | |
| currentCategory: 'All', | |
| }); | |
| it('filters notes by body content correctly', () => { | |
| const result = filterNotes(testNotes, 'birthday'); | |
| expect(result).toHaveLength(1); | |
| expect(result[0].NoteId).toBe('2'); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_view', { | |
| noteCount: 0, | |
| hasSearchQuery: false, | |
| currentCategory: 'All', | |
| }); | |
| }); | |
| it('tracks notes view analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { useNotesStore } = require('@/stores/notes/store'); | |
| const { trackEvent } = useAnalytics(); | |
| const { notes } = useNotesStore(); | |
| // Simulate the analytics tracking that happens in useFocusEffect | |
| trackEvent('notes_viewed', { | |
| noteCount: notes.length, | |
| hasSearchQuery: false, | |
| currentCategory: 'All', | |
| }); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_viewed', { | |
| noteCount: 0, | |
| hasSearchQuery: false, | |
| currentCategory: 'All', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/app/(app)/__tests__/notes.test.tsx around lines 37 to 56, the test
currently uses the event name "notes_view" but the app emits "notes_viewed";
update the test to call and expect "notes_viewed" instead of "notes_view" so the
asserted event name matches the implementation (change both the trackEvent
invocation and the expect to use "notes_viewed").
| const { useAnalytics } = require('@/hooks/use-analytics'); | ||
| const { trackEvent } = useAnalytics(); | ||
|
|
||
| it('performs case-insensitive filtering', () => { | ||
| expect(filterNotes(testNotes, 'IMPORTANT')).toHaveLength(1); | ||
| expect(filterNotes(testNotes, 'Personal')).toHaveLength(2); | ||
| expect(filterNotes(testNotes, 'DEVELOPMENT')).toHaveLength(1); | ||
| }); | ||
| // Simulate search tracking | ||
| const searchQuery = 'business'; | ||
| const resultCount = 2; | ||
|
|
||
| it('handles partial matches correctly', () => { | ||
| const result = filterNotes(testNotes, 'review'); | ||
| expect(result).toHaveLength(1); // Matches "Code Review" title and "Review implementation" body (same note) | ||
| expect(result[0].NoteId).toBe('3'); | ||
| trackEvent('notes_search', { | ||
| searchQuery, | ||
| resultCount, | ||
| }); | ||
|
|
||
| it('returns empty array for non-matching queries', () => { | ||
| expect(filterNotes(testNotes, 'nonexistent')).toHaveLength(0); | ||
| expect(filterNotes(testNotes, 'xyz123')).toHaveLength(0); | ||
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_search', { | ||
| searchQuery: 'business', | ||
| resultCount: 2, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Rename to “notes_searched” to match implementation
Update test to use the emitted event name.
Apply this diff:
- trackEvent('notes_search', {
+ trackEvent('notes_searched', {
searchQuery,
resultCount,
});
- expect(mockTrackEvent).toHaveBeenCalledWith('notes_search', {
+ expect(mockTrackEvent).toHaveBeenCalledWith('notes_searched', {
searchQuery: 'business',
resultCount: 2,
});📝 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.
| it('tracks search analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { trackEvent } = useAnalytics(); | |
| it('performs case-insensitive filtering', () => { | |
| expect(filterNotes(testNotes, 'IMPORTANT')).toHaveLength(1); | |
| expect(filterNotes(testNotes, 'Personal')).toHaveLength(2); | |
| expect(filterNotes(testNotes, 'DEVELOPMENT')).toHaveLength(1); | |
| }); | |
| // Simulate search tracking | |
| const searchQuery = 'business'; | |
| const resultCount = 2; | |
| it('handles partial matches correctly', () => { | |
| const result = filterNotes(testNotes, 'review'); | |
| expect(result).toHaveLength(1); // Matches "Code Review" title and "Review implementation" body (same note) | |
| expect(result[0].NoteId).toBe('3'); | |
| trackEvent('notes_search', { | |
| searchQuery, | |
| resultCount, | |
| }); | |
| it('returns empty array for non-matching queries', () => { | |
| expect(filterNotes(testNotes, 'nonexistent')).toHaveLength(0); | |
| expect(filterNotes(testNotes, 'xyz123')).toHaveLength(0); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_search', { | |
| searchQuery: 'business', | |
| resultCount: 2, | |
| }); | |
| }); | |
| it('tracks search analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { trackEvent } = useAnalytics(); | |
| // Simulate search tracking | |
| const searchQuery = 'business'; | |
| const resultCount = 2; | |
| trackEvent('notes_searched', { | |
| searchQuery, | |
| resultCount, | |
| }); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_searched', { | |
| searchQuery: 'business', | |
| resultCount: 2, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
src/app/(app)/__tests__/notes.test.tsx around lines 58 to 75: the test asserts
the analytics event using the wrong event name 'notes_search' but the
implementation emits 'notes_searched'; update the test to call and expect
'notes_searched' instead (replace both trackEvent invocation and the expect
assertion to use 'notes_searched') so the test matches the actual emitted event
name.
| const { useAnalytics } = require('@/hooks/use-analytics'); | ||
| const { useNotesStore } = require('@/stores/notes/store'); | ||
|
|
||
| // Simulate initial load and refresh | ||
| mockRefresh(); // Initial load | ||
| mockRefresh(); // User refresh | ||
| const { trackEvent } = useAnalytics(); | ||
| const { notes } = useNotesStore(); | ||
|
|
||
| expect(refreshCount).toBe(2); | ||
| // Simulate refresh tracking | ||
| trackEvent('notes_refresh', { | ||
| noteCount: notes.length, | ||
| hasSearchQuery: false, | ||
| }); | ||
|
|
||
| it('validates search state management concept', () => { | ||
| // Mock search state | ||
| let searchQuery = ''; | ||
| const setSearchQuery = (query: string) => { | ||
| searchQuery = query; | ||
| }; | ||
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_refresh', { | ||
| noteCount: 0, | ||
| hasSearchQuery: false, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Rename to “notes_refreshed” to match implementation
Align with the Notes screen’s event name.
Apply this diff:
- trackEvent('notes_refresh', {
+ trackEvent('notes_refreshed', {
noteCount: notes.length,
hasSearchQuery: false,
});
- expect(mockTrackEvent).toHaveBeenCalledWith('notes_refresh', {
+ expect(mockTrackEvent).toHaveBeenCalledWith('notes_refreshed', {
noteCount: 0,
hasSearchQuery: 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.
| it('tracks refresh analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { useNotesStore } = require('@/stores/notes/store'); | |
| // Simulate initial load and refresh | |
| mockRefresh(); // Initial load | |
| mockRefresh(); // User refresh | |
| const { trackEvent } = useAnalytics(); | |
| const { notes } = useNotesStore(); | |
| expect(refreshCount).toBe(2); | |
| // Simulate refresh tracking | |
| trackEvent('notes_refresh', { | |
| noteCount: notes.length, | |
| hasSearchQuery: false, | |
| }); | |
| it('validates search state management concept', () => { | |
| // Mock search state | |
| let searchQuery = ''; | |
| const setSearchQuery = (query: string) => { | |
| searchQuery = query; | |
| }; | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_refresh', { | |
| noteCount: 0, | |
| hasSearchQuery: false, | |
| }); | |
| }); | |
| it('tracks refresh analytics event with correct data', () => { | |
| const { useAnalytics } = require('@/hooks/use-analytics'); | |
| const { useNotesStore } = require('@/stores/notes/store'); | |
| const { trackEvent } = useAnalytics(); | |
| const { notes } = useNotesStore(); | |
| // Simulate refresh tracking | |
| trackEvent('notes_refreshed', { | |
| noteCount: notes.length, | |
| hasSearchQuery: false, | |
| }); | |
| expect(mockTrackEvent).toHaveBeenCalledWith('notes_refreshed', { | |
| noteCount: 0, | |
| hasSearchQuery: false, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/app/(app)/__tests__/notes.test.tsx around lines 103 to 120, the test uses
the wrong event name 'notes_refresh' while the implementation emits
'notes_refreshed'; update the test so both the trackEvent invocation and the
expectation use 'notes_refreshed' (i.e., replace 'notes_refresh' with
'notes_refreshed' in the simulated trackEvent call and the toHaveBeenCalledWith
assertion) so the test matches the actual implementation.
| trackEvent('map_viewed', { | ||
| timestamp: new Date().toISOString(), | ||
| isMapLocked: location.isMapLocked, | ||
| hasLocation: !!(location.latitude && location.longitude), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t truthiness-check numeric coordinates; 0 is valid and currently treated as “no location”
Using expressions like location.latitude && location.longitude and !!(lat && lon) breaks at (0, 0). This causes:
- Incorrect hasLocation analytics
- Suppressed UI (PointAnnotation and recenter button won’t render)
- Camera not resetting/recentering in valid 0/0 cases
Switch to null/undefined checks.
Apply this diff:
@@
- trackEvent('map_viewed', {
- timestamp: new Date().toISOString(),
- isMapLocked: location.isMapLocked,
- hasLocation: !!(location.latitude && location.longitude),
- });
+ trackEvent('map_viewed', {
+ timestamp: new Date().toISOString(),
+ isMapLocked: location.isMapLocked,
+ hasLocation: location.latitude != null && location.longitude != null,
+ });
@@
- if (isMapReady && location.latitude && location.longitude) {
+ if (isMapReady && location.latitude != null && location.longitude != null) {
@@
- if (isMapReady && location.latitude && location.longitude) {
+ if (isMapReady && location.latitude != null && location.longitude != null) {
@@
- if (isMapReady && location.latitude && location.longitude) {
+ if (isMapReady && location.latitude != null && location.longitude != null) {
@@
- const handleRecenterMap = () => {
- if (location.latitude && location.longitude) {
+ const handleRecenterMap = () => {
+ if (location.latitude != null && location.longitude != null) {
@@
- const showRecenterButton = !location.isMapLocked && hasUserMovedMap && location.latitude && location.longitude;
+ const showRecenterButton = !location.isMapLocked && hasUserMovedMap && location.latitude != null && location.longitude != null;
@@
- {location.latitude && location.longitude && (
+ {location.latitude != null && location.longitude != null && (Also applies to: 78-104, 134-164, 167-190, 228-252, 307-309, 345-372
🤖 Prompt for AI Agents
In src/app/(app)/map.tsx around lines 68-73 (and similarly at 78-104, 134-164,
167-190, 228-252, 307-309, 345-372), the code uses truthiness checks like
location.latitude && location.longitude or !!(lat && lon) which treat 0 as falsy
and break valid (0,0) coordinates; replace those truthiness checks with explicit
null/undefined checks (e.g., check latitude != null && longitude != null or
latitude !== undefined && longitude !== undefined) wherever hasLocation or
conditional rendering/logic depends on coordinates so that 0 is treated as a
valid value and PointAnnotation, recentering, analytics, and camera logic behave
correctly.
| trackEvent('settings_login_info_updated', { | ||
| timestamp: new Date().toISOString(), | ||
| username: data.username, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Stop sending raw usernames to analytics (PII leak)
Sending the entered username is personally identifiable and unnecessary for telemetry. Replace it with a non-identifying flag.
Apply this diff to remove the PII:
- trackEvent('settings_login_info_updated', {
- timestamp: new Date().toISOString(),
- username: data.username,
- });
+ trackEvent('settings_login_info_updated', {
+ timestamp: new Date().toISOString(),
+ usernamePresent: Boolean(data.username),
+ });📝 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.
| trackEvent('settings_login_info_updated', { | |
| timestamp: new Date().toISOString(), | |
| username: data.username, | |
| }); | |
| trackEvent('settings_login_info_updated', { | |
| timestamp: new Date().toISOString(), | |
| usernamePresent: Boolean(data.username), | |
| }); |
🤖 Prompt for AI Agents
In src/app/(app)/settings.tsx around lines 59 to 63, stop sending the raw
username (PII) to analytics; replace the username field with a non-identifying
flag such as a boolean or coarse bucket. Remove data.username from the
trackEvent payload and instead send a field like "username_present" set to
!!data.username (or a simple length bucket like "username_length_bucket":
"0","1-3","4+"), so telemetry records presence/size without exposing the actual
username.
| NotesCount: 2, | ||
| ImgagesCount: 1, | ||
| FileCount: 3, | ||
| Latitude: '40.7128', |
There was a problem hiding this comment.
Typo in mockCall breaks images analytics/count assertions
The mock data uses ImgagesCount instead of the expected ImagesCount. This will likely lead to imagesCount being undefined/0 in the component and cause analytics assertions to fail.
Apply this diff to fix the typo:
- ImgagesCount: 1,
+ ImagesCount: 1,📝 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.
| NotesCount: 2, | |
| ImgagesCount: 1, | |
| FileCount: 3, | |
| Latitude: '40.7128', | |
| NotesCount: 2, | |
| ImagesCount: 1, | |
| FileCount: 3, | |
| Latitude: '40.7128', |
🤖 Prompt for AI Agents
In src/app/call/__tests__/[id].test.tsx around lines 280 to 283, the mock call
object has a typo property "ImgagesCount" which prevents the component from
reading the expected "ImagesCount"; rename the property to "ImagesCount"
(preserve value) in the mock data so the component/analytics receive the correct
images count.
| it('should handle useFocusEffect callback properly', () => { | ||
| // Test that useFocusEffect can be called with a callback | ||
| let focusCallback: (() => void) | undefined; | ||
| mockUseFocusEffect.mockImplementation((callback: () => void) => { | ||
| focusCallback = callback; | ||
| }); | ||
|
|
||
| // This simulates what happens in the component | ||
| const { useAnalytics } = require('@/hooks/use-analytics'); | ||
| const { trackEvent } = useAnalytics(); | ||
|
|
||
| // Simulate component registering focus callback | ||
| mockUseFocusEffect(() => { | ||
| trackEvent('call_detail_viewed', { | ||
| timestamp: new Date().toISOString(), | ||
| callId: '123', | ||
| }); | ||
| }); | ||
|
|
||
| expect(mockUseFocusEffect).toHaveBeenCalledWith(expect.any(Function)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Actually invoke the focus callback and assert telemetry
Currently this only asserts registration. Call the captured callback and verify trackEvent is fired to validate the flow.
Apply this diff:
- it('should handle useFocusEffect callback properly', () => {
+ it('should handle useFocusEffect callback properly', () => {
// Test that useFocusEffect can be called with a callback
let focusCallback: (() => void) | undefined;
mockUseFocusEffect.mockImplementation((callback: () => void) => {
focusCallback = callback;
});
// This simulates what happens in the component
const { useAnalytics } = require('@/hooks/use-analytics');
const { trackEvent } = useAnalytics();
// Simulate component registering focus callback
mockUseFocusEffect(() => {
trackEvent('call_detail_viewed', {
timestamp: new Date().toISOString(),
callId: '123',
});
});
- expect(mockUseFocusEffect).toHaveBeenCalledWith(expect.any(Function));
+ expect(mockUseFocusEffect).toHaveBeenCalledWith(expect.any(Function));
+ // Now simulate focus
+ focusCallback && focusCallback();
+ expect(mockTrackEvent).toHaveBeenCalledWith('call_detail_viewed', expect.objectContaining({
+ callId: '123',
+ timestamp: expect.any(String),
+ }));
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/call/__tests__/analytics-integration.test.ts around lines 177 to 197,
the test only asserts that useFocusEffect was registered but doesn't actually
invoke the captured callback or verify telemetry; modify the test to call the
captured focusCallback (guarding that it's defined) and assert that the mocked
trackEvent was called with the expected event name and payload (or at least
called once), e.g., after mockUseFocusEffect assigns focusCallback, invoke
focusCallback() and add an expect(trackEvent).toHaveBeenCalledWith(...) or
expect(trackEvent).toHaveBeenCalled() to validate the flow.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit