Skip to content

Nt 1768#57

Merged
David Nalchevanidze (nalchevanidze) merged 50 commits into
mainfrom
NT-1768
Nov 6, 2025
Merged

Nt 1768#57
David Nalchevanidze (nalchevanidze) merged 50 commits into
mainfrom
NT-1768

Conversation

@nalchevanidze
Copy link
Copy Markdown
Contributor

No description provided.

@wiz-inc-38d59fb8d7
Copy link
Copy Markdown

wiz-inc-38d59fb8d7 Bot commented Oct 28, 2025

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
SAST Finding SAST Findings 3 Low
Total 3 Low

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file may not be necessary after the latest changes, especially since the latest changes cover all requests (more or less)

@nalchevanidze
Copy link
Copy Markdown
Contributor Author

The changes in this file may not be necessary after the latest changes, especially since the latest changes cover all requests (more or less)

in which file?

Comment thread implementations/node-ssr/src/app.ts Outdated
Comment thread implementations/node-ssr/src/app.ts Outdated
})
}

if (anonymousId) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't identify using the anonymous ID. The Experience system should already have that; it's intended to be an ID that comes from the Experience system itself and is stored/reused to ensure the profile remains consistent. userId is a non-anonymous ID that is something that comes from the customer's system. So in this case, like I had stated before, you would use something that may come from a form or a query parameter, at least in a simple E2E example where we don't have state or user data saved in long-term storage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but cookie is keeping information about if its logged in or not? server stores cookie-> client uses it. ... on next visit server checks if it already user which has profile id and identifies it without generaring new id for it ... this is workflow i understand and think is worth of demonstrating

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The profileId or any of its other names like anonymousId or stableId are only indirectly related to the userId as I had explained. The profileId et al are handled internally except where it is passed from BE to FE via cookie. The userId can literally be anything. All we need to know from the returned profile data at least for our purposes is that the profile has some kind of data returned in traits. Identified users may or may not have data inside traits but unidentified users should never have data inside traits. But in our mocks, the identified user profile does have data in traits, so that makes it easier.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. but still based what you said i don't got whats wrong in codebase? it reflects what you described.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the code comment I just left.

Comment thread implementations/node-ssr/src/app.ts Outdated
Comment on lines +86 to +88
const identified = anonymousId
? await sdk.personalization.identify({ userId: anonymousId })
: undefined
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was talking about, the userId is not the anonymousId and to use it as such would be confusing to anyone using this reference implementation as a reference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. ok.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but how i should call identify in that case? use id is required field?

Copy link
Copy Markdown
Collaborator

@phobetron Charles Hudson (phobetron) Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I've mentioned before, you could add query param support to your route, expecting an optional userId param. That param's value could pretty much be anything, since the mock server isn't aware of the data on that level. Just shovel it to the mock server (via the SDK of course) and it should give you an identified user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, i don't got what you wrote ... and quite confused. it would be way easier if we paired ... as its not so intuitive to decode it as a async text messages .... otherwise just add your changes on my pr and merge it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i played around and i think i got what is whole point we wanted to test. hopefully this is what you meant.

Copy link
Copy Markdown
Contributor Author

@nalchevanidze David Nalchevanidze (nalchevanidze) Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw i was reading documentation and actually userId should not be required if anonymousId is set. see this link: https://segment.com/docs/connections/spec/identify/

Copy link
Copy Markdown
Contributor Author

@nalchevanidze David Nalchevanidze (nalchevanidze) Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this way we could still test what i was trying to test. the case when u identify based anonymousId. and not require userId as parameters.

Copy link
Copy Markdown
Collaborator

@phobetron Charles Hudson (phobetron) Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you still misunderstand userId. Even if we do update userId to be optional, we would still never set the userId to the profile ID. You would instead set anonymousId to the profile ID. But that happens internally using getAnonymousId so you don't need to deal with it manually per event.

We depart from Segment a bit here, so we don't need to validate either/or on userId vs anonymousId. The SDK handles anonymousId and profileId separately from events in our system, and userId is simply extra data. More info about that in my new comment with the suggested change.

Copy link
Copy Markdown
Collaborator

@phobetron Charles Hudson (phobetron) Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead and update the userId stuff in the schema, it's something I can do fairly quickly so you'll be able to update the PR in the morning.

Comment thread implementations/node-ssr/src/app.ts Outdated
Co-authored-by: Charles Hudson <265039+phobetron@users.noreply.github.com>
@nalchevanidze David Nalchevanidze (nalchevanidze) merged commit a9a6363 into main Nov 6, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants