From fbee2684883954f9efca1978da8afc6789eb9d7e Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Sun, 7 Dec 2025 17:23:08 +0000 Subject: [PATCH 01/26] Initial code comments from PR reviews --- injected/.cursor/rules/injected.mdc | 106 ++++++++++++ injected/docs/README.md | 4 +- injected/docs/coding-guidelines.md | 201 +++++++++++++++++++++++ injected/docs/testing-guide.md | 76 +++++++++ injected/src/features/autofill-import.js | 5 +- injected/src/features/click-to-load.js | 9 + injected/src/features/page-context.js | 18 +- injected/src/features/web-compat.js | 33 +++- special-pages/.cursorrules | 2 + special-pages/docs/coding-guidelines.md | 174 ++++++++++++++++++++ 10 files changed, 615 insertions(+), 13 deletions(-) create mode 100644 injected/.cursor/rules/injected.mdc create mode 100644 injected/docs/coding-guidelines.md create mode 100644 special-pages/docs/coding-guidelines.md diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc new file mode 100644 index 0000000000..d07d63d819 --- /dev/null +++ b/injected/.cursor/rules/injected.mdc @@ -0,0 +1,106 @@ +--- +description: Guidelines for injected privacy features development +globs: injected/** +--- + +# Injected Features + +JavaScript features injected into web pages for privacy protections. Features extend `ConfigFeature` and integrate with remote configuration for per-site enable/disable. + +**See also:** `docs/coding-guidelines.md` for code style and best practices. + +## Structure + +``` +src/features/.js # Feature implementation (extends ContentFeature) +src/features.js # Feature registry +entry-points/.js # Platform-specific bundles +integration-test/ # Playwright integration tests +unit-test/ # Jasmine unit tests +``` + +## Feature Pattern + +Features extend `ContentFeature` and implement lifecycle methods: + +```javascript +import ContentFeature from '../content-feature.js'; + +export default class MyFeature extends ContentFeature { + init() { + // Main initialization - feature is enabled for this site + if (this.getFeatureSettingEnabled('someSetting')) { + this.applySomeFix(); + } + } + + load() { + // Early load - before remote config (use sparingly) + } + + update(data) { + // Receive updates from browser + } +} +``` + +## Remote Configuration + +Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: + +```javascript +// Boolean check with default +if (this.getFeatureSettingEnabled('settingName')) { ... } +if (this.getFeatureSettingEnabled('settingName', 'disabled')) { ... } // default disabled + +// Get setting value +const settings = this.getFeatureSetting('settingName'); +``` + +## Messaging + +Use inherited messaging methods: + +```javascript +// Fire-and-forget +this.notify('messageName', { data }); + +// Request/response +const response = await this.request('messageName', { data }); + +// Subscribe to updates +this.subscribe('eventName', (data) => { ... }); +``` + +## TypeScript + +JSDoc types in JavaScript files. Import types via `@typedef`: + +```javascript +/** @typedef {import('../types/feature.js').SettingType} SettingType */ +``` + +## Testing + +Run from `injected/` directory: + +| Command | Purpose | +|---------|---------| +| `npm run test-unit` | Jasmine unit tests | +| `npm run test-int` | Playwright integration tests | +| `npm run build` | Build all platform bundles | + +Integration tests use test pages in `integration-test/test-pages/`. + +## Key Patterns + +- **Event listeners**: Use `handleEvent` pattern or stored references (avoid `.bind(this)`) +- **API existence**: Check `typeof API === 'function'` before wrapping +- **DOM timing**: Check `document.readyState` before accessing DOM +- **Error types**: Use appropriate types (`DOMException`, `TypeError`) in shims + +## Notes + +- When running Playwright commands, use `--reporter list` to prevent hanging +- Features can be enabled/disabled per-site via remote config +- Add debug flags with `this.addDebugFlag()` for breakage reports diff --git a/injected/docs/README.md b/injected/docs/README.md index 802032362d..2a4fe2cb04 100644 --- a/injected/docs/README.md +++ b/injected/docs/README.md @@ -15,6 +15,7 @@ This directory contains detailed documentation for the Content Scope Scripts pro - **[Development Utilities](./development-utilities.md)** - Scope injection utilities and development tools - **[Testing Guide](./testing-guide.md)** - Local testing and development workflow +- **[Coding Guidelines](./coding-guidelines.md)** - Code style, architecture, security, and performance guidelines ### Existing Documentation @@ -25,7 +26,8 @@ This directory contains detailed documentation for the Content Scope Scripts pro If you're new to Content Scope Scripts, start with the main [README](../README.md) for a high-level overview, then dive into the specific documentation based on your needs: -- **Building features?** → [Features Guide](./features-guide.md) +- **Building features?** → [Features Guide](./features-guide.md) (feature files contain inline guidelines) +- **Writing code?** → [Coding Guidelines](./coding-guidelines.md) - **Integrating with a platform?** → [Platform Integration](./platform-integration.md) and [Platform-Specific Build & Troubleshooting Tips](./build-and-troubleshooting.md) - **Using the API?** → [API Reference](./api-reference.md) - **Developing utilities?** → [Development Utilities](./development-utilities.md) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md new file mode 100644 index 0000000000..f640861658 --- /dev/null +++ b/injected/docs/coding-guidelines.md @@ -0,0 +1,201 @@ +# Coding Guidelines + +> Guidelines for injected features development. + +## Code Style + +### Naming Conventions +- Use descriptive names for variables, functions, and classes +- Follow existing naming patterns in the codebase + +### Event Listener Management + +Use stored references or the class-based `handleEvent` pattern to ensure proper removal: + +#### Stored reference pattern: +```js +this.scrollListener = () => {...}; +document.addEventListener('scroll', this.scrollListener); +document.removeEventListener('scroll', this.scrollListener); +``` + +#### Class-based handleEvent pattern: +```js +class MyFeature extends ContentFeature { + init() { + document.addEventListener('scroll', this); + } + destroy() { + document.removeEventListener('scroll', this); + } + handleEvent(e) { + if (e.type === 'scroll') { + requestAnimationFrame(() => this.updatePosition()); + } + } +} +``` + +**Avoid** using `.bind(this)` directly in addEventListener—it creates a new reference each time, preventing removal. + +### Module Structure +- Extract reusable logic into separate files for focused unit testing +- Security-sensitive operations (e.g., markdown to HTML conversion) should be isolated with extensive tests + +### Error Handling and Debugging +- Avoid hardcoding debug flags; ensure they are configurable and environment-dependent +- Remove `console.log` statements from production code + +## Architecture & Design + +### Action Execution Flow +- Execute secondary actions from the top level to avoid context handling issues +- Avoid re-calling execution functions within an action + +### Navigation Event Handling +- Use `navigatesuccess` event for URL change detection (ensures navigation is committed): + +```js +globalThis.navigation.addEventListener('navigatesuccess', handleURLChange); +``` + +### Re-entrancy Pattern +- Check `document.readyState` to avoid missing DOM elements: + +```js +if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', () => this.applyFix()); +} else { + this.applyFix(); +} +``` + +### Documentation Updates +- Update documentation to reflect critical changes +- Document workarounds for bugs clearly for future maintenance + +### Regression Handling +- Address regressions promptly, especially those affecting critical functionality +- Ensure changes do not introduce unintended side effects + +## Security & Privacy + +### XSS Vulnerabilities +- Isolate and unit test any logic that manipulates HTML or sets content dynamically + +### Error Handling and Edge Cases +- Validate elements and their types before operations: + +```javascript +if (!element) { + return PirError.create(`could not find element`); +} +if ((isInputElement(element) && ['text', 'hidden'].includes(element.type)) || isTextAreaElement(element)) { + element.value = token; +} else { + return PirError.create(`element is neither a text input nor textarea`); +} +``` + +### Caching and State Management +- Avoid global or static caching mechanisms that could lead to race conditions +- Use instance-scoped storage or include all relevant identifiers in cache keys + +### Permissions Handling +- Ensure custom permission handling does not bypass native permission models +- Handle permissions with custom behaviors or name overrides correctly + +### API Usage in Iframes +- Be cautious enabling APIs like Web Share within iframes—may cause security issues or runtime errors + +## Performance + +### Memory Management +- Use `WeakSet`/`WeakMap` for DOM element references to allow garbage collection +- Delete entries from collections after use: + +```js +navigations.delete(event.target); +``` + +### Message Bridge Caching +- Avoid global or static caches for message bridges +- Include all relevant parameters (`featureName`, `messageSecret`) in cache keys + +### Console Log Statements +- Remove `console.log` statements from production code + +## Error Handling + +### Error Messages +- Ensure error messages are accurate and specific +- Replace generic 'unknown error' with context-specific messages + +### Async/Await Usage +- Use `await` to ensure errors are caught and flow is maintained: + +```js +await someAsyncFunction(); +``` + +### Error Handling in Promises +- Ensure promises have both resolve and reject paths: + +```js +new Promise((resolve, reject) => { + if (condition) { + resolve(result); + } else { + reject(new Error('specific error message')); + } +}); +``` + +### Null Checks +- Perform null checks before using objects: + +```js +if (object != null) { + // Use the object +} +``` + +### Captcha Error Handling +- Bubble up errors to avoid silent failures: + +```js +if (PirError.isError(captchaProvider)) { + return createError(captchaProvider.error.message); +} +``` + +## Testing + +### Test Independence in Playwright +- Ensure tests are independent for safe parallel execution +- Use Playwright's test fixtures for complex setup + +### Async/Await in Tests +- Always use `await` with asynchronous operations: + +```javascript +// Correct +await overlays.opensShort(url); + +// Incorrect +overlays.opensShort(url); +``` + +### Specificity in Test File Selection +- Use specific glob patterns to include only test files: + +```javascript +// Correct +'integration-test/**/*.spec.js' + +// Incorrect - includes non-test files +'integration-test/**' +``` + +### Unit Testing Security-sensitive Code +- Extract security-sensitive logic to separate files for focused unit testing diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index 688df48c2c..791b27ad12 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -68,3 +68,79 @@ If you drop a `debugger;` line in the scripts and open DevTools window, the DevT ### Verifying CSS is Loaded Open DevTools, go to the Console tab and enter `navigator.duckduckgo`. If it's defined, then Content Scope Scripts is running. + +## Testing Best Practices + +### Test Independence in Playwright + +- Ensure Playwright tests are independent to allow safe parallel execution. Avoid shared setup outside of each test block +- Utilize Playwright's test fixtures for complex setup requirements + +### Async/Await in Tests +- Always use `await` with asynchronous operations to ensure the test waits for the operation to complete. This is crucial for accurately testing operations that may throw errors: + +```javascript +// Correct +await overlays.opensShort(url) + +// Incorrect +overlays.opensShort(url) +``` + +### Test Case Isolation +- Refactor tests to ensure independence and allow for safe parallel execution. Avoid shared setup that prevents running tests in parallel: + +```javascript +// Refactor shared setup into individual test blocks for isolation +test('example test', async ({ page }) => { + // Setup code here +}); +``` + +### Specificity in Test File Selection +- Use specific glob patterns to include only test files, avoiding the inclusion of non-test files which could cause unexpected behavior or test failures: + +```javascript +// Correct +'integration-test/broker-protection-tests/**/*.spec.js' + +// Incorrect +'integration-test/broker-protection-tests/**' +``` + +### Unit Testing Security-sensitive Code +- Extract security-sensitive logic, such as markdown-to-HTML conversion, to a separate file. This allows for focused unit testing to prevent security vulnerabilities like XSS: + +```javascript +// Extract to markdownToHtml.js +export function convertMarkdownToHTML(markdown) { + // Conversion logic here +} +``` + +### Mocking and Test Data +- Include comprehensive scenarios in mocks to accurately test behavior, especially for edge cases. This ensures tests cover a wide range of possible states: + +```javascript +// Adding missing scenario in mocks +case 'new_scenario': { + // Mock logic here + break; +} +``` + +### Correct Test Setup for Feature Flags +- Ensure test setup accurately reflects the intended test conditions, especially when testing with feature flags or configurations that enable or disable features: + +```javascript +// Correct setup for disabled feature +await setupTestWithFeatureDisabled(page); +``` + +### Handling Undefined in Test Expectations +- Account for `undefined` values in test expectations, especially when testing conditions that may not set a variable or return a value: + +```javascript +// Correct expectation handling for undefined values +expect(window.someTestVariable).toBeUndefined(); +``` diff --git a/injected/src/features/autofill-import.js b/injected/src/features/autofill-import.js index 1b5e3513d2..763d96b720 100644 --- a/injected/src/features/autofill-import.js +++ b/injected/src/features/autofill-import.js @@ -59,7 +59,7 @@ export default class AutofillImport extends ActionExecutorBase { #isBookmarkModalVisible = false; - /** @type {WeakSet} */ + /** @type {WeakSet} Use WeakSet for DOM refs to allow GC */ #tappedElements = new WeakSet(); /** @@ -430,6 +430,8 @@ export default class AutofillImport extends ActionExecutorBase { } /** + * Event handler using class instance pattern. + * Use `addEventListener('scroll', this)` not `.bind(this)` to allow removal. * @param {Event} event */ handleEvent(event) { @@ -451,6 +453,7 @@ export default class AutofillImport extends ActionExecutorBase { /** * Checks if the path is supported for animation. + * NOTE: All paths here must have corresponding handlers in getElementAndStyleFromPath() * @param {string} path * @returns {boolean} */ diff --git a/injected/src/features/click-to-load.js b/injected/src/features/click-to-load.js index 2e1876f5a5..38f4082f9f 100644 --- a/injected/src/features/click-to-load.js +++ b/injected/src/features/click-to-load.js @@ -1,3 +1,12 @@ +/** + * @file Click to Load Feature + * @see injected/docs/coding-guidelines.md for general patterns + * + * Key reminders: + * - Don't set `inset: 'auto'` after copying positioning styles + * - Use `{ key: value }` message payloads, not primitives + * - Handler name is `contentScopeScripts` (not `contentScopeScriptsIsolated`) + */ import { Messaging, TestTransportConfig, WebkitMessagingConfig } from '../../../messaging/index.js'; import { createCustomEvent, originalWindowDispatchEvent } from '../utils.js'; import { logoImg, loadingImages, closeIcon, facebookLogo } from './click-to-load/ctl-assets.js'; diff --git a/injected/src/features/page-context.js b/injected/src/features/page-context.js index 8d375b49f0..b1ce8d515e 100644 --- a/injected/src/features/page-context.js +++ b/injected/src/features/page-context.js @@ -1,3 +1,7 @@ +/** + * @file Page Context Feature + * @see injected/docs/coding-guidelines.md for general patterns + */ import ContentFeature from '../content-feature.js'; import { getFaviconList } from './favicon.js'; import { isDuckAi, isBeingFramed, getTabUrl } from '../utils.js'; @@ -179,6 +183,7 @@ export function domToMarkdown(node, settings, depth = 0) { } /** + * Safely get attribute or empty string to avoid null in markdown output * @param {Element} node * @param {string} attr * @returns {string} @@ -192,15 +197,17 @@ function collapseAndTrim(str) { return collapseWhitespace(str).trim(); } +/** + * Note: The whitespace difference between href/non-href cases is intentional. + * With href: collapse AND trim (for clean markdown links) + * Without href: collapse only (retain surrounding space context) + */ function getLinkText(node, children, settings) { const href = node.getAttribute('href'); const trimmedContent = collapseAndTrim(children); if (settings.trimBlankLinks && trimmedContent.length === 0) { return ''; } - // The difference in whitespace handling is intentional here. - // Where we don't wrap in a link: - // we should retain at least one preceding and following space. return href ? `[${trimmedContent}](${href})` : collapseWhitespace(children); } @@ -373,6 +380,8 @@ export default class PageContext extends ContentFeature { this.#delayedRecheckTimer = setTimeout(() => { this.log.info('Performing delayed recheck after navigation'); this.recheckCount++; + // Note: invalidateCache() is safe here because we pass false to prevent + // resetRecheckCount, avoiding unintended recheck loops this.invalidateCache(); this.handleContentCollectionRequest(false); @@ -445,6 +454,8 @@ export default class PageContext extends ContentFeature { } // Cache the result - setter handles timestamp and observer + // Note: We only cache if content exists. Consider caching empty content too + // if mutation observation is needed for initially empty pages. if (content.content.length > 0) { this.cachedContent = content; } @@ -504,6 +515,7 @@ export default class PageContext extends ContentFeature { const result = domToMarkdown(root, { maxLength: upperLimit, maxDepth, + // Use getFeatureSettingEnabled with default, not `|| true` which ignores explicit false includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled'), excludeSelectors: excludeSelectorsString, trimBlankLinks: this.getFeatureSettingEnabled('trimBlankLinks', 'enabled'), diff --git a/injected/src/features/web-compat.js b/injected/src/features/web-compat.js index bd6c8a2d5e..cd2b1b5a49 100644 --- a/injected/src/features/web-compat.js +++ b/injected/src/features/web-compat.js @@ -1,10 +1,15 @@ +/** + * @file Web Compatibility Feature + * @see injected/docs/coding-guidelines.md for general patterns + */ import ContentFeature from '../content-feature.js'; // eslint-disable-next-line no-redeclare import { URL } from '../captured-globals.js'; import { DDGProxy, DDGReflect } from '../utils'; import { wrapToString, wrapFunction } from '../wrapper-utils.js'; /** - * Fixes incorrect sizing value for outerHeight and outerWidth + * Fixes incorrect sizing value for outerHeight and outerWidth. + * Note: Avoid hardcoding window geometry values - use calculations or config where possible. */ function windowSizingFix() { if (window.outerHeight !== 0 && window.outerWidth !== 0) { @@ -167,7 +172,10 @@ export class WebCompat extends ContentFeature { } } - /** Shim Web Share API in Android WebView */ + /** + * Shim Web Share API in Android WebView + * Note: Always verify API existence before shimming + */ shimWebShare() { if (typeof navigator.canShare === 'function' || typeof navigator.share === 'function') return; @@ -341,7 +349,7 @@ export class WebCompat extends ContentFeature { try { const result = await nativeRequest('requestPermission', {}); const resultPermission = /** @type {NotificationPermission} */ (result?.permission || 'denied'); - // Update cached permission so Notification.permission reflects the new state + // Update cached permission so Notification.permission getter reflects new state permission = resultPermission; if (deprecatedCallback) { deprecatedCallback(resultPermission); @@ -393,9 +401,9 @@ export class WebCompat extends ContentFeature { return; } nativeNotify('closeNotification', { id: this.#id }); - // Remove from map first to prevent duplicate onclose from native event + // Remove from map BEFORE firing onclose to prevent duplicate events from native feature.#webNotifications.delete(this.#id); - // Fire onclose handler + // Fire onclose handler after cleanup if (typeof this.onclose === 'function') { try { // @ts-expect-error - NotificationPolyfill doesn't fully implement Notification interface @@ -535,14 +543,17 @@ export class WebCompat extends ContentFeature { "Failed to execute 'query' on 'Permissions': Failed to read the 'name' property from 'PermissionDescriptor': Required member is undefined.", ); } + // Don't bypass - all supportedPermissions need handling, including non-native with name overrides if (!settings.supportedPermissions || !(query.name in settings.supportedPermissions)) { throw new TypeError( `Failed to execute 'query' on 'Permissions': Failed to read the 'name' property from 'PermissionDescriptor': The provided value '${query.name}' is not a valid enum value of type PermissionName.`, ); } const permSetting = settings.supportedPermissions[query.name]; + // Use custom permission name if configured, else original query name const returnName = permSetting.name || query.name; let returnStatus = settings.permissionResponse || 'prompt'; + // Only query native for permissions marked native:true if (permSetting.native) { try { const response = await this.messaging.request(MSG_PERMISSIONS_QUERY, query); @@ -566,6 +577,7 @@ export class WebCompat extends ContentFeature { /** * Fixes screen lock/unlock APIs for Android WebView. + * Uses wrapProperty to match original property descriptors. */ screenLockFix() { const validOrientations = [ @@ -869,19 +881,22 @@ export class WebCompat extends ContentFeature { // This should never occur, but keeps typescript happy if (!settings) return; + // Handler strategies: 'reflect' (pass through), 'undefined' (hide), 'polyfill' (stub) const proxy = new Proxy(globalThis.webkit.messageHandlers, { get(target, messageName, receiver) { const handlerName = String(messageName); - // handle known message names, such as DDG webkit messaging + // 'reflect': pass through to real handler (e.g., DDG webkit messaging) if (settings.handlerStrategies.reflect.includes(handlerName)) { return Reflect.get(target, messageName, receiver); } + // 'undefined': hide handler existence if (settings.handlerStrategies.undefined.includes(handlerName)) { return undefined; } + // 'polyfill': return stub (use '*' for catch-all) if (settings.handlerStrategies.polyfill.includes('*') || settings.handlerStrategies.polyfill.includes(handlerName)) { return { postMessage() { @@ -901,12 +916,14 @@ export class WebCompat extends ContentFeature { } viewportWidthFix() { + // Note: This guard is intentional for legacy mode. onUserPreferencesMerged() calls + // this without the guard for cases where re-application is needed. if (this._viewportWidthFixApplied) { return; } this._viewportWidthFixApplied = true; + // Re-entrancy pattern: check readyState to avoid missing DOM elements if (document.readyState === 'loading') { - // if the document is not ready, we may miss the original viewport tag document.addEventListener('DOMContentLoaded', () => this.viewportWidthFixInner()); } else { this.viewportWidthFixInner(); @@ -933,7 +950,7 @@ export class WebCompat extends ContentFeature { viewportWidthFixInner() { /** @type {NodeListOf} **/ const viewportTags = document.querySelectorAll('meta[name=viewport i]'); - // Chrome respects only the last viewport tag + // Chrome respects only the last viewport tag - modify existing rather than adding new const viewportTag = viewportTags.length === 0 ? null : viewportTags[viewportTags.length - 1]; const viewportContent = viewportTag?.getAttribute('content') || ''; /** @type {readonly string[]} **/ diff --git a/special-pages/.cursorrules b/special-pages/.cursorrules index 8597f6543e..46a689b9ce 100644 --- a/special-pages/.cursorrules +++ b/special-pages/.cursorrules @@ -2,6 +2,8 @@ Preact-based HTML/CSS/JS applications embedded in DuckDuckGo browsers. Each page is an isolated app with privileged native API access. +**See also:** `docs/coding-guidelines.md` for code style and best practices. + ## Structure ``` diff --git a/special-pages/docs/coding-guidelines.md b/special-pages/docs/coding-guidelines.md new file mode 100644 index 0000000000..1d7208345e --- /dev/null +++ b/special-pages/docs/coding-guidelines.md @@ -0,0 +1,174 @@ +# Special Pages Coding Guidelines + +> Guidelines for Preact-based special pages development. + +## Code Style + +### Naming Conventions +- Use descriptive names for variables, functions, and classes +- Follow existing naming patterns in the codebase + +### Component and Hook Usage + +#### Preact Attributes +- Use `class` instead of `className` in Preact components + +#### Key Placement +- Avoid unnecessary re-renders by correctly placing keys: + +```jsx +// Incorrect - key on wrapper div +
+ {/* Component content */} +
+ +// Correct - key on component + +``` + +#### Dependency Management in Hooks +- Include all used values in the dependency array of `useCallback` to prevent stale closures: + +```js +const setter = useCallback(() => { + // Function body using dep1 and dep2 +}, [dep1, dep2]); // Include all dependencies +``` + +### Error Handling and Debugging +- Avoid hardcoding debug flags; ensure they are configurable and environment-dependent +- Remove `console.log` statements from production code + +## Architecture & Design + +### Internationalization +- Use translation functions via `useTypedTranslations()`: + +```js +const { t } = useTypedTranslations(); +const message = t('key_name'); +``` + +- Store translations in `public/locales/.json` + +### Type Consistency +- Use JSDoc types imported via `@typedef`: + +```js +/** @typedef {import('./types.js').MyType} MyType */ +``` + +- Import types from generated files in `types/` directory + +### Platform-Specific Styling +- Use `data-platform-name` attribute for platform detection +- Use `data-theme` for dark/light mode instead of media queries + +### Component Design +- Avoid rendering children multiple times within components +- Ensure props (especially `style`) are handled correctly +- Do not couple functionality to UI visibility—control functionality independently from whether UI elements are shown + +### AI Functionality and UI Coupling +- Decouple AI functionality from its setting UI visibility +- Ensure functionality is controlled independently from UI elements + +### Feature State Management +- Maintain parameter order in method signatures to prevent breaking changes +- Avoid overriding falsy values incorrectly + +### Documentation Updates +- Update documentation to reflect critical changes, such as method parameter reordering + +## Security & Privacy + +### XSS Vulnerabilities +- Isolate and unit test any logic that manipulates HTML or sets content dynamically +- Markdown to HTML conversion should be in a separate file with extensive testing: + +```javascript +// In a separate utility file with unit tests +export function convertMarkdownToHTML(markdown) { + const regex = /\*\*(.*?)\*\*/g; + return markdown.replace(regex, '$1'); +} +``` + +### Controlled Component State +- Avoid directly manipulating properties like `value` on controlled components +- Let Preact manage input state through props and event handlers + +## Performance + +### Memory Management +- Clean up subscriptions and event listeners in component unmount + +### Console Log Statements +- Remove `console.log` statements from production code + +## Error Handling + +### Error Messages +- Ensure error messages are accurate and specific +- Replace generic 'unknown error' with specific error messages + +### Async/Await Usage +- Use `await` with asynchronous operations to ensure errors are caught: + +```js +await someAsyncFunction(); +``` + +### Error Handling in Promises +- Ensure promises have both resolve and reject paths: + +```js +new Promise((resolve, reject) => { + if (condition) { + resolve(result); + } else { + reject(new Error('specific error message')); + } +}); +``` + +### Null Checks +- Perform null checks before using objects: + +```js +if (object != null) { + // Use the object +} +``` + +### Element Visibility and Accessibility +- Avoid changing element visibility in a way that prevents focus: + +```css +/* Ensure focusable elements remain accessible */ +[aria-expanded="true"] { + visibility: visible; /* Not hidden */ +} +``` + +## Testing + +### Test Independence in Playwright +- Ensure tests are independent to allow safe parallel execution +- Avoid shared setup outside of each test block +- Use Playwright's test fixtures for complex setup + +### Async/Await in Tests +- Always use `await` with asynchronous operations: + +```javascript +// Correct +await page.click('button'); + +// Incorrect - may not wait for completion +page.click('button'); +``` + +### Mocking and Test Data +- Include comprehensive scenarios in mocks to cover edge cases + From 9ff5118da2e53692cc2f4f55daf58af939d9b966 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Sun, 7 Dec 2025 17:37:47 +0000 Subject: [PATCH 02/26] Make some tweaks to wording --- injected/.cursor/rules/injected.mdc | 2 +- injected/docs/coding-guidelines.md | 7 ++----- injected/docs/testing-guide.md | 15 ++++----------- 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index d07d63d819..8daa05d372 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -5,7 +5,7 @@ globs: injected/** # Injected Features -JavaScript features injected into web pages for privacy protections. Features extend `ConfigFeature` and integrate with remote configuration for per-site enable/disable. +JavaScript features injected into web pages for privacy protections. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. **See also:** `docs/coding-guidelines.md` for code style and best practices. diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index f640861658..7297ec9be5 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -44,7 +44,7 @@ class MyFeature extends ContentFeature { ### Error Handling and Debugging - Avoid hardcoding debug flags; ensure they are configurable and environment-dependent -- Remove `console.log` statements from production code +- Remove `console.log` statements from production code and prefer `this.log.info` instead as this will be disabled in release. ## Architecture & Design @@ -106,7 +106,7 @@ if ((isInputElement(element) && ['text', 'hidden'].includes(element.type)) || is - Handle permissions with custom behaviors or name overrides correctly ### API Usage in Iframes -- Be cautious enabling APIs like Web Share within iframes—may cause security issues or runtime errors +- Be cautious enabling APIs like Web Share within iframes, understand that you're exposing message overhead and potential side effects to a third party. ## Performance @@ -196,6 +196,3 @@ overlays.opensShort(url); // Incorrect - includes non-test files 'integration-test/**' ``` - -### Unit Testing Security-sensitive Code -- Extract security-sensitive logic to separate files for focused unit testing diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index 791b27ad12..6643aa3b84 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -53,9 +53,9 @@ npm run build ## Test Builds for Ship Review -Test builds are created with a GitHub workflow. The assets for Content Scope Scripts will be created on demand if they are absent (which they will be, if you're pointing to a branch of CSS). +Test builds are created with a GitHub workflow. The assets for Content Scope Scripts will be created on demand if they are absent (which they will be, if you're pointing to a branch of C-S-S). -1. Commit any changes to CSS and push a branch to the remote +1. Commit any changes to C-S-S and push a branch to the remote 2. Make sure you commit the submodule reference update in the Windows PR 3. Continue with "Build an installer for ship review / test" @@ -65,7 +65,7 @@ Test builds are created with a GitHub workflow. The assets for Content Scope Scr If you drop a `debugger;` line in the scripts and open DevTools window, the DevTools will breakpoint and navigate to that exact line in code when the debug point has been hit. -### Verifying CSS is Loaded +### Verifying C-S-S is Loaded Open DevTools, go to the Console tab and enter `navigator.duckduckgo`. If it's defined, then Content Scope Scripts is running. @@ -109,14 +109,7 @@ test('example test', async ({ page }) => { ``` ### Unit Testing Security-sensitive Code -- Extract security-sensitive logic, such as markdown-to-HTML conversion, to a separate file. This allows for focused unit testing to prevent security vulnerabilities like XSS: - -```javascript -// Extract to markdownToHtml.js -export function convertMarkdownToHTML(markdown) { - // Conversion logic here -} -``` +- Extract security-sensitive logic, such as content conversion, to a separate file. This allows for focused unit testing to prevent security vulnerabilities like XSS. ### Mocking and Test Data - Include comprehensive scenarios in mocks to accurately test behavior, especially for edge cases. This ensures tests cover a wide range of possible states: From b5d33c95c96aad55d90cf55702c58c0c5c10c079 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 17:48:20 +0000 Subject: [PATCH 03/26] Refactor docs and feature examples for clarity and best practices Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 7 ++- injected/docs/coding-guidelines.md | 80 ++++++++++++----------------- injected/docs/testing-guide.md | 79 +++++++++++++--------------- 3 files changed, 75 insertions(+), 91 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 8daa05d372..786182310d 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -21,7 +21,7 @@ unit-test/ # Jasmine unit tests ## Feature Pattern -Features extend `ContentFeature` and implement lifecycle methods: +Features extend `ContentFeature` (which itself extends `ConfigFeature`). Use `ContentFeature` for features that need messaging, logging, and DOM interaction. Implement lifecycle methods: ```javascript import ContentFeature from '../content-feature.js'; @@ -41,6 +41,11 @@ export default class MyFeature extends ContentFeature { update(data) { // Receive updates from browser } + + destroy() { + // Cleanup: remove event listeners, observers, etc. + this.sideEffects.destroy(); + } } ``` diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 7297ec9be5..6088f2acb2 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -4,15 +4,12 @@ ## Code Style -### Naming Conventions -- Use descriptive names for variables, functions, and classes -- Follow existing naming patterns in the codebase - ### Event Listener Management Use stored references or the class-based `handleEvent` pattern to ensure proper removal: #### Stored reference pattern: + ```js this.scrollListener = () => {...}; document.addEventListener('scroll', this.scrollListener); @@ -20,6 +17,7 @@ document.removeEventListener('scroll', this.scrollListener); ``` #### Class-based handleEvent pattern: + ```js class MyFeature extends ContentFeature { init() { @@ -39,20 +37,24 @@ class MyFeature extends ContentFeature { **Avoid** using `.bind(this)` directly in addEventListener—it creates a new reference each time, preventing removal. ### Module Structure + - Extract reusable logic into separate files for focused unit testing - Security-sensitive operations (e.g., markdown to HTML conversion) should be isolated with extensive tests ### Error Handling and Debugging + - Avoid hardcoding debug flags; ensure they are configurable and environment-dependent - Remove `console.log` statements from production code and prefer `this.log.info` instead as this will be disabled in release. ## Architecture & Design ### Action Execution Flow + - Execute secondary actions from the top level to avoid context handling issues - Avoid re-calling execution functions within an action ### Navigation Event Handling + - Use `navigatesuccess` event for URL change detection (ensures navigation is committed): ```js @@ -60,6 +62,7 @@ globalThis.navigation.addEventListener('navigatesuccess', handleURLChange); ``` ### Re-entrancy Pattern + - Check `document.readyState` to avoid missing DOM elements: ```js @@ -70,47 +73,43 @@ if (document.readyState === 'loading') { } ``` -### Documentation Updates -- Update documentation to reflect critical changes -- Document workarounds for bugs clearly for future maintenance - -### Regression Handling -- Address regressions promptly, especially those affecting critical functionality -- Ensure changes do not introduce unintended side effects - ## Security & Privacy ### XSS Vulnerabilities + - Isolate and unit test any logic that manipulates HTML or sets content dynamically -### Error Handling and Edge Cases +### Element Validation + - Validate elements and their types before operations: ```javascript if (!element) { - return PirError.create(`could not find element`); + return; // or throw appropriate error } -if ((isInputElement(element) && ['text', 'hidden'].includes(element.type)) || isTextAreaElement(element)) { - element.value = token; -} else { - return PirError.create(`element is neither a text input nor textarea`); +if (element instanceof HTMLInputElement || element instanceof HTMLTextAreaElement) { + element.value = value; } ``` ### Caching and State Management + - Avoid global or static caching mechanisms that could lead to race conditions - Use instance-scoped storage or include all relevant identifiers in cache keys ### Permissions Handling + - Ensure custom permission handling does not bypass native permission models - Handle permissions with custom behaviors or name overrides correctly ### API Usage in Iframes + - Be cautious enabling APIs like Web Share within iframes, understand that you're exposing message overhead and potential side effects to a third party. ## Performance ### Memory Management + - Use `WeakSet`/`WeakMap` for DOM element references to allow garbage collection - Delete entries from collections after use: @@ -119,19 +118,23 @@ navigations.delete(event.target); ``` ### Message Bridge Caching + - Avoid global or static caches for message bridges - Include all relevant parameters (`featureName`, `messageSecret`) in cache keys ### Console Log Statements + - Remove `console.log` statements from production code ## Error Handling ### Error Messages + - Ensure error messages are accurate and specific - Replace generic 'unknown error' with context-specific messages ### Async/Await Usage + - Use `await` to ensure errors are caught and flow is maintained: ```js @@ -139,6 +142,7 @@ await someAsyncFunction(); ``` ### Error Handling in Promises + - Ensure promises have both resolve and reject paths: ```js @@ -152,6 +156,7 @@ new Promise((resolve, reject) => { ``` ### Null Checks + - Perform null checks before using objects: ```js @@ -160,39 +165,20 @@ if (object != null) { } ``` -### Captcha Error Handling -- Bubble up errors to avoid silent failures: - -```js -if (PirError.isError(captchaProvider)) { - return createError(captchaProvider.error.message); -} -``` - -## Testing +### Broker Protection Error Handling -### Test Independence in Playwright -- Ensure tests are independent for safe parallel execution -- Use Playwright's test fixtures for complex setup +> **Note:** These patterns are specific to the `broker-protection` feature. -### Async/Await in Tests -- Always use `await` with asynchronous operations: +- Use `PirError` for typed errors in broker-protection: -```javascript -// Correct -await overlays.opensShort(url); +```js +import { PirError } from './broker-protection/types.js'; -// Incorrect -overlays.opensShort(url); +if (PirError.isError(result)) { + return createError(result.error.message); +} ``` -### Specificity in Test File Selection -- Use specific glob patterns to include only test files: - -```javascript -// Correct -'integration-test/**/*.spec.js' +## Testing -// Incorrect - includes non-test files -'integration-test/**' -``` +See [Testing Guide](./testing-guide.md) for comprehensive testing documentation. diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index 6643aa3b84..89a7ea4075 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -71,69 +71,62 @@ Open DevTools, go to the Console tab and enter `navigator.duckduckgo`. If it's d ## Testing Best Practices -### Test Independence in Playwright +### Test Independence -- Ensure Playwright tests are independent to allow safe parallel execution. Avoid shared setup outside of each test block -- Utilize Playwright's test fixtures for complex setup requirements - -### Async/Await in Tests -- Always use `await` with asynchronous operations to ensure the test waits for the operation to complete. This is crucial for accurately testing operations that may throw errors: +Playwright tests must be independent for safe parallel execution: ```javascript -// Correct -await overlays.opensShort(url) +// ✅ Each test is self-contained +test('overlay displays correctly', async ({ page }) => { + const overlays = OverlaysPage.create(page); + await overlays.openPage(url); + await expect(overlays.element).toBeVisible(); +}); -// Incorrect -overlays.opensShort(url) +// ❌ Avoid shared state between tests +let sharedPage; // Don't do this ``` -### Test Case Isolation -- Refactor tests to ensure independence and allow for safe parallel execution. Avoid shared setup that prevents running tests in parallel: +Use Playwright's [test fixtures](https://playwright.dev/docs/test-fixtures) for complex setup. -```javascript -// Refactor shared setup into individual test blocks for isolation -test('example test', async ({ page }) => { - // Setup code here -}); -``` +### Async/Await -### Specificity in Test File Selection -- Use specific glob patterns to include only test files, avoiding the inclusion of non-test files which could cause unexpected behavior or test failures: +Always `await` async operations—missing `await` causes flaky tests: ```javascript -// Correct -'integration-test/broker-protection-tests/**/*.spec.js' +// ✅ Correct +await overlays.opensShort(url); -// Incorrect -'integration-test/broker-protection-tests/**' +// ❌ Test passes incorrectly (promise not awaited) +overlays.opensShort(url); ``` -### Unit Testing Security-sensitive Code -- Extract security-sensitive logic, such as content conversion, to a separate file. This allows for focused unit testing to prevent security vulnerabilities like XSS. +### Glob Patterns -### Mocking and Test Data -- Include comprehensive scenarios in mocks to accurately test behavior, especially for edge cases. This ensures tests cover a wide range of possible states: +Use specific patterns to avoid including non-test files: ```javascript -// Adding missing scenario in mocks -case 'new_scenario': { - // Mock logic here - break; -} +// ✅ Correct - only .spec.js files +'integration-test/**/*.spec.js'; + +// ❌ Incorrect - includes config, fixtures, etc. +'integration-test/**'; ``` -### Correct Test Setup for Feature Flags -- Ensure test setup accurately reflects the intended test conditions, especially when testing with feature flags or configurations that enable or disable features: +### Feature Flag Testing + +Match test setup to the feature state being tested: ```javascript -// Correct setup for disabled feature -await setupTestWithFeatureDisabled(page); +// Testing disabled state +test('feature disabled shows fallback', async ({ page }) => { + await page.addInitScript(() => { + window.__ddg_config__ = { features: { myFeature: { state: 'disabled' } } }; + }); + // ... +}); ``` -### Handling Undefined in Test Expectations -- Account for `undefined` values in test expectations, especially when testing conditions that may not set a variable or return a value: +### Security-Sensitive Code -```javascript -// Correct expectation handling for undefined values -expect(window.someTestVariable).toBeUndefined(); -``` +Extract DOM manipulation and HTML generation to separate files for focused unit testing. This prevents XSS vulnerabilities from slipping through integration-only testing. From 85686c0a2452cd632cbd36001df7603b489f2fa8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:22:01 +0000 Subject: [PATCH 04/26] Refactor: Update coding guidelines and error handling Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 56 ++++++++++++++++++- injected/docs/coding-guidelines.md | 14 ----- .../broker-protection/broker-protection.md | 30 ++++++++++ 3 files changed, 84 insertions(+), 16 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 786182310d..ebca12551c 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -58,10 +58,27 @@ Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: if (this.getFeatureSettingEnabled('settingName')) { ... } if (this.getFeatureSettingEnabled('settingName', 'disabled')) { ... } // default disabled -// Get setting value +// Get setting value (returns typed object from privacy-configuration schema) const settings = this.getFeatureSetting('settingName'); ``` +Config structure example (from privacy-configuration): +```json +{ + "features": { + "webCompat": { + "state": "enabled", + "settings": { + "permissions": { "supportedPermissions": { "geolocation": { "native": true } } }, + "viewportWidth": { "forcedDesktopValue": "width=1280" } + } + } + } +} +``` + +Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. + ## Messaging Use inherited messaging methods: @@ -102,7 +119,42 @@ Integration tests use test pages in `integration-test/test-pages/`. - **Event listeners**: Use `handleEvent` pattern or stored references (avoid `.bind(this)`) - **API existence**: Check `typeof API === 'function'` before wrapping - **DOM timing**: Check `document.readyState` before accessing DOM -- **Error types**: Use appropriate types (`DOMException`, `TypeError`) in shims +- **Error types**: Match native error signatures in API shims (see below) + +## API Shims & Error Types + +When shimming browser APIs, use the correct error types to match native behavior: + +```javascript +// TypeError for invalid arguments +throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); + +// DOMException with name for API-specific errors +throw new DOMException('Share already in progress', 'InvalidStateError'); +throw new DOMException('Permission denied', 'NotAllowedError'); +return Promise.reject(new DOMException('No device selected.', 'NotFoundError')); +``` + +Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`, `DataError`, `SecurityError`. + +## Platform Considerations + +Features are bundled per-platform in `entry-points/`: +- `apple.js` - iOS/macOS +- `android.js` - Android +- `windows.js` - Windows +- `extension-mv3.js` - Browser extension + +Platform-specific behavior: +```javascript +// Check platform in feature code +if (this.platform.name === 'ios' || this.platform.name === 'android') { + // Mobile-specific logic +} + +// Some features are platform-specific (see utils.js platformSpecificFeatures) +const platformSpecificFeatures = ['navigatorInterface', 'windowsPermissionUsage', 'messageBridge', 'favicon']; +``` ## Notes diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 6088f2acb2..e78c43659e 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -165,20 +165,6 @@ if (object != null) { } ``` -### Broker Protection Error Handling - -> **Note:** These patterns are specific to the `broker-protection` feature. - -- Use `PirError` for typed errors in broker-protection: - -```js -import { PirError } from './broker-protection/types.js'; - -if (PirError.isError(result)) { - return createError(result.error.message); -} -``` - ## Testing See [Testing Guide](./testing-guide.md) for comprehensive testing documentation. diff --git a/injected/src/features/broker-protection/broker-protection.md b/injected/src/features/broker-protection/broker-protection.md index 9f92289046..4d53bed73e 100644 --- a/injected/src/features/broker-protection/broker-protection.md +++ b/injected/src/features/broker-protection/broker-protection.md @@ -19,3 +19,33 @@ sequenceDiagram Note over N: Some time passes... N->>W: 📩 onActionReceived(action, data) ``` + +## Error Handling + +Use `PirError` for typed errors throughout the broker-protection feature: + +```js +import { PirError } from './types.js'; + +// Creating errors +if (!element) { + return PirError.create('could not find element'); +} + +// Checking for errors +if (PirError.isError(result)) { + return createError(result.error.message); +} + +// Element validation pattern +if ((isInputElement(element) && ['text', 'hidden'].includes(element.type)) || isTextAreaElement(element)) { + element.value = token; +} else { + return PirError.create('element is neither a text input nor textarea'); +} +``` + +**Key principles:** +- Always bubble up errors to avoid silent failures +- Use specific error messages that identify the failing operation +- Check `PirError.isError()` before accessing result values From fea2bfd49a971b1453f171e5ec2d09bdf397a18f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:23:51 +0000 Subject: [PATCH 05/26] Docs: Add link to privacy-configuration experiments guide Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index ebca12551c..724c7b9014 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -7,7 +7,9 @@ globs: injected/** JavaScript features injected into web pages for privacy protections. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. -**See also:** `docs/coding-guidelines.md` for code style and best practices. +**See also:** +- `docs/coding-guidelines.md` for code style and best practices +- [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc) for A/B testing features ## Structure @@ -79,6 +81,8 @@ Config structure example (from privacy-configuration): Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. +For A/B testing features, use `contentScopeExperiments` with `conditionalChanges` in the target feature. See [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). + ## Messaging Use inherited messaging methods: From a5db180af2769a0c2eccb9fc9d6973397638430a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:27:06 +0000 Subject: [PATCH 06/26] Refactor conditional changes documentation Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 47 ++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 724c7b9014..f2c54b8ac9 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -81,7 +81,52 @@ Config structure example (from privacy-configuration): Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. -For A/B testing features, use `contentScopeExperiments` with `conditionalChanges` in the target feature. See [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). +### Conditional Changes + +Use `conditionalChanges` to apply JSON Patch operations based on runtime conditions. Conditions are evaluated in [`src/config-feature.js`](src/config-feature.js) (see `ConditionBlock` typedef and `_matchConditionalBlock`). + +**Supported conditions:** + +| Condition | Description | Example | +|-----------|-------------|---------| +| `domain` | Match hostname | `"domain": "example.com"` | +| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | +| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | +| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | +| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | +| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | +| `injectName` | Match inject context | `"injectName": "apple-isolated"` | +| `internal` | Internal builds only | `"internal": true` | +| `preview` | Preview builds only | `"preview": true` | + +**Config example:** +```json +{ + "settings": { + "conditionalChanges": [ + { + "condition": { "domain": "example.com" }, + "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] + }, + { + "condition": [ + { "urlPattern": "https://site1.com/*" }, + { "urlPattern": "https://site2.com/path/*" } + ], + "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] + } + ] + } +} +``` + +**Key rules:** +- All conditions in a block must match (AND logic) +- Array of condition blocks uses OR logic (any block matching applies the patch) +- Use JSON Pointer paths (`/setting/nested`) in `patchSettings` +- Unsupported conditions cause the block to fail (for backwards compatibility) + +For A/B testing, see [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). ## Messaging From 0036c9177c949049543418b98aa597cadb968f72 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:46:58 +0000 Subject: [PATCH 07/26] Add SideEffects helper and update documentation Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 34 +++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index f2c54b8ac9..4a1d76e288 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -15,12 +15,14 @@ JavaScript features injected into web pages for privacy protections. Features ex ``` src/features/.js # Feature implementation (extends ContentFeature) -src/features.js # Feature registry +src/features.js # Feature registry (baseFeatures + otherFeatures arrays) entry-points/.js # Platform-specific bundles integration-test/ # Playwright integration tests unit-test/ # Jasmine unit tests ``` +**Adding a new feature:** Add to `baseFeatures` or `otherFeatures` in `src/features.js`, then add to relevant platform arrays in `platformSupport`. + ## Feature Pattern Features extend `ContentFeature` (which itself extends `ConfigFeature`). Use `ContentFeature` for features that need messaging, logging, and DOM interaction. Implement lifecycle methods: @@ -51,6 +53,32 @@ export default class MyFeature extends ContentFeature { } ``` +### SideEffects Helper + +Use `SideEffects` from `src/features/duckplayer/util.js` to track and cleanup side effects: + +```javascript +import { SideEffects } from './duckplayer/util.js'; + +class MyFeature extends ContentFeature { + sideEffects = new SideEffects(); + + init() { + // Wrap side effects with descriptive names for debugging + this.sideEffects.add('observing DOM mutations', () => { + const observer = new MutationObserver(this.handleMutation); + observer.observe(document.body, { childList: true }); + return () => observer.disconnect(); // Return cleanup function + }); + } + + destroy() { + this.sideEffects.destroy(); // Calls all cleanup functions + // Or destroy specific effect: this.sideEffects.destroy('observing DOM mutations'); + } +} +``` + ## Remote Configuration Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: @@ -79,6 +107,8 @@ Config structure example (from privacy-configuration): } ``` +**Feature state values:** `"enabled"` or `"disabled"`. Features default to disabled unless explicitly enabled. + Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. ### Conditional Changes @@ -123,7 +153,7 @@ Use `conditionalChanges` to apply JSON Patch operations based on runtime conditi **Key rules:** - All conditions in a block must match (AND logic) - Array of condition blocks uses OR logic (any block matching applies the patch) -- Use JSON Pointer paths (`/setting/nested`) in `patchSettings` +- `patchSettings` uses [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) paths (`/setting/nested`) - Unsupported conditions cause the block to fail (for backwards compatibility) For A/B testing, see [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). From ecd6d76e601c74b6a2c5b8574f46a11d83b435bc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:49:34 +0000 Subject: [PATCH 08/26] Refactor: Remove unused SideEffects helper from MyFeature Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 31 ----------------------------- 1 file changed, 31 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 4a1d76e288..032a6164eb 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -45,37 +45,6 @@ export default class MyFeature extends ContentFeature { update(data) { // Receive updates from browser } - - destroy() { - // Cleanup: remove event listeners, observers, etc. - this.sideEffects.destroy(); - } -} -``` - -### SideEffects Helper - -Use `SideEffects` from `src/features/duckplayer/util.js` to track and cleanup side effects: - -```javascript -import { SideEffects } from './duckplayer/util.js'; - -class MyFeature extends ContentFeature { - sideEffects = new SideEffects(); - - init() { - // Wrap side effects with descriptive names for debugging - this.sideEffects.add('observing DOM mutations', () => { - const observer = new MutationObserver(this.handleMutation); - observer.observe(document.body, { childList: true }); - return () => observer.disconnect(); // Return cleanup function - }); - } - - destroy() { - this.sideEffects.destroy(); // Calls all cleanup functions - // Or destroy specific effect: this.sideEffects.destroy('observing DOM mutations'); - } } ``` From 6734087704651aea71261bf013a01579cba4f491 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:53:23 +0000 Subject: [PATCH 09/26] Refactor: Remove AI and XSS sections from coding guidelines Co-authored-by: jkingston --- special-pages/docs/coding-guidelines.md | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/special-pages/docs/coding-guidelines.md b/special-pages/docs/coding-guidelines.md index 1d7208345e..09454f7e05 100644 --- a/special-pages/docs/coding-guidelines.md +++ b/special-pages/docs/coding-guidelines.md @@ -69,10 +69,6 @@ const message = t('key_name'); - Ensure props (especially `style`) are handled correctly - Do not couple functionality to UI visibility—control functionality independently from whether UI elements are shown -### AI Functionality and UI Coupling -- Decouple AI functionality from its setting UI visibility -- Ensure functionality is controlled independently from UI elements - ### Feature State Management - Maintain parameter order in method signatures to prevent breaking changes - Avoid overriding falsy values incorrectly @@ -82,18 +78,6 @@ const message = t('key_name'); ## Security & Privacy -### XSS Vulnerabilities -- Isolate and unit test any logic that manipulates HTML or sets content dynamically -- Markdown to HTML conversion should be in a separate file with extensive testing: - -```javascript -// In a separate utility file with unit tests -export function convertMarkdownToHTML(markdown) { - const regex = /\*\*(.*?)\*\*/g; - return markdown.replace(regex, '$1'); -} -``` - ### Controlled Component State - Avoid directly manipulating properties like `value` on controlled components - Let Preact manage input state through props and event handlers From 1d61561ed347228307eef5808008f12e32549484 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 18:58:36 +0000 Subject: [PATCH 10/26] Add C-S-S usage guidelines and decision checklist Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 032a6164eb..70a4d5a8c8 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -11,6 +11,39 @@ JavaScript features injected into web pages for privacy protections. Features ex - `docs/coding-guidelines.md` for code style and best practices - [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc) for A/B testing features +## When to Use C-S-S + +**C-S-S injected scripts** — Best for: +- In-page changes or DOM manipulations +- Anything modifying a DOM API or manipulating page content +- Code that benefits from config, breakage tooling, and DOM wrappers +- Testing/linting/bundling makes code more robust than native-only + +**Native code** — Best for: +- Listening to bridge/handler messages +- When WebView APIs do the job better +- Avoid injecting scripts when native can handle it—harder to debug, no tooling + +**SERP/Static pages** — Can expose/call bridge messages, cleaner than scraping page content. + +### Decision Checklist + +| Question | Recommendation | +|----------|----------------| +| Modifying DOM API or manipulating page? | **Must** be in C-S-S | +| Need config handling, testing? | Prefer C-S-S (free tooling) | +| Code relies on page structure? | Use config to alter behavior; avoid if possible | +| Internal page involved? | Consider having the page expose an API/fire a message | +| Writing conditionals for iframe/domain? | Use `conditionalChanges` (see below) | +| Exposing new APIs? | Check if `apiManipulation` feature handles it | +| Need isolated context? | Prefer isolated entry point if possible | + +### Message Handlers vs Bridge + +- **Message handlers**: Limit to pages that need them; use script evaluation over messaging unless payload size is problematic +- **Bridge**: Config-enabled messages that pass through C-S-S; use subscriptions for exposing custom behavior to native +- If our site is involved, push messages to native rather than scraping + ## Structure ``` From 513e962796c0f4e292b82687de6e365bde94ae76 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 19:19:40 +0000 Subject: [PATCH 11/26] Refactor: Remove outdated coding guidelines Co-authored-by: jkingston --- injected/docs/coding-guidelines.md | 13 ------------- special-pages/docs/coding-guidelines.md | 14 -------------- 2 files changed, 27 deletions(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index e78c43659e..bef9aab57b 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -75,10 +75,6 @@ if (document.readyState === 'loading') { ## Security & Privacy -### XSS Vulnerabilities - -- Isolate and unit test any logic that manipulates HTML or sets content dynamically - ### Element Validation - Validate elements and their types before operations: @@ -122,17 +118,8 @@ navigations.delete(event.target); - Avoid global or static caches for message bridges - Include all relevant parameters (`featureName`, `messageSecret`) in cache keys -### Console Log Statements - -- Remove `console.log` statements from production code - ## Error Handling -### Error Messages - -- Ensure error messages are accurate and specific -- Replace generic 'unknown error' with context-specific messages - ### Async/Await Usage - Use `await` to ensure errors are caught and flow is maintained: diff --git a/special-pages/docs/coding-guidelines.md b/special-pages/docs/coding-guidelines.md index 09454f7e05..f3f8bdc889 100644 --- a/special-pages/docs/coding-guidelines.md +++ b/special-pages/docs/coding-guidelines.md @@ -4,10 +4,6 @@ ## Code Style -### Naming Conventions -- Use descriptive names for variables, functions, and classes -- Follow existing naming patterns in the codebase - ### Component and Hook Usage #### Preact Attributes @@ -73,9 +69,6 @@ const message = t('key_name'); - Maintain parameter order in method signatures to prevent breaking changes - Avoid overriding falsy values incorrectly -### Documentation Updates -- Update documentation to reflect critical changes, such as method parameter reordering - ## Security & Privacy ### Controlled Component State @@ -87,15 +80,8 @@ const message = t('key_name'); ### Memory Management - Clean up subscriptions and event listeners in component unmount -### Console Log Statements -- Remove `console.log` statements from production code - ## Error Handling -### Error Messages -- Ensure error messages are accurate and specific -- Replace generic 'unknown error' with specific error messages - ### Async/Await Usage - Use `await` with asynchronous operations to ensure errors are caught: From b630592dc5e3c2edcce81e8808d3896610a071e5 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 19:27:55 +0000 Subject: [PATCH 12/26] Refactor: Clarify scope of injected features Co-authored-by: jkingston --- injected/.cursor/rules/injected.mdc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index 70a4d5a8c8..ec56562ade 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -1,11 +1,11 @@ --- -description: Guidelines for injected privacy features development +description: Guidelines for features running in web page DOM environments globs: injected/** --- # Injected Features -JavaScript features injected into web pages for privacy protections. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. +JavaScript features injected into web pages or running in DOM environments of DuckDuckGo browsers. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. **See also:** - `docs/coding-guidelines.md` for code style and best practices From 76266341a3ea82a9f1d624e5f37fab8e9dd41ad2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 19:30:41 +0000 Subject: [PATCH 13/26] Refactor: Clarify README for injected features Co-authored-by: jkingston --- injected/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/injected/README.md b/injected/README.md index f99f0245fb..e157237ec8 100644 --- a/injected/README.md +++ b/injected/README.md @@ -4,7 +4,7 @@ Content Scope Scripts handles injecting DOM modifications in a browser context; ## Quick Start -Content Scope Scripts provides a unified API for browser privacy features across multiple platforms (Firefox, Chrome, Safari, Android, iOS). Features are loaded dynamically based on remote configuration and can be enabled/disabled per site. +Content Scope Scripts provides a unified API for features running in web page DOM environments across multiple platforms (Firefox, Chrome, Safari, Android, iOS). Features are loaded dynamically based on remote configuration and can be enabled/disabled per site. ## Documentation @@ -23,12 +23,12 @@ Content Scope Scripts provides a unified API for browser privacy features across Content Scope Scripts contains two main sub-projects: - **[Special Pages](../special-pages/)** - HTML/CSS/JS applications loaded into browsers (DuckPlayer, Release Notes, New Tab page, etc.) -- **Injected Features** - Features injected into websites (privacy protections, compatibility fixes) +- **Injected Features** - Features injected into websites (privacy protections, compatibility fixes, DOM manipulations) > **For Special Pages development**, see the [Special Pages README](../special-pages/README.md) for detailed getting started instructions. ### Features -Features are JavaScript modules that implement privacy protections. Each feature: +Features are JavaScript modules running in web page DOM environments. Each feature: - Extends the `ConfigFeature` class for remote configuration support - Implements the feature lifecycle (`load`, `init`, `update`) - Can be enabled/disabled per site via remote configuration From b42f1e56ff480e9e030b48b5651815d03e3b70af Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 19:38:41 +0000 Subject: [PATCH 14/26] Add documentation rules for AI agents Co-authored-by: jkingston --- .cursor/rules/documentation-for-agents.mdc | 125 +++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 .cursor/rules/documentation-for-agents.mdc diff --git a/.cursor/rules/documentation-for-agents.mdc b/.cursor/rules/documentation-for-agents.mdc new file mode 100644 index 0000000000..5a9f01a5d7 --- /dev/null +++ b/.cursor/rules/documentation-for-agents.mdc @@ -0,0 +1,125 @@ +--- +description: Guidelines for writing and reviewing documentation optimized for AI agents +globs: "**/*.md,**/*.mdc,**/.cursorrules" +--- + +# Documentation for AI Agents + +Guidelines for creating and reviewing documentation that AI coding agents can effectively use. + +## Core Principles + +**Actionable over advisory** — Agents need specifics they can apply, not general best practices. + +**Technical over generic** — "Use `WeakMap` for DOM references" beats "manage memory properly." + +**Examples over explanations** — Show the pattern, don't just describe it. + +**Concrete over abstract** — Include actual config schemas, error messages, API signatures. + +## What to Include + +### High-Value Content + +- **Decision trees** — When to use X vs Y, with clear criteria +- **Config/schema examples** — Real JSON structures agents can reference +- **Error signatures** — Exact error types and messages for API shims +- **Platform-specific behavior** — Entry points, conditionals, platform checks +- **Lifecycle methods** — What runs when, with initialization order +- **Registration patterns** — How to add new features/components to the system + +### Links Over Duplication + +- Reference external docs rather than copying content +- Link to source code for canonical definitions (e.g., `ConditionBlock` typedef) +- Point to generated types rather than manually documenting schemas + +## What to Remove + +### Low-Value Content + +- Generic advice: "Use descriptive names", "Write tests", "Update documentation" +- Process guidance: "Address regressions promptly", "Review PRs carefully" +- Obvious patterns: Basic null checks, standard async/await usage +- Duplicated sections across multiple files +- Security warnings without specific mitigations + +### Red Flags + +- Sections that could apply to any codebase +- Advice that doesn't reference project-specific APIs or patterns +- Abstract examples with placeholder code (`// do something here`) +- Warnings without actionable guidance + +## Structure Guidelines + +### Feature-Specific Docs + +Place documentation close to the code it describes: +- Feature-specific patterns → in the feature directory +- Cross-cutting concerns → in shared docs with clear scope + +### Cursor Rules (`.mdc` files) + +Keep concise and scannable: +- Lead with the most important patterns +- Use tables for quick reference (commands, conditions, options) +- Include one complete example showing the full pattern +- Link to detailed docs for deep dives + +### Scope Accuracy + +Be precise about what the docs cover: +- ❌ "privacy features" (too narrow) +- ✅ "features running in web page DOM environments" (accurate scope) + +## Review Checklist + +When reviewing documentation for agent usefulness: + +1. **Would an agent produce correct code from this?** If not, what's missing? +2. **Is there project-specific information?** Generic advice should be removed. +3. **Are there concrete examples?** Abstract descriptions need code samples. +4. **Is it in the right place?** Feature-specific docs belong with the feature. +5. **Are there broken/missing links?** External references must be valid. +6. **Is there duplication?** Consolidate or link instead. + +## Example Transformations + +### Before (Low Value) +```markdown +### Error Handling +- Handle errors appropriately +- Use try/catch blocks +- Log errors for debugging +``` + +### After (High Value) +```markdown +### API Shim Errors +Match native error signatures: +```js +throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); +throw new DOMException('Permission denied', 'NotAllowedError'); +``` +Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`. +``` + +### Before (Too Generic) +```markdown +## Architecture +- Keep code modular +- Separate concerns appropriately +``` + +### After (Decision-Oriented) +```markdown +## When to Use C-S-S vs Native + +| Scenario | Recommendation | +|----------|----------------| +| DOM API modification | **Must** be in C-S-S | +| WebView API available | Prefer Native | +| Needs config/breakage tooling | Prefer C-S-S | +| Page structure dependent | Use config to alter; avoid if possible | +``` From b626de793d3dd3581323f1f735efc03c6d5d8476 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 7 Dec 2025 19:42:22 +0000 Subject: [PATCH 15/26] Remove documentation for AI agents Co-authored-by: jkingston --- .cursor/rules/documentation-for-agents.mdc | 125 --------------------- 1 file changed, 125 deletions(-) delete mode 100644 .cursor/rules/documentation-for-agents.mdc diff --git a/.cursor/rules/documentation-for-agents.mdc b/.cursor/rules/documentation-for-agents.mdc deleted file mode 100644 index 5a9f01a5d7..0000000000 --- a/.cursor/rules/documentation-for-agents.mdc +++ /dev/null @@ -1,125 +0,0 @@ ---- -description: Guidelines for writing and reviewing documentation optimized for AI agents -globs: "**/*.md,**/*.mdc,**/.cursorrules" ---- - -# Documentation for AI Agents - -Guidelines for creating and reviewing documentation that AI coding agents can effectively use. - -## Core Principles - -**Actionable over advisory** — Agents need specifics they can apply, not general best practices. - -**Technical over generic** — "Use `WeakMap` for DOM references" beats "manage memory properly." - -**Examples over explanations** — Show the pattern, don't just describe it. - -**Concrete over abstract** — Include actual config schemas, error messages, API signatures. - -## What to Include - -### High-Value Content - -- **Decision trees** — When to use X vs Y, with clear criteria -- **Config/schema examples** — Real JSON structures agents can reference -- **Error signatures** — Exact error types and messages for API shims -- **Platform-specific behavior** — Entry points, conditionals, platform checks -- **Lifecycle methods** — What runs when, with initialization order -- **Registration patterns** — How to add new features/components to the system - -### Links Over Duplication - -- Reference external docs rather than copying content -- Link to source code for canonical definitions (e.g., `ConditionBlock` typedef) -- Point to generated types rather than manually documenting schemas - -## What to Remove - -### Low-Value Content - -- Generic advice: "Use descriptive names", "Write tests", "Update documentation" -- Process guidance: "Address regressions promptly", "Review PRs carefully" -- Obvious patterns: Basic null checks, standard async/await usage -- Duplicated sections across multiple files -- Security warnings without specific mitigations - -### Red Flags - -- Sections that could apply to any codebase -- Advice that doesn't reference project-specific APIs or patterns -- Abstract examples with placeholder code (`// do something here`) -- Warnings without actionable guidance - -## Structure Guidelines - -### Feature-Specific Docs - -Place documentation close to the code it describes: -- Feature-specific patterns → in the feature directory -- Cross-cutting concerns → in shared docs with clear scope - -### Cursor Rules (`.mdc` files) - -Keep concise and scannable: -- Lead with the most important patterns -- Use tables for quick reference (commands, conditions, options) -- Include one complete example showing the full pattern -- Link to detailed docs for deep dives - -### Scope Accuracy - -Be precise about what the docs cover: -- ❌ "privacy features" (too narrow) -- ✅ "features running in web page DOM environments" (accurate scope) - -## Review Checklist - -When reviewing documentation for agent usefulness: - -1. **Would an agent produce correct code from this?** If not, what's missing? -2. **Is there project-specific information?** Generic advice should be removed. -3. **Are there concrete examples?** Abstract descriptions need code samples. -4. **Is it in the right place?** Feature-specific docs belong with the feature. -5. **Are there broken/missing links?** External references must be valid. -6. **Is there duplication?** Consolidate or link instead. - -## Example Transformations - -### Before (Low Value) -```markdown -### Error Handling -- Handle errors appropriately -- Use try/catch blocks -- Log errors for debugging -``` - -### After (High Value) -```markdown -### API Shim Errors -Match native error signatures: -```js -throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); -throw new DOMException('Permission denied', 'NotAllowedError'); -``` -Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`. -``` - -### Before (Too Generic) -```markdown -## Architecture -- Keep code modular -- Separate concerns appropriately -``` - -### After (Decision-Oriented) -```markdown -## When to Use C-S-S vs Native - -| Scenario | Recommendation | -|----------|----------------| -| DOM API modification | **Must** be in C-S-S | -| WebView API available | Prefer Native | -| Needs config/breakage tooling | Prefer C-S-S | -| Page structure dependent | Use config to alter; avoid if possible | -``` From 7e1655e99b1cbf84ba4164d3fc0789d2d22fe610 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 10:37:21 +0000 Subject: [PATCH 16/26] More automated comments --- injected/docs/coding-guidelines.md | 203 ++++++++++++++++++++++++++++- injected/docs/testing-guide.md | 57 ++++++++ 2 files changed, 256 insertions(+), 4 deletions(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index bef9aab57b..9d77ceb08e 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -4,6 +4,10 @@ ## Code Style +### Constants + +Avoid constants in the code and prefer using `this.getFeatureSetting('constantName') ?? defaultValue` to allow for remote configuration to modify the value. + ### Event Listener Management Use stored references or the class-based `handleEvent` pattern to ensure proper removal: @@ -61,6 +65,27 @@ class MyFeature extends ContentFeature { globalThis.navigation.addEventListener('navigatesuccess', handleURLChange); ``` +### URL Change Lifecycle + +Enable URL tracking for features that need to respond to SPA navigation: + +```js +export default class MyFeature extends ContentFeature { + listenForUrlChanges = true; // Enable URL change tracking + + init() { + this.applyFeature(); + } + + urlChanged(navigationType) { // Called automatically on URL changes + this.recomputeSiteObject(); // Update config for new path + this.applyFeature(); + } +} +``` + +Navigation types: `'push'`, `'replace'`, `'traverse'`, `'reload'`. + ### Re-entrancy Pattern - Check `document.readyState` to avoid missing DOM elements: @@ -73,13 +98,59 @@ if (document.readyState === 'loading') { } ``` +### DDGProxy Pattern + +Use `DDGProxy` for wrapping browser APIs safely. It automatically adds debug flags, checks exemptions, and preserves `toString()` behavior: + +```js +import { DDGProxy, DDGReflect } from '../utils'; + +// Wrap a method +const proxy = new DDGProxy(this, Navigator.prototype, 'getBattery', { + apply(target, thisArg, args) { + return Promise.reject(new DOMException('Not allowed', 'NotAllowedError')); + }, +}); +proxy.overload(); + +// Wrap a property getter +const propProxy = new DDGProxy(this, Screen.prototype, 'width', { + get(target, prop, receiver) { + return 1920; + }, +}); +propProxy.overloadProperty(); +``` + +### Retry Utilities + +Use built-in retry utilities for operations that may need multiple attempts: + +```js +import { retry, withRetry } from '../timer-utils'; + +// Simple retry with config (returns { result, exceptions } for debugging) +const { result, exceptions } = await retry( + () => findElement(selector), + { interval: { ms: 1000 }, maxAttempts: 30 } +); + +// Retry with automatic error handling +const element = await withRetry( + () => document.querySelector(selector), + 4, // maxAttempts + 500, // delay ms + 'exponential' // strategy: 'linear' | 'exponential' +); +``` + ## Security & Privacy ### Element Validation - Validate elements and their types before operations: -```javascript +```js if (!element) { return; // or throw appropriate error } @@ -102,17 +173,114 @@ if (element instanceof HTMLInputElement || element instanceof HTMLTextAreaElemen - Be cautious enabling APIs like Web Share within iframes, understand that you're exposing message overhead and potential side effects to a third party. +### Captured Globals Pattern + +Use captured globals to avoid page-tampered native APIs: + +```js +import * as capturedGlobals from '../captured-globals.js'; + +// Use captured versions instead of global +const myMap = new capturedGlobals.Map(); +const mySet = new capturedGlobals.Set(); + +// Dispatch events safely +capturedGlobals.dispatchEvent(new capturedGlobals.CustomEvent('name', { detail })); +``` + +### Frame and Context Guards + +Validate execution context at feature initialization: + +```js +import { isBeingFramed } from '../utils'; + +init(args) { + if (isBeingFramed()) return; // Skip if in a frame + if (!isSecureContext) return; // Skip if not HTTPS + if (!args.messageSecret) return; // Skip if missing required args +} +``` + +### Message Secret Pattern + +For cross-world communication, use message secrets to prevent spoofing: + +```js +function appendToken(eventName) { + return `${eventName}-${args.messageSecret}`; +} + +// Listen with token +captured.addEventListener(appendToken('MessageType'), handler); + +// Dispatch with token +const event = new captured.CustomEvent(appendToken('Response'), { detail: payload }); +captured.dispatchEvent(event); +``` + ## Performance -### Memory Management +### Memory Management with WeakCollections -- Use `WeakSet`/`WeakMap` for DOM element references to allow garbage collection -- Delete entries from collections after use: +Use `WeakSet`/`WeakMap` for DOM element references to allow garbage collection: ```js +const elementCache = new WeakMap(); + +function getOrCompute(element) { + if (elementCache.has(element)) { + return elementCache.get(element); + } + const result = expensiveComputation(element); + elementCache.set(element, result); + return result; +} + +// Delete entries from regular collections after use navigations.delete(event.target); ``` +### Timed Execution Strategy + +For dynamic content, use multiple passes at staggered intervals: + +```js +const hideTimeouts = [0, 100, 300, 500, 1000, 2000, 3000]; +const unhideTimeouts = [1250, 2250, 3000]; + +hideTimeouts.forEach((timeout) => { + setTimeout(() => hideAdNodes(rules), timeout); +}); + +// Clear caches after all operations complete +const clearCacheTimer = Math.max(...hideTimeouts, ...unhideTimeouts) + 100; +setTimeout(() => { + appliedRules = new Set(); + hiddenElements = new WeakMap(); +}, clearCacheTimer); +``` + +Note: Timers are a useful heuristic to save resources but should be remotely configurable and often other techniques such as carefully engineered MutationObservers would be preferred. + +### Batch DOM Operations + +Use semaphores to batch frequent DOM updates: + +```js +let updatePending = false; + +function scheduleUpdate() { + if (!updatePending) { + updatePending = true; + setTimeout(() => { + performDOMUpdate(); + updatePending = false; + }, 10); + } +} +``` + ### Message Bridge Caching - Avoid global or static caches for message bridges @@ -152,6 +320,33 @@ if (object != null) { } ``` +### Structured Error Responses + +Use typed error classes for action-based features: + +```js +import { ErrorResponse } from './broker-protection/types.js'; + +const response = new ErrorResponse({ + actionID: action.id, + message: 'Descriptive error message', +}); +this.messaging.notify('actionError', { error: response }); +``` + +### Silent Feature Initialization Failures + +Catch errors without breaking other features: + +```js +try { + customElements.define('ddg-element', DDGElement); +} catch (e) { + // May fail on extension reload or conflicts + console.error('Custom element definition failed:', e); +} +``` + ## Testing See [Testing Guide](./testing-guide.md) for comprehensive testing documentation. diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index 89a7ea4075..b788b0b822 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -130,3 +130,60 @@ test('feature disabled shows fallback', async ({ page }) => { ### Security-Sensitive Code Extract DOM manipulation and HTML generation to separate files for focused unit testing. This prevents XSS vulnerabilities from slipping through integration-only testing. + +## Integration Test Structure + +### Test Page Directory Layout + +``` +integration-test/ +├── test-pages/ +│ └── / +│ ├── config/ +│ │ └── config.json # Feature config fixtures +│ ├── pages/ +│ │ └── test-page.html # Test HTML pages +│ └── .spec.js # Playwright tests +``` + +### Config Fixture Pattern + +Create config fixtures that match privacy-configuration schema: + +```json +{ + "features": { + "featureName": { + "state": "enabled", + "settings": { + "settingName": "value", + "conditionalChanges": [ + { + "condition": { "domain": "localhost" }, + "patchSettings": [ + { "op": "replace", "path": "/settingName", "value": "testValue" } + ] + } + ] + } + } + } +} +``` + +### Playwright Test Pattern + +```javascript +test('feature behavior', async ({ page }) => { + await page.goto('/test-pages/feature/pages/test.html'); + + // Wait for feature initialization + await page.waitForFunction(() => window.__ddg_feature_ready__); + + // Test feature behavior + const result = await page.evaluate(() => someAPI()); + expect(result).toBe(expectedValue); +}); +``` + +For conditional changes and config schema details, see the [injected cursor rules](../.cursor/rules/injected.mdc#conditional-changes). From 8065bab4f16c86f30d544a7485c5a45bdb47aebf Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 12:56:58 +0000 Subject: [PATCH 17/26] Lint fix --- injected/docs/coding-guidelines.md | 22 ++++++++++---------- injected/docs/testing-guide.md | 32 ++++++++++++++---------------- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 9d77ceb08e..73512578de 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -71,14 +71,15 @@ Enable URL tracking for features that need to respond to SPA navigation: ```js export default class MyFeature extends ContentFeature { - listenForUrlChanges = true; // Enable URL change tracking - + listenForUrlChanges = true; // Enable URL change tracking + init() { this.applyFeature(); } - - urlChanged(navigationType) { // Called automatically on URL changes - this.recomputeSiteObject(); // Update config for new path + + urlChanged(navigationType) { + // Called automatically on URL changes + this.recomputeSiteObject(); // Update config for new path this.applyFeature(); } } @@ -130,17 +131,14 @@ Use built-in retry utilities for operations that may need multiple attempts: import { retry, withRetry } from '../timer-utils'; // Simple retry with config (returns { result, exceptions } for debugging) -const { result, exceptions } = await retry( - () => findElement(selector), - { interval: { ms: 1000 }, maxAttempts: 30 } -); +const { result, exceptions } = await retry(() => findElement(selector), { interval: { ms: 1000 }, maxAttempts: 30 }); // Retry with automatic error handling const element = await withRetry( () => document.querySelector(selector), - 4, // maxAttempts - 500, // delay ms - 'exponential' // strategy: 'linear' | 'exponential' + 4, // maxAttempts + 500, // delay ms + 'exponential', // strategy: 'linear' | 'exponential' ); ``` diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index b788b0b822..1aa4c8f3eb 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -152,22 +152,20 @@ Create config fixtures that match privacy-configuration schema: ```json { - "features": { - "featureName": { - "state": "enabled", - "settings": { - "settingName": "value", - "conditionalChanges": [ - { - "condition": { "domain": "localhost" }, - "patchSettings": [ - { "op": "replace", "path": "/settingName", "value": "testValue" } - ] - } - ] - } + "features": { + "featureName": { + "state": "enabled", + "settings": { + "settingName": "value", + "conditionalChanges": [ + { + "condition": { "domain": "localhost" }, + "patchSettings": [{ "op": "replace", "path": "/settingName", "value": "testValue" }] + } + ] + } + } } - } } ``` @@ -176,10 +174,10 @@ Create config fixtures that match privacy-configuration schema: ```javascript test('feature behavior', async ({ page }) => { await page.goto('/test-pages/feature/pages/test.html'); - + // Wait for feature initialization await page.waitForFunction(() => window.__ddg_feature_ready__); - + // Test feature behavior const result = await page.evaluate(() => someAPI()); expect(result).toBe(expectedValue); From 7b2bdafb6dec2186e977e5a73e5896b6f355b749 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 13:02:18 +0000 Subject: [PATCH 18/26] Move comment to where it applies --- injected/src/features/page-context.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/injected/src/features/page-context.js b/injected/src/features/page-context.js index b1ce8d515e..0f38d678c1 100644 --- a/injected/src/features/page-context.js +++ b/injected/src/features/page-context.js @@ -380,10 +380,10 @@ export default class PageContext extends ContentFeature { this.#delayedRecheckTimer = setTimeout(() => { this.log.info('Performing delayed recheck after navigation'); this.recheckCount++; - // Note: invalidateCache() is safe here because we pass false to prevent - // resetRecheckCount, avoiding unintended recheck loops this.invalidateCache(); + // Note: Pass false to handleContentCollectionRequest to prevent + // resetRecheckCount, avoiding unintended recheck loops this.handleContentCollectionRequest(false); }, delayMs); } From 11ca663492eb1ae92131981283d4833295088517 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 13:06:19 +0000 Subject: [PATCH 19/26] Make getFeatureSettingEnabled default state message more prominent --- injected/docs/coding-guidelines.md | 10 ++++++++++ injected/src/features/page-context.js | 1 - 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 73512578de..39c0e0397c 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -8,6 +8,16 @@ Avoid constants in the code and prefer using `this.getFeatureSetting('constantName') ?? defaultValue` to allow for remote configuration to modify the value. +When using `getFeatureSettingEnabled()`, use its built-in default parameter rather than `|| true`: + +```js +// ✅ Correct - uses second parameter for default +includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled') + +// ❌ Wrong - || true ignores explicit false from config +includeIframes: this.getFeatureSettingEnabled('includeIframes') || true +``` + ### Event Listener Management Use stored references or the class-based `handleEvent` pattern to ensure proper removal: diff --git a/injected/src/features/page-context.js b/injected/src/features/page-context.js index 0f38d678c1..6f9a586b60 100644 --- a/injected/src/features/page-context.js +++ b/injected/src/features/page-context.js @@ -515,7 +515,6 @@ export default class PageContext extends ContentFeature { const result = domToMarkdown(root, { maxLength: upperLimit, maxDepth, - // Use getFeatureSettingEnabled with default, not `|| true` which ignores explicit false includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled'), excludeSelectors: excludeSelectorsString, trimBlankLinks: this.getFeatureSettingEnabled('trimBlankLinks', 'enabled'), From 396deda19b0c1bce1620b4dcb951b5c49f0f298c Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 15:56:44 +0000 Subject: [PATCH 20/26] Update lint fix after dep updates --- injected/docs/coding-guidelines.md | 4 ++-- special-pages/pages/new-tab/app/mock-transport.js | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 39c0e0397c..233f1f0f2e 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -12,10 +12,10 @@ When using `getFeatureSettingEnabled()`, use its built-in default parameter rath ```js // ✅ Correct - uses second parameter for default -includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled') +includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled'); // ❌ Wrong - || true ignores explicit false from config -includeIframes: this.getFeatureSettingEnabled('includeIframes') || true +includeIframes: this.getFeatureSettingEnabled('includeIframes') || true; ``` ### Event Listener Management diff --git a/special-pages/pages/new-tab/app/mock-transport.js b/special-pages/pages/new-tab/app/mock-transport.js index 3c4f88a8d5..bf2b351f5f 100644 --- a/special-pages/pages/new-tab/app/mock-transport.js +++ b/special-pages/pages/new-tab/app/mock-transport.js @@ -465,7 +465,6 @@ export function mockTransport() { return true; }) .map((id) => { - // eslint-disable-next-line object-shorthand return { id: /** @type {any} */ (id) }; }), }; From 3daaa78c6a636327c524275d9e3a6bdbaba9daa0 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 8 Dec 2025 16:03:29 +0000 Subject: [PATCH 21/26] Simplify README and add a CONTRIBUTING --- CONTRIBUTING.md | 112 ++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 91 +-------------------------------------- 2 files changed, 114 insertions(+), 89 deletions(-) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000000..0112663fb6 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,112 @@ +# Contributing + +## Workspace-specific guides + +- [Injected coding guidelines](./injected/docs/coding-guidelines.md) +- [Injected testing guide](./injected/docs/testing-guide.md) +- [Special Pages README](./special-pages/README.md) +- [Messaging docs](./messaging/docs/messaging.md) + +## Development + +Consider using [nvm](https://github.com/nvm-sh/nvm) to manage node versions, after installing in the project directory run: + +``` +nvm use +``` + +From the top-level root folder of this npm workspace, you can run the following npm commands: + +**Install dependencies**: + +Will install all the dependencies we need to build and run the project: +``` +npm install +``` + +**Build all workspaces**: + +Use this to produce the same output as a release. The `build` directory will be populated with +various artifacts. + +```sh +npm run build +``` + +> [!TIP] +> You can run the `build` command from within any sub-project too, the artifacts will always be +> lifted out to the root-level `build` folder. + +**Run unit tests for all workspaces**: + +```sh +npm run test-unit +``` + +**Run integration tests for all workspaces**: +```sh +npm run test-int +``` + +**Run extended integration tests for all workspaces**: +```sh +npm run test-int-x +``` + +**Clean tree and check for changes**: +```sh +npm run test-clean-tree +``` + +**Generate documentation using TypeDoc**: +```sh +npm run docs +``` + +**Generate and watch documentation using TypeDoc**: +```sh +npm run docs-watch +``` + +**Compile TypeScript files**: +```sh +npm run tsc +``` + +**Watch and compile TypeScript files**: +```sh +npm run tsc-watch +``` + +**Lint the codebase using ESLint**: +```sh +npm run lint +``` + +**Lint and automatically fix issues**: +```sh +npm run lint-fix +``` + +**Serve integration test pages on port 3220**: +```sh +npm run serve +``` + +**Serve special pages on port 3221**: +```sh +npm run serve-special-pages +``` + +## Release Process + +Releases are created via GitHub Actions: [Release workflow](https://github.com/duckduckgo/content-scope-scripts/actions/workflows/build.yml) + +### Creating a release + +1. Go to [Actions → Release](https://github.com/duckduckgo/content-scope-scripts/actions/workflows/build.yml) +2. Click "Run workflow" +3. Select version bump type +4. Click "Run workflow" + +The workflow creates a tag and GitHub release automatically. Build artifacts on the `releases` branch are consumed by native app repos. diff --git a/README.md b/README.md index a8b2625204..d5f8e2dfa2 100644 --- a/README.md +++ b/README.md @@ -30,93 +30,6 @@ Utilities to automatically generate TypeScript types from JSON Schema files. --- -## NPM commands +## Contributing -Consider using [nvm](https://github.com/nvm-sh/nvm) to manage node versions, after installing in the project directory run: - -``` -nvm use -``` - -From the top-level root folder of this npm workspace, you can run the following npm commands: - -**Install dependencies**: - -Will install all the dependencies we need to build and run the project: -``` -npm install -``` - -**Build all workspaces**: - -Use this to produce the same output as a release. The `build` directory will be populated with -various artifacts. - -```sh -npm run build -``` - -> [!TIP] -> You can run the `build` command from within any sub-project too, the artifacts will always be -> lifted out to the root-level `build` folder. - -**Run unit tests for all workspaces**: - -```sh -npm run test-unit -``` - -**Run integration tests for all workspaces**: -```sh -npm run test-int -``` - -**Run extended integration tests for all workspaces**: -```sh -npm run test-int-x -``` - -**Clean tree and check for changes**: -```sh -npm run test-clean-tree -``` - -**Generate documentation using TypeDoc**: -```sh -npm run docs -``` - -**Generate and watch documentation using TypeDoc**: -```sh -npm run docs-watch -``` - -**Compile TypeScript files**: -```sh -npm run tsc -``` - -**Watch and compile TypeScript files**: -```sh -npm run tsc-watch -``` - -**Lint the codebase using ESLint**: -```sh -npm run lint -``` - -**Lint and automatically fix issues**: -```sh -npm run lint-fix -``` - -**Serve integration test pages on port 3220**: -```sh -npm run serve -``` - -**Serve special pages on port 3221**: -```sh -npm run serve-special-pages -``` +See [CONTRIBUTING.md](./CONTRIBUTING.md) for development setup, npm commands, and release process. From 49cf3b04966436f3026d4db28cd02642e5359fe5 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Thu, 11 Dec 2025 11:43:41 +0000 Subject: [PATCH 22/26] Slim down heavily mdc file --- injected/.cursor/rules/injected.mdc | 184 +--------------------------- injected/docs/coding-guidelines.md | 120 ++++++++++++++++++ injected/docs/css-decision-guide.md | 41 +++++++ 3 files changed, 166 insertions(+), 179 deletions(-) create mode 100644 injected/docs/css-decision-guide.md diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc index ec56562ade..2922e90fa7 100644 --- a/injected/.cursor/rules/injected.mdc +++ b/injected/.cursor/rules/injected.mdc @@ -8,42 +8,10 @@ globs: injected/** JavaScript features injected into web pages or running in DOM environments of DuckDuckGo browsers. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. **See also:** -- `docs/coding-guidelines.md` for code style and best practices +- `docs/coding-guidelines.md` for code style, patterns, and examples +- `docs/css-decision-guide.md` for when to use C-S-S vs native code - [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc) for A/B testing features -## When to Use C-S-S - -**C-S-S injected scripts** — Best for: -- In-page changes or DOM manipulations -- Anything modifying a DOM API or manipulating page content -- Code that benefits from config, breakage tooling, and DOM wrappers -- Testing/linting/bundling makes code more robust than native-only - -**Native code** — Best for: -- Listening to bridge/handler messages -- When WebView APIs do the job better -- Avoid injecting scripts when native can handle it—harder to debug, no tooling - -**SERP/Static pages** — Can expose/call bridge messages, cleaner than scraping page content. - -### Decision Checklist - -| Question | Recommendation | -|----------|----------------| -| Modifying DOM API or manipulating page? | **Must** be in C-S-S | -| Need config handling, testing? | Prefer C-S-S (free tooling) | -| Code relies on page structure? | Use config to alter behavior; avoid if possible | -| Internal page involved? | Consider having the page expose an API/fire a message | -| Writing conditionals for iframe/domain? | Use `conditionalChanges` (see below) | -| Exposing new APIs? | Check if `apiManipulation` feature handles it | -| Need isolated context? | Prefer isolated entry point if possible | - -### Message Handlers vs Bridge - -- **Message handlers**: Limit to pages that need them; use script evaluation over messaging unless payload size is problematic -- **Bridge**: Config-enabled messages that pass through C-S-S; use subscriptions for exposing custom behavior to native -- If our site is involved, push messages to native rather than scraping - ## Structure ``` @@ -56,125 +24,6 @@ unit-test/ # Jasmine unit tests **Adding a new feature:** Add to `baseFeatures` or `otherFeatures` in `src/features.js`, then add to relevant platform arrays in `platformSupport`. -## Feature Pattern - -Features extend `ContentFeature` (which itself extends `ConfigFeature`). Use `ContentFeature` for features that need messaging, logging, and DOM interaction. Implement lifecycle methods: - -```javascript -import ContentFeature from '../content-feature.js'; - -export default class MyFeature extends ContentFeature { - init() { - // Main initialization - feature is enabled for this site - if (this.getFeatureSettingEnabled('someSetting')) { - this.applySomeFix(); - } - } - - load() { - // Early load - before remote config (use sparingly) - } - - update(data) { - // Receive updates from browser - } -} -``` - -## Remote Configuration - -Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: - -```javascript -// Boolean check with default -if (this.getFeatureSettingEnabled('settingName')) { ... } -if (this.getFeatureSettingEnabled('settingName', 'disabled')) { ... } // default disabled - -// Get setting value (returns typed object from privacy-configuration schema) -const settings = this.getFeatureSetting('settingName'); -``` - -Config structure example (from privacy-configuration): -```json -{ - "features": { - "webCompat": { - "state": "enabled", - "settings": { - "permissions": { "supportedPermissions": { "geolocation": { "native": true } } }, - "viewportWidth": { "forcedDesktopValue": "width=1280" } - } - } - } -} -``` - -**Feature state values:** `"enabled"` or `"disabled"`. Features default to disabled unless explicitly enabled. - -Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. - -### Conditional Changes - -Use `conditionalChanges` to apply JSON Patch operations based on runtime conditions. Conditions are evaluated in [`src/config-feature.js`](src/config-feature.js) (see `ConditionBlock` typedef and `_matchConditionalBlock`). - -**Supported conditions:** - -| Condition | Description | Example | -|-----------|-------------|---------| -| `domain` | Match hostname | `"domain": "example.com"` | -| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | -| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | -| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | -| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | -| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | -| `injectName` | Match inject context | `"injectName": "apple-isolated"` | -| `internal` | Internal builds only | `"internal": true` | -| `preview` | Preview builds only | `"preview": true` | - -**Config example:** -```json -{ - "settings": { - "conditionalChanges": [ - { - "condition": { "domain": "example.com" }, - "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] - }, - { - "condition": [ - { "urlPattern": "https://site1.com/*" }, - { "urlPattern": "https://site2.com/path/*" } - ], - "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] - } - ] - } -} -``` - -**Key rules:** -- All conditions in a block must match (AND logic) -- Array of condition blocks uses OR logic (any block matching applies the patch) -- `patchSettings` uses [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) paths (`/setting/nested`) -- Unsupported conditions cause the block to fail (for backwards compatibility) - -For A/B testing, see [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). - -## Messaging - -Use inherited messaging methods: - -```javascript -// Fire-and-forget -this.notify('messageName', { data }); - -// Request/response -const response = await this.request('messageName', { data }); - -// Subscribe to updates -this.subscribe('eventName', (data) => { ... }); -``` - ## TypeScript JSDoc types in JavaScript files. Import types via `@typedef`: @@ -200,23 +49,9 @@ Integration tests use test pages in `integration-test/test-pages/`. - **Event listeners**: Use `handleEvent` pattern or stored references (avoid `.bind(this)`) - **API existence**: Check `typeof API === 'function'` before wrapping - **DOM timing**: Check `document.readyState` before accessing DOM -- **Error types**: Match native error signatures in API shims (see below) - -## API Shims & Error Types - -When shimming browser APIs, use the correct error types to match native behavior: - -```javascript -// TypeError for invalid arguments -throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); +- **Error types**: Match native error signatures in API shims -// DOMException with name for API-specific errors -throw new DOMException('Share already in progress', 'InvalidStateError'); -throw new DOMException('Permission denied', 'NotAllowedError'); -return Promise.reject(new DOMException('No device selected.', 'NotFoundError')); -``` - -Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`, `DataError`, `SecurityError`. +See `docs/coding-guidelines.md` for DDGProxy, captured globals, retry utilities, and other patterns. ## Platform Considerations @@ -226,16 +61,7 @@ Features are bundled per-platform in `entry-points/`: - `windows.js` - Windows - `extension-mv3.js` - Browser extension -Platform-specific behavior: -```javascript -// Check platform in feature code -if (this.platform.name === 'ios' || this.platform.name === 'android') { - // Mobile-specific logic -} - -// Some features are platform-specific (see utils.js platformSpecificFeatures) -const platformSpecificFeatures = ['navigatorInterface', 'windowsPermissionUsage', 'messageBridge', 'favicon']; -``` +Platform-specific features: `navigatorInterface`, `windowsPermissionUsage`, `messageBridge`, `favicon` (see `utils.js` `platformSpecificFeatures`). ## Notes diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 233f1f0f2e..5654a8803a 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -2,6 +2,126 @@ > Guidelines for injected features development. +## Feature Pattern + +Features extend `ContentFeature` (which itself extends `ConfigFeature`). Use `ContentFeature` for features that need messaging, logging, and DOM interaction. Implement lifecycle methods: + +```js +import ContentFeature from '../content-feature.js'; + +export default class MyFeature extends ContentFeature { + init() { + // Main initialization - feature is enabled for this site + if (this.getFeatureSettingEnabled('someSetting')) { + this.applySomeFix(); + } + } + + load() { + // Early load - before remote config (use sparingly) + } + + update(data) { + // Receive updates from browser + } +} +``` + +## Remote Configuration + +Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: + +```js +// Boolean check with default +if (this.getFeatureSettingEnabled('settingName')) { ... } +if (this.getFeatureSettingEnabled('settingName', 'disabled')) { ... } // default disabled + +// Get setting value (returns typed object from privacy-configuration schema) +const settings = this.getFeatureSetting('settingName'); +``` + +**Feature state values:** `"enabled"` or `"disabled"`. Features default to disabled unless explicitly enabled. + +Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. + +### Conditional Changes + +Use `conditionalChanges` to apply JSON Patch operations based on runtime conditions. Conditions are evaluated in `src/config-feature.js` (see `ConditionBlock` typedef and `_matchConditionalBlock`). + +**Supported conditions:** + +| Condition | Description | Example | +|-----------|-------------|---------| +| `domain` | Match hostname | `"domain": "example.com"` | +| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | +| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | +| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | +| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | +| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | +| `injectName` | Match inject context | `"injectName": "apple-isolated"` | +| `internal` | Internal builds only | `"internal": true` | +| `preview` | Preview builds only | `"preview": true` | + +**Config example:** +```json +{ + "settings": { + "conditionalChanges": [ + { + "condition": { "domain": "example.com" }, + "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] + }, + { + "condition": [ + { "urlPattern": "https://site1.com/*" }, + { "urlPattern": "https://site2.com/path/*" } + ], + "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] + } + ] + } +} +``` + +**Key rules:** +- All conditions in a block must match (AND logic) +- Array of condition blocks uses OR logic (any block matching applies the patch) +- `patchSettings` uses [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) paths (`/setting/nested`) +- Unsupported conditions cause the block to fail (for backwards compatibility) + +For A/B testing, see [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). + +## Messaging + +Use inherited messaging methods: + +```js +// Fire-and-forget +this.notify('messageName', { data }); + +// Request/response +const response = await this.request('messageName', { data }); + +// Subscribe to updates +this.subscribe('eventName', (data) => { ... }); +``` + +## API Shims & Error Types + +When shimming browser APIs, use the correct error types to match native behavior: + +```js +// TypeError for invalid arguments +throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); + +// DOMException with name for API-specific errors +throw new DOMException('Share already in progress', 'InvalidStateError'); +throw new DOMException('Permission denied', 'NotAllowedError'); +return Promise.reject(new DOMException('No device selected.', 'NotFoundError')); +``` + +Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`, `DataError`, `SecurityError`. + ## Code Style ### Constants diff --git a/injected/docs/css-decision-guide.md b/injected/docs/css-decision-guide.md new file mode 100644 index 0000000000..9066091776 --- /dev/null +++ b/injected/docs/css-decision-guide.md @@ -0,0 +1,41 @@ +# When to Use C-S-S + +Decision guide for when to use C-S-S injected scripts vs native code. + +## C-S-S Injected Scripts + +Best for: +- In-page changes or DOM manipulations +- Anything modifying a DOM API or manipulating page content +- Code that benefits from config, breakage tooling, and DOM wrappers +- Testing/linting/bundling makes code more robust than native-only + +## Native Code + +Best for: +- Listening to bridge/handler messages +- When WebView APIs do the job better +- Avoid injecting scripts when native can handle it—harder to debug, no tooling + +## SERP/Static Pages + +Can expose/call bridge messages, cleaner than scraping page content. + +## Decision Checklist + +| Question | Recommendation | +|----------|----------------| +| Modifying DOM API or manipulating page? | **Must** be in C-S-S | +| Need config handling, testing? | Prefer C-S-S (free tooling) | +| Code relies on page structure? | Use config to alter behavior; avoid if possible | +| Internal page involved? | Consider having the page expose an API/fire a message | +| Writing conditionals for iframe/domain? | Use `conditionalChanges` in config | +| Exposing new APIs? | Check if `apiManipulation` feature handles it | +| Need isolated context? | Prefer isolated entry point if possible | + +## Message Handlers vs Bridge + +- **Message handlers**: Limit to pages that need them; use script evaluation over messaging unless payload size is problematic +- **Bridge**: Config-enabled messages that pass through C-S-S; use subscriptions for exposing custom behavior to native +- If our site is involved, push messages to native rather than scraping + From 768ccf614afae5266f232294b1ff0f96666f3e0b Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Thu, 11 Dec 2025 19:54:19 +0000 Subject: [PATCH 23/26] Lint fix --- injected/docs/coding-guidelines.md | 55 ++++++++++++++--------------- injected/docs/css-decision-guide.md | 21 +++++------ 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md index 5654a8803a..2f500e27fc 100644 --- a/injected/docs/coding-guidelines.md +++ b/injected/docs/coding-guidelines.md @@ -16,11 +16,11 @@ export default class MyFeature extends ContentFeature { this.applySomeFix(); } } - + load() { // Early load - before remote config (use sparingly) } - + update(data) { // Receive updates from browser } @@ -50,40 +50,39 @@ Use `conditionalChanges` to apply JSON Patch operations based on runtime conditi **Supported conditions:** -| Condition | Description | Example | -|-----------|-------------|---------| -| `domain` | Match hostname | `"domain": "example.com"` | -| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | -| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | -| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | -| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | -| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | -| `injectName` | Match inject context | `"injectName": "apple-isolated"` | -| `internal` | Internal builds only | `"internal": true` | -| `preview` | Preview builds only | `"preview": true` | +| Condition | Description | Example | +| --------------------- | -------------------------- | ------------------------------------------------------------------- | +| `domain` | Match hostname | `"domain": "example.com"` | +| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | +| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | +| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | +| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | +| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | +| `injectName` | Match inject context | `"injectName": "apple-isolated"` | +| `internal` | Internal builds only | `"internal": true` | +| `preview` | Preview builds only | `"preview": true` | **Config example:** + ```json { - "settings": { - "conditionalChanges": [ - { - "condition": { "domain": "example.com" }, - "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] - }, - { - "condition": [ - { "urlPattern": "https://site1.com/*" }, - { "urlPattern": "https://site2.com/path/*" } - ], - "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] - } - ] - } + "settings": { + "conditionalChanges": [ + { + "condition": { "domain": "example.com" }, + "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] + }, + { + "condition": [{ "urlPattern": "https://site1.com/*" }, { "urlPattern": "https://site2.com/path/*" }], + "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] + } + ] + } } ``` **Key rules:** + - All conditions in a block must match (AND logic) - Array of condition blocks uses OR logic (any block matching applies the patch) - `patchSettings` uses [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) paths (`/setting/nested`) diff --git a/injected/docs/css-decision-guide.md b/injected/docs/css-decision-guide.md index 9066091776..a8c5a1d1a9 100644 --- a/injected/docs/css-decision-guide.md +++ b/injected/docs/css-decision-guide.md @@ -5,6 +5,7 @@ Decision guide for when to use C-S-S injected scripts vs native code. ## C-S-S Injected Scripts Best for: + - In-page changes or DOM manipulations - Anything modifying a DOM API or manipulating page content - Code that benefits from config, breakage tooling, and DOM wrappers @@ -13,6 +14,7 @@ Best for: ## Native Code Best for: + - Listening to bridge/handler messages - When WebView APIs do the job better - Avoid injecting scripts when native can handle it—harder to debug, no tooling @@ -23,19 +25,18 @@ Can expose/call bridge messages, cleaner than scraping page content. ## Decision Checklist -| Question | Recommendation | -|----------|----------------| -| Modifying DOM API or manipulating page? | **Must** be in C-S-S | -| Need config handling, testing? | Prefer C-S-S (free tooling) | -| Code relies on page structure? | Use config to alter behavior; avoid if possible | -| Internal page involved? | Consider having the page expose an API/fire a message | -| Writing conditionals for iframe/domain? | Use `conditionalChanges` in config | -| Exposing new APIs? | Check if `apiManipulation` feature handles it | -| Need isolated context? | Prefer isolated entry point if possible | +| Question | Recommendation | +| --------------------------------------- | ----------------------------------------------------- | +| Modifying DOM API or manipulating page? | **Must** be in C-S-S | +| Need config handling, testing? | Prefer C-S-S (free tooling) | +| Code relies on page structure? | Use config to alter behavior; avoid if possible | +| Internal page involved? | Consider having the page expose an API/fire a message | +| Writing conditionals for iframe/domain? | Use `conditionalChanges` in config | +| Exposing new APIs? | Check if `apiManipulation` feature handles it | +| Need isolated context? | Prefer isolated entry point if possible | ## Message Handlers vs Bridge - **Message handlers**: Limit to pages that need them; use script evaluation over messaging unless payload size is problematic - **Bridge**: Config-enabled messages that pass through C-S-S; use subscriptions for exposing custom behavior to native - If our site is involved, push messages to native rather than scraping - From 73e45f406ba436c2633eca3666a1b89af8c98494 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Thu, 11 Dec 2025 20:01:29 +0000 Subject: [PATCH 24/26] Stale netlify cache on updates of pr --- netlify.toml | 2 ++ package.json | 1 + 2 files changed, 3 insertions(+) diff --git a/netlify.toml b/netlify.toml index e8b6b7c465..944ade142a 100644 --- a/netlify.toml +++ b/netlify.toml @@ -1,2 +1,4 @@ [build] +command = "npm ci && npm run docs-netlify" +publish = "docs" ignore = "git log -1 --pretty=%B | grep dependabot" diff --git a/package.json b/package.json index 24f2c36f83..11c83b3ccb 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "test-int-snapshots-update": "npm run test-int-snapshots-update --workspaces --if-present", "test-clean-tree": "npm run build && sh scripts/check-for-changes.sh", "docs": "typedoc", + "docs-netlify": "npm run build -w special-pages && npm run docs && cp -R build/integration/ docs/build", "docs-watch": "typedoc --watch", "tsc": "tsc", "tsc-watch": "tsc --watch", From 059131ee459b8991de6a6b877e90f3076dee0022 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Thu, 11 Dec 2025 20:04:27 +0000 Subject: [PATCH 25/26] Flying blind with netifly ci --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 11c83b3ccb..339b3c6467 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "test-int-snapshots-update": "npm run test-int-snapshots-update --workspaces --if-present", "test-clean-tree": "npm run build && sh scripts/check-for-changes.sh", "docs": "typedoc", - "docs-netlify": "npm run build -w special-pages && npm run docs && cp -R build/integration/ docs/build", + "docs-netlify": "npm run build-locales -w injected && npm run build -w special-pages && npm run docs && cp -R build/integration/ docs/build", "docs-watch": "typedoc --watch", "tsc": "tsc", "tsc-watch": "tsc --watch", From 6a02bfd964dcf47d3230f9a0e0dbc1dfd9b4b619 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Thu, 11 Dec 2025 20:27:06 +0000 Subject: [PATCH 26/26] Dropping special pages docs --- special-pages/docs/coding-guidelines.md | 144 ------------------------ 1 file changed, 144 deletions(-) delete mode 100644 special-pages/docs/coding-guidelines.md diff --git a/special-pages/docs/coding-guidelines.md b/special-pages/docs/coding-guidelines.md deleted file mode 100644 index f3f8bdc889..0000000000 --- a/special-pages/docs/coding-guidelines.md +++ /dev/null @@ -1,144 +0,0 @@ -# Special Pages Coding Guidelines - -> Guidelines for Preact-based special pages development. - -## Code Style - -### Component and Hook Usage - -#### Preact Attributes -- Use `class` instead of `className` in Preact components - -#### Key Placement -- Avoid unnecessary re-renders by correctly placing keys: - -```jsx -// Incorrect - key on wrapper div -
- {/* Component content */} -
- -// Correct - key on component - -``` - -#### Dependency Management in Hooks -- Include all used values in the dependency array of `useCallback` to prevent stale closures: - -```js -const setter = useCallback(() => { - // Function body using dep1 and dep2 -}, [dep1, dep2]); // Include all dependencies -``` - -### Error Handling and Debugging -- Avoid hardcoding debug flags; ensure they are configurable and environment-dependent -- Remove `console.log` statements from production code - -## Architecture & Design - -### Internationalization -- Use translation functions via `useTypedTranslations()`: - -```js -const { t } = useTypedTranslations(); -const message = t('key_name'); -``` - -- Store translations in `public/locales/.json` - -### Type Consistency -- Use JSDoc types imported via `@typedef`: - -```js -/** @typedef {import('./types.js').MyType} MyType */ -``` - -- Import types from generated files in `types/` directory - -### Platform-Specific Styling -- Use `data-platform-name` attribute for platform detection -- Use `data-theme` for dark/light mode instead of media queries - -### Component Design -- Avoid rendering children multiple times within components -- Ensure props (especially `style`) are handled correctly -- Do not couple functionality to UI visibility—control functionality independently from whether UI elements are shown - -### Feature State Management -- Maintain parameter order in method signatures to prevent breaking changes -- Avoid overriding falsy values incorrectly - -## Security & Privacy - -### Controlled Component State -- Avoid directly manipulating properties like `value` on controlled components -- Let Preact manage input state through props and event handlers - -## Performance - -### Memory Management -- Clean up subscriptions and event listeners in component unmount - -## Error Handling - -### Async/Await Usage -- Use `await` with asynchronous operations to ensure errors are caught: - -```js -await someAsyncFunction(); -``` - -### Error Handling in Promises -- Ensure promises have both resolve and reject paths: - -```js -new Promise((resolve, reject) => { - if (condition) { - resolve(result); - } else { - reject(new Error('specific error message')); - } -}); -``` - -### Null Checks -- Perform null checks before using objects: - -```js -if (object != null) { - // Use the object -} -``` - -### Element Visibility and Accessibility -- Avoid changing element visibility in a way that prevents focus: - -```css -/* Ensure focusable elements remain accessible */ -[aria-expanded="true"] { - visibility: visible; /* Not hidden */ -} -``` - -## Testing - -### Test Independence in Playwright -- Ensure tests are independent to allow safe parallel execution -- Avoid shared setup outside of each test block -- Use Playwright's test fixtures for complex setup - -### Async/Await in Tests -- Always use `await` with asynchronous operations: - -```javascript -// Correct -await page.click('button'); - -// Incorrect - may not wait for completion -page.click('button'); -``` - -### Mocking and Test Data -- Include comprehensive scenarios in mocks to cover edge cases -