Skip to content

Implement new analytics composable with tests - #5847

Open
bjester wants to merge 2 commits into
learningequality:unstablefrom
bjester:analytics-composable
Open

Implement new analytics composable with tests#5847
bjester wants to merge 2 commits into
learningequality:unstablefrom
bjester:analytics-composable

Conversation

@bjester

@bjester bjester commented Apr 17, 2026

Copy link
Copy Markdown
Member

Summary

As we move towards replacing Vuex and the composition architecture, utilities such as the analytics plugin, that have widespread usage, are prime targets for migrating first. This avoids friction migrating other things that may rely on using them.

  • Moves existing Analytics class to a new file for the composable
  • Adds singleton functionality to Analytics instance
  • Implements a new composable that exposes the Analytics instance methods
  • Adds tests for the composable and functionality of Analytics
  • Updates the existing Vue plugin to use the same Analytics instance
  • Reconfigures initialization and tests for refactor

References

closes #5845

Reviewer guidance

Load Studio and verify there are no frontend errors. Verify you can see Analytics log statements in the console as you open topics, edit content, etc.

AI usage

I used AI to plan and implement this, then simplified it by reusing the Analytics class and removed cruft from tests.

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean refactor — replaces a convoluted self-init-and-merge pattern with a proper singleton that correctly wires the Vue plugin and composable to share one Analytics instance.

CI passing. No UI files changed (Phase 3 skipped). All 6 acceptance-criteria methods present and tested.

  • suggestion: jest.resetModules() in beforeEach is a no-op for static imports — see inline
  • suggestion: Timer mocks set up after destroyInstance() means real clearInterval is called with a mocked ID on teardown — see inline
  • praise: Singleton pattern and bind() approach — see inline

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

@bjester
bjester force-pushed the analytics-composable branch 2 times, most recently from 6ffdfe3 to 247fe4b Compare April 17, 2026 18:47

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good — the singleton refactor is clean and the two prior suggestions have been addressed.

CI passing. No UI files changed (Phase 3 skipped).

Prior findings

Resolved:

  • jest.resetModules() no-op (suggestion) — removed in commit 247fe4b

Acknowledged (not re-raised):

  • clearInterval/setInterval mock ordering (suggestion) — author disagreed; added destroyInstance test instead

2/2 prior findings resolved or acknowledged.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

  • Retrieved prior bot reviews deterministically via the GitHub API
  • Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
  • Only raised NEW findings for newly introduced code
  • Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
  • Checked CI status and linked issue acceptance criteria

@AlexVelezLl AlexVelezLl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me, and tests provide good confidence. Just a couple of comments/questions, but non-blocking!

@bjester
bjester force-pushed the analytics-composable branch from 247fe4b to 05265e1 Compare August 11, 2026 19:54
@bjester
bjester requested a review from AlexVelezLl August 11, 2026 19:54
@rtibblesbot

rtibblesbot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🔵 Review posted

Last updated: 2026-08-11 20:14 UTC

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #5847 — 1 of 3 prior findings resolved; the other 2 were acknowledged/withdrawn last round. Head unchanged since the last pass (05265e1cf). CI passing. No UI files changed, so visual verification and manual QA do not apply.

Two new suggestions inline, neither blocking. All issue deliverables are met: six methods exported with matching signatures, 5-minute reset interval preserved, files at the specified paths.

Prior-finding status

RESOLVED — contentcuration/contentcuration/frontend/shared/composables/tests/useAnalytics.spec.js:9 — jest.resetModules() is a no-op for statically-imported modules
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/composables/tests/useAnalytics.spec.js:21 — clearInterval/setInterval mocked after destroyInstance() (withdrawn last round)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/composables/useAnalytics.js:198 — praise on binding methods to the singleton


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:

  • Retrieved prior bot reviews via the GitHub API
  • Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
  • Only raised NEW findings for newly introduced code
  • Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
  • Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence

*/
static getInstance() {
if (!_instance) {
_instance = new Analytics(window.dataLayer || []);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The || [] fallback detaches the instance from GTM if it ever fires. base.html:101 assigns window.dataLayer = [] in <head> before the app bundle, so it doesn't today — but if it did, the instance would hold a private array GTM never reads and every trackEvent would be silently dropped, with no error and no signal.

The old plugin covered this: the module-level AnalyticsPlugin(Vue, { dataLayer: [] }) created a placeholder that Vue.use later merged into the real layer. That merge is gone (correctly), but the fallback it existed for remains.

Assigning back matches the GTM snippet's own idiom (w[l]=w[l]||[]) and makes the fallback actually work:

window.dataLayer = window.dataLayer || [];
_instance = new Analytics(window.dataLayer);

It also gives the issue's "verify the window.dataLayer fallback works" test something to assert — no test currently exercises the undefined branch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for "verify the window.dataLayer fallback works" is to ensure the Analytics utility is fault tolerant with regards to the window.dataLayer being defined. You're right, there should be a test for it. Although, we don't need to worry about assigning it globally if it doesn't exist, which is something the old code did not do either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood — deferring to your call. You're right that the old code never assigned back to window either; fault tolerance is the requirement, not attachment. Withdrawing the suggestion.

});
});

describe('method signatures', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This block has no independent failure mode. The six typeof x === 'function' tests (239–267) can only fail in ways the behavior tests at 34–217 already catch, since those call every method through the destructured binding, and toHaveProperty at 231–236 duplicates the six that follow it. Consider dropping the block — 42 lines that will never be the thing that goes red first.

Vue.$analytics.destroy();
}
export default function AnalyticsPlugin(Vue) {
const analytics = Analytics.getInstance();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Dropping the module-level self-registration removes the double-instantiate-then-merge dance entirely — one instance, no import-time vs Vue.use ordering dependency.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create useAnalytics composable with comprehensive tests

3 participants