Dev 3800 Privacy Plugin race condition when listening on enabled features

[BraunreutherA]

PR Type

Enhancement, Bug fix

---

Description

• Improved logger initialization by adding sinks for onLog and onError in the Ninetailed class constructor.
• Refactored the handling of SET_ENABLED_FEATURES in NinetailedCorePlugin by changing it to a public async method and removing the inline event listener.
• Renamed the SET_ENABLED_FEATURES constant to setEnabledFeatures for consistency.

---

Changes walkthrough 📝

	Relevant files
Enhancement	Ninetailed.ts Improved logger initialization and error handling in Ninetailed class. packages/sdks/javascript/src/lib/Ninetailed.ts Added logger sinks for onLog and onError in the constructor. Moved logger initialization before event builder initialization. +10/-9    constants.ts Renamed `SET_ENABLED_FEATURES` constant for consistency.  packages/sdks/javascript/src/lib/NinetailedCorePlugin/constants.ts Renamed SET_ENABLED_FEATURES constant to setEnabledFeatures. +1/-1
Bug fix	NinetailedCorePlugin.ts Refactored `SET_ENABLED_FEATURES` handling in NinetailedCorePlugin. packages/sdks/javascript/src/lib/NinetailedCorePlugin/NinetailedCorePlugin.ts Changed SET_ENABLED_FEATURES to a public async method. Removed inline event listener for SET_ENABLED_FEATURES. +4/-7

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[codiumai-pr-agent-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The refactoring in NinetailedCorePlugin.ts changes the method of handling SET_ENABLED_FEATURES from an event listener to a public async method. This could potentially alter the timing and conditions under which enabledFeatures are set, affecting the plugin's behavior. Ensure that this change does not introduce race conditions or state inconsistencies, especially under concurrent operations.
	Code Clarity: The renaming of the SET_ENABLED_FEATURES constant to setEnabledFeatures in constants.ts improves consistency but requires thorough checking to ensure all references across the project have been updated to avoid reference errors.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add null checks for the logger object before calling addSink methods To avoid potential issues with the logger object being undefined or null, add a null check before calling addSink methods. packages/sdks/javascript/src/lib/Ninetailed.ts [180-185] -if (typeof onLog === 'function') { +if (logger && typeof onLog === 'function') { logger.addSink(new OnLogLogSink(onLog)); } -if (typeof onError === 'function') { +if (logger && typeof onError === 'function') { logger.addSink(new OnErrorLogSink(onError)); } Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding null checks for the logger object is a good practice to prevent runtime errors. The suggestion correctly identifies a potential issue and offers a solution that enhances the robustness of the code.	7
Best practice	Use the EventFn type for the SET_ENABLED_FEATURES method's parameter for consistency To ensure consistency and avoid potential issues, consider using the EventFn type for the SET_ENABLED_FEATURES method's parameter. packages/sdks/javascript/src/lib/NinetailedCorePlugin/NinetailedCorePlugin.ts [149-151] -public [SET_ENABLED_FEATURES] = async ({ payload }: EventFn) => { - this.enabledFeatures = payload.features || []; +public [SET_ENABLED_FEATURES] = async (event: EventFn) => { + this.enabledFeatures = event.payload.features || []; }; Apply this suggestion Suggestion importance[1-10]: 6 Why: Using consistent types for method parameters is a best practice that improves code readability and maintainability. The suggestion is correct and aligns with good coding practices, although it's not a critical change.	6
Maintainability	Group the logger initialization code together for better readability and maintainability To improve readability and maintainability, consider grouping the logger initialization code together. packages/sdks/javascript/src/lib/Ninetailed.ts [180-188] -if (typeof onLog === 'function') { - logger.addSink(new OnLogLogSink(onLog)); +if (logger) { + if (typeof onLog === 'function') { + logger.addSink(new OnLogLogSink(onLog)); + } + + if (typeof onError === 'function') { + logger.addSink(new OnErrorLogSink(onError)); + } + + this.logger = logger; } -if (typeof onError === 'function') { - logger.addSink(new OnErrorLogSink(onError)); -} - -this.logger = logger; - Apply this suggestion Suggestion importance[1-10]: 5 Why: Grouping related code together can indeed improve readability and maintainability. However, the suggested change is not critical and does not address any functional issues or bugs.	5

[codiumai-pr-agent-pro]

CI Failure Feedback 🧐

Action: Tests 🧪

Failed stage: Test 🧪 [❌]

Failed test name: plugins-privacy:test

Failure summary: The action failed because the test plugins-privacy:test failed. The test encountered the following issues: Multiple warnings about ReactDOM.render no longer being supported in React 18, suggesting to use createRoot instead. An unhandled exception indicating that "The component using the context must be a descendant of the NinetailedProvider". A specific test failure in src/NinetailedPrivacyPlugin.spec.ts where testPlugin[SET_ENABLED_FEATURES] was expected to be called once but failed.

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 617: [command]/usr/bin/tar -xf /home/runner/work/_temp/d963cc1d-65f8-414e-96c6-210fcad1a050/cache.tzst -P -C /home/runner/work/experience.js/experience.js --use-compress-program unzstd 618: Received 412540113 of 412540113 (100.0%), 196.4 MBs/sec 619: Cache restored successfully 620: Cache restored from key: Linux-node-modules-yarn-c3f1d1144619d2e920e20f3f2da854d0e90a3b6c4e02a5b6e3c9bf6d54d61320 621: ##[group]Run nrwl/nx-set-shas@v3 622: with: 623: main-branch-name: main 624: set-environment-variables-for-job: true 625: error-on-no-successful-workflow: false 626: last-successful-event: push 627: working-directory: . 628: ##[endgroup] 629: ##[group]Run node "$GITHUB_ACTION_PATH/dist/index.js" "$gh_token" "$main_branch_name" "$error_on_no_successful_workflow" "$last_successful_event" "$working_directory" "$working_id" 630: �[36;1mnode "$GITHUB_ACTION_PATH/dist/index.js" "$gh_token" "$main_branch_name" "$error_on_no_successful_workflow" "$last_successful_event" "$working_directory" "$working_id"�[0m 631: shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0} 632: env: 633: gh_token: *** 634: main_branch_name: main 635: error_on_no_successful_workflow: false ... 744: Ran all test suites. 745: > nx run plugins-preview:test 746: No tests found, exiting with code 0 747: > nx run sdks-react:test 748: PASS sdks-react packages/sdks/react/src/lib/Experience/ComponentMarker/ComponentMarker.spec.tsx (14.163 s) 749: PASS sdks-react packages/sdks/react/src/lib/MergeTag/helpers.spec.ts 750: PASS sdks-react packages/sdks/react/src/lib/Experience/useExperience.spec.tsx (24.34 s) 751: ● Console 752: console.error 753: Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot 754: 88 | }; 755: 89 | 756: > 90 | const { result } = renderHook( 757: | ^ 758: 91 | () => 759: 92 | useExperience({ 760: 93 | baseline: { 761: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 762: at printWarning (../../../node_modules/react-dom/cjs/react-dom.development.js:86:30) 763: at error (../../../node_modules/react-dom/cjs/react-dom.development.js:60:7) ... 765: at ../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:80:18 766: at act (../../../node_modules/react/cjs/react.development.js:2512:16) 767: at render (../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:79:26) 768: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 769: at src/lib/Experience/useExperience.spec.tsx:90:34 770: at ../../../node_modules/tslib/tslib.js:118:75 771: at Object.__awaiter (../../../node_modules/tslib/tslib.js:114:16) 772: at Object.<anonymous> (src/lib/Experience/useExperience.spec.tsx:59:79) 773: console.error 774: Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot 775: 151 | }; 776: 152 | 777: > 153 | const { result } = renderHook( 778: | ^ 779: 154 | () => 780: 155 | useExperience({ 781: 156 | baseline: { 782: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 783: at printWarning (../../../node_modules/react-dom/cjs/react-dom.development.js:86:30) 784: at error (../../../node_modules/react-dom/cjs/react-dom.development.js:60:7) ... 789: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 790: at src/lib/Experience/useExperience.spec.tsx:153:34 791: at ../../../node_modules/tslib/tslib.js:118:75 792: at Object.__awaiter (../../../node_modules/tslib/tslib.js:114:16) 793: at Object.<anonymous> (src/lib/Experience/useExperience.spec.tsx:119:61) 794: PASS sdks-react packages/sdks/react/src/lib/MergeTag/MergeTag.spec.tsx (24.419 s) 795: PASS sdks-react packages/sdks/react/src/lib/useNinetailed.spec.tsx (6.028 s) 796: ● Console 797: console.error 798: Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot 799: 17 | }; 800: 18 | 801: > 19 | const { result } = renderHook(() => useNinetailed(), { 802: | ^ 803: 20 | wrapper: contextWrapper, 804: 21 | }); 805: 22 | 806: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 807: at printWarning (../../../node_modules/react-dom/cjs/react-dom.development.js:86:30) 808: at error (../../../node_modules/react-dom/cjs/react-dom.development.js:60:7) 809: at Object.render (../../../node_modules/react-dom/cjs/react-dom.development.js:29670:5) 810: at ../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:80:18 811: at act (../../../node_modules/react/cjs/react.development.js:2512:16) 812: at render (../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:79:26) 813: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 814: at Object.<anonymous> (src/lib/useNinetailed.spec.tsx:19:34) 815: console.error 816: Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot 817: 26 | 818: 27 | it('should throw an error if ninetailed context is not provided', () => { 819: > 28 | const { result } = renderHook(() => useNinetailed()); 820: | ^ 821: 29 | 822: 30 | expect(result.error).toEqual( 823: 31 | new Error( 824: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 825: at printWarning (../../../node_modules/react-dom/cjs/react-dom.development.js:86:30) 826: at error (../../../node_modules/react-dom/cjs/react-dom.development.js:60:7) 827: at Object.render (../../../node_modules/react-dom/cjs/react-dom.development.js:29670:5) 828: at ../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:80:18 829: at act (../../../node_modules/react/cjs/react.development.js:2512:16) 830: at render (../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:79:26) 831: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 832: at Object.<anonymous> (src/lib/useNinetailed.spec.tsx:28:34) 833: console.error 834: Error: Uncaught [Error: The component using the the context must be a descendant of the NinetailedProvider] 835: at reportException (/home/runner/work/experience.js/experience.js/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24) ... 864: at _runTestsForDescribeBlock (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:126:9) 865: at _runTestsForDescribeBlock (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:121:9) 866: at run (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:71:3) 867: at runAndTransformResultsToJestFormat (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) 868: at jestAdapter (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) 869: at runTestInternal (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:367:16) 870: at runTest (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:444:34) 871: at Object.worker (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/testWorker.js:106:12) { 872: detail: Error: The component using the the context must be a descendant of the NinetailedProvider ... 912: at runAndTransformResultsToJestFormat (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) 913: at jestAdapter (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) 914: at runTestInternal (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:367:16) 915: at runTest (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:444:34) 916: at Object.worker (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/testWorker.js:106:12), 917: type: 'unhandled exception' 918: } 919: 26 | 920: 27 | it('should throw an error if ninetailed context is not provided', () => { 921: > 28 | const { result } = renderHook(() => useNinetailed()); 922: | ^ 923: 29 | 924: 30 | expect(result.error).toEqual( 925: 31 | new Error( 926: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 927: at reportException (../../../node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:70:28) ... 942: at legacyCreateRootFromDOMContainer (../../../node_modules/react-dom/cjs/react-dom.development.js:29575:5) 943: at legacyRenderSubtreeIntoContainer (../../../node_modules/react-dom/cjs/react-dom.development.js:29601:12) 944: at Object.render (../../../node_modules/react-dom/cjs/react-dom.development.js:29685:10) 945: at ../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:80:18 946: at act (../../../node_modules/react/cjs/react.development.js:2512:16) 947: at render (../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:79:26) 948: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 949: at Object.<anonymous> (src/lib/useNinetailed.spec.tsx:28:34) 950: console.error 951: Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot 952: 44 | }; 953: 45 | 954: > 46 | const { result } = renderHook(() => useNinetailed(), { 955: | ^ 956: 47 | wrapper: contextWrapper, 957: 48 | }); 958: 49 | 959: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 960: at printWarning (../../../node_modules/react-dom/cjs/react-dom.development.js:86:30) 961: at error (../../../node_modules/react-dom/cjs/react-dom.development.js:60:7) 962: at Object.render (../../../node_modules/react-dom/cjs/react-dom.development.js:29670:5) 963: at ../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:80:18 964: at act (../../../node_modules/react/cjs/react.development.js:2512:16) 965: at render (../../../node_modules/@testing-library/react-hooks/lib/dom/pure.js:79:26) 966: at renderHook (../../../node_modules/@testing-library/react-hooks/lib/core/index.js:114:5) 967: at Object.<anonymous> (src/lib/useNinetailed.spec.tsx:46:34) 968: console.error 969: Error: Uncaught [Error: The component using the the context must be a descendant of the NinetailedProvider] 970: at reportException (/home/runner/work/experience.js/experience.js/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24) ... 999: at _runTestsForDescribeBlock (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:126:9) 1000: at _runTestsForDescribeBlock (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:121:9) 1001: at run (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/run.js:71:3) 1002: at runAndTransformResultsToJestFormat (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) 1003: at jestAdapter (/home/runner/work/experience.js/experience.js/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) 1004: at runTestInternal (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:367:16) 1005: at runTest (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/runTest.js:444:34) 1006: at Object.worker (/home/runner/work/experience.js/experience.js/node_modules/jest-runner/build/testWorker.js:106:12) { 1007: detail: Error: The component using the the context must be a descendant of the NinetailedProvider ... 1053: } 1054: 44 | }; 1055: 45 | 1056: > 46 | const { result } = renderHook(() => useNinetailed(), { 1057: | ^ 1058: 47 | wrapper: contextWrapper, 1059: 48 | }); 1060: 49 | 1061: at console.error (../../../node_modules/@testing-library/react-hooks/lib/core/console.js:19:7) 1062: at reportException (../../../node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:70:28) ... 1211: 148 | 1212: > 149 | expect(testPlugin[SET_ENABLED_FEATURES]).toHaveBeenCalledTimes(1); 1213: | ^ 1214: 150 | expect(testPlugin[SET_ENABLED_FEATURES]).toHaveBeenCalledWith( 1215: 151 | expect.objectContaining({ 1216: 152 | payload: expect.objectContaining({ 1217: at src/NinetailedPrivacyPlugin.spec.ts:149:46 1218: at fulfilled (../../../node_modules/tslib/tslib.js:115:62) 1219: Test Suites: 1 failed, 1 total 1220: Tests: 1 failed, 6 passed, 7 total ... 1229: PASS playgrounds-gatsby packages/playgrounds/gatsby/src/pages/index.spec.tsx 1230: Index 1231: ✓ should render successfully (60 ms) 1232: Test Suites: 1 passed, 1 total 1233: Tests: 1 passed, 1 total 1234: Snapshots: 0 total 1235: Time: 5.308 s 1236: Ran all test suites. 1237: > NX Running target test for 19 projects failed 1238: Failed tasks: 1239: - plugins-privacy:test 1240: error Command failed with exit code 1. 1241: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. 1242: ##[error]Process completed with exit code 1.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.