feat: add core interaction trace monitoring implementation#8
Conversation
16b76f7 to
aedc2a7
Compare
Augment Navigator interface with deviceMemory property from the Device Memory API for type-safe device capability detection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add complete interaction trace monitoring functionality: - Size bucket utilities for device capability categorization - Device info collection (memory/CPU) - Browser feature detection (LoAF/INP support) - InteractionTrace class for performance observation - Trace controller with init/sign APIs and enrollment support Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change import extensions from .mts to .mjs to eliminate the TS5096 warning during Rollup build. The @rollup/plugin-typescript sets noEmit: false internally which conflicts with allowImportingTsExtensions. Using .mjs extensions in imports works with moduleResolution: bundler without requiring allowImportingTsExtensions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Allow consumers to pass an AbortSignal to initInteractionTraceMonitor that automatically triggers cleanup when aborted. This enables integration with boot service patterns and lifecycle management. - Add abortSignal option to InteractionTraceConfig - Return no-op immediately if signal already aborted at init - Register abort listener that triggers idempotent cleanup - Remove listener after cleanup to prevent memory leaks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TraceDetails type with required name field - Remove separate name and context fields from TraceReport - Remove setContextGetter (context will be handled by React bindings) - signInteractionTrace(name, details) now merges name into details internally This aligns with the original todoist-web contract where details contains all trace metadata including the name. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
A duration of 0 is valid but was being treated as falsy. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f578183 to
1fa8961
Compare
- Rename `dispose()` to `cancel()` with clearer semantics: cancels the trace without creating measurements or calling the reporter - Add `clearMarks()` private method to clean up performance marks - Add public `isProcessed` and `isCancelled` getters for state inspection - Update trace-controller to use the new `cancel()` method - Fix test setup/teardown order in trace-controller tests to ensure performance.clearMarks stub is available during cancel() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1fa8961 to
329a92a
Compare
Change interaction trace lifecycle so traces are created automatically on pointerup events rather than when signInteractionTrace is called. signInteractionTrace now signs the most recent pending trace. - Add pointerup event listener that creates a new trace and cancels any previous pending traces - Add cleanup timer (1s) to remove processed/cancelled traces - Update signInteractionTrace to sign the last trace instead of creating one - Update tests to dispatch pointerup events before signing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cancelled traces serve no purpose after cancel() is called - observers are disconnected, timers cleared, and they cannot be signed or processed. Clearing the array immediately prevents memory buildup with rapid clicks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove `as TDetails` assertion on line 134 - the spread object is assignable to TraceDetails without casting - Document remaining reporter assertion as runtime-safe The reporter assertion cannot be removed without architectural changes (module-level variables cannot preserve generic type parameters), but is safe because TDetails extends TraceDetails and the actual details flow through correctly at runtime. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Explain that traces are created on pointerup events, not by signInteractionTrace() - Update TraceReport interface to match actual types (name inside details, correct device buckets) - Add Known Limitations section documenting keyboard interaction exclusion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Thank you for porting over the core implementation for interaction trace monitoring. The logic is well-structured and is accompanied by a thorough suite of tests, aligning well with the stated goals. There may be correctness issues in server-side rendering or Node environments that could cause failures.
Remove self-explanatory comments that merely restate what the code obviously does, improving test readability. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove 14 redundant tests that duplicate behavior already covered elsewhere: - Delete device-info.test.mts (trivial pass-through function) - Remove memoization tests from feature-detection (implementation detail) - Remove duplicate isBrowserSupported false cases - Remove getter tests from interaction-trace (implicitly tested) - Remove redundant enrollment, lifecycle, and abort signal tests from trace-controller Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Bloomca
left a comment
There was a problem hiding this comment.
Looks good, just a few small questions
| "lint-staged": { | ||
| "*.{js,ts,mjs,mts,tsx,json,md}": "biome check --write --no-errors-on-unmatched", | ||
| "*.{ts,mts,tsx}": "tsc --noEmit" | ||
| "*.{ts,mts,tsx}": "bash -c 'tsc --noEmit'" |
There was a problem hiding this comment.
I am curious why this change? I checked and I have bash on Windows, but to be honest I do not know if it is generally available, so why not just keep it as an npm command?
There was a problem hiding this comment.
It's to prevent lint-staged from passing file arguments to tsc, as that causes it to ignore tsconfig.json, but the lint-staged docs have a better way to do this: https://github.com/lint-staged/lint-staged#example-run-tsc-on-changes-to-typescript-files-but-do-not-pass-any-filename-arguments
| TraceReport, | ||
| TraceReporter, | ||
| } from './types.mts' | ||
| } from './types.mjs' |
There was a problem hiding this comment.
I am a bit confused by this change, shouldn't it stay types.mts?
There was a problem hiding this comment.
To use the mts extension in the import path, we'd need the allowImportingTsExtensions option enabled in tsconfig.json, however, that will cause rollup to fail with:
(!) [plugin typescript] @rollup/plugin-typescript TS5096: Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set.
So, because we're transpiling from ts -> js, we can't have this enabled. But, we should be able to go back to plain .ts extensions. I'll explore that when working on CI/CD and the release flow.
…config Move lint-staged config from package.json to a TypeScript config file using a function-based approach. When commands are returned from a function, lint-staged does not append staged filenames, ensuring tsc respects tsconfig.json settings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move duplicate PerformanceObserver and performance API mock implementations from test files into a reusable src/test-utils/performance-mocks.mts module. This reduces code duplication and provides a documented API for mocking browser Performance APIs that HappyDOM doesn't support. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@doistbot /review |
There was a problem hiding this comment.
This PR ports over the core logic for interaction trace monitoring, including the InteractionTrace class and the trace-controller, establishing a great foundation for standardizing our performance tooling. The review flagged a couple of potential issues with how the performance observers are handled, one related to ensuring runtime safety in unsupported browsers and another concerning the management of performance marks.
| }, LONG_FRAME_TIMEOUT) | ||
| }) | ||
|
|
||
| this.loafObserver.observe({ type: 'long-animation-frame' }) |
There was a problem hiding this comment.
[P1] Creating the LoAF/INP PerformanceObservers without checking support will throw in browsers that don’t support long-animation-frame or event entry types (e.g., Safari), which crashes on any interaction. Since feature detection exists in this PR, guard observeLongAnimationFrames()/observeNextPaint() with isLoafSupported()/isInpSupported() (or wrap observe in a try/catch) so unsupported browsers safely skip observers.
There was a problem hiding this comment.
@doistbot initInteractionTraceMonitor won't initialize if there's no support, so we don't need to feature-check each individual use case.
There was a problem hiding this comment.
Thanks for the clarification, Frankie. I understand the intention is for initInteractionTraceMonitor to act as the feature-detection guard before this class is ever used.
However, since InteractionTrace is an exported class, it could be instantiated directly elsewhere in the future, bypassing that check. This would re-introduce the crash in unsupported browsers because the constructor unconditionally creates the observers:
constructor(onComplete: TraceReporter<TDetails>) {
// ...
this.observeLongAnimationFrames()
this.observeNextPaint()
}To make the class more robust and prevent potential future issues, it would be best for it to be self-contained. Could we add the support checks within the observeLongAnimationFrames() and observeNextPaint() methods? This ensures InteractionTrace is always safe to instantiate, regardless of how it's imported or used.
There was a problem hiding this comment.
It's not meant to be a part of the public API and is not documented as a use case, so IMHO it's fair we only feature-check at the publicly documented entry point.
| if (this.frameTimeout) { | ||
| clearTimeout(this.frameTimeout) | ||
| } | ||
| performance.mark(this.markEndName) |
There was a problem hiding this comment.
[P2] The performance.mark(this.markEndName) call on each LoAF callback overwrites the previous end mark without clearing it first. While performance.mark with the same name creates a new mark entry rather than updating it, calling clearMarks(this.markEndName) before setting the new one would prevent accumulating multiple end marks in the performance buffer. In practice this is unlikely to cause issues since performance.measure uses the most recent mark, but it could leave stale entries in the buffer.
…erver Prevents accumulating multiple end marks in the performance buffer when the LoAF observer fires multiple times during a single interaction trace. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This PR ports over the core Interaction Trace implementation from Todoist, bringing over the
InteractionTraceclass, and thetrace-controllermodule. This does not yet include the React layer (just a think hook wrapper aroundsignInteractionTrace), which will come in a later PR.Note
Please don't be alarmed by the large diff 😅 the implementation mostly resembles the original implementation we have running on production for Todoist. I'll highlight the differences further below.
Example
Trace lifecycle
Differences compared to Todoist's internal implementation
todoist-web@doist/interaction-tracecancel()behaviorprocess(), creates measurementsuuidnpm packagecrypto.randomUUID()Cancelled traces in particular has a more significant change:
todoist-web:Controller:
Trace instance:
@doist/interaction-trace:Controller:
Trace instance:
Test plan
Nothing to test for for now. The next PR will be accompanied by a Todoist PR to test the integration.