[No QA] Send current policy ID to Sentry#73848
[No QA] Send current policy ID to Sentry#73848sosek108 wants to merge 7 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Patch coverage is
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Context Policies is present in Sentry and we can filter by policyID 
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Onyx.connectWithoutView({ | ||
| key: ONYXKEYS.SESSION, | ||
| callback: (value) => { | ||
| if (!value?.email) { |
There was a problem hiding this comment.
Security Concern: Potential PII Exposure
The sendPoliciesContext() function sends an array of all policy IDs the user is a member of to Sentry. This could potentially expose sensitive information about the user's workspace memberships.
Considerations:
- Policy IDs can be considered sensitive data as they reveal which workspaces a user belongs to
- The array
activePoliciescould become quite large for users with many workspace memberships - This data will be attached to every error reported to Sentry for this session
Recommendation:
Consider whether sending all policy IDs is necessary, or if just sending the count would suffice:
const activePoliciesCount = getActivePolicies(policies, session.email).length;
Sentry.setTag(CONST.TELEMETRY.TAG_ACTIVE_POLICY, activePolicyID);
Sentry.setContext(CONST.TELEMETRY.CONTEXT_POLICIES, {activePolicyID, activePoliciesCount});If the full list is needed for debugging, ensure this is documented and approved by the security/privacy team.
|
|
||
| Onyx.connectWithoutView({ | ||
| key: ONYXKEYS.SESSION, | ||
| callback: (value) => { |
There was a problem hiding this comment.
Performance: Race Condition and Multiple Unnecessary Calls
The current implementation has a race condition where sendPoliciesContext() may be called multiple times unnecessarily:
- When
activePolicyIDupdates → callssendPoliciesContext() - When
sessionupdates → callssendPoliciesContext() - When
policiesupdates → callssendPoliciesContext()
Issues:
- If all three Onyx connections update simultaneously (e.g., during app initialization),
sendPoliciesContext()could be called 3 times - Each call processes the entire policies collection with
getActivePolicies()and.map(), which can be expensive with many policies - Repeated calls to
Sentry.setTag()andSentry.setContext()with the same data
Recommendation:
Debounce the sendPoliciesContext() calls or use a flag to prevent redundant updates:
let hasInitialized = false;
function sendPoliciesContext() {
if (!policies || !session?.email || !activePolicyID) {
return;
}
const activePolicies = getActivePolicies(policies, session.email).map((policy) => policy.id);
const newContext = {activePolicyID, activePolicies};
// Only update if data has changed
if (!hasInitialized || previousActivePolicyID !== activePolicyID) {
Sentry.setTag(CONST.TELEMETRY.TAG_ACTIVE_POLICY, activePolicyID);
Sentry.setContext(CONST.TELEMETRY.CONTEXT_POLICIES, newContext);
hasInitialized = true;
}
}
| if (!value?.email) { | ||
| return; | ||
| } | ||
| session = value; |
There was a problem hiding this comment.
Performance: Inefficient Array Transformation
The code performs two array operations on potentially large collections:
getActivePolicies(policies, session.email)- filters all policies.map((policy) => policy.id)- transforms the filtered array
Issue:
For users with many policies, this creates unnecessary intermediate arrays and can impact performance, especially since this runs on every policy/session/activePolicyID change.
Recommendation:
Combine the operations for better performance:
const activePolicies = Object.values(policies ?? {})
.filter<Policy>(
(policy): policy is Policy =>
!!policy &&
policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
!!policy.name &&
!!policy.id &&
!!getPolicyRole(policy, session.email)
)
.map((policy) => policy.id);Or even better, directly extract IDs without creating intermediate Policy objects:
const activePolicies = Object.values(policies ?? {})
.reduce<string[]>((ids, policy) => {
if (policy &&
policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
policy.name &&
policy.id &&
getPolicyRole(policy, session.email)) {
ids.push(policy.id);
}
return ids;
}, []);
| let activePolicyID: OnyxEntry<string>; | ||
| let policies: OnyxCollection<Policy>; | ||
|
|
||
| Onyx.connectWithoutView({ |
There was a problem hiding this comment.
Missing Import Statement
The code uses getPolicyRole() function from PolicyUtils in line 50, but there's no import statement for it.
Current imports:
import * as Sentry from '@sentry/react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Policy, Session} from '@src/types/onyx';
import FS from './Fullstory';
import {getActivePolicies} from './PolicyUtils';Issue:
The function getPolicyRole is used inside getActivePolicies but if you're calling getActivePolicies, the function internally uses getPolicyRole, so this should work. However, if the recommendation from my previous comment is followed (inlining the logic), you'll need to import getPolicyRole.
Required addition if inlining the logic:
import {getActivePolicies, getPolicyRole} from './PolicyUtils';Or import from CONST:
// getPolicyRole uses CONST.RED_BRICK_ROAD_PENDING_ACTION internallyThis is a note for maintainability - the current code works because getActivePolicies is properly imported.
| import type {Policy, Session} from '@src/types/onyx'; | ||
| import FS from './Fullstory'; | ||
| import {getActivePolicies} from './PolicyUtils'; | ||
|
|
There was a problem hiding this comment.
TypeScript: Missing Type Safety for Onyx Callbacks
The Onyx callback functions lack explicit type annotations, which reduces type safety.
Current:
Onyx.connectWithoutView({
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (value) => {
// ...
},
});Issue:
- The
valueparameter is implicitly typed - No type checking ensures the correct data structure is being handled
- Harder to catch type errors during development
Recommendation:
Add explicit types to all callback parameters:
Onyx.connectWithoutView({
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (value: OnyxEntry<string>) => {
if (!value) {
return;
}
activePolicyID = value;
sendPoliciesContext();
},
});
Onyx.connectWithoutView({
key: ONYXKEYS.SESSION,
callback: (value: OnyxEntry<Session>) => {
if (!value?.email) {
return;
}
session = value;
sendPoliciesContext();
},
});
Onyx.connectWithoutView({
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (value: OnyxCollection<Policy>) => {
if (!value) {
return;
}
policies = value;
sendPoliciesContext();
},
});
|
|
||
| /** | ||
| * Connect to Onyx to retrieve information about user's active policies. | ||
| */ |
There was a problem hiding this comment.
Error Handling: Missing Error Boundaries
The code lacks error handling for potential failures in the Sentry integration or the getActivePolicies function.
Potential failure points:
getActivePolicies()could throw if policies data is corruptedSentry.setTag()orSentry.setContext()could fail if Sentry isn't initialized- Accessing
policy.idcould fail if the policy object is malformed
Issue:
If any error occurs in sendPoliciesContext(), it could:
- Break the telemetry synchronization silently
- Prevent other Onyx callbacks from executing properly
- Leave the app in an inconsistent state
Recommendation:
Wrap the logic in try-catch blocks:
function sendPoliciesContext() {
try {
if (!policies || !session?.email || !activePolicyID) {
return;
}
const activePolicies = getActivePolicies(policies, session.email).map((policy) => policy.id);
Sentry.setTag(CONST.TELEMETRY.TAG_ACTIVE_POLICY, activePolicyID);
Sentry.setContext(CONST.TELEMETRY.CONTEXT_POLICIES, {activePolicyID, activePolicies});
} catch (error) {
// Log error but don't throw - telemetry failures shouldn't break the app
console.error('[TelemetrySynchronizer] Failed to send policies context:', error);
}
}Also consider wrapping individual Onyx callbacks:
Onyx.connectWithoutView({
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (value: OnyxEntry<string>) => {
try {
if (!value) {
return;
}
activePolicyID = value;
sendPoliciesContext();
} catch (error) {
console.error('[TelemetrySynchronizer] Error in activePolicyID callback:', error);
}
},
});
| let policies: OnyxCollection<Policy>; | ||
|
|
||
| Onyx.connectWithoutView({ | ||
| key: ONYXKEYS.NVP_ACTIVE_POLICY_ID, |
There was a problem hiding this comment.
Onyx Pattern: Consider Using initWithStoredValues: false
For the ONYXKEYS.COLLECTION.POLICY connection, you may want to set initWithStoredValues: false to prevent unnecessary initial processing.
Current behavior:
- When the app starts, Onyx will immediately call the callback with all stored policies
- This triggers
sendPoliciesContext()even before the user's session is fully loaded - Could result in unnecessary processing of potentially stale data
Issue:
The callback will fire with cached policy data from storage, which might be outdated or irrelevant if the user isn't logged in yet.
Recommendation:
Onyx.connectWithoutView({
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
initWithStoredValues: false, // Only react to fresh updates, not cached data
callback: (value: OnyxCollection<Policy>) => {
if (!value) {
return;
}
policies = value;
sendPoliciesContext();
},
});Alternatively, if you do want to use cached values, ensure the guard in sendPoliciesContext() properly handles stale data by checking if the session is valid and active.
| * This file contains the logic for sending additional data to Sentry. | ||
| * | ||
| * It uses Onyx.connectWithoutView as nothing here is related to the UI. We only send data to external provider and want to keep this outside of the render loop. | ||
| * It uses Onyx.connectWithoutView as nothing here is related to the UI. We only send data to the external provider and want to keep this outside of the render loop. |
There was a problem hiding this comment.
Documentation: Update File Comment
The file-level comment should be updated to reflect the new functionality.
Current comment:
/**
* This file contains the logic for sending additional data to Sentry.
*
* It uses Onyx.connectWithoutView as nothing here is related to the UI. We only send data to external provider and want to keep this outside of the render loop.
*/Recommendation:
Update to describe what data is being sent:
/**
* This file contains the logic for sending additional data to Sentry.
*
* Currently synchronizes:
* - FullStory session ID for cross-platform debugging
* - Active policy ID and user's policy memberships for workspace context
*
* It uses Onyx.connectWithoutView as nothing here is related to the UI.
* We only send data to the external provider and want to keep this outside of the render loop.
*/This helps future developers understand what telemetry data is being collected and sent to Sentry.
| @@ -1614,6 +1614,8 @@ const CONST = { | |||
| }, | |||
| TELEMETRY: { | |||
| CONTEXT_FULLSTORY: 'Fullstory', | |||
There was a problem hiding this comment.
Code Organization: Consider Grouping Related Constants
The new telemetry constants are well-named, but consider adding a comment to document their purpose:
TELEMETRY: {
// FullStory integration
CONTEXT_FULLSTORY: 'Fullstory',
// Policy/Workspace tracking
TAG_ACTIVE_POLICY: 'active_policy_id',
CONTEXT_POLICIES: 'Policies',
},This makes it clearer which constants are related and makes it easier to find relevant constants when working on telemetry features.
Minor style note: Consider consistent casing - CONTEXT_FULLSTORY uses 'Fullstory' (Pascal case) while CONTEXT_POLICIES uses 'Policies'. Should these both be uppercase for consistency?
CONTEXT_FULLSTORY: 'FullStory', // or 'fullstory'
CONTEXT_POLICIES: 'Policies', // matches
|
|
||
| Onyx.connectWithoutView({ | ||
| key: ONYXKEYS.SESSION, | ||
| callback: (value) => { |
There was a problem hiding this comment.
Logic Issue: No Cleanup When User Logs Out
The current implementation doesn't clear Sentry context when a user logs out or when the session becomes invalid.
Issue:
If a user logs out:
session.emailbecomesundefinedor the session object is cleared- The guard
if (!session?.email)prevents updating Sentry - However, the previous user's policy data remains in Sentry's context
- This stale data will be attached to error reports even after logout
Security/Privacy Concern:
After logout, errors should not include the previous user's policy information.
Recommendation:
Clear Sentry context when session is invalidated:
Onyx.connectWithoutView({
key: ONYXKEYS.SESSION,
callback: (value: OnyxEntry<Session>) => {
// Clear context if session is cleared/invalid
if (!value?.email) {
Sentry.setTag(CONST.TELEMETRY.TAG_ACTIVE_POLICY, null);
Sentry.setContext(CONST.TELEMETRY.CONTEXT_POLICIES, null);
session = null;
return;
}
session = value;
sendPoliciesContext();
},
});Similarly, consider clearing when activePolicyID is cleared:
Onyx.connectWithoutView({
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (value: OnyxEntry<string>) => {
if (!value) {
// Clear the active policy tag when it's unset
Sentry.setTag(CONST.TELEMETRY.TAG_ACTIVE_POLICY, null);
activePolicyID = null;
return;
}
activePolicyID = value;
sendPoliciesContext();
},
});
| /** | ||
| * Connect to Onyx to retrieve information about user's active policies. | ||
| */ | ||
| let session: OnyxEntry<Session>; |
There was a problem hiding this comment.
Code Style: Inconsistent Variable Initialization
The module-level variables are declared but not initialized:
let session: OnyxEntry<Session>;
let activePolicyID: OnyxEntry<string>;
let policies: OnyxCollection<Policy>;Issue:
- These variables are declared but not initialized
- TypeScript allows accessing them before Onyx populates them
- Could lead to undefined behavior if
sendPoliciesContext()is called before all connections are established
Recommendation:
Initialize with explicit null for clarity:
let session: OnyxEntry<Session> = null;
let activePolicyID: OnyxEntry<string> = null;
let policies: OnyxCollection<Policy> = null;This makes it explicit that these values start as null. Note: This follows some patterns in the codebase, though PolicyUtils.ts also doesn't initialize its allPolicies variable, so either pattern is acceptable for consistency.
Code Review SummaryI've reviewed PR #73848 and identified several areas for improvement: Critical Issues
Performance Concerns
Code Quality
Minor Issues
All issues have been documented with inline comments containing specific recommendations and code examples. The implementation is functional but would benefit from addressing the security/privacy and cleanup concerns before merging. |
1 participant
Explanation of Change
Sent active
policyIdto Sentry.This is follow up to another PR #73843.
Fixed Issues
$ #73324
PROPOSAL:
Tests
active_policy_idtagpoliciescontext inside of Span and it consist proper policy idsOffline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop