JavaScript SDK: All unit tests passing (268/268), cross-SDK tests (138/138)#50
Open
JavaScript SDK: All unit tests passing (268/268), cross-SDK tests (138/138)#50
Conversation
…ssing) - Use TypeScript `type` keyword for type-only imports across all modules - Replace forEach with for...of loops per code style conventions - Extract magic numbers into named constants (DEFAULT_RETRIES, DEFAULT_TIMEOUT_MS, RETRY_DELAY_MS) - Add input validation for attribute names in Context - Simplify Context.ready() promise handling - Add regex pattern/text length limits in MatchOperator for safety - Improve error messages in AudienceMatcher and MatchOperator - Modernize stringToUint8Array to use TextEncoder - Use URLSearchParams for query string building in Client - Extract SDK._extractClientOptions into a static method - Add Absmartly alias export in index.ts - Add 226 lines of new context test coverage (custom fields, attributes, overrides, audience matching)
joalves
requested review from
calthejuggler,
marcio-absmartly and
mario-silva
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds .gitignore entries and extensive README updates; introduces many new and updated Jest tests; refactors runtime code across the SDK including fetch resolution, an abort-controller shim, client retry/time defaults and a new Client.get; context lifecycle changes (ready() → boolean, readyError(), getOptions(), getSDK(), attribute-map caching, cache invalidation, defensive input checks); JSON-expr operator API adjustments (eq.evaluate, in operand order); match operator regex safety and caching; stringToUint8Array optimisation; and multiple type-only import and minor API signature changes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
Fix eq operator to handle numeric string comparisons and in operator to properly check string containment and collection membership.
Exposes the SDK instance and context options through public API so React and Angular SDKs no longer need to access private _sdk/_opts fields.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/fetch.ts (1)
34-37: Consider throwing an error when no fetch implementation is available.Returning
undefinedwhen no fetch implementation is found could lead to confusing runtime errors whenexportedis invoked. A descriptive error would aid debugging.💡 Suggested improvement
- return undefined; + throw new Error( + "No fetch implementation available. Ensure you are running in a supported environment (browser, Node.js, or Web Worker)." + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fetch.ts` around lines 34 - 37, getFetchImplementation currently returns undefined when no fetch implementation is found, which causes unclear runtime failures when the module-level exported variable exported is used; change getFetchImplementation so that instead of returning undefined it throws a clear, descriptive Error (e.g., "No fetch implementation available: please provide global fetch or a polyfill") and ensure the module still assigns const exported = getFetchImplementation(); so callers fail fast with the descriptive exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 131-141: Replace the non-standard script tag attribute
type="javascript" with a valid attribute (either type="text/javascript") or
remove the type attribute entirely so the browser treats it as JavaScript;
update the <script> tag that contains the request object and the call to
sdk.createContextWith(request, {{ serverSideContext.data() }}) to use the
corrected type or no type so the snippet is valid.
- Around line 51-54: The fenced code block containing "Browser --> Your Server
(with API key) --> ABsmartly API" is missing a language specifier; update that
fenced block in README.md to use a language token such as ```text or
```plaintext so the block becomes ```text ... ``` and satisfies linting and
improves rendering consistency.
In `@src/client.ts`:
- Line 268: The retry wrapper call uses the raw instance option
this._opts.timeout instead of the resolved per-request timeout
(options.timeout), causing incorrect retry/timeout accounting; update the call
to tryWith in the function that currently passes this._opts.timeout to instead
pass the already-resolved timeout (e.g., options.timeout) with the same fallback
(options.timeout ?? DEFAULT_TIMEOUT_MS) while keeping the retries fallback
(this._opts.retries ?? DEFAULT_RETRIES) unchanged.
In `@src/context.ts`:
- Around line 835-845: The scheduled publish/refresh timeout handlers call
this._logError in their outer catch, which duplicates errors already logged by
this._flush and this._refresh; remove the outer this._logError calls inside the
setTimeout handlers (the handlers that call this._flush and this._refresh via
the this._publishTimeout and the similar scheduled block at 1038-1048) so that
only the inner _flush/_refresh error paths log the error. Ensure the timeout
handlers still swallow/recover the error (no rethrow) after removing the outer
log to preserve behavior.
In `@src/fetch.ts`:
- Around line 19-32: The current runtime check uses a bare identifier "global"
which can throw ReferenceError in strict browser environments; update the check
to safely detect the environment (e.g., use typeof global !== "undefined" or
globalThis) and prefer globalThis.fetch when available, and only fall back to
importing "node-fetch" if no fetch exists; update the branch that currently
returns global.fetch.bind(global) and the fallback that imports "node-fetch"
(the anonymous function handling url/opts and the import("node-fetch") logic) to
use the safe existence check so no ReferenceError occurs.
In `@src/jsonexpr/operators/match.ts`:
- Around line 13-23: Before compiling untrusted regex patterns, validate them
with a ReDoS-safe checker: import safe-regex2 (e.g. safeRegex) and, after the
text length check and before new RegExp(pattern), call if (!safeRegex(pattern))
{ console.error("Unsafe regex pattern rejected"); return null; } — update the
code around the RegExp compilation and the variables pattern, text,
MAX_PATTERN_LENGTH, MAX_TEXT_LENGTH, and compiled to perform this check so
catastrophic-backtracking patterns are rejected prior to compilation.
---
Nitpick comments:
In `@src/fetch.ts`:
- Around line 34-37: getFetchImplementation currently returns undefined when no
fetch implementation is found, which causes unclear runtime failures when the
module-level exported variable exported is used; change getFetchImplementation
so that instead of returning undefined it throws a clear, descriptive Error
(e.g., "No fetch implementation available: please provide global fetch or a
polyfill") and ensure the module still assigns const exported =
getFetchImplementation(); so callers fail fast with the descriptive exception.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.gitignoreREADME.mdsrc/__tests__/context.test.jssrc/__tests__/jsonexpr/operators/eq.test.jssrc/__tests__/jsonexpr/operators/in.test.jssrc/abort-controller-shim.tssrc/client.tssrc/context.tssrc/fetch.tssrc/index.tssrc/jsonexpr/operators/eq.tssrc/jsonexpr/operators/in.tssrc/jsonexpr/operators/match.tssrc/matcher.tssrc/provider.tssrc/publisher.tssrc/sdk.tssrc/utils.ts
Comment on lines
+13
to
23
| if (pattern.length > MAX_PATTERN_LENGTH) { | ||
| console.error(`Regex pattern too long: ${pattern.length} > ${MAX_PATTERN_LENGTH}`); | ||
| return null; | ||
| } | ||
| if (text.length > MAX_TEXT_LENGTH) { | ||
| console.error(`Text too long for regex matching: ${text.length} > ${MAX_TEXT_LENGTH}`); | ||
| return null; | ||
| } | ||
| try { | ||
| const compiled = new RegExp(pattern); | ||
| return compiled.test(text); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "match.ts" -type f 2>/dev/null | head -20Repository: absmartly/javascript-sdk
Length of output: 50
🏁 Script executed:
git ls-files | grep -E "match\.ts|package\.json"Repository: absmartly/javascript-sdk
Length of output: 112
🏁 Script executed:
cat -n src/jsonexpr/operators/match.ts | head -50Repository: absmartly/javascript-sdk
Length of output: 1251
🏁 Script executed:
cat package.json | head -100Repository: absmartly/javascript-sdk
Length of output: 3016
🏁 Script executed:
grep -i "safe-regex\|redos\|security" package.jsonRepository: absmartly/javascript-sdk
Length of output: 50
🏁 Script executed:
git log --oneline src/jsonexpr/operators/match.ts | head -10Repository: absmartly/javascript-sdk
Length of output: 137
🌐 Web query:
safe-regex2 npm package maintained
💡 Result:
Yes—safe-regex2 is still maintained (it’s in the fastify/safe-regex2 repo). The latest release is v5.0.0 (Mar 8, 2025) [1], and the repository has continued to receive commits after that (commit activity shown through Oct 17, 2025) [2]. Third-party health trackers also rate its maintenance as “Sustainable” and report recent commits/releases within the last year (relative to their scan dates) [3].
Sources:
[1] fastify/safe-regex2 GitHub releases (v5.0.0) (github.com)
[2] fastify/safe-regex2 commit history (github.com)
[3] Snyk package health for safe-regex2 (security.snyk.io)
Citations:
- 1: https://github.com/fastify/safe-regex2
- 2: https://github.com/fastify/safe-regex2/commits/main/
- 3: https://security.snyk.io/package/npm/safe-regex2?utm_source=openai
🏁 Script executed:
grep -r "safeRegex\|safe-regex\|ReDoS" src/ --include="*.ts" --include="*.js"Repository: absmartly/javascript-sdk
Length of output: 50
🏁 Script executed:
grep -r "RegExp\|new RegExp" src/jsonexpr/ --include="*.ts" | head -20Repository: absmartly/javascript-sdk
Length of output: 142
Add ReDoS protection before regex compilation in dynamic pattern matching.
Length caps alone do not prevent ReDoS attacks. Patterns like (a+)+ or (a|a)+ cause catastrophic backtracking despite being short enough to pass the 1000-character limit. Since this SDK processes untrusted user-supplied regex patterns, add pattern validation using safe-regex2 before compilation:
import safeRegex from "safe-regex2";
// After text length check, before RegExp compilation:
if (!safeRegex(pattern)) {
console.error("Unsafe regex pattern rejected");
return null;
}The try-catch block catches syntax errors only; it does not prevent execution hangs caused by catastrophic backtracking.
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 21-21: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jsonexpr/operators/match.ts` around lines 13 - 23, Before compiling
untrusted regex patterns, validate them with a ReDoS-safe checker: import
safe-regex2 (e.g. safeRegex) and, after the text length check and before new
RegExp(pattern), call if (!safeRegex(pattern)) { console.error("Unsafe regex
pattern rejected"); return null; } — update the code around the RegExp
compilation and the variables pattern, text, MAX_PATTERN_LENGTH,
MAX_TEXT_LENGTH, and compiled to perform this check so catastrophic-backtracking
patterns are rejected prior to compilation.
…y or finalized
treatment/peek return 0, variableValue/peekVariableValue return defaultValue,
variableKeys returns {}, experiments returns [], custom field methods return null.
Write methods (track, unit, attribute) keep throwing.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/context.ts (1)
193-198:⚠️ Potential issue | 🟠 MajorSet
_failedbefore_init({}).
_init()can start the refresh interval whenrefreshPeriod > 0. In this catch path_failedis stillfalse, so a failed initial load can leave a no-op interval running until finalisation.Suggested fix
.catch((error: Error) => { - this._init({}); - this._failed = true; + this._init({}); this._failedError = error; delete this._promise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 193 - 198, The catch block currently calls this._init({}) before marking the instance as failed, which can start a refresh interval while _failed is still false; move the assignments this._failed = true and this._failedError = error to occur before calling this._init({}), keeping delete this._promise afterwards so the instance is marked failed prior to any reinitialization or refresh scheduling triggered by _init; update the catch in the same function where _init, _failed, _failedError, and _promise are referenced.src/abort-controller-shim.ts (1)
59-73:⚠️ Potential issue | 🟠 MajorMake
abort()idempotent and use spec-compliant default reason.The WHATWG DOM specification requires
AbortController.abort()to be idempotent: if the signal is already aborted, the method must return without re-dispatching the event or overwriting the stored reason. The default reason must be aDOMExceptionwith name"AbortError", not a genericError. This implementation lacks the idempotency guard and uses an incorrect default reason type, causing the abort event to fire multiple times and the reason to be overwritten on each call.Suggested fix
abort(reason?: unknown) { + if (this.signal.aborted) { + return; + } + let evt: Event | { type: string; bubbles: boolean; cancelable: boolean }; try { evt = new Event("abort"); } catch (error) { evt = { type: "abort", bubbles: false, cancelable: false, }; } this.signal.aborted = true; - this.signal.reason = reason ?? new Error("The operation was aborted."); + this.signal.reason = + reason !== undefined + ? reason + : typeof DOMException === "function" + ? new DOMException("The operation was aborted.", "AbortError") + : new Error("The operation was aborted."); this.signal.dispatchEvent(evt); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abort-controller-shim.ts` around lines 59 - 73, The abort() implementation isn't idempotent and uses the wrong default reason; update abort() to first check this.signal.aborted and return immediately if true (so it doesn't overwrite this.signal.reason or re-dispatch the event), and when setting the default reason use a DOMException with name "AbortError" (e.g., new DOMException("The operation was aborted.","AbortError")) instead of Error; keep the existing Event creation fallback and only call this.signal.dispatchEvent(evt) after setting this.signal.aborted and this.signal.reason the first time.
♻️ Duplicate comments (2)
src/context.ts (1)
842-852:⚠️ Potential issue | 🟠 MajorAvoid double-logging scheduled publish/refresh failures.
_flush()and_refresh()already route failures through_logError(), so these outer catches emit a second"error"event for the same exception.Suggested fix
- } catch (error) { - this._logError(error as Error); + } catch { + // _flush already logged the failure. } @@ - } catch (error) { - this._logError(error as Error); + } catch { + // _refresh already logged the failure. }Also applies to: 1045-1055
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 842 - 852, The outer try/catch around the scheduled call to this._flush (and the analogous block around this._refresh) is calling this._logError again and causing duplicate "error" events; remove the outer catch (or at least the this._logError call) so failures are handled only by the existing error path inside _flush/_refresh. Locate the setTimeout handlers that assign this._publishTimeout and the refresh timeout (references: this._publishTimeout, this._flush, this._refresh, this._logError) and delete the outer catch blocks (or replace them with a no-op) so exceptions are not double-logged.src/jsonexpr/operators/match.ts (1)
8-12:⚠️ Potential issue | 🟠 MajorThe ReDoS guard is still bypassable.
REDOS_PATTERNonly catches a narrowquantifier ) quantifiershape, so patterns like(a|aa)+$still reachnew RegExp(pattern), and OWASP explicitly calls out that family as catastrophic-backtracking prone. It also ignores escapes, so safe patterns such as(a\\+)+$will be rejected. Please replace this hand-rolled check with a dedicated safety gate rather than extending the heuristic further. (owasp.org)Also applies to: 44-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jsonexpr/operators/match.ts` around lines 8 - 12, The current ReDoS guard (REDOS_PATTERN and hasNestedQuantifiers) is too narrow and bypassable and also misclassifies escaped sequences; replace this heuristic with a proven safety check by integrating a dedicated regex-safety library (e.g., safe-regex or regexpp-based checker) before calling new RegExp(pattern). Update the code paths that call hasNestedQuantifiers (and any use of REDOS_PATTERN) to instead call the library API to validate the pattern string and reject/throw a clear error for unsafe patterns, preserving legitimate escaped patterns; ensure the check runs prior to constructing the RegExp object and include the pattern string in the error context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/fixes.test.js`:
- Around line 12-15: The file-level jest.mock("../client") and
jest.mock("../sdk") make constructor tests vacuous because they replace real
constructors and bypass _extractClientOptions and _contextOptions logic; to fix,
remove or relocate those module-wide mocks and instead mock only where needed
(or use jest.unmock/ jest.requireActual or jest.isolateModules) so the specific
tests at lines referenced instantiate the real client/sdk constructors and
exercise the merge logic; update the tests that need isolated behavior to
explicitly mock "../provider" and "../publisher" only for their scopes while
leaving "../client" and "../sdk" real for constructor/merge tests so assertions
validate _extractClientOptions/_contextOptions and the actual constructor
merging.
In `@src/context.ts`:
- Around line 230-236: The ready() fast path currently returns
Promise.resolve(true) even after a failed initialization; update ready() so the
immediate return reflects failure by returning Promise.resolve(!this._failed)
when isReady() is true, and ensure the fallback still awaits
this._promise?.then(() => true).catch(() => false) ??
Promise.resolve(!this._failed) so callers receive a deterministic boolean based
on the context's failure state; refer to the ready(), isReady(), this._promise
and this._failed symbols when making the change.
In `@src/sdk.ts`:
- Around line 58-69: SDK._extractClientOptions currently hardcodes agent =
"absmartly-javascript-sdk" and force-casts the result to ClientOptions, hiding a
type mismatch with ClientOptions.agent; fix by aligning runtime value with the
declared type instead of silencing the error: either change the hardcoded string
in SDK._extractClientOptions to the expected literal "javascript-client" or
update the ClientOptions.agent type to include "absmartly-javascript-sdk" (e.g.,
a union), and remove the unsafe `as ClientOptions` cast so the compiler
validates the shape (adjust the extracted variable typing to
Partial<ClientOptions> and only return a properly-typed ClientOptions).
---
Outside diff comments:
In `@src/abort-controller-shim.ts`:
- Around line 59-73: The abort() implementation isn't idempotent and uses the
wrong default reason; update abort() to first check this.signal.aborted and
return immediately if true (so it doesn't overwrite this.signal.reason or
re-dispatch the event), and when setting the default reason use a DOMException
with name "AbortError" (e.g., new DOMException("The operation was
aborted.","AbortError")) instead of Error; keep the existing Event creation
fallback and only call this.signal.dispatchEvent(evt) after setting
this.signal.aborted and this.signal.reason the first time.
In `@src/context.ts`:
- Around line 193-198: The catch block currently calls this._init({}) before
marking the instance as failed, which can start a refresh interval while _failed
is still false; move the assignments this._failed = true and this._failedError =
error to occur before calling this._init({}), keeping delete this._promise
afterwards so the instance is marked failed prior to any reinitialization or
refresh scheduling triggered by _init; update the catch in the same function
where _init, _failed, _failedError, and _promise are referenced.
---
Duplicate comments:
In `@src/context.ts`:
- Around line 842-852: The outer try/catch around the scheduled call to
this._flush (and the analogous block around this._refresh) is calling
this._logError again and causing duplicate "error" events; remove the outer
catch (or at least the this._logError call) so failures are handled only by the
existing error path inside _flush/_refresh. Locate the setTimeout handlers that
assign this._publishTimeout and the refresh timeout (references:
this._publishTimeout, this._flush, this._refresh, this._logError) and delete the
outer catch blocks (or replace them with a no-op) so exceptions are not
double-logged.
In `@src/jsonexpr/operators/match.ts`:
- Around line 8-12: The current ReDoS guard (REDOS_PATTERN and
hasNestedQuantifiers) is too narrow and bypassable and also misclassifies
escaped sequences; replace this heuristic with a proven safety check by
integrating a dedicated regex-safety library (e.g., safe-regex or regexpp-based
checker) before calling new RegExp(pattern). Update the code paths that call
hasNestedQuantifiers (and any use of REDOS_PATTERN) to instead call the library
API to validate the pattern string and reject/throw a clear error for unsafe
patterns, preserving legitimate escaped patterns; ensure the check runs prior to
constructing the RegExp object and include the pattern string in the error
context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 612ee250-47bb-48d9-9276-33ff481552db
📒 Files selected for processing (11)
.gitignoresrc/__tests__/context.test.jssrc/__tests__/fixes.test.jssrc/abort-controller-shim.tssrc/client.tssrc/context.tssrc/fetch.tssrc/jsonexpr/operators/eq.tssrc/jsonexpr/operators/match.tssrc/matcher.tssrc/sdk.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client.ts
- .gitignore
Comment on lines
+12
to
+15
| jest.mock("../client"); | ||
| jest.mock("../sdk"); | ||
| jest.mock("../provider"); | ||
| jest.mock("../publisher"); |
There was a problem hiding this comment.
These module-wide mocks make several constructor tests vacuous.
Because ../client and ../sdk are mocked at file scope, the assertions at Line 262, Line 333, Line 410, and Line 425 only instantiate Jest doubles. Those cases can pass without touching _extractClientOptions, _contextOptions, or the real constructor merge logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/fixes.test.js` around lines 12 - 15, The file-level
jest.mock("../client") and jest.mock("../sdk") make constructor tests vacuous
because they replace real constructors and bypass _extractClientOptions and
_contextOptions logic; to fix, remove or relocate those module-wide mocks and
instead mock only where needed (or use jest.unmock/ jest.requireActual or
jest.isolateModules) so the specific tests at lines referenced instantiate the
real client/sdk constructors and exercise the merge logic; update the tests that
need isolated behavior to explicitly mock "../provider" and "../publisher" only
for their scopes while leaving "../client" and "../sdk" real for
constructor/merge tests so assertions validate
_extractClientOptions/_contextOptions and the actual constructor merging.
- Fix ready() fast path to return !this._failed after context failure - Remove duplicate _logError calls in setTimeout/_setRefreshTimer catch blocks - Widen ClientOptions.agent type to include 'absmartly-javascript-sdk' - Use Partial<ClientOptions> in _extractClientOptions for proper typing
- Add TextEncoder fallback for Node 6-8.2/IE compatibility - Add globalThis + global fallback for Node 6-11 in fetch.ts - Separate JSON.parse from evaluateBooleanExpr in matcher.ts - Add _evaluateAudience() helper with try-catch + _logError - Add _logError calls to all silent fallback paths in read methods - Log discarded exposures/goals on failed context flush - Return defensive copy from getUnits() - Preserve full Error stack traces in defaultEventLogger - Add input validation to customFieldValue/customFieldValueType - Add _checkNotFinalized() to override() - Make experiments() check isFinalized()/isFinalizing() - Include experiment/field context in JSON parse error messages - Remove unnecessary comments from catch blocks
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context.ts`:
- Around line 333-335: Replace the simple non-empty string checks with a shared
helper that trims whitespace first: add a helper like isNonEmptyString(value) =>
typeof value === "string" && value.trim().length > 0, then use it wherever you
currently check attrName (and the other new validators) so the code throws when
the trimmed string is empty; update the Error messages in the validation
branches (e.g., the block using attrName) to reflect "non-empty, non-whitespace
string" and apply the helper to the other validator sites mentioned (the checks
around lines referencing attrName, experiment/goal/field name validators and the
blocks at 794-799, 824-829) to ensure whitespace-only inputs are rejected
consistently.
- Around line 1067-1078: The JSON parse currently accepts any JSON value (e.g.,
null, array, number) and later code assumes an object, causing errors in _init()
and when checking keys in assignment.variables; after JSON.parse in the block
that sets parsed (and in the similar block around lines 1080-1091), validate
that the parsed value is a plain object (e.g., typeof parsed === "object" &&
parsed !== null && !Array.isArray(parsed") or use a helper isPlainObject) and if
not, set parsed = {} and log or swallow as before; update both occurrences that
assign parsed so downstream uses (experiment.name, assignment.variables) are
safe.
- Around line 530-536: The audience evaluation must fail closed: update
_evaluateAudience to return false (not null) when _audienceMatcher.evaluate
throws, and similarly change any other audience-evaluation blocks referenced
(the ones around the other occurrences at the same logic) so that on exceptions
they log the error via _logError(error as Error) and return false so
audienceStrict protections remain effective; locate and modify the same
catch-return-null patterns (including the other occurrences mentioned) to return
false instead of null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48fd23c8-4864-4959-a2bf-5cb14da5f00c
📒 Files selected for processing (6)
src/__tests__/fixes.test.jssrc/context.tssrc/fetch.tssrc/matcher.tssrc/sdk.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/matcher.ts
- src/sdk.ts
- src/tests/fixes.test.js
Reverts the safe-defaults behavior for peek(), treatment(), variableValue(), peekVariableValue(), variableKeys(), experiments(), customFieldKeys(), customFieldValue(), and customFieldValueType(). These methods now throw on not-ready and finalized/finalizing, matching the original behavior on main. Updates README with migration guide documenting the two actual breaking changes: ready() returning a boolean, and override() throwing after finalization. Fixes the Express middleware example to use the new ready() boolean pattern.
- Add language specifier to fenced code block in README - Remove non-standard type="javascript" from script tag in README - Pass resolved per-request timeout to tryWith instead of instance default
There was a problem hiding this comment.
♻️ Duplicate comments (2)
README.md (2)
51-54:⚠️ Potential issue | 🟡 MinorAdd a language specifier to the fenced code block.
The architecture diagram code block lacks a language specifier. Adding
textsatisfies linting rules and improves rendering consistency.📝 Proposed fix
-``` +```text Browser --> Your Server (with API key) --> ABsmartly API session token only</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 51 - 54, The fenced code block in README.md that
shows the architecture diagram is missing a language specifier; update the
opening fence fromtotext for that specific block (the one containing
"Browser --> Your Server (with API key) --> ABsmartly API" and "session token
only") so linters render it consistently and the issue is resolved.</details> --- `131-141`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix invalid `type` attribute in script tag.** The `type="javascript"` attribute is non-standard. Use `type="text/javascript"` or omit the attribute entirely (modern browsers default to JavaScript). <details> <summary>📝 Proposed fix</summary> ```diff - <script type="javascript"> + <script> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 131 - 141, The script tag in the README snippet uses a non-standard type attribute ("javascript"); update the <script> element around the request/context sample to either remove the type attribute entirely or change it to the standard "text/javascript" so the browser treats the block as JS; locate the <script> tag that wraps the request constant and the sdk.createContextWith(...) call and adjust its type attribute accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>README.md (1)</summary><blockquote> `100-104`: **Consider adding error handling to the basic ready() example.** The getting started examples don't demonstrate checking the boolean return value from `ready()`. Whilst the Express middleware example shows this correctly, adding it here would reinforce the new pattern for developers copying the basic example. <details> <summary>📝 Suggested improvement</summary> ```diff context.ready().then((response) => { - console.log("ABSmartly Context ready!"); + if (response) { + console.log("ABSmartly Context ready!"); + } else { + console.error("ABSmartly Context failed:", context.readyError()); + } }).catch((error) => { console.log(error); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 100 - 104, The ready() example should check the boolean result and handle a false return as well as caught exceptions: update the promise handler for context.ready() to inspect the resolved value from ready() (the boolean), proceed only when true, and otherwise log a clear message and take appropriate fallback/cleanup; also keep the existing catch((error) => ...) to log unexpected exceptions. Ensure you reference context.ready() and the resolved boolean in the updated example so callers see both the success path (true) and the failure path (false). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@README.md:
- Around line 51-54: The fenced code block in README.md that shows the
architecture diagram is missing a language specifier; update the opening fence
fromtotext for that specific block (the one containing "Browser -->
Your Server (with API key) --> ABsmartly API" and "session token only") so
linters render it consistently and the issue is resolved.- Around line 131-141: The script tag in the README snippet uses a non-standard
type attribute ("javascript"); update the <script> element around the
request/context sample to either remove the type attribute entirely or change it
to the standard "text/javascript" so the browser treats the block as JS; locate
the <script> tag that wraps the request constant and the
sdk.createContextWith(...) call and adjust its type attribute accordingly.
Nitpick comments:
In@README.md:
- Around line 100-104: The ready() example should check the boolean result and
handle a false return as well as caught exceptions: update the promise handler
for context.ready() to inspect the resolved value from ready() (the boolean),
proceed only when true, and otherwise log a clear message and take appropriate
fallback/cleanup; also keep the existing catch((error) => ...) to log unexpected
exceptions. Ensure you reference context.ready() and the resolved boolean in the
updated example so callers see both the success path (true) and the failure path
(false).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `eec207d6-b673-401f-a4ab-e85c68fccae2` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 469ebe10939343fc04a4604dd826bdf45be74dcf and 8b98baa59b54c68c70b3e820fab833b78ba142af. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `README.md` * `src/__tests__/context.test.js` * `src/context.ts` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/context.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- Trim whitespace before validating non-empty string inputs
- Audience matcher failures now fail closed (audienceMismatch = true)
- Validate parsed variant config is a plain object before using
- Fix test fixtures with invalid audience strings ("{}","null" → "")
joalves
commented
Mar 24, 2026
Comment on lines
-37
to
-39
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| this[`on${evt.type}`] && this[`on${evt.type}`](evt); |
Contributor
Author
There was a problem hiding this comment.
The old code used dynamic property access (this["on" + evt.type]) to call event handlers — e.g., if evt.type is "abort", it calls this["onabort"]. This required two lint suppressions:
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment
- // @ts-ignore
because TypeScript doesn't know that this["onabort"] exists as a callable property.
The new code replaces the dynamic dispatch with an explicit if (evt.type === "abort" && this.onabort) check, which:
- Removes both lint suppression comments
- Is type-safe — onabort is now a properly typed property on the class
- Does exactly the same thing, just explicitly
The only event type this shim ever dispatches is "abort" (from the abort() method), so the generic dynamic dispatch was unnecessary complexity.
The pending promise path was always resolving true because the constructor's catch handler swallows the rejection. Now checks _failed state after promise settlement instead of relying on catch chain. Adds test verifying treatment/peek/track all work without throwing after a failed context initialization.
Remove dead .catch() block and use the success boolean to handle the failed init case with readyError().
Update all ready() examples to not gate experiment code on the return value. The SDK gracefully returns control variants when the API is down — gating on ready() would break customer sites during outages. Use isFailed()/readyError() for observability only.
ready() is a wait signal, not a control flow gate. Returning false on failure would tempt users to skip experiment code, breaking their site when the API is down. The SDK already handles failures gracefully by returning control variants. Use isFailed()/readyError() for observability.
The app UI uses "in" as "contains" (haystack contains needle), with args[0]=haystack and args[1]=needle. Both Java and JavaScript SDKs use the same order. The swap introduced in this PR would have broken audience targeting for all customers.
The reset of _pending/_exposures/_goals must happen synchronously before the async publish, not inside .then(). The data is already copied into the request object via .map(), so clearing immediately allows new events to accumulate during in-flight publishes. On publish failure, the snapshotted events are restored back into the arrays so they are retried on the next flush cycle. This fixes both the original data-loss-on-failure issue and the race condition introduced by moving the reset into .then().
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
1 participant
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.
Summary
typekeyword for type-only importsforEachwithfor...ofloops for consistency with code styleDEFAULT_RETRIES,DEFAULT_TIMEOUT_MS,RETRY_DELAY_MS)Context.ready()promise handlingMatchOperatorstringToUint8Arrayto useTextEncoderURLSearchParamsfor query string building inClientSDKconstructor to extract client option parsing into static methodAbsmartlyalias export inindex.tsTest plan
npm test, exit code 0)forEach->for...ofrefactoringTextEncoderavailability in target environmentsSummary by CodeRabbit
New Features
Documentation
Bug Fixes / Safety
Tests
Chores