Review follow-ups for #3661 (record_session)#3665
Open
jodeleeuw wants to merge 7 commits intoclaude/add-hifi-data-recording-jfxSPfrom
Open
Review follow-ups for #3661 (record_session)#3665jodeleeuw wants to merge 7 commits intoclaude/add-hifi-data-recording-jfxSPfrom
jodeleeuw wants to merge 7 commits intoclaude/add-hifi-data-recording-jfxSPfrom
Conversation
…Context
Three follow-ups from the PR review, all in `SessionRecorder`:
- Module-level refcount for `CanvasRenderingContext2D` and `Math.random`
patches. The previous per-recorder patching captured the *previous
recorder's wrapper* as "original" when two `JsPsych` instances ran
with `record_session: true` concurrently; restoring on stop in
interleaved order then reinstalled stale wrappers on the prototype.
The first recorder to register installs the wrapper; the last to
unregister restores the true original.
- Wrap `HTMLCanvasElement.prototype.getContext` to learn each canvas's
committed context type without ever calling `getContext` ourselves.
`getReadable2dContext` previously called `getContext("2d")` on
every tracked canvas, which permanently locks a fresh canvas into
2D — silently breaking any plugin that creates a canvas, attaches
it to the DOM, and later asks for a WebGL context. Canvases of
unknown or non-2d type now fall through to `toDataURL` snapshots.
- Preserve user-installed `Math.random` reassignments. If the user
replaces `Math.random` mid-session, the recorder's `unpatchMathRandom`
no longer clobbers the replacement on stop; it only restores the
pre-patch reference when our wrapper is still the installed function.
- Replace the `bind()` mutationObserver-presence sentinel with explicit
`bindSession`/`bindTrial` methods. The old code worked but only by
side-effect ordering between `attachSessionListeners` and
`attachTrialListeners`.
Adds three regression tests covering the canvas-context, mid-session
`Math.random` reassignment, and concurrent multi-instance scenarios.
All 54 record-session tests pass; full jspsych suite (417 tests) green.
The previous implementation kicked `writer.write` and `writer.close` without awaiting them, on the theory that awaiting would deadlock on backpressure. The reality is the opposite: a fully-asynchronous write + close runs concurrently with the read loop just fine, but a *bare* write whose promise is dropped surfaces any write/close error as an unhandled rejection. Capture the write/close in a single async IIFE and await it after draining the reader so errors propagate to the returned promise.
`getSessionRecording()` returns a `SessionRecording`, but consumers couldn't import the type from the package entry to type their storage code. Re-export the schema's public types alongside the existing `JsPsych` exports. Adds the missing changeset for the `record_session` feature so the release pipeline picks it up.
🦋 Changeset detectedLatest commit: 4bceaf4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…hers Follow-ups from a code-review pass over the recorder. Removed redundant local flags: - `canvasContextPatched` and `mathRandomPatched`. The module-level `register*Recorder()` helpers already early-return on duplicate registration; the per-instance flags were dead bookkeeping. The patch/unpatch wrapper methods inlined into `start()`/`stop()`. Tightened canvas snapshot scheduling: - Collapsed `scheduleInitialCanvasSnapshot` and `scheduleCanvasSnapshot` (which differed only by their `force` argument) into a single `scheduleCanvasSnapshot(force)` plus a `pendingCanvasSnapshotForce` bool for the in-flight RAF. If both forced and unforced fire in the same frame, forced wins (strict superset). Unified the three RAF-throttled flushers (mouse / scroll / input): - Replaced three `*RafScheduled` flags + three near-identical scheduling blocks with one `rafScheduled: Set<string>` and a `scheduleRafFlush(key, flush)` helper. Hot-path optimizations: - `notifyCanvasDraw` now short-circuits when the canvas is already marked dirty, eliminating ~99% of WeakMap writes during animated canvas redraws. - Hoisted `RNG_NO_ARGS` so each `Math.random` call no longer allocates a fresh empty `args` array. Misc: - Re-privatized `canvasTrackedElements` and `canvasDirty` (only `notifyCanvasDraw` reads them now; the wrapper goes through that method). - Replaced two near-identical media `addEventListener` / `removeEventListener` blocks with a `MEDIA_EVENTS` constant + loop, and merged `mediaTrackedElements` into the existing `mediaListeners` Map. - Pruned schema-restating comments and trimmed the `getSessionRecordingCompressed` JSDoc to one example. Net: +94 / -201 in `recording.ts` and `JsPsych.ts`. All 54 record-session tests pass; full workspace test suite (724 tests across 80 suites) green; `tsc --noEmit` clean.
The recorder kept five parallel WeakMaps keyed by `HTMLCanvasElement` plus a strong-ref `Set` for iteration. Cleanup paths had to remember each map by name; lookups in `snapshotCanvas` did 2-4 map gets per call. Replace all six with a single `Map<HTMLCanvasElement, CanvasState>` where `CanvasState` carries `lastSnapshot`, `lastSnapshotTime`, `shadowData`, `animationLastTime`, and `dirty`. The strong-ref Map is intentional — every entry is explicitly removed on canvas DOM removal (`releaseSubtree`) or trial end (`detachTrialListeners`), so there's no cleanup concern that would justify the parallel WeakMap+Set design. Iteration plus state lookup is now one structure. All 54 record-session tests pass; tsc clean.
…+ memory bound
`record_session` now accepts either a boolean (the existing API) or an
options object so experimenters can opt out of specific capture
categories or bound the recording's memory footprint:
initJsPsych({
record_session: {
capture_inputs: false, // skip form values (privacy)
capture_canvas: false, // skip pixel snapshots (perf)
capture_random: false, // leave Math.random alone (perf)
max_events: 100_000, // stop at this many recorded events
},
});
All flags default to `true`, so passing `record_session: true` or
`{}` retains the original behavior. Each `false` setting skips both
the listener attachment and the corresponding global patches:
- `capture_inputs: false` doesn't bind `input` / `change` listeners.
- `capture_canvas: false` doesn't register with the module-level
`CanvasRenderingContext2D` patches at all.
- `capture_random: false` doesn't register with the `Math.random`
patches.
`max_events` enforces a session-wide cap across per-trial events,
`rng_calls`, `viewport_changes`, and `stylesheet_events`. When the cap
is exceeded, the recorder schedules a `stop("memory_limit")` on the
next microtask (lets any in-flight synchronous batch complete) and
sets `end_reason: "memory_limit"` on the recording. Default is
`Infinity`, preserving existing behavior.
Schema changes: `end_reason` gains the `"memory_limit"` value;
`schema_version` stays at 1 since this is an additive union extension.
The new `RecordSessionOptions` type is exported from the package
entry alongside the other recorder types.
4 new tests (58 total record-session, 728 total workspace). tsc clean.
The single-file `modules/recording.ts` had grown to ~1840 lines holding schema types, options, internal constants, pure helpers, the global prototype-patch coordination, and the `SessionRecorder` class itself. Split into focused siblings under `modules/recording/`: index.ts (32 LOC) — public re-exports types.ts (194 LOC) — schema types only, no runtime options.ts (51 LOC) — RecordSessionOptions + resolver constants.ts (69 LOC) — internal numeric/string constants + CanvasState helpers.ts (162 LOC) — pure utilities (computeDiffBbox, serializeJson, …) global-patches.ts (174 LOC) — Math.random / Canvas refcounted patches SessionRecorder.ts (1240 LOC) — the main class The split avoids a circular import between `SessionRecorder` and `global-patches` by giving the patch module a minimal `RecorderHooks` interface (`notifyCanvasDraw` + `recordRngCall`) that the concrete class structurally satisfies. The public API is unchanged: `import ... from "./modules/recording"` (or `from "jspsych"`) resolves to `index.ts` which re-exports everything that was exported from the monolith. Tests + JsPsych.ts needed no import-path changes. All 728 workspace tests pass; tsc clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-ups for #3661, targeting that PR's branch directly so the diff is just the cleanup.
Summary
CanvasRenderingContext2DandMath.randompatches are now installed once at module level. The previous per-recorder approach captured a previous recorder's wrapper as "original" when twoJsPsychinstances ran withrecord_session: trueconcurrently, leaving stale wrappers on the prototype after interleaved stops.getContext("2d")calls until a context is committed. WrapHTMLCanvasElement.prototype.getContextto learn each canvas's committed context type without ever creating one ourselves. Fixes the WebGL footgun where the recorder's diff path would lock a fresh canvas into 2D and silently break a plugin's latergetContext("webgl")call.Math.randomreassignments. If user code replacesMath.randommid-session,unpatchMathRandomno longer clobbers the replacement on stop; it only restores the pre-patch reference when our wrapper is still installed.bind()mutationObserver-presence sentinel with explicitbindSession/bindTrialmethods.getSessionRecordingCompressed. Captures the write+close in an awaited promise so any failures propagate to the returned promise instead of becoming unhandled rejections.SessionRecordingand related schema types from the package entry so consumers can type their storage code.jspsych: minor).Test plan
npx jest tests/core/record-session.test.ts— 54/54 (51 original + 3 new regression tests for canvas-context, mid-sessionMath.randomreassignment, and concurrent multi-instance).npx tsc --noEmitclean.https://claude.ai/code/session_01WCetXEmRj6Y2cBVsvYz7vM
Generated by Claude Code