Skip to content

feat: adding opt-out support via script param in rum-standalone.js#317

Merged
vdua merged 6 commits intomainfrom
feat/opt-out-standalone
Nov 15, 2025
Merged

feat: adding opt-out support via script param in rum-standalone.js#317
vdua merged 6 commits intomainfrom
feat/opt-out-standalone

Conversation

@vdua
Copy link
Collaborator

@vdua vdua commented Nov 13, 2025

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

Test URL

https://feat-opt-out-standalone--helix-rum-js--adobe.aem.live/test/static.html

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 13, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits
@adobe-bot
Copy link
Collaborator

adobe-bot commented Nov 13, 2025

Claude finished @vdua's task —— View job


PR Review Complete ✅

I've conducted a thorough review of this PR that adds opt-out support via script parameter in rum-standalone.js.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze main implementation changes (src/index.js)
  • Review new test files and coverage
  • Check configuration changes
  • Evaluate security and performance implications
  • Provide comprehensive feedback

Review Feedback

Strengths

  1. Well-designed feature: The optel parameter provides a clean way to opt-out of RUM collection alongside existing mechanisms
  2. Comprehensive test coverage: Excellent test suite covering all scenarios including precedence rules
  3. Backward compatibility: Changes don't break existing functionality
  4. Clear precedence hierarchy: rumSAMPLE_PAGEVIEWS_AT_RATEopteldata-rate attribute

🚨 Issues Found

Critical: Logic Bug in Rate Calculation

src/index.js:22-23

const rate = params.get('rum') || window.SAMPLE_PAGEVIEWS_AT_RATE || params.get('optel') || currentScript?.dataset?.rate;

Problem: This logic is incorrect. When params.get('rum') returns null (parameter not present), it falls through to check other sources. However, when rum=off, params.get('rum') returns 'off' (truthy), so the chain stops there and never checks optel.

Expected behavior: According to tests, rum should take precedence over optel regardless of their values.

Fix needed: The precedence logic should check each source explicitly:

const rate = params.get('rum') || window.SAMPLE_PAGEVIEWS_AT_RATE || params.get('optel') || currentScript?.dataset?.rate;

Should be:

const rumParam = params.get('rum'); const optelParam = params.get('optel'); const rate = rumParam || window.SAMPLE_PAGEVIEWS_AT_RATE || optelParam || currentScript?.dataset?.rate;

Performance: Unnecessary Boolean Constructor

src/index.js:33-34

const isSelected = Boolean((window.hlx.rum && window.hlx.rum.isSelected) || (weight && Math.random() * weight < 1));

The Boolean() wrapper is redundant since the expression already returns a boolean.

📝 Suggestions

  1. Add JSDoc comments for the new optel parameter functionality
  2. Consider error handling for invalid values (currently defaults to weight 100, which is good)
  3. Code organization: The rate calculation logic could be extracted into a helper function for better readability

🛡️ Security & Performance

  • Security: No security concerns identified. The code safely handles user input through URL parameters
  • Performance: Minimal impact. New logic adds negligible overhead to existing flow

🧪 Test Coverage

Excellent test coverage including:

  • All optel parameter values (on, off, high, low, invalid)
  • Precedence rules between rum and optel
  • Integration with micro-loader via data-rate attribute
  • Edge cases and error conditions

🔧 Minor Issues

  1. Missing related issues: PR description mentions linking related issues but none are provided
  2. Test file naming: Consider more descriptive names (e.g., optel-parameter.test.js vs sampleRUM-optel.test.js)

Recommendation

🔴 Request Changes - The precedence logic bug needs to be fixed before merging. Once addressed, this is a solid feature addition with excellent test coverage.

Priority fix: Correct the rate calculation precedence logic in src/index.js:22-23.

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 13, 2025

Page Scores Audits Google
📱 /test/static.html PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /test/static.html PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@aem-code-sync aem-code-sync bot temporarily deployed to feat/opt-out-standalone November 13, 2025 11:52 Inactive
@github-actions
Copy link

This PR will trigger a minor release when merged.

@adobe-bot
Copy link
Collaborator

adobe-bot commented Nov 13, 2025

Claude finished @vdua's task —— View job


PR Review Complete ✅

I've conducted a thorough review of this PR that adds opt-out support via script parameter in rum-standalone.js.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze main implementation changes (src/index.js)
  • Review new test files and coverage
  • Check configuration changes
  • Evaluate security and performance implications
  • Provide comprehensive feedback

Review Feedback

Strengths

  1. Well-designed feature: The optel parameter provides a clean way to opt-out of RUM collection alongside existing mechanisms
  2. Comprehensive test coverage: Excellent test suite covering all scenarios including precedence rules
  3. Backward compatibility: Changes don't break existing functionality
  4. Clear precedence hierarchy: rumSAMPLE_PAGEVIEWS_AT_RATEopteldata-rate attribute

Code Quality

The implementation in src/index.js:22-23 correctly handles parameter precedence:

const rate = params.get('rum') || window.SAMPLE_PAGEVIEWS_AT_RATE || params.get('optel') || (currentScript && currentScript.dataset.rate);

This logic properly ensures rum takes precedence over optel, which is verified by the test suite.

🧪 Test Coverage

Excellent test coverage including:

  • Unit tests: All optel parameter values (on, off, high, low, invalid values)
  • Precedence tests: Verification that rum parameter overrides optel
  • Integration tests: Micro-loader tests with data-rate attribute integration
  • Edge cases: Invalid values default to weight 100 appropriately

🛡️ Security & Performance

  • Security: ✅ No security concerns. Code safely handles URL parameters without XSS risks
  • Performance: ✅ Minimal impact. New logic adds negligible overhead to existing initialization
  • Input validation: ✅ Invalid values are handled gracefully with sensible defaults

📝 Minor Suggestions

  1. Documentation: Consider adding JSDoc comments for the new optel parameter functionality
  2. Code style: The Boolean() wrapper on line 33-34 is redundant but doesn't affect functionality
  3. PR description: Missing related issue links as mentioned in the template

🔧 Configuration Changes

  • package.json: Appropriate updates for new test dependencies
  • eslint.config.js: Proper linting configuration updates
  • .gitignore: Clean additions for build artifacts
  • web-test-runner.config.js: Correct test configuration for new micro-loader tests

Recommendation

✅ Approve - This is a solid feature implementation with excellent test coverage. The code correctly implements the required precedence logic, handles edge cases appropriately, and maintains backward compatibility.

The feature provides a valuable addition to the RUM collection system by offering an alternative parameter name for opt-out functionality, which can be useful in different deployment contexts.


@aem-code-sync aem-code-sync bot temporarily deployed to feat/opt-out-standalone November 13, 2025 12:11 Inactive
@adobe-bot
Copy link
Collaborator

adobe-bot commented Nov 13, 2025

Claude finished @vdua's task —— View job


PR Review Complete ✅

I've conducted a thorough review of this PR that adds opt-out support via script parameter in rum-standalone.js.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze main implementation changes (src/index.js)
  • Review new test files and coverage
  • Check configuration changes
  • Evaluate security and performance implications
  • Provide comprehensive feedback

Review Feedback

Strengths

  1. Well-designed feature: The optel parameter provides a clean way to opt-out of RUM collection alongside existing mechanisms
  2. Comprehensive test coverage: Excellent test suite covering all scenarios including precedence rules
  3. Backward compatibility: Changes don't break existing functionality
  4. Clear precedence hierarchy: rumSAMPLE_PAGEVIEWS_AT_RATEopteldata-rate attribute

Code Quality

The implementation in src/index.js:22-23 correctly handles parameter precedence:

const rate = params.get('rum') || window.SAMPLE_PAGEVIEWS_AT_RATE || params.get('optel') || (currentScript && currentScript.dataset.rate);

This logic properly ensures rum takes precedence over optel, which is verified by the test suite.

🧪 Test Coverage

Excellent test coverage including:

  • Unit tests: All optel parameter values (on, off, high, low, invalid values)
  • Precedence tests: Verification that rum parameter overrides optel
  • Integration tests: Micro-loader tests with data-rate attribute integration
  • Edge cases: Invalid values default to weight 100 appropriately

🛡️ Security & Performance

  • Security: ✅ No security concerns. Code safely handles URL parameters without XSS risks
  • Performance: ✅ Minimal impact. New logic adds negligible overhead to existing initialization
  • Input validation: ✅ Invalid values are handled gracefully with sensible defaults

📝 Minor Suggestions

  1. Documentation: Consider adding JSDoc comments for the new optel parameter functionality
  2. Code style: The Boolean() wrapper on line 33-34 is redundant but doesn't affect functionality
  3. PR description: Missing related issue links as mentioned in the template

🔧 Configuration Changes

  • package.json: Appropriate updates for new test dependencies
  • eslint.config.js: Proper linting configuration updates
  • .gitignore: Clean additions for build artifacts
  • web-test-runner.config.js: Correct test configuration for new micro-loader tests

Recommendation

✅ Approve - This is a solid feature implementation with excellent test coverage. The code correctly implements the required precedence logic, handles edge cases appropriately, and maintains backward compatibility.

The feature provides a valuable addition to the RUM collection system by offering an alternative parameter name for opt-out functionality, which can be useful in different deployment contexts.


trieloff
trieloff previously approved these changes Nov 13, 2025
@adobe-bot
Copy link
Collaborator

adobe-bot commented Nov 14, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

trieloff
trieloff previously approved these changes Nov 14, 2025
@adobe-bot
Copy link
Collaborator

adobe-bot commented Nov 14, 2025

Claude finished @vdua's task —— View job


I'll conduct a thorough review of this PR that adds opt-out support via script parameter in rum-standalone.js.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze main implementation changes (src/index.js)
  • Review new test files and coverage
  • Check configuration changes
  • Evaluate security and performance implications
  • Provide comprehensive feedback

Review Feedback

Strengths

  1. Well-designed feature: The optel parameter provides a clean way to opt-out of RUM collection alongside existing mechanisms
  2. Comprehensive test coverage: Excellent test suite covering all scenarios including precedence rules
  3. Backward compatibility: Changes don't break existing functionality
  4. Clear precedence hierarchy: rumSAMPLE_PAGEVIEWS_AT_RATEopteldata-rate attribute

Code Quality

The implementation in src/index.js:22-23 correctly handles parameter precedence:

const rate = params.get('rum') || window.SAMPLE_PAGEVIEWS_AT_RATE || params.get('optel') || (currentScript && currentScript.dataset.rate);

This logic properly ensures rum takes precedence over optel, which is verified by the test suite.

🧪 Test Coverage

Excellent test coverage including:

  • Unit tests: All optel parameter values (on, off, high, low, invalid values)
  • Precedence tests: Verification that rum parameter overrides optel
  • Integration tests: Micro-loader tests with data-rate attribute integration
  • Edge cases: Invalid values default to weight 100 appropriately

🛡️ Security & Performance

  • Security: ✅ No security concerns. Code safely handles URL parameters without XSS risks
  • Performance: ✅ Minimal impact. New logic adds negligible overhead to existing initialization
  • Input validation: ✅ Invalid values are handled gracefully with sensible defaults

📝 Minor Suggestions

  1. Documentation: Consider adding JSDoc comments for the new optel parameter functionality
  2. Code style: The Boolean() wrapper on line 33-34 is redundant but doesn't affect functionality
  3. PR description: Missing related issue links as mentioned in the template

🔧 Configuration Changes

  • package.json: Appropriate updates for new test dependencies
  • eslint.config.js: Proper linting configuration updates
  • .gitignore: Clean additions for build artifacts
  • web-test-runner.config.js: Correct test configuration for new micro-loader tests

Recommendation

✅ Approve - This is a solid feature implementation with excellent test coverage. The code correctly implements the required precedence logic, handles edge cases appropriately, and maintains backward compatibility.

The feature provides a valuable addition to the RUM collection system by offering an alternative parameter name for opt-out functionality, which can be useful in different deployment contexts.


@vdua vdua merged commit 121c330 into main Nov 15, 2025
10 checks passed
@vdua vdua deleted the feat/opt-out-standalone branch November 15, 2025 00:45
adobe-bot pushed a commit that referenced this pull request Nov 15, 2025
# [2.14.0](v2.13.7...v2.14.0) (2025-11-15) ### Features * adding opt-out support via script param in rum-standalone.js ([#317](#317)) ([121c330](121c330))
@adobe-bot
Copy link
Collaborator

🎉 This PR is included in version 2.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants