-
Notifications
You must be signed in to change notification settings - Fork 83
[FSSDK-11638] cleanups in event processing #1073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors how user events are built by introducing a generic buildBaseEvent
helper, removes legacy V1 log builders, updates tests to use the unified batch builder, and adds a caching check in the batch processor.
- Extracted common event fields into a new
buildBaseEvent
function and updatedbuildImpressionEvent
/buildConversionEvent
to use it - Removed
buildImpressionEventV1
/buildConversionEventV1
fromlog_event.ts
and revised specs to targetmakeEventBatch
- Added an early-return cache in
BatchEventProcessor.readEventCountInStore
to prevent redundant store reads
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
lib/event_processor/event_builder/user_event.ts | Refactored user events to use a generic BaseUserEvent<T> and buildBaseEvent helper |
lib/event_processor/event_builder/log_event.ts | Deleted legacy V1 event builders |
lib/event_processor/event_builder/log_event.spec.ts | Updated tests to call makeEventBatch and renamed suite |
lib/event_processor/batch_event_processor.ts | Added a guard to skip store lookup when eventCountInStore is already set |
Comments suppressed due to low confidence (3)
lib/event_processor/event_builder/user_event.ts:112
- [nitpick] Consider adding a JSDoc comment to
buildBaseEvent
to clarify its purpose and parameters for future maintainers.
const buildBaseEvent = <T extends EventType>({
lib/event_processor/event_builder/user_event.ts:47
- [nitpick] If
EventType
is intended for reuse outside this module, consider exporting it (export type EventType
) to improve discoverability.
type EventType = 'impression' | 'conversion';
lib/event_processor/event_builder/user_event.ts:146
- [nitpick] The new
buildImpressionEvent
andbuildConversionEvent
functions rely onbuildBaseEvent
; consider adding direct unit tests to cover their output structure.
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
Issues