[sc-13778] introduce new setting "analyticsKeywordInterceptor"#161
[sc-13778] introduce new setting "analyticsKeywordInterceptor"#161haoAddsearch wants to merge 2 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a keyword interceptor to analytics: configuration option and instance method, analytics utility support (intercept/transform keyword for search, click, autocomplete events with safe fallbacks), README docs update, and unit tests covering normal, non-string, and exception cases. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Analytics as Analytics Module
participant Interceptor as Keyword Interceptor
participant Client as Internal Stats Client
participant External as External Analytics Callback
App->>Analytics: trigger analytics (search/click/autocomplete, keyword, payload)
Analytics->>Interceptor: call interceptor({ action, keyword, payload })
alt interceptor returns string
Interceptor-->>Analytics: modified_keyword
else returns non-string or throws
Interceptor-->>Analytics: (no valid string) => original_keyword
end
Analytics->>Client: sendStatsEvent(action, analytics_keyword, payload)
Analytics->>External: callExternalAnalyticsCallback(action, analytics_keyword, payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/util/analytics.js (2)
4-20: Consider usingglobalThisfor better compatibility.SonarCloud flags the use of
globaloverglobalThis. Whileglobalworks in Node.js,globalThisis the standardized way to access the global object across environments.♻️ Suggested refactor
function withImmediateTimers(cb) { - const originalSetTimeout = global.setTimeout; - const originalClearTimeout = global.clearTimeout; + const originalSetTimeout = globalThis.setTimeout; + const originalClearTimeout = globalThis.clearTimeout; - global.setTimeout = (fn) => { + globalThis.setTimeout = (fn) => { fn(); return 1; }; - global.clearTimeout = () => {}; + globalThis.clearTimeout = () => {}; try { cb(); } finally { - global.setTimeout = originalSetTimeout; - global.clearTimeout = originalClearTimeout; + globalThis.setTimeout = originalSetTimeout; + globalThis.clearTimeout = originalClearTimeout; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/analytics.js` around lines 4 - 20, The helper withImmediateTimers uses Node-specific global instead of standardized globalThis; update all references to global (e.g., global.setTimeout, global.clearTimeout, originalSetTimeout, originalClearTimeout) to use globalThis so the function operates across environments: capture original functions from globalThis, override globalThis.setTimeout/globalThis.clearTimeout inside the function, and restore them in the finally block.
61-116: Good test coverage for core interceptor behavior.The tests cover the happy path, non-string return fallback, and exception fallback scenarios. Consider adding tests for the click analytics path to ensure full coverage.
💡 Additional test suggestion
it('should modify keyword for click analytics', () => { const sentEvents = []; analytics.setAnalyticsKeywordInterceptor(({ action, keyword }) => action === 'click' ? `clicked:${keyword}` : keyword ); const client = { sendStatsEvent: (action, keyword, payload) => sentEvents.push({ action, keyword, payload }), getSettings: () => ({ paging: { pageSize: 10 }, keyword: 'test' }) }; // Test addClickTrackers / onLinkClick behavior });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/analytics.js` around lines 61 - 116, Add a unit test covering the click analytics path: register an interceptor with analytics.setAnalyticsKeywordInterceptor that changes keywords only for action === 'click', create a fake client with sendStatsEvent and getSettings (returning paging and keyword), then either call analytics.sendClickStats(client, 'Original', ...) or wire up analytics.addClickTrackers and invoke the onLinkClick handler to simulate a click; assert that the sent event's action === 'click' and that the keyword was transformed (e.g., 'clicked:Original') while non-click actions remain unchanged.README.md (1)
1119-1129: Documentation example could show full event object structure.The example is functional, but the interceptor actually receives
{ action, keyword, payload }per the implementation. Consider expanding the example to show the full signature available to users, which would help them leverage all available context.analyticsKeywordInterceptor: function (event) { // event contains: { action, keyword, payload } // action: 'search' | 'click' // payload: contains contextual data like numberOfResults, documentId, etc. return event.keyword.replace(/\+?\d[\d\s().-]{6,}\d/g, '[REDACTED]'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 1119 - 1129, The README example for analyticsKeywordInterceptor shows only a string input but the interceptor actually receives an object ({ action, keyword, payload }); update the example usage of analyticsKeywordInterceptor to use a full event object parameter name and document the fields (action, keyword, payload) and example payload properties (e.g., numberOfResults, documentId) so users can access contextual data when modifying keywords; refer to analyticsKeywordInterceptor, the event parameter, and its action/keyword/payload properties when editing the example text and comments.src/util/analytics.js (1)
44-46: Consider logging interceptor errors in debug mode.The empty catch block silently swallows errors, which is correct for production resilience but could make debugging difficult for developers implementing custom interceptors. Consider adding a console warning when debug mode is enabled.
🔧 Suggested improvement
} catch (e) { - // Keep original keyword on interceptor failure + // Keep original keyword on interceptor failure + if (typeof console !== 'undefined' && console.warn) { + console.warn('analyticsKeywordInterceptor threw an error:', e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/analytics.js` around lines 44 - 46, In src/util/analytics.js update the empty catch (e) block so it still swallows errors in production but emits a debug-level warning when debug mode is enabled: detect the module's debug flag (e.g., process.env.DEBUG, an options.debug value, or the existing analytics debug/logging facility) and call console.warn or the module logger to print the interceptor error and context (include the caught error e and the keyword/interceptor name) only when debug is true; leave the swallow behavior unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/analytics.js`:
- Around line 99-103: sendAutocompleteStats currently computes the intercepted
keyword via getAnalyticsKeyword(action, keyword, payload) and only calls
c.client.sendStatsEvent, so external integrations never receive the modified
keyword; update sendAutocompleteStats inside the statsArray.forEach loop (where
c, sourceIndex, payload and analyticsKeyword are available) to also invoke
callExternalAnalyticsCallback(action, analyticsKeyword, { numberOfResults:
c.numberOfResults, sourceIndex }) (or the existing payload shape used by
sendSearchStats) so the external callback receives the intercepted keyword for
each source just like sendSearchStats does.
---
Nitpick comments:
In `@README.md`:
- Around line 1119-1129: The README example for analyticsKeywordInterceptor
shows only a string input but the interceptor actually receives an object ({
action, keyword, payload }); update the example usage of
analyticsKeywordInterceptor to use a full event object parameter name and
document the fields (action, keyword, payload) and example payload properties
(e.g., numberOfResults, documentId) so users can access contextual data when
modifying keywords; refer to analyticsKeywordInterceptor, the event parameter,
and its action/keyword/payload properties when editing the example text and
comments.
In `@src/util/analytics.js`:
- Around line 44-46: In src/util/analytics.js update the empty catch (e) block
so it still swallows errors in production but emits a debug-level warning when
debug mode is enabled: detect the module's debug flag (e.g., process.env.DEBUG,
an options.debug value, or the existing analytics debug/logging facility) and
call console.warn or the module logger to print the interceptor error and
context (include the caught error e and the keyword/interceptor name) only when
debug is true; leave the swallow behavior unchanged otherwise.
In `@test/util/analytics.js`:
- Around line 4-20: The helper withImmediateTimers uses Node-specific global
instead of standardized globalThis; update all references to global (e.g.,
global.setTimeout, global.clearTimeout, originalSetTimeout,
originalClearTimeout) to use globalThis so the function operates across
environments: capture original functions from globalThis, override
globalThis.setTimeout/globalThis.clearTimeout inside the function, and restore
them in the finally block.
- Around line 61-116: Add a unit test covering the click analytics path:
register an interceptor with analytics.setAnalyticsKeywordInterceptor that
changes keywords only for action === 'click', create a fake client with
sendStatsEvent and getSettings (returning paging and keyword), then either call
analytics.sendClickStats(client, 'Original', ...) or wire up
analytics.addClickTrackers and invoke the onLinkClick handler to simulate a
click; assert that the sent event's action === 'click' and that the keyword was
transformed (e.g., 'clicked:Original') while non-click actions remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f8f117-ab7b-4e3d-9742-f864486029fe
📒 Files selected for processing (4)
README.mdsrc/index.jssrc/util/analytics.jstest/util/analytics.js
| statsArray.forEach((c, sourceIndex) => { | ||
| const payload = { numberOfResults: c.numberOfResults, sourceIndex }; | ||
| const analyticsKeyword = getAnalyticsKeyword(action, keyword, payload); | ||
| c.client.sendStatsEvent(action, analyticsKeyword, { numberOfResults: c.numberOfResults }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if external analytics callback is called elsewhere for autocomplete
rg -n -A5 'callExternalAnalyticsCallback' src/util/analytics.jsRepository: AddSearch/search-ui
Length of output: 695
🏁 Script executed:
#!/bin/bash
# Get the full context around lines 99-103 to understand the function
sed -n '80,120p' src/util/analytics.jsRepository: AddSearch/search-ui
Length of output: 1519
🏁 Script executed:
#!/bin/bash
# Also check the sendSearchStats function for comparison
sed -n '70,80p' src/util/analytics.jsRepository: AddSearch/search-ui
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Search for the function containing the autocomplete code (lines 99-103)
rg -n 'function|const.*=.*function|\s*\w+\s*\(' src/util/analytics.js | head -20Repository: AddSearch/search-ui
Length of output: 1115
Autocomplete analytics doesn't call external callback with intercepted keyword.
The sendAutocompleteStats function intercepts the keyword via getAnalyticsKeyword() but only sends it to c.client.sendStatsEvent(). Unlike sendSearchStats, it never calls callExternalAnalyticsCallback(), so external analytics integrations won't receive autocomplete events with the modified keyword. If this omission is intentional due to multiple sources, it should be documented; otherwise, the external callback should be invoked inside the statsArray.forEach() loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/analytics.js` around lines 99 - 103, sendAutocompleteStats currently
computes the intercepted keyword via getAnalyticsKeyword(action, keyword,
payload) and only calls c.client.sendStatsEvent, so external integrations never
receive the modified keyword; update sendAutocompleteStats inside the
statsArray.forEach loop (where c, sourceIndex, payload and analyticsKeyword are
available) to also invoke callExternalAnalyticsCallback(action,
analyticsKeyword, { numberOfResults: c.numberOfResults, sourceIndex }) (or the
existing payload shape used by sendSearchStats) so the external callback
receives the intercepted keyword for each source just like sendSearchStats does.
There was a problem hiding this comment.
sendAutocompleteStats has historically sent stats only via client.sendStatsEvent (per source) and not through analyticsCallback. Keeping that behavior unchanged in this PR to avoid altering callback event volume/semantics for existing integrations; we can address autocomplete callback behavior separately as an explicit enhancement.
There was a problem hiding this comment.
@haoAddsearch, understood — that's a reasonable call to keep the existing sendAutocompleteStats semantics intact and avoid unexpected changes in callback event volume for existing integrations. I'll note this for future reviews.
Would you like me to open a follow-up GitHub issue to track the autocomplete external callback enhancement so it doesn't get lost?
ʕ •ᴥ•ʔ scribbles notes carefully
✏️ Learnings added
Learnt from: haoAddsearch
Repo: AddSearch/search-ui PR: 161
File: src/util/analytics.js:99-103
Timestamp: 2026-03-25T14:50:20.995Z
Learning: In `src/util/analytics.js` (AddSearch/search-ui), `sendAutocompleteStats` intentionally does NOT call `callExternalAnalyticsCallback`. It only calls `c.client.sendStatsEvent` per source. This is by design to avoid altering callback event volume/semantics for existing integrations. Adding the external callback for autocomplete is a separate planned enhancement, not a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests