Extra destination field validation - #1431
Conversation
f819c53 to
3183e78
Compare
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the current PR diff (PostHog host validation additions in , , and the new schema helpers/tests). I focused on correctness, security, and user-visible regressions and did not find actionable bugs in the current change set. The client-side validator wiring and test coverage look consistent with the intended behavior.
|
We probably can gather all destination validations in a single PR |
There was a problem hiding this comment.
Reviewed the PostHog host validation changes across , schema wiring, and the new unit tests. The validation flow and tests line up with the intended behavior (strict cloud-domain normalization while allowing general HTTP(S) self-hosted URLs). I didn’t find actionable bugs, security issues, or correctness regressions in the PR diff.
that70schris
changed the title
71b102b to
7e643f9
Compare
There was a problem hiding this comment.
Reviewed the PR diff for validation, security, and behavior changes across destination schemas and console validation wiring. I found two follow-ups: (1) a compile-time bug in destinations.tsx from duplicate imports, and (2) a potential compatibility regression from making Segment apiBase an enum-only value set.
|
|
||
| export const SegmentCredentials = z.object({ | ||
| apiBase: z.string().default("https://api.segment.io/v1").describe("API Base::Segment API Base"), | ||
| apiBase: z.enum(SEGMENT_API_BASES).default(SEGMENT_API_BASES[0]).describe("API Base::Segment API Base"), |
There was a problem hiding this comment.
Question: switching apiBase to z.enum(...) means any existing destination configs using a custom Segment-compatible endpoint will now fail schema validation. Do we have a migration/compatibility plan for already-saved configs that are outside these three values?
There was a problem hiding this comment.
it should just revert to the default if it's not one of these options
There was a problem hiding this comment.
Reviewed the diff from \ to , focusing on destination schema hardening, URL/client validation wiring in the console editor, and corresponding unit tests.\n\nI did not find additional actionable bugs or security regressions beyond the existing open discussion thread on Segment compatibility.
There was a problem hiding this comment.
Reviewed the diff from c9fae06 to 7e643f9, focusing on destination schema hardening, URL/client validation wiring in the console editor, and the added unit tests.
I did not find additional actionable bugs or security regressions beyond the existing open discussion thread on Segment apiBase compatibility.
2 participants
Validation for URL fields, removed GA4 URL, made Segment API Base a dropdown