feat: surface AIM log response via onResultsLogged callback

[devandrepascoa]

The engine already POSTs final results to the AIM endpoint but discarded the response, so consumers had no way to read the requestId it assigns. This adds an onResultsLogged(response) callback that fires with that response ({ requestId }) once logging completes — letting speed.cloudflare.com and Radar show/look up a test by its id instead of monkey-patching window.fetch. Kept separate from onFinish (logging happens after, and is optional/can fail). Additive, backwards-compatible.

[ask-bonk]

Summary

This PR surfaces the AIM logging endpoint's response (notably the assigned requestId) to consumers, instead of discarding it.

Changes:

• logAimResults becomes async: it now awaits the POST, and on a 2xx response parses & returns the JSON body (AimLogResponse). Returns undefined on network error, non-OK status, or non-JSON body. Still wrapped so it never throws.
• New public onResultsLogged callback on SpeedTestEngine, fired with the parsed response after the log request resolves.
• New exported AimLogResponse type.
• #logFinalResults rewritten from short-circuit && to an early-return guard, then .then()-chains the callback.
• New unit test tests/unit/logging/logFinalResults.test.ts (5 cases) covering POST, parsed body, non-OK, rejection, and non-JSON.

Verification: I ran pnpm install, pnpm test:unit (98 passing), and pnpm lint (clean) locally — all green. The change is additive and backwards-compatible; the rationale for a separate callback (vs. folding into onFinish) is sound and preserves onFinish semantics.

Overall this is a clean, well-tested change. A few minor points are noted inline — none are blocking.

[ask-bonk]

Review posted successfully to PR #123 with 5 inline comments. Here's a recap of my review.

Review summary

What the PR does: Surfaces the AIM logging endpoint's response (the assigned requestId) to consumers. Previously logAimResults was fire-and-forget and discarded the response.

• logAimResults → async; awaits the POST, parses the JSON body on 2xx, returns AimLogResponse | undefined (undefined on network error / non-OK / non-JSON). Still never throws.
• New public onResultsLogged callback on SpeedTestEngine, fired with the parsed response.
• Exports new AimLogResponse type.
• #logFinalResults switched from && short-circuit to an early-return guard, then .then()-chains the callback.
• Adds a 5-case unit test.

Verification: pnpm test:unit (98 passing), pnpm lint (clean), pnpm build — all green locally. Change is additive and backwards-compatible. No double-logging issue (the onFinish setter replaces the constructor's super.onFinish).

Inline issues flagged (all minor, none blocking):

1. Doc/behavior mismatch (onResultsLogged JSDoc) — says "fires when the request succeeds," but the real gate is a truthy parsed body: a successful empty/non-JSON 2xx won't fire; a {} body will fire with no requestId.
2. Unhandled rejection risk — the .then() isn't .catch()-ed; a consumer's throwing onResultsLogged becomes an unhandled rejection. Suggest wrapping the callback in try/catch.
3. Style nit — AimLogResponse is both imported and separately re-exported in index.ts; could consolidate.
4. Robustness — response.json() as AimLogResponse trusts the body is an object; a bare JSON string/number would propagate. Optional strictness guard.
5. Test gap — tests assert URL/method but not the POST body's formatted payload; suggest asserting one representative field.

No commits were pushed.

github run

[devandrepascoa]

/bonk

[ask-bonk]

Summary

Surfaces the AIM logging endpoint's response (notably the assigned requestId) to consumers instead of discarding it.

Changes (current head, incl. commit 29ae2c9):

• logAimResults is now async. It awaits the POST and, on a 2xx, returns the parsed JSON body typed as AimLogResponse. On network error, non-OK status, or non-JSON body it returns { requestId: undefined }. Still wrapped so it never throws.
• New public onResultsLogged callback on SpeedTestEngine, fired with the parsed response after the log request resolves. #logFinalResults switched from && short-circuit to an early-return guard, then .then()-chains the callback.
• New exported AimLogResponse type.
• New unit test tests/unit/logging/logFinalResults.test.ts (POST, parsed body, non-OK, rejection, non-JSON).
• e2e test reworked to point logAimApiUrl at a stubbed /__results and assert onResultsLogged fires with the stubbed requestId.
• eslint: adds a tests/**/*.ts block declaring Response (and the existing polyfill set) so the e2e new Response(...) usage passes eslint-plugin-compat.

Assessment: The design is sound and backwards-compatible (additive). The latest commit changing logAimResults to always resolve with an AimLogResponse (never undefined) resolves the earlier doc/behavior-mismatch concern: onResultsLogged now fires on every logging attempt, matching the JSDoc. The double-logging concern is also fine — the onFinish setter replaces the constructor's super.onFinish, so logging happens exactly once.

A few minor points inline; none are blocking.

[ask-bonk]

Review posted to PR #123 with 5 inline comments: #123 (review)

What the PR changes

It surfaces the AIM logging endpoint's response (the assigned requestId) to consumers, which was previously discarded.

• src/logging/logFinalResults.ts — logAimResults becomes async: awaits the POST and returns the parsed JSON body typed as AimLogResponse on 2xx, or { requestId: undefined } on network error / non-OK / non-JSON. Never throws.
• src/index.ts — adds a public onResultsLogged callback fired with the parsed response; #logFinalResults switches from && short-circuit to an early-return guard and .then()-chains the callback. Exports the new AimLogResponse type.
• Tests — new unit test (5 cases) and a reworked e2e test that stubs /__results and asserts onResultsLogged fires with the stubbed requestId.
• eslint.config.js — adds a tests/**/*.ts block declaring Response so the e2e new Response(...) passes eslint-plugin-compat.

Notable: the review on file is partly stale

The prior ask-bonk review predates commit 29ae2c9 ("onResultsLogged always receives AimLogResponse"). That commit makes logAimResults always resolve with an AimLogResponse (never undefined) and #logFinalResults no longer has a truthy if (response) gate. So two earlier comments are now resolved and I did not re-raise them:

• The doc/behavior mismatch is gone — onResultsLogged now fires on every logging attempt, matching the JSDoc.
• No double-logging — the onFinish setter replaces the constructor's super.onFinish, so logging runs exactly once.

Issues I flagged (all minor, none blocking)

1. Unhandled rejection (index.ts:766) — .then() has no .catch(); a throwing consumer onResultsLogged becomes an unhandled rejection. Suggested a try/catch.
2. Cast robustness (logFinalResults.ts:140) — as AimLogResponse trusts the body is an object; a bare JSON string/number/array propagates as a non-object.
3. Style nit (index.ts:16) — AimLogResponse is both imported and separately re-exported from the same module.
4. Test gap (unit test) — POST body payload isn't asserted; suggested checking one representative field.
5. e2e robustness — the test can hang to the 120s Vitest timeout if onResultsLogged stops firing, and the onFinish resolve path doesn't restore the patched global fetch.

No commits were pushed.

github run

[devandrepascoa]

/bonk

[devandrepascoa]

Thanks @ask-bonk — addressed in fd43cc4 (and 29ae2c9). Recap per finding:

✅ Fixed

• AimLogResponse imported + re-exported (src/index.ts): now re-exported via the local import (export type { AimLogResponse };) instead of a second path re-export.
• Unit test didn't assert the POST body: added expect(JSON.parse(init.body).totalDurationMs).toBe(1234) so a regression in logData formatting is caught.
• e2e could hang on a regression: the onResultsLogged wait is now bounded by a 15s timeout that starts after onFinish (the timer is cleared on settle and restoreFetch() runs in a finally), so a future regression fails fast instead of hanging to the test timeout.

✅ Already resolved

• Doc/behavior mismatch on onResultsLogged: fixed in 29ae2c9 — logAimResults now always resolves with an AimLogResponse ({ requestId: undefined } on failure) and #logFinalResults no longer gates on a truthy body, so the JSDoc ("fires after the log request completes, success or failure") now matches behavior.

❌ Intentionally not changed

• Unhandled-rejection if a consumer's onResultsLogged throws: declining for now to stay consistent with the engine's other callbacks (onFinish, onResultsChange), which are also invoked unwrapped. Happy to revisit if we want to wrap all consumer callbacks uniformly.
• as AimLogResponse cast / non-object body validation: declining — the AIM endpoint always returns a JSON object, so runtime shape-validation is disproportionate for best-effort telemetry. (The only value that would throw on the consumer's { requestId } destructure is null, which the endpoint never returns.)

[ask-bonk]

Summary

Surfaces the AIM logging endpoint's response (notably the assigned requestId) to consumers, which was previously discarded by the fire-and-forget logAimResults.

Changes (current head fd43cc4):

• src/logging/logFinalResults.ts — logAimResults is now async. It awaits the POST and, on a 2xx, returns the parsed JSON body typed as AimLogResponse; on network error / non-OK / non-JSON it returns { requestId: undefined }. Still wrapped so it never throws.
• src/index.ts — new public onResultsLogged callback fired with the parsed response. #logFinalResults switched from && short-circuit to an early-return guard, then .then()-chains the callback. Exports the new AimLogResponse type.
• New unit test tests/unit/logging/logFinalResults.test.ts (5 cases) — now also asserts the POST body's totalDurationMs.
• e2e test reworked to stub /__results and assert onResultsLogged fires with the stubbed requestId, with a 15s Promise.race guard and try/finally fetch restore.
• eslint.config.js — adds a tests/**/*.ts block declaring Response.

Verification: Ran locally — pnpm test:unit (98 passing), pnpm lint (clean), pnpm build (succeeds). Design is additive and backwards-compatible; the separate-callback rationale (vs. folding into onFinish) is sound. Double-logging is not an issue — the subclass onFinish setter replaces the constructor's super.onFinish, so logging runs exactly once.

Two prior review points are now resolved at this head and I did not re-raise them: the export duplication (line 16/775 is a single local re-export now) and the e2e fetch-restore leak (handled by try/finally).

A few minor points inline; none are blocking.

[ask-bonk]

Review posted to PR #123 with 3 inline comments: #123 (review)

What the PR changes

Surfaces the AIM logging endpoint's response (the assigned requestId) to consumers, which logAimResults previously discarded (fire-and-forget).

• src/logging/logFinalResults.ts — logAimResults becomes async: awaits the POST and returns the parsed JSON body typed AimLogResponse on 2xx, or { requestId: undefined } on network error / non-OK / non-JSON. Never throws.
• src/index.ts — adds the public onResultsLogged callback; #logFinalResults switches from && short-circuit to an early-return guard and .then()-chains the callback. Exports AimLogResponse.
• Tests — new unit test (5 cases, now also asserts the POST body) and a reworked e2e test that stubs /__results and asserts onResultsLogged fires with the stubbed requestId.
• eslint.config.js — adds a tests/**/*.ts polyfill block declaring Response.

Verification

Ran locally: pnpm test:unit (98 passing), pnpm lint (clean), pnpm build (succeeds). Design is additive/backwards-compatible. Confirmed no double-logging (the subclass onFinish setter replaces the constructor's super.onFinish).

Issues flagged (all minor, none blocking)

1. Unhandled rejection (index.ts:768) — the .then() has no .catch(); a throwing consumer onResultsLogged becomes an unhandled rejection. Suggested a try/catch around the callback.
2. Cast robustness (logFinalResults.ts:140) — as AimLogResponse trusts an object body; a bare JSON null would make a consumer's { requestId } destructure throw. Suggested an object guard.
3. Dead eslint config (eslint.config.js:70) — the new tests/**/*.ts block is never exercised because the lint script only globs src/**/*.ts and *.json. New finding not in prior reviews — either widen the lint glob or drop the block.

I did not re-raise two earlier points that are resolved at the current head: the AimLogResponse export duplication (now a single local re-export) and the e2e fetch-restore leak (handled by try/finally).

No commits were pushed.

github run