Fix EventTracker silently dormant in real browsers#1327
Conversation
`window.screen` and `window.history` are accessor properties on `Window.prototype`, so `Object.getOwnPropertyDescriptor(window, X)?.value` returned undefined in real browsers, causing `start()` to short-circuit and never capture or send any $page-view / $click events. Read the globals directly instead; the jsdom-based regression test pins the accessor-descriptor shape so this can't silently come back.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaced descriptor-based reads of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes Confidence Score: 5/5Safe to merge — targeted, well-tested fix for a clear root cause with no functional regressions. All four changed lines apply the identical, correct substitution. The new test suite explicitly asserts the accessor-descriptor shape in jsdom and end-to-end verifies that events are captured and sent after start(). No new logic is introduced; no existing paths are altered. All remaining observations are P2 or lower. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["EventTracker.start()"] --> B{"isBrowserLike()?"}
B -- No --> Z["return (no-op)"]
B -- Yes --> C{"addEventListener present?\nremoveEventListener present?\nhasScreenDimensions(window.screen)?"}
C -- "Any false\n(was always false in real browsers\nbefore this fix)" --> Z
C -- All true --> D["_started = true"]
D --> E["_setupPageViewCapture()"]
E --> F["_capturePageView('initial')\nuses window.screen directly"]
E --> G["historyObject = window.history\nmonkey-patch pushState / replaceState"]
D --> H["_setupClickCapture()"]
D --> I["_setupPageHideListeners()"]
D --> J["setInterval → _tick() every 10s"]
J --> K["fetch access token async"]
J --> L{"has token & events?"}
L -- Yes --> M["_flush() → sendBatch()"]
L -- No --> J
Reviews (1): Last reviewed commit: "fix" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts (2)
53-57: Avoid silently skipping the click dispatch in the test flow.Line 53 uses optional chaining and can no-op if the button lookup fails. Make this an explicit error so the test fails at the real setup fault.
♻️ Proposed fail-loud diff
- document.querySelector("button")?.dispatchEvent(new MouseEvent("click", { + const button = document.querySelector("button"); + if (button == null) { + throw new Error("Expected test button to exist before dispatching click."); + } + button.dispatchEvent(new MouseEvent("click", { bubbles: true, clientX: 12, clientY: 34, }));As per coding guidelines: "Use
?? throwErr(...)instead of non-null assertions or silent fallback values; include explicit error messages" and "Fail early, fail loud; fail fast with an error instead of silently continuing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts` around lines 53 - 57, The test currently uses optional chaining on document.querySelector("button") which can silently skip dispatching the click if the button is missing; change this to explicitly fail when the button isn't found by grabbing the element into a variable (e.g., const btn = document.querySelector("button")), assert or throw a clear Error if btn is null (use your throwErr helper if available), then call btn.dispatchEvent(...) so the test fails loudly on setup errors instead of silently no-oping.
14-23: Harden payload parsing to fail fast with explicit test errors.If no batch is sent,
JSON.parsecurrently fails with a generic parse error. Also, each event’sevent_typeis assumed but not asserted before mapping. Add explicit guards so failures point to the exact contract break.♻️ Proposed test-hardening diff
function getSentEventTypes(sentBodies: string[]) { const [body] = sentBodies; + if (body == null) { + throw new Error("Expected at least one analytics batch body to be sent."); + } - const payload = JSON.parse(body); + const payload: unknown = JSON.parse(body); if (typeof payload !== "object" || payload === null || !("events" in payload) || !Array.isArray(payload.events)) { throw new Error("Expected analytics batch payload to include an events array."); } - return payload.events.map((event) => event.event_type); + return payload.events.map((event) => { + if (typeof event !== "object" || event === null || !("event_type" in event) || typeof event.event_type !== "string") { + throw new Error("Expected each analytics event to include a string event_type."); + } + return event.event_type; + }); }As per coding guidelines: "Validate all assumptions through type system (preferred), assertions, or tests; ideally two out of three" and "Fail early, fail loud; fail fast with an error instead of silently continuing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts` around lines 14 - 23, The helper getSentEventTypes should fail fast with clear errors: check that sentBodies is non-empty and throw a descriptive error if no batch was sent, wrap JSON.parse in a try/catch to rethrow a clear "invalid JSON payload" error, assert that the parsed payload is a non-null object with an "events" array, and validate each event in payload.events has an event_type of type string (throwing an explicit error identifying the bad event/index) before returning the array of event_type values; update the function getSentEventTypes to perform these guards and throw meaningful messages when any assumption is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts`:
- Around line 53-57: The test currently uses optional chaining on
document.querySelector("button") which can silently skip dispatching the click
if the button is missing; change this to explicitly fail when the button isn't
found by grabbing the element into a variable (e.g., const btn =
document.querySelector("button")), assert or throw a clear Error if btn is null
(use your throwErr helper if available), then call btn.dispatchEvent(...) so the
test fails loudly on setup errors instead of silently no-oping.
- Around line 14-23: The helper getSentEventTypes should fail fast with clear
errors: check that sentBodies is non-empty and throw a descriptive error if no
batch was sent, wrap JSON.parse in a try/catch to rethrow a clear "invalid JSON
payload" error, assert that the parsed payload is a non-null object with an
"events" array, and validate each event in payload.events has an event_type of
type string (throwing an explicit error identifying the bad event/index) before
returning the array of event_type values; update the function getSentEventTypes
to perform these guards and throw meaningful messages when any assumption is
violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0c7bf8a-ab08-46bb-8626-edb13fd6c519
📒 Files selected for processing (1)
packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts
`payload.events` comes from `JSON.parse`, so after the narrow it's `any[]` and the `.map` callback parameter trips noImplicitAny in the generated SDK packages. Cast to a concrete element type before mapping.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts`:
- Around line 14-23: The helper getSentEventTypes should guard against an empty
sentBodies array and remove the unsafe type cast: first check sentBodies and
throw a clear error if no body is present, then JSON.parse the body into payload
and validate payload.events is an array; iterate payload.events and at runtime
verify each item has a string event_type (throwing a descriptive error if not)
before collecting event_type values—update references to sentBodies, payload,
events, and event_type accordingly and eliminate the `as` cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b43b4c0-1b31-4808-99e1-5b2567f1dc48
📒 Files selected for processing (1)
packages/template/src/lib/stack-app/apps/implementations/event-tracker.test.ts
2 participants
window.screenandwindow.historyare accessor properties onWindow.prototype, soObject.getOwnPropertyDescriptor(window, X)?.valuereturned undefined in real browsers, causingstart()to short-circuit and never capture or send any $page-view / $click events. Read the globals directly instead; the jsdom-based regression test pins the accessor-descriptor shape so this can't silently come back.Summary by CodeRabbit
Tests
Bug Fixes