-
Notifications
You must be signed in to change notification settings - Fork 84
Add gzip compression option for consent cookies #7142
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
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile OverviewGreptile SummaryThis PR adds optional gzip compression for Fides consent cookies to address cookie size limitations, particularly for TCF (Transparency & Consent Framework) implementations with large vendor lists that can exceed browser cookie limits. The feature introduces a new The implementation uses the browser's native CompressionStream API with automatic feature detection and graceful fallback to uncompressed cookies. Since the CompressionStream API is inherently asynchronous, this required converting all cookie operations throughout the codebase from synchronous to async, affecting 23 files across the FidesJS SDK, Privacy Center, and test suites. React components were updated to handle async cookie loading using useState/useEffect patterns. The change includes comprehensive test coverage and environment variable support for the Privacy Center. Important Files Changed
Confidence score: 4/5
|
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.
Additional Comments (1)
-
clients/privacy-center/components/modals/consent-request-modal/useConsentRequestForm.ts, line 141 (link)logic: The
saveFidesCookiecall should be awaited since it may now be async for compression operationsShould this saveFidesCookie call be awaited since compression operations are async?
28 files reviewed, 1 comment
7b0f963 to
e2c2dca
Compare
tvandort
left a comment
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.
Thanks for getting this done so quick!
Outside of the specific feedback I do think we want cypress tests around this given that we can't actually test the compression in jest unit tests / Node.
| if (!cookie) { | ||
| return newPreferences; | ||
| } |
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.
Do we need tests for the changes in this file?
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.
I'd say tests are not strictly required for this PR, but would be a good follow-up task since there's no existing test file for this component in general. The changes are low-risk defensive programming that makes the component safer with async cookie operations.
tvandort
left a comment
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.
meant to request changes on the prior review 🙃
@tvandort I actually started doing that, but in the end it felt like I was testing the browser's ability to compress, not necessarily testing our code. Probably doesn't hurt though. |
f2c97a6 to
f8b1986
Compare
|
@tvandort thanks! ready for another pass |
8d33eeb to
eb9d3ef
Compare
ENG-2232
Description Of Changes
Adds optional gzip compression for consent cookies to reduce cookie size, particularly beneficial for TCF implementations with large vendor lists.
Code Changes
fidesCookieCompressionoption to FidesOptions ("gzip"or"none")getFidesConsentCookieandsaveFidesCookieto handle async compression (CompressionStream requires this)FIDES_COOKIE_COMPRESSIONenvironment variable to Privacy CenterSteps to Confirm
/fides-js-demo.html?geolocation=eea&fides_cookie_compression=gzip)fides_consentcookie - it should be prefixed withgzip:and base64url-encodedPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works