-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore(persons): Refactor person properties update code so it can be reused #33368
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.
PR Summary
Refactored person properties update logic by moving it into a dedicated directory structure and extracting update functionality for reusability in batch processing.
- Moved
person-state.ts
toplugin-server/src/worker/ingestion/persons/
and split property update logic into newperson-update.ts
file - Added
personPropertyKeyUpdateCounter
inmetrics.ts
to track property updates with proper Prometheus instrumentation - Updated all import paths from
../person-state
to../persons/person-state
across multiple files - Maintained thorough test coverage by extending
person-state.test.ts
with JSONB optimization flag compatibility tests - Renamed parameter
distinctIdBatchStore
topersonStoreBatch
for better clarity in function signatures
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
}) | ||
}) |
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.
There appears to be an extra closing bracket })
on line 113 that doesn't correspond to any opening bracket. This will cause a syntax error when the code is executed. Please remove this redundant bracket to ensure the code compiles correctly.
}) | |
}) | |
}) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
This LGTM
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
8624b1f
to
7ebb096
Compare
Important
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Problem
This refactor moves the
person-state.ts
file into persons as well as moving all of the update logic into its own separate file so we can reuse this code when making the changes for batch writing.Changes
Did you write or update any docs for this change?
How did you test this code?