-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Use the props passed to identify to determine sessions #3334
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
base: dev
Are you sure you want to change the base?
Conversation
@perso182 is attempting to deploy a commit to the umami-software Team on Vercel. A member of the Team first needs to authorize it. |
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
Overall, these changes integrate custom identity properties into session definitions, leveraging localStorage to differentiate sessions on shared PCs.
- In /src/tracker/index.js, the track function now reads the identity from localStorage and includes it in event payloads, while identify writes new props and clears cached sessions.
- In /src/app/api/send/route.ts, the identity is stringified and factored into session ID generation, ensuring different identities yield distinct sessions.
- Be cautious: JSON.parse on localStorage may throw errors; consider error handling for robustness.
- Review security implications of storing identity in localStorage.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
We too would love to have something like this. We have multiple users using the same IP and browser, on a thin client, and the possibility of sending in a hashed username or some other identifier would be great! |
src/app/api/send/route.ts
Outdated
@@ -18,6 +18,7 @@ const schema = z.object({ | |||
payload: z.object({ | |||
website: z.string().uuid(), | |||
data: anyObjectParam.optional(), | |||
identity: anyObjectParam.optional(), |
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 should be a string, not an object. Should be renamed to just id
.
src/tracker/index.js
Outdated
@@ -233,21 +233,34 @@ | |||
}; | |||
|
|||
const track = (obj, data) => { | |||
let identity; | |||
try { | |||
const parsedIdentity = JSON.parse(localStorage.getItem('umami.identity')); |
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.
We can't use localStorage for privacy reasons. We might have to rely on umami.identify and change the signature to umami.identify(id, data)
.
…moved the usage of localstorage
@mikecao I did a new commit changing the signature of |
Hey!
I added the custom props sent to umami.identify to also be used for defining the session. This way if the same PC visits a website that uses Umami, but different props has been passed to umami.identify, it will result in different sessions rather than one session with the properties of the last visit.
Fixes #3181
Not sure if saving the identity in localstorage is the way to go so let me know if this is not a reasonable solution.