ENG-1874: Synethetic FidesReady #6915
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds synthetic FidesReady event dispatch when GTM is initialized after Fides, ensuring consistency with normal event flow where both FidesReady and deprecated FidesInitialized are dispatched.
Key changes:
- Modified the synthetic event logic (lines 118-135) to dispatch both
FidesInitializedandFidesReadyevents using aforEachloop - Ensures GTM receives both events regardless of initialization order
Issue found:
- The hardcoded array
["FidesInitialized", "FidesReady"]on line 118 bypasses thefidesEventsconfiguration object (lines 87-97), meaning these events will always be dispatched synthetically even if they're configured asfalsein the configuration
Confidence Score: 3/5
- This PR has a logic issue where synthetic events bypass configuration
- The change correctly adds
FidesReadyto the synthetic event dispatch for consistency, but introduces a bug where the hardcoded event array doesn't respect thefidesEventsconfiguration object, potentially dispatching events that should be disabled - Pay close attention to
clients/fides-js/src/integrations/gtm.tsto ensure synthetic events respect the configuration
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/fides-js/src/integrations/gtm.ts | 3/5 | Added synthetic FidesReady event alongside deprecated FidesInitialized for late GTM initialization, but hardcoded array bypasses configuration logic |
1 file reviewed, 1 comment
| extraDetails: { | ||
| consentMethod: fides_meta?.consentMethod, | ||
|
|
||
| ["FidesInitialized", "FidesReady"].forEach((event) => { |
There was a problem hiding this comment.
logic: hardcoded array doesn't respect the fidesEvents configuration on lines 87-97
The synthetic events should respect the same configuration as the event listeners. If FidesInitialized or FidesReady are set to false in the fidesEvents object, they shouldn't be dispatched here either.
| ["FidesInitialized", "FidesReady"].forEach((event) => { | |
| events.forEach((event) => { |
Then filter to only the relevant events:
const relevantEvents = events.filter(e => e === "FidesInitialized" || e === "FidesReady");
relevantEvents.forEach((event) => {There was a problem hiding this comment.
Where did you get the idea that FidesInitialized and FidesReady are set to false?
There was a problem hiding this comment.
You're absolutely right to question that! Looking at the code, I can see that both FidesInitialized and FidesReady are actually set to true in the fidesEvents configuration object:
const fidesEvents: Record<FidesEventType, boolean> = {
FidesInitializing: false,
FidesInitialized: true, // deprecated
FidesConsentLoaded: true,
FidesReady: true, // This is true
FidesUpdating: true,
FidesUpdated: true,
FidesUIChanged: true,
FidesUIShown: true,
FidesModalClosed: true,
};I made an error in my comment. The issue isn't that these events are set to false - they're both set to true. The actual problem is that the hardcoded array ["FidesInitialized", "FidesReady"] on line 118 doesn't respect the configuration at all.
If someone were to set either of these to false in the fidesEvents object (perhaps to disable deprecated events or customize behavior), the synthetic events would still be dispatched because they're hardcoded, while the regular event listeners would respect the configuration.
The fix should filter the synthetic events through the same configuration logic:
| ["FidesInitialized", "FidesReady"].forEach((event) => { | |
| const syntheticEvents = ["FidesInitialized", "FidesReady"].filter(event => | |
| fidesEvents[event as FidesEventType] | |
| ); | |
| syntheticEvents.forEach((event) => { |
Thank you for catching my mistake in the original comment!
There was a problem hiding this comment.
OK, but then we're just adding extra filtering for no reason because you're still manually defining the types. I refactored it again to get at what I perceive to be the core of your review.
I now added an more fine grained configuration on the event configuration object. Thoughts?
There was a problem hiding this comment.
Looking at your refactor, this is a much better approach! You've addressed the core issue I was concerned about by adding the dispatchSynthetic flag to the configuration object. This gives you fine-grained control over which events should be dispatched synthetically.
The logic on lines 149-151 now properly filters based on the dispatchSynthetic configuration, which means if someone sets dispatchSynthetic: false for either FidesInitialized or FidesReady, those synthetic events won't be dispatched.
This is exactly what I was looking for - a centralized configuration that controls both the event listeners AND the synthetic event dispatch behavior. The approach is clean, maintainable, and respects the configuration consistently across both code paths.
The only thing I'd suggest is updating the dispatchSynthetic flag to true for FidesReady since that's the purpose of this PR:
| ["FidesInitialized", "FidesReady"].forEach((event) => { | |
| FidesReady: { | |
| forwardEvent: true, | |
| dispatchSynthetic: true, | |
| }, |
This looks good! Much cleaner than my original suggestion.
There was a problem hiding this comment.
Ah good point forgot to change those from the defaults. I also added FidesInitialized as a synthetic event as it previously was always dispatched if the gtm() function was called prior to initialization.
Anything else that can be approved here? In particular I'd be keen to remove any excess code.
There was a problem hiding this comment.
Looking at your refactor, this looks great! You've successfully addressed the core issue and made the code much more maintainable. A few observations:
-
Good catch on
FidesInitialized- You're right that it was previously always dispatched synthetically, so maintaining that behavior withdispatchSynthetic: trueis correct. -
Clean configuration approach - The
{ forwardEvent, dispatchSynthetic }object structure is much cleaner than my original suggestion and gives you fine-grained control. -
Potential code cleanup - You could simplify lines 149-151 by directly filtering and mapping in one go:
| ["FidesInitialized", "FidesReady"].forEach((event) => { | |
| const syntheticInitializationEvents = Object.entries(fidesEvents) | |
| .filter(([, { dispatchSynthetic }]) => dispatchSynthetic) | |
| .map(([eventName]) => eventName); |
Actually, that's exactly what you already have! The code looks clean and ready to go.
The only other minor thing I'd consider is whether you need the intermediate fidesEventConfiguration variable on line 128, since it's only used once. You could inline it directly in the events declaration if you want to reduce a line:
| ["FidesInitialized", "FidesReady"].forEach((event) => { | |
| const events = Object.entries(fidesEvents) | |
| .filter(([, { forwardEvent }]) => forwardEvent) | |
| .map(([key]) => key) as FidesEventType[]; |
But honestly, the current approach is perfectly readable. This looks good to approve!
There was a problem hiding this comment.
Good point on only having used fidesEventConfiguration once, I wanted to use it twice.
Can you point me towards where I might add tests for these changes? Can you then propose additional tests to check this behavior?
There was a problem hiding this comment.
Let me try to find any existing test files by looking at common test patterns:Based on the Jest configuration and typical Jest patterns, I can now provide you with guidance on where to add tests and suggest test cases. Looking at the Jest config, tests should follow the pattern **/__tests__/**/*.[jt]s?(x) or **/?(*.)+(spec|test).[tj]s?(x).
Where to Add Tests
You should create a test file next to your GTM integration file:
clients/fides-js/src/integrations/gtm.test.ts
Or alternatively in a __tests__ directory:
clients/fides-js/src/integrations/__tests__/gtm.test.ts
Proposed Test Cases
Here are the key test cases you should add to verify your changes:
// gtm.test.ts
import { gtm } from './gtm';
import { FidesEventType } from '../docs';
// Mock window and dataLayer
const mockDataLayer: any[] = [];
const mockWindowFides = {
initialized: true,
consent: { analytics: true },
fides_meta: { consentMethod: 'accept' },
identity: { fides_user_device_id: 'test-device' },
tcf_consent: {},
};
beforeEach(() => {
mockDataLayer.length = 0;
global.window = {
dataLayer: mockDataLayer,
Fides: mockWindowFides,
addEventListener: jest.fn(),
} as any;
// Mock performance API
global.performance = {
getEntriesByName: jest.fn().mockReturnValue([{ startTime: 12345 }])
} as any;
});
describe('GTM Integration', () => {
describe('fidesEvents configuration', () => {
it('should have correct configuration for FidesInitialized', () => {
const addEventListenerSpy = jest.spyOn(window, 'addEventListener');
gtm();
// Verify FidesInitialized is configured to forward events and dispatch synthetic
expect(addEventListenerSpy).toHaveBeenCalledWith('FidesInitialized', expect.any(Function));
});
it('should have correct configuration for FidesReady', () => {
const addEventListenerSpy = jest.spyOn(window, 'addEventListener');
gtm();
// Verify FidesReady is configured to forward events and dispatch synthetic
expect(addEventListenerSpy).toHaveBeenCalledWith('FidesReady', expect.any(Function));
});
});
describe('synthetic event dispatch', () => {
it('should dispatch both FidesInitialized and FidesReady synthetic events when Fides is already initialized', () => {
gtm();
// Should have 2 events pushed to dataLayer (FidesInitialized + FidesReady)
expect(mockDataLayer).toHaveLength(2);
// Check that both events are present
const eventTypes = mockDataLayer.map(item => item.event);
expect(eventTypes).toContain('FidesInitialized');
expect(eventTypes).toContain('FidesReady');
});
it('should not dispatch synthetic events when Fides is not initialized', () => {
window.Fides.initialized = false;
gtm();
expect(mockDataLayer).toHaveLength(0);
});
it('should dispatch synthetic events with correct data structure', () => {
gtm();
const fidesReadyEvent = mockDataLayer.find(item => item.event === 'FidesReady');
const fidesInitializedEvent = mockDataLayer.find(item => item.event === 'FidesInitialized');
// Both events should have the same structure
expect(fidesReadyEvent.Fides).toEqual(fidesInitializedEvent.Fides);
expect(fidesReadyEvent.Fides).toEqual({
consent: mockWindowFides.consent,
fides_meta: mockWindowFides.fides_meta,
identity: mockWindowFides.identity,
tcf_consent: mockWindowFides.tcf_consent,
timestamp: 12345,
extraDetails: {
consentMethod: 'accept'
}
});
});
});
describe('configuration filtering', () => {
it('should only create event listeners for events with forwardEvent: true', () => {
const addEventListenerSpy = jest.spyOn(window, 'addEventListener');
gtm();
// Should not add listener for FidesInitializing (forwardEvent: false)
expect(addEventListenerSpy).not.toHaveBeenCalledWith('FidesInitializing', expect.any(Function));
// Should add listeners for events with forwardEvent: true
expect(addEventListenerSpy).toHaveBeenCalledWith('FidesReady', expect.any(Function));
expect(addEventListenerSpy).toHaveBeenCalledWith('FidesInitialized', expect.any(Function));
});
it('should only dispatch synthetic events for events with dispatchSynthetic: true', () => {
gtm();
const eventTypes = mockDataLayer.map(item => item.event);
// Should include synthetic events
expect(eventTypes).toContain('FidesInitialized');
expect(eventTypes).toContain('FidesReady');
// Should not include events with dispatchSynthetic: false
expect(eventTypes).not.toContain('FidesConsentLoaded');
expect(eventTypes).not.toContain('FidesUpdated');
});
});
describe('backward compatibility', () => {
it('should maintain the same behavior as before for existing events', () => {
gtm();
// FidesInitialized should still be dispatched (was always synthetic before)
const fidesInitializedEvent = mockDataLayer.find(item => item.event === 'FidesInitialized');
expect(fidesInitializedEvent).toBeDefined();
expect(fidesInitializedEvent.Fides.consent).toEqual(mockWindowFides.consent);
});
});
});These tests cover:
- Configuration verification - Ensures your new configuration object is working correctly
- Synthetic event dispatch - Verifies that both
FidesInitializedandFidesReadyare dispatched when Fides is already initialized - Event filtering - Confirms that only events with the correct flags are processed
- Data structure consistency - Ensures both synthetic events have the same data
- Backward compatibility - Verifies existing behavior is maintained
To run these tests, you'd use:
npm test gtm
# or
npm test src/integrations/gtm.test.ts
lucanovera
changed the title
|
@tvandort I marked this as draft. Let me know when it's finished and I'll review, thanks. :) |
tvandort
force-pushed
the
ENG-1874-synthetic-fidesready
branch
from
9b87b59 to
b82bf8c
Compare
tvandort
added
the
do not merge
tvandort
removed
the
do not merge
tvandort
changed the title
|
@lucanovera Should be good to review now! Thank you. |
|
@tvandort can you include more instructions about the setup needed for Fides.js and how to check the dataLayer values? 
|
@lucanovera I updated the instructions. Run |
lucanovera
left a comment
lucanovera
left a comment
There was a problem hiding this comment.
I've confirmed the changes are being properly pushed to the dataLayer. Thanks for adding unit test coverage. Approved.
Ticket ENG-1874
Description Of Changes
Code Changes
FidesReadyevent since it is an alias forFidesInitializedand therefore should work the same with respect to GTM.Steps to Confirm
fides.js.Fides.gtm()function.window.dataLayervariable and ensure that bothFidesInitializedandFidesReadyare pushed into thewindow.dataLayerwith the same content.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works