feat: add time on site #975

rmi22186 · 2025-02-05T14:49:33Z

Instructions

PR target branch should be against development
PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

This initial PR gets a class for Foreground

Testing Plan

Was this tested locally? If not, explain why. - Tested between multiple tabs to ensure the timer works across tabs.
Added unit/integration tests as well

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Closes https://go.mparticle.com/work/SQDSDKS-7054-tos

src/foregroundTimeTracker.ts

rmi22186 · 2025-02-05T14:57:29Z

src/foregroundTimeTracker.ts

+    }
+
+    private handleVisibilityChange(): void {
+        if (document.hidden) {


can we abstract the document out? could this be some sort of "visibility handler"?

rmi22186 · 2025-02-05T14:58:52Z

src/foregroundTimeTracker.ts

+
+    private startTracking(): void {
+        if (!document.hidden) {
+            this.startTime = performance.now();


check for existence of it

rmi22186 · 2025-02-05T15:01:16Z

src/foregroundTimeTracker.ts

+            this.isActive = true;
+            setInterval(() => {
+                console.log(this.totalTime);
+            });


Suggested change

});

}, 1000);

alexs-mparticle · 2025-02-06T21:11:01Z

src/foregroundTimeTracker.ts

+    public startTime: number = 0;
+    public totalTime: number = 0;
+
+    constructor(apiKey: string) {


Nit: Technically this isn't really the apiKey but a timerKey since it doesn't make a call to the api. I'm not adamant about this though.

true and actually i should pass in the workspaceToken which we use for our general storage as well.

actually seems like when i try to use workspaceToken that I need to instantiate the foreground timer in the store which is when the workspaceToken i available. so for now i'm going to keep using it as the apikey but call it the timerKey in line. this is a small detail that I think we can discuss once the other PRs are in.

alexs-mparticle · 2025-02-06T21:14:16Z

src/mp-instance.ts

@@ -66,6 +67,7 @@ export type IntegrationDelays = Dictionary<boolean>;
 // https://go.mparticle.com/work/SQDSDKS-6949
 export interface IMParticleWebSDKInstance extends MParticleWebSDK {
    // Private Properties
+    _timer: ForegroundTimer;


_timer is too generic. I would call this something like _foregroundTimer, _timeOnSiteTimer or something relevant to the use case.

good call. this was originally just a filler to get a POC going. i'll update now

alexs-mparticle · 2025-02-06T21:17:11Z

src/sessionManager.ts

@@ -132,6 +132,7 @@ export default function SessionManager(
            });

            mpInstance._Store.nullifySession();
+            mpInstance._timer.resetTimer();


Seeing that you're adding this here, I would think we should also add it to the Messages.InformationMessages.NoSessionToEnd and Messages.InformationMessages.AbandonEndSession lines

alexs-mparticle · 2025-02-06T21:21:26Z

test/jest/foregroundTimeTracker.spec.ts

+        jest.restoreAllMocks();
+    });
+
+    // in Jest, document.hidden by default is false


I would put this comment in the beforeEach section

alexs-mparticle · 2025-02-07T16:07:18Z

src/foregroundTimeTracker.ts

+        window.addEventListener('storage', this.syncAcrossTabs);
+        this.startTracking();
+
+        // TODO: this is just to ensure when we load it in an app we can see the timer updates in the console


Sustained Engineering: There could be some value in adding our debug logger here so that if debug === true, we can fire this log. I'm fine with it being a ticket for later.

i decided to move this ahead, because by calling getForegroundTime we end up calling updateTotalTime and i just don't want it to create unintended consequences.

test/jest/foregroundTimeTracker.spec.ts

src/foregroundTimeTracker.ts

alexs-mparticle · 2025-02-11T16:20:59Z

test/src/tests-batchUploader.ts

@@ -136,11 +137,11 @@ describe('batch uploader', () => {
                fetchMock.restore();
            });

-            it('should reject batches without events', (done) => {
+            it.only('should reject batches without events', async () => {


Do you still need this?

alexs-mparticle · 2025-02-11T16:21:19Z

test/src/tests-batchUploader.ts

@@ -155,6 +156,8 @@ describe('batch uploader', () => {
                };

                const actualBatch = batchValidator.returnBatch(baseEvent);
+                console.log('actualBatch.events[0]');


Do you still need this?

alexs-mparticle · 2025-02-11T16:21:23Z

test/src/tests-batchUploader.ts

@@ -174,12 +177,11 @@ describe('batch uploader', () => {
                );

                expect(actualBatchResult.events.length).to.equal(1);
+                console.log('actualBatchResult.events[0]')


Do you still need this?

alexs-mparticle · 2025-02-11T16:23:28Z

test/jest/foregroundTimeTracker.spec.ts

+        });
+
+        afterEach(() => {
+            jest.restoreAllMocks();


Do you need to repeat this?

each describe block should have this, right? i'm being extra cautious.

IIRC describe blocks should inherit from their parents.

ah. from testing it looks like you're right

alexs-mparticle · 2025-02-11T16:24:27Z

test/jest/foregroundTimeTracker.spec.ts

+    });
+
+    describe('constructor', () => {
+        let tracker: ForegroundTimeTracker;


I don't see you using this in this block.

Suggested change

let tracker: ForegroundTimeTracker;

alexs-mparticle · 2025-02-11T16:56:18Z

test/jest/foregroundTimeTracker.spec.ts

+
+        it('should call startTracking if the document is not hidden', () => {
+            Object.defineProperty(document, 'hidden', { value: false });
+            const startTrackingSpy = jest.spyOn(ForegroundTimeTracker.prototype as any, 'startTracking');


I think you should avoid using any here.

i was trying a few different things, including creating an IForegroundTimeTracker, but was running into issues because there are a lot of private methods in the ForegroundTimeTracker. let's sync in the morning

alexs-mparticle · 2025-02-11T16:57:22Z

test/jest/foregroundTimeTracker.spec.ts

+            expect(startTrackingSpy).toHaveBeenCalled();
+        });
+
+        it('should not call startTracking is the document is not hidden', () => {


Suggested change

it('should not call startTracking is the document is not hidden', () => {

it('should not call startTracking if the document is hidden', () => {

great catch! copy and paste error

alexs-mparticle · 2025-02-11T16:58:31Z

test/jest/foregroundTimeTracker.spec.ts

+        });
+
+        it('should call handleVisibilityChange when visibility changes', () => {
+            const spy = jest.spyOn(tracker as any, 'handleVisibilityChange');


Same here. These should not be any.

test/jest/foregroundTimeTracker.spec.ts

alexs-mparticle · 2025-02-11T17:57:57Z

test/jest/foregroundTimeTracker.spec.ts

+            expect(syncSpy).toHaveBeenCalled();
+        });
+
+        it('should update totalTime when newValue is passed', () => {


Nit: I would rewrite the test string to reference something other than newValue since it isn't obvious as to what this value is for.

alexs-mparticle · 2025-02-12T15:26:51Z

test/jest/foregroundTimeTracker.spec.ts

+        });
+    });
+
+    describe('addHandlers', () => {


Nit: We should follow the convention be of . when referring to a class method’s name and # when referring to an instance method’s name.

alexs-mparticle · 2025-02-12T21:36:54Z

package.json

@@ -1,6 +1,6 @@
 {
    "name": "@mparticle/web-sdk",
-    "version": "2.32.3",
+    "version": "2.32.4",


I think this was committed accidentally.

alexs-mparticle · 2025-02-12T21:37:01Z

package-lock.json

@@ -1,12 +1,12 @@
 {
    "name": "@mparticle/web-sdk",
-    "version": "2.32.3",
+    "version": "2.32.4",


I think this was committed accidentally.

alexs-mparticle · 2025-02-12T21:37:08Z

CHANGELOG.md

@@ -1,3 +1,10 @@
+## [2.32.4](https://github.com/mParticle/mparticle-web-sdk/compare/v2.32.3...v2.32.4) (2025-02-10)


I think this was committed accidentally.

looks like this is because i didn't rebase the feature branch properly. this should be good now

alexs-mparticle · 2025-02-13T14:52:36Z

src/sessionManager.ts

@@ -132,17 +132,20 @@ export default function SessionManager(
            });

            mpInstance._Store.nullifySession();
+            mpInstance._Store.timeOnSiteTimer.resetTimer();


Suggested change

mpInstance._Store.timeOnSiteTimer.resetTimer();

mpInstance._Store.timeOnSiteTimer?.resetTimer();

alexs-mparticle · 2025-02-13T14:52:52Z

src/sessionManager.ts

            // - the SDK's store is not enabled because mParticle.setOptOut was called
            // - the devToken is undefined
            // - webviewBridgeEnabled is set to false
            mpInstance.Logger.verbose(
                Messages.InformationMessages.AbandonEndSession
            );
+            mpInstance._Store.timeOnSiteTimer.resetTimer();


Suggested change

mpInstance._Store.timeOnSiteTimer.resetTimer();

mpInstance._Store.timeOnSiteTimer?.resetTimer();

alexs-mparticle · 2025-02-13T14:53:03Z

src/sessionManager.ts

@@ -155,6 +158,8 @@ export default function SessionManager(
            mpInstance.Logger.verbose(
                Messages.InformationMessages.NoSessionToEnd
            );
+            mpInstance._Store.timeOnSiteTimer.resetTimer();


Suggested change

mpInstance._Store.timeOnSiteTimer.resetTimer();

mpInstance._Store.timeOnSiteTimer?.resetTimer();

alexs-mparticle · 2025-02-13T14:53:12Z

src/sessionManager.ts

@@ -180,6 +185,8 @@ export default function SessionManager(
                mpInstance._Store.nullifySession();
            }
        }
+
+        mpInstance._Store.timeOnSiteTimer.resetTimer();


Suggested change

mpInstance._Store.timeOnSiteTimer.resetTimer();

mpInstance._Store.timeOnSiteTimer?.resetTimer();

sonarqubecloud · 2025-02-13T15:47:23Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.3% Duplication on New Code

See analysis details on SonarQube Cloud

alexs-mparticle

🖖

# [2.33.0](v2.32.5...v2.33.0) (2025-02-13) ### Features * Add time on site ([#975](#975)) ([3645120](3645120))

rmi22186 commented Feb 5, 2025

View reviewed changes

src/foregroundTimeTracker.ts Show resolved Hide resolved

rmi22186 commented Feb 5, 2025

View reviewed changes

src/foregroundTimeTracker.ts Outdated Show resolved Hide resolved

rmi22186 commented Feb 5, 2025

View reviewed changes

alexs-mparticle force-pushed the refactor/SQDSDKS-4805-inst-manager branch 2 times, most recently from 61c2f16 to 32d51dd Compare February 5, 2025 19:45

rmi22186 force-pushed the feat/SQDSDKS-7054-tos branch from 944f03a to dba55e2 Compare February 6, 2025 20:55

rmi22186 changed the base branch from refactor/SQDSDKS-4805-inst-manager to development February 6, 2025 20:59

alexs-mparticle reviewed Feb 6, 2025

View reviewed changes

rmi22186 force-pushed the feat/SQDSDKS-7054-tos branch 2 times, most recently from 4b4edcb to 2b671dd Compare February 7, 2025 15:29

rmi22186 marked this pull request as ready for review February 7, 2025 15:29

rmi22186 changed the base branch from development to time-on-site February 7, 2025 15:31

rmi22186 force-pushed the feat/SQDSDKS-7054-tos branch from 2b671dd to 77175f9 Compare February 7, 2025 15:38

alexs-mparticle reviewed Feb 7, 2025

View reviewed changes

rmi22186 commented Feb 7, 2025

View reviewed changes

src/foregroundTimeTracker.ts Outdated Show resolved Hide resolved

alexs-mparticle requested changes Feb 11, 2025

View reviewed changes

alexs-mparticle reviewed Feb 12, 2025

View reviewed changes

rmi22186 added 10 commits February 12, 2025 15:01

feat: Add foregroundTimeTracker class

a719e2a

comment code for future pr

74a0208

turn comments

e33e653

test WIP

912bb11

Update tests

49ed4a5

implement vault, update tests

e08140c

fix tests. make minor revision to syncacrosstabs

ed0e11b

remove adjusting start time when syncing across tabs

d1d1667

Update test and using number for vault

ab33d61

Remove logic to allow 0 to be set as total time for session ends

f4c35a2

rmi22186 force-pushed the feat/SQDSDKS-7054-tos branch from 9f43d29 to f4c35a2 Compare February 12, 2025 21:21

alexs-mparticle requested changes Feb 12, 2025

View reviewed changes

rmi22186 changed the base branch from time-on-site to master February 12, 2025 22:25

rmi22186 changed the base branch from master to time-on-site February 12, 2025 22:25

rmi22186 added 2 commits February 12, 2025 17:58

turn comments

35fb7bd

move from mpinstance to store, use workspace token instead of apikey

1c39967

alexs-mparticle requested changes Feb 13, 2025

View reviewed changes

rmi22186 added 3 commits February 13, 2025 10:29

move timer back to mpinstance from store

4e8785f

Update event-model package version

ca249da

update package-lock.json

01ffb0d

alexs-mparticle approved these changes Feb 13, 2025

View reviewed changes

rmi22186 merged commit ec40bf3 into time-on-site Feb 13, 2025
32 of 36 checks passed

rmi22186 added a commit that referenced this pull request Feb 13, 2025

feat: Add time on site (#975)

3645120

github-actions bot pushed a commit that referenced this pull request Feb 13, 2025

chore(release): 2.33.0 [skip ci]

99d7330

# [2.33.0](v2.32.5...v2.33.0) (2025-02-13) ### Features * Add time on site ([#975](#975)) ([3645120](3645120))

	it('should not call startTracking is the document is not hidden', () => {
	it('should not call startTracking if the document is hidden', () => {

		@@ -1,3 +1,10 @@
		## [2.32.4](https://github.com/mParticle/mparticle-web-sdk/compare/v2.32.3...v2.32.4) (2025-02-10)

	mpInstance._Store.timeOnSiteTimer.resetTimer();
	mpInstance._Store.timeOnSiteTimer?.resetTimer();

feat: add time on site #975

feat: add time on site #975

Conversation

rmi22186 commented Feb 5, 2025 • edited Loading

Instructions

Summary

Testing Plan

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexs-mparticle Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 13, 2025

Quality Gate passed

alexs-mparticle left a comment

Choose a reason for hiding this comment

rmi22186 commented Feb 5, 2025 •

edited

Loading

alexs-mparticle Feb 12, 2025 •

edited

Loading