React SDK: All tests passing (73/73 unit, 99/138 cross-SDK)#31
Open
React SDK: All tests passing (73/73 unit, 99/138 cross-SDK)#31
Conversation
- Initialize state with real values when context is already ready - Skip useEffect async fetch when context is pre-loaded (SSR) - Extract getVariantAndVariables helper for code reuse - Prevent unnecessary state updates on server-rendered pages
- Add withABSmartly HOC tests - Add useTreatment hook edge case tests - Add utility function tests - Add SSR support tests - Add SDKProvider edge case tests - Add integration scenario tests Total: 44 new tests added, all 73 tests pass (99.48% coverage)
- Fix SSR support for Treatment components and useTreatment hook - Add ErrorBoundary component for graceful error handling - Add useContextReady hook for async context readiness - Improve type safety across all components - Update convertLetterToNumber utility - 73/73 unit tests passing across 8 test suites - 99/138 cross-SDK tests passing
WalkthroughThis pull request introduces error handling and improved context management for the ABsmartly React SDK. Key additions include a new ErrorBoundary component, an optional context hook (useOptionalABSmartly), and a context readiness hook (useContextReady). The SDKProvider, Treatment, and TreatmentFunction components are refactored to support server-side rendering and better error states. Dependencies are updated to newer Vitest versions, TypeScript strictness is enabled in the compiler configuration, comprehensive test coverage is added for SSR scenarios and component integration, and the README documentation is significantly expanded with examples and API references. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
Fix ErrorBoundary, SDKProvider, and TreatmentFunction components. Update tsconfig and package-lock.
…ccess Replace (context as any)["_sdk"] and (context as any)._opts with context.getSDK() and context.getOptions() from the JS SDK public API.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/SDKProvider.test.tsx (1)
1-302:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting issues.
The pipeline indicates Prettier found code style issues in this file. Run
prettier --write tests/SDKProvider.test.tsxto fix formatting before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SDKProvider.test.tsx` around lines 1 - 302, Prettier flagged formatting issues in tests/SDKProvider.test.tsx; run the formatter or apply equivalent edits to restore style (e.g., spacing, trailing commas, import ordering) and re-run the linter. Specifically, format the file containing the SDKProvider and useABSmartly tests and mocks (symbols: SDKProvider, useABSmartly, mockCreateContext, mockCreateContextWith, TestComponent) by running prettier --write tests/SDKProvider.test.tsx (or your project's formatting command) so the test file matches the repository Prettier rules before merging.
🧹 Nitpick comments (19)
src/types.ts (1)
57-57: TypeCharsignificantly widened.Changing
Charfrom a specific union of literal characters tostring | numberis a significant type relaxation. While this simplifies the type, it loses the compile-time safety of only allowing specific character values. If other code relied on the stricter typing, this could mask bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` at line 57, The Char type was overly widened to string | number which loses compile-time safety; change the Char alias back to the original narrow union of allowed character literals (or introduce a new distinct alias like BroadChar = string | number and keep Char as the specific literal union) so that functions/types depending on Char (the Char type) retain strict validation—update the Char definition accordingly and adjust any call sites to use BroadChar only where truly needed.tsconfig.json (1)
3-23: Redundant strict mode options.With
"strict": trueenabled on line 3, the following options are automatically enabled and thus redundant:
strictNullChecks(line 19)strictFunctionTypes(line 20)strictBindCallApply(line 21)strictPropertyInitialization(line 22)alwaysStrict(line 23)While not harmful, these explicit declarations are unnecessary and could be removed for cleaner configuration.
♻️ Suggested simplification
{ "compilerOptions": { "strict": true, "skipLibCheck": true, "outDir": "lib/esm", "allowJs": true, "module": "esnext", "target": "es5", "lib": ["es6", "dom", "es2016", "es2017", "es2019"], "jsx": "react", "declaration": true, "moduleResolution": "node", "noUnusedLocals": true, "noUnusedParameters": true, "esModuleInterop": true, "noImplicitReturns": true, - "noImplicitThis": true, - "noImplicitAny": true, - "strictNullChecks": true, - "strictFunctionTypes": true, - "strictBindCallApply": true, - "strictPropertyInitialization": true, - "alwaysStrict": true, "suppressImplicitAnyIndexErrors": true, "allowSyntheticDefaultImports": true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 3 - 23, Remove the redundant explicit strict-mode flags since "strict": true already enables them; delete the entries "strictNullChecks", "strictFunctionTypes", "strictBindCallApply", "strictPropertyInitialization", and "alwaysStrict" from the tsconfig so only "strict": true remains as the single source of truth for those behaviors.tests/SDKProvider.test.tsx (1)
34-64: Consider extracting duplicated mock context factory.The mock implementations for
mockCreateContext(lines 34-48) andmockCreateContextWith(lines 50-64) are nearly identical. Consider extracting a shared factory function to reduce duplication:♻️ Suggested refactor
+const createMockContextInstance = () => ({ + data: vi.fn().mockReturnValue(mockContextData), + isReady: vi.fn().mockReturnValue(true), + isFailed: vi.fn().mockReturnValue(false), + treatment: vi.fn().mockReturnValue(0), + ready: vi.fn().mockResolvedValue(undefined), + attributes: vi.fn(), + variableKeys: vi.fn().mockReturnValue({}), + peekVariableValue: vi.fn(), + finalize: vi.fn().mockResolvedValue(undefined), + getOptions: vi.fn().mockReturnValue(mockContextOptions), + getSDK: vi.fn().mockReturnValue({}), +}); + -const mockCreateContext = vi.fn().mockImplementation(function () { - return { - data: vi.fn().mockReturnValue(mockContextData), - isReady: vi.fn().mockReturnValue(true), - // ... etc - }; -}); +const mockCreateContext = vi.fn().mockImplementation(createMockContextInstance); -const mockCreateContextWith = vi.fn().mockImplementation(function () { - return { - data: vi.fn().mockReturnValue(mockContextData), - isReady: vi.fn().mockReturnValue(true), - // ... etc - }; -}); +const mockCreateContextWith = vi.fn().mockImplementation(createMockContextInstance);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SDKProvider.test.tsx` around lines 34 - 64, The two mocks mockCreateContext and mockCreateContextWith are duplicated; extract a single factory (e.g., buildMockContext or createMockContextFactory) that returns the shared mock object (data, isReady, isFailed, treatment, ready, attributes, variableKeys, peekVariableValue, finalize, getOptions, getSDK) and then have mockCreateContext and mockCreateContextWith both use vi.fn().mockImplementation(buildMockContext); optionally accept an overrides parameter in the factory to customize per-test behavior without duplicating the whole implementation.src/hooks/useContextReady.ts (1)
31-33: Attributes are set on every effect re-run.When any dependency changes (context, attributes, name, isContextReady, onReady), attributes will be re-applied to the context. If
context.attributes()has side effects or is not idempotent, this could cause unintended behaviour.Consider guarding with a ref to track if attributes have already been applied:
♻️ Suggested guard using ref
+import { useEffect, useState, useRef } from "react"; import { Context } from "@absmartly/javascript-sdk"; export const useContextReady = ({ context, name, attributes, onReady, }: UseContextReadyOptions): UseContextReadyResult => { const isContextReady = context.isReady(); const [loading, setLoading] = useState<boolean>(!isContextReady); const [error, setError] = useState<Error | null>(null); const [initiallyReady] = useState(isContextReady); + const attributesApplied = useRef(false); useEffect(() => { let cancelled = false; - if (attributes) { + if (attributes && !attributesApplied.current) { context.attributes(attributes); + attributesApplied.current = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useContextReady.ts` around lines 31 - 33, In useContextReady, avoid calling context.attributes(attributes) on every effect run by adding a guard: in the useContextReady hook keep a ref (e.g., appliedRef or prevAttributesRef) and only call context.attributes(attributes) when attributes are present and either they haven't been applied yet (appliedRef is false) or the attributes value actually changed (compare current attributes to prevAttributesRef, e.g., shallow/equality check or JSON.stringify for simple objects); after applying, set the ref so subsequent effect runs don’t re-apply unless attributes change. This touches the useContextReady hook and the context.attributes(attributes) call.tests/TreatmentFunction.test.tsx (3)
179-204: Mock resetContext should return a Promise.Per the
ABSmartlytype definition insrc/types.ts,resetContextreturnsPromise<void>. The current mock returnsundefined, which could cause issues if code awaits the result.Proposed fix
mockedUseOptionalABSmartly.mockReturnValue({ context: mocks.context, sdk: null as unknown as SDK, - resetContext: () => {}, + resetContext: vi.fn().mockResolvedValue(undefined), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TreatmentFunction.test.tsx` around lines 179 - 204, The test's mockedUseOptionalABSmartly return value uses resetContext that currently returns undefined but must return a Promise<void> per the ABSmartly type; update the mock in this test so resetContext is an async function or a function that returns a resolved Promise (e.g., async () => {} or () => Promise.resolve()), keeping the rest of the mocked object (context, sdk) unchanged and ensuring any other tests that rely on this mock are updated similarly; target the mockedUseOptionalABSmartly mock return in this spec to make resetContext return a Promise.
104-107: Unused variableattributes.The
attributesvariable is declared but never used in this test. Consider removing it if it's no longer needed.Proposed fix
- const attributes = { - attr1: 15, - attr2: 50, - }; - render(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TreatmentFunction.test.tsx` around lines 104 - 107, Remove the unused local variable declaration "attributes" from the test to eliminate the unused-variable warning; locate the const attributes = { attr1: 15, attr2: 50 } in the TreatmentFunction.test.tsx test and either delete that declaration entirely or (if intended) replace its removal by actually using "attributes" in the relevant test setup or function call (e.g., pass it into the function under test or mock setup) so the variable is referenced.
83-83: Unused variableconfig.The
configvariable on line 83 (and similarly on line 130) is declared but never used. Consider removing these if they're no longer needed after refactoring.Proposed fix
- const config = { a: 1, b: 2 }; mocks.context.isReady.mockReturnValue(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TreatmentFunction.test.tsx` at line 83, Remove the unused const config declarations (variable name: config) introduced in the test file—either delete both unused declarations or use the variable where intended; search for the two occurrences of "const config = { a: 1, b: 2 }" in the test and remove them if they are vestigial after refactor, or replace them with the actual fixture/argument the tests expect.README.md (1)
26-28: Add language specifier to fenced code block.The code block showing the proxy pattern lacks a language specifier, which triggers a markdownlint warning.
Proposed fix
- ``` +- ```text Browser --> Your Backend (with secret key) --> ABSmartly API</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 26 - 28, The fenced code block containing the proxy
pattern line "Browser --> Your Backend (with secret key) --> ABSmartly API" is
missing a language specifier; update that triple-backtick fence to include a
language (e.g.,text) so Markdown linters stop warning—modify the block around that arrow string in README.md to start withtext and keep the same
content and closing ``` unchanged.</details> </blockquote></details> <details> <summary>tests/utils.test.ts (1)</summary><blockquote> `57-71`: **Consider adding tests for invalid input handling.** Based on the implementation in `src/utils/convertLetterToNumber.ts`, invalid inputs (multi-character strings, special characters) return `0` with a console warning. Adding tests for these edge cases would improve coverage. <details> <summary>Suggested additional tests</summary> ```typescript describe("invalid input handling", () => { it("should return 0 for multi-character strings", () => { expect(convertLetterToNumber("ab" as any)).toBe(0); }); it("should return 0 for special characters", () => { expect(convertLetterToNumber("!" as any)).toBe(0); expect(convertLetterToNumber("@" as any)).toBe(0); }); it("should return 0 for empty string", () => { expect(convertLetterToNumber("" as any)).toBe(0); }); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/utils.test.ts` around lines 57 - 71, Add tests for invalid input handling for convertLetterToNumber: create a new "invalid input handling" describe block that asserts multi-character strings (e.g., "ab"), special characters (e.g., "!", "@"), and empty string all return 0 when passed to convertLetterToNumber, and optionally spy on/expect console.warn to have been called for those cases to verify the warning behavior; reference the function name convertLetterToNumber and add these tests alongside the existing "edge cases" tests in tests/utils.test.ts. ``` </details> </blockquote></details> <details> <summary>tests/withABSmartly.test.tsx (2)</summary><blockquote> `2-2`: **Unused import.** `forwardRef` is imported but not used in any test. Consider removing it to keep imports clean. <details> <summary>🧹 Remove unused import</summary> ```diff -import React, { forwardRef, ComponentType } from "react"; +import React, { ComponentType } from "react"; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/withABSmartly.test.tsx` at line 2, Remove the unused forwardRef import from the React import line in the tests/withABSmartly.test.tsx file (leave React and ComponentType intact); locate the import statement that currently reads something like "import React, { forwardRef, ComponentType } from 'react'" and delete only forwardRef to clean up unused imports. ``` </details> --- `32-33`: **Mock relies on private implementation details.** The mock returns `_opts` and `_sdk` which are private/internal properties. This couples the test to implementation details and may break if the SDK's internal structure changes. Consider whether these properties are actually needed for the tests, or if the mock can be simplified. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/withABSmartly.test.tsx` around lines 32 - 33, The test mock in withABSmartly.test.tsx exposes internal SDK properties _opts and _sdk which couples tests to implementation details; remove or replace them with only the public API the test uses (e.g., methods or return values the code under test calls) by updating the mock setup used in the test to provide public functions/properties instead of _opts and _sdk, or stub the module exports the component relies on, ensuring references to _opts/_sdk (the private fields) are removed from the mock and any assertions so the test only depends on the SDK's public surface. ``` </details> </blockquote></details> <details> <summary>src/components/Treatment/TreatmentFunction.tsx (2)</summary><blockquote> `71-74`: **Unnecessary `attributes` in dependency array.** The `handleContextReady` callback doesn't use `attributes` directly—it only uses `ensuredContext` and `name`. Including `attributes` causes unnecessary callback recreation. <details> <summary>🔧 Remove unnecessary dependency</summary> ```diff const handleContextReady = useCallback(() => { const result = getVariantAndVariables(ensuredContext, name); setVariantAndVariables(result); -}, [ensuredContext, name, attributes]); +}, [ensuredContext, name]); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/components/Treatment/TreatmentFunction.tsx` around lines 71 - 74, The handleContextReady useCallback unnecessarily lists attributes in its dependency array even though the callback only references ensuredContext, name, getVariantAndVariables, and setVariantAndVariables; update the dependency array for handleContextReady to remove attributes so it reads only [ensuredContext, name] (keeping any other referenced stable symbols like getVariantAndVariables and setVariantAndVariables as appropriate). ``` </details> --- `83-90`: **Duplicated error UI with Treatment.tsx.** The error rendering block is identical to the one in `Treatment.tsx`. Consider extracting this to a shared `ErrorAlert` component to reduce duplication and ensure consistent error presentation. <details> <summary>♻️ Extract shared component</summary> Create a shared component: ```typescript // src/components/ErrorAlert.tsx export const ErrorAlert: FC<{ name: string; error: Error }> = ({ name, error }) => ( <div role="alert" style={{ padding: '10px', border: '1px solid red', borderRadius: '4px', backgroundColor: '#fee' }}> <strong>Failed to load experiment "{name}":</strong> <p>{error.message}</p> </div> ); ``` Then use it in both components: ```diff -if (error) { - return ( - <div role="alert" style={{ padding: '10px', border: '1px solid red', borderRadius: '4px', backgroundColor: '#fee' }}> - <strong>Failed to load experiment "{name}":</strong> - <p>{error.message}</p> - </div> - ); -} +if (error) { + return <ErrorAlert name={name} error={error} />; +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/components/Treatment/TreatmentFunction.tsx` around lines 83 - 90, Extract the duplicated error UI into a shared React component and use it from both places: create an ErrorAlert functional component (e.g., ErrorAlert: FC<{ name: string; error: Error }>) that renders the alert markup, then replace the duplicated JSX in TreatmentFunction (the error return block) and in Treatment with <ErrorAlert name={name} error={error} />; ensure you export ErrorAlert and import it into both TreatmentFunction and Treatment so the UI is centralized and consistent. ``` </details> </blockquote></details> <details> <summary>tests/integration.test.tsx (2)</summary><blockquote> `41-52`: **Unused SDK and context options.** `sdkOptions` and `contextOptions` are defined but never used since all tests pass mock contexts directly. Consider removing these if not needed, or adding an SDKProvider-based integration test. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/integration.test.tsx` around lines 41 - 52, The variables sdkOptions and contextOptions in tests/integration.test.tsx are declared but never used; either remove these unused constants to clean up the test file or add an SDKProvider-based integration test that consumes them (e.g., instantiate the SDK with sdkOptions and mount components with SDKProvider so contextOptions is passed as initial context/units). Locate sdkOptions and contextOptions in integration.test.tsx and either delete their declarations or replace the current direct-mock usage by creating the SDK instance (using the SDK constructor or createSdk function) with sdkOptions and wrapping tested components in an SDKProvider that receives the SDK and contextOptions. ``` </details> --- `7-7`: **Unused `SDKProvider` import.** `SDKProvider` is imported but not used in any test. All tests pass context directly via props. Consider removing the unused import, or adding a test that verifies SDKProvider integration. <details> <summary>🧹 Remove unused import</summary> ```diff -import { SDKProvider } from "../src/components/SDKProvider"; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/integration.test.tsx` at line 7, Remove the unused SDKProvider import from the test file: delete the line importing SDKProvider since tests pass context via props, or alternatively add a test that mounts the SDKProvider (e.g., wrap existing render calls with <SDKProvider ...>) to assert provider integration; reference the SDKProvider symbol and the test file's existing render/assert logic when applying the change. ``` </details> </blockquote></details> <details> <summary>src/utils/convertLetterToNumber.ts (1)</summary><blockquote> `3-3`: **Redundant type in parameter signature.** The `Char` type is already defined as `string | number` in `src/types.ts`, so `Char | number` is redundant. Consider simplifying to just `Char`. <details> <summary>🔧 Suggested simplification</summary> ```diff -export const convertLetterToNumber = (char: Char | number): number => { +export const convertLetterToNumber = (char: Char): number => { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/utils/convertLetterToNumber.ts` at line 3, The parameter type for convertLetterToNumber is redundant: replace the signature's parameter type from "Char | number" to just "Char" (since Char is already defined as string | number in src/types.ts); update the function declaration convertLetterToNumber(char: Char): number and ensure any callers still compile against the simplified type. ``` </details> </blockquote></details> <details> <summary>tests/ssr.test.tsx (1)</summary><blockquote> `66-77`: **Unused `TestComponent` and redundant wrapper.** `TestComponent` (lines 66-73) is defined but never rendered or used in the test. The `wrapper` (lines 75-77) only wraps children in fragments, which has no effect. Consider removing both to simplify the test. <details> <summary>🧹 Simplified test</summary> ```diff it("should support static rendering without async operations when context is ready", () => { const mockContext = createMockContext(true); mockContext.treatment.mockReturnValue(1); mockedUseABSmartly.mockReturnValue({ context: mockContext as unknown as Context, sdk: null as unknown as SDK, resetContext: () => {}, }); - const TestComponent = () => { - const { variant, loading } = useTreatment("test-experiment"); - return ( - <div> - {loading ? "Loading..." : `Variant: ${variant}`} - </div> - ); - }; - - const wrapper: FC<PropsWithChildren> = ({ children }) => ( - <>{children}</> - ); - - const { result } = renderHook(() => useTreatment("test-experiment"), { - wrapper, - }); + const { result } = renderHook(() => useTreatment("test-experiment")); expect(result.current.loading).toBe(false); expect(result.current.variant).toBe(1); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/ssr.test.tsx` around lines 66 - 77, Remove the unused TestComponent and the no-op wrapper to simplify the test: locate the TestComponent declaration (const TestComponent = () => { ... }) and the wrapper constant (const wrapper: FC<PropsWithChildren> = ({ children }) => <>{children}</>), delete both definitions and any references to them in the test file, and run the tests to ensure nothing else depends on these symbols; if the test actually intended to render TestComponent, instead replace the removal by rendering it via your testing utility rather than keeping an unused definition. ``` </details> </blockquote></details> <details> <summary>src/components/Treatment/Treatment.tsx (2)</summary><blockquote> `42-42`: **Unsafe type cast bypasses type safety.** Using `(child.props as any).variant` loses type safety. Consider defining a type for the expected child props or using a type guard. <details> <summary>🔧 Type-safe approach</summary> ```diff +interface VariantChildProps { + variant?: number | Char; +} + const childrenInfo = React.Children.map(children, (child) => { if (!React.isValidElement(child)) { console.warn( `Treatment "${name}": Non-element child detected. Using variant 0.` ); return { variant: 0 }; } - const variant = (child.props as any).variant; + const variant = (child.props as VariantChildProps).variant; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/components/Treatment/Treatment.tsx` at line 42, The current direct cast (child.props as any).variant bypasses type safety; instead define a typed interface for the expected child props (e.g., interface ChildProps { variant?: string } ) and use a React type guard to narrow the child to ReactElement<ChildProps> before accessing variant (e.g., check React.isValidElement(child) and/or implement a type guard like isVariantChild(child): child is ReactElement<ChildProps>), then read child.props.variant without using any; update references in the Treatment component where variant is read to use this safe narrowing. ``` </details> --- `98-105`: **Hardcoded error UI styles.** The inline error styling is hardcoded. Consider allowing customisation via an `errorComponent` prop similar to `loadingComponent`, giving consumers control over error presentation. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/components/Treatment/Treatment.tsx` around lines 98 - 105, The component currently renders a hardcoded error UI inside the Treatment component; add an optional errorComponent prop (mirroring loadingComponent) to the Treatment props/interface and use it when error is truthy (i.e., if props.errorComponent return that, otherwise render the existing inline fallback using name and error.message). Update the Treatment prop types (or interface) to include errorComponent?: React.ReactNode | ((props:{name:string,error:Error})=>React.ReactNode) and ensure any existing callers continue to work by preserving the inline fallback. Also update any defaultProps or function signature so the new prop is optional and properly typed. ``` </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.
Inline comments:
In@src/components/ErrorBoundary/ErrorBoundary.tsx:
- Around line 46-53: The render path in ErrorBoundary may call the functional
fallback with a null errorInfo because getDerivedStateFromError sets hasError
and error before componentDidCatch populates errorInfo; remove the non-null
assertion on this.state.errorInfo and handle the nullable case by either (a)
passing this.state.errorInfo as possibly null/undefined to the fallback (update
the invocation in render inside the hasError branch where props.fallback is a
function), or (b) change the fallback prop type to accept ErrorInfo | null so
the function signature matches; ensure references to getDerivedStateFromError,
componentDidCatch, render, props.fallback and this.state.errorInfo are updated
accordingly.In
@src/components/Treatment/Treatment.tsx:
- Around line 86-88: handleContextReady currently reads getSelectedChildIndex
which closes over childrenInfo (derived from children) but children/childrenInfo
aren't in the dependency list; update the useCallback dependency array to
include children or childrenInfo (e.g., [ensuredContext, name, childrenInfo] or
[ensuredContext, name, children]) so the callback sees the latest child data, or
alternatively memoize getSelectedChildIndex with useCallback/useMemo and then
add that memoized function to handleContextReady's dependencies; reference
handleContextReady, getSelectedChildIndex, childrenInfo, children,
ensuredContext, and setSelectedTreatment when making the change.In
@src/hooks/useContextReady.ts:
- Around line 35-40: The onReady callback is skipped when the context is already
ready on first render because the code checks !initiallyReady before calling
onReady; update the logic in useContextReady so that when isContextReady is true
you call onReady(context) if onReady is provided regardless of initiallyReady
(i.e., remove the initiallyReady gate), and ensure you don't introduce a
duplicate call elsewhere by keeping the place that later triggers onReady only
when initiallyReady is false or by tracking that onReady has been invoked;
reference variables/functions: useContextReady, isContextReady, initiallyReady,
onReady, context.In
@src/hooks/useTreatment.ts:
- Around line 10-15: The initial state in the useTreatment hook uses the raw
result of context.peek(name) / context.treatment(name) which can be undefined,
but the async fetch path uses the fallback treatment ?? 0; make the initial
state consistent by applying the same fallback (coerce undefined to 0) when
computing the initial variant in the useState initializer so both SSR and client
hydration produce a numeric variant; update the initializer inside useTreatment
(where peek and treatment are called) to return (peek ? context.peek(name) :
context.treatment(name)) ?? 0.In
@src/types.ts:
- Around line 1-8: Remove the invalid module augmentation that adds getSDK() and
getOptions() to the SDK Context: delete the declare module block (the
augmentation of Context that defines getSDK() and getOptions()) in src/types.ts,
and rely on the SDK's native Context type instead; if you need access to SDK or
options, refactor callers to obtain them via the context creation return values
or explicit parameters rather than adding non-existent methods to Context.
Outside diff comments:
In@tests/SDKProvider.test.tsx:
- Around line 1-302: Prettier flagged formatting issues in
tests/SDKProvider.test.tsx; run the formatter or apply equivalent edits to
restore style (e.g., spacing, trailing commas, import ordering) and re-run the
linter. Specifically, format the file containing the SDKProvider and
useABSmartly tests and mocks (symbols: SDKProvider, useABSmartly,
mockCreateContext, mockCreateContextWith, TestComponent) by running prettier
--write tests/SDKProvider.test.tsx (or your project's formatting command) so the
test file matches the repository Prettier rules before merging.
Nitpick comments:
In@README.md:
- Around line 26-28: The fenced code block containing the proxy pattern line
"Browser --> Your Backend (with secret key) --> ABSmartly API" is missing a
language specifier; update that triple-backtick fence to include a language
(e.g.,text) so Markdown linters stop warning—modify the block around that arrow string in README.md to start withtext and keep the same content and
closing ``` unchanged.In
@src/components/Treatment/Treatment.tsx:
- Line 42: The current direct cast (child.props as any).variant bypasses type
safety; instead define a typed interface for the expected child props (e.g.,
interface ChildProps { variant?: string } ) and use a React type guard to narrow
the child to ReactElement before accessing variant (e.g., check
React.isValidElement(child) and/or implement a type guard like
isVariantChild(child): child is ReactElement), then read
child.props.variant without using any; update references in the Treatment
component where variant is read to use this safe narrowing.- Around line 98-105: The component currently renders a hardcoded error UI
inside the Treatment component; add an optional errorComponent prop (mirroring
loadingComponent) to the Treatment props/interface and use it when error is
truthy (i.e., if props.errorComponent return that, otherwise render the existing
inline fallback using name and error.message). Update the Treatment prop types
(or interface) to include errorComponent?: React.ReactNode |
((props:{name:string,error:Error})=>React.ReactNode) and ensure any existing
callers continue to work by preserving the inline fallback. Also update any
defaultProps or function signature so the new prop is optional and properly
typed.In
@src/components/Treatment/TreatmentFunction.tsx:
- Around line 71-74: The handleContextReady useCallback unnecessarily lists
attributes in its dependency array even though the callback only references
ensuredContext, name, getVariantAndVariables, and setVariantAndVariables; update
the dependency array for handleContextReady to remove attributes so it reads
only [ensuredContext, name] (keeping any other referenced stable symbols like
getVariantAndVariables and setVariantAndVariables as appropriate).- Around line 83-90: Extract the duplicated error UI into a shared React
component and use it from both places: create an ErrorAlert functional component
(e.g., ErrorAlert: FC<{ name: string; error: Error }>) that renders the alert
markup, then replace the duplicated JSX in TreatmentFunction (the error return
block) and in Treatment with ; ensure
you export ErrorAlert and import it into both TreatmentFunction and Treatment so
the UI is centralized and consistent.In
@src/hooks/useContextReady.ts:
- Around line 31-33: In useContextReady, avoid calling
context.attributes(attributes) on every effect run by adding a guard: in the
useContextReady hook keep a ref (e.g., appliedRef or prevAttributesRef) and only
call context.attributes(attributes) when attributes are present and either they
haven't been applied yet (appliedRef is false) or the attributes value actually
changed (compare current attributes to prevAttributesRef, e.g., shallow/equality
check or JSON.stringify for simple objects); after applying, set the ref so
subsequent effect runs don’t re-apply unless attributes change. This touches the
useContextReady hook and the context.attributes(attributes) call.In
@src/types.ts:
- Line 57: The Char type was overly widened to string | number which loses
compile-time safety; change the Char alias back to the original narrow union of
allowed character literals (or introduce a new distinct alias like BroadChar =
string | number and keep Char as the specific literal union) so that
functions/types depending on Char (the Char type) retain strict
validation—update the Char definition accordingly and adjust any call sites to
use BroadChar only where truly needed.In
@src/utils/convertLetterToNumber.ts:
- Line 3: The parameter type for convertLetterToNumber is redundant: replace the
signature's parameter type from "Char | number" to just "Char" (since Char is
already defined as string | number in src/types.ts); update the function
declaration convertLetterToNumber(char: Char): number and ensure any callers
still compile against the simplified type.In
@tests/integration.test.tsx:
- Around line 41-52: The variables sdkOptions and contextOptions in
tests/integration.test.tsx are declared but never used; either remove these
unused constants to clean up the test file or add an SDKProvider-based
integration test that consumes them (e.g., instantiate the SDK with sdkOptions
and mount components with SDKProvider so contextOptions is passed as initial
context/units). Locate sdkOptions and contextOptions in integration.test.tsx and
either delete their declarations or replace the current direct-mock usage by
creating the SDK instance (using the SDK constructor or createSdk function) with
sdkOptions and wrapping tested components in an SDKProvider that receives the
SDK and contextOptions.- Line 7: Remove the unused SDKProvider import from the test file: delete the
line importing SDKProvider since tests pass context via props, or alternatively
add a test that mounts the SDKProvider (e.g., wrap existing render calls with
<SDKProvider ...>) to assert provider integration; reference the SDKProvider
symbol and the test file's existing render/assert logic when applying the
change.In
@tests/SDKProvider.test.tsx:
- Around line 34-64: The two mocks mockCreateContext and mockCreateContextWith
are duplicated; extract a single factory (e.g., buildMockContext or
createMockContextFactory) that returns the shared mock object (data, isReady,
isFailed, treatment, ready, attributes, variableKeys, peekVariableValue,
finalize, getOptions, getSDK) and then have mockCreateContext and
mockCreateContextWith both use vi.fn().mockImplementation(buildMockContext);
optionally accept an overrides parameter in the factory to customize per-test
behavior without duplicating the whole implementation.In
@tests/ssr.test.tsx:
- Around line 66-77: Remove the unused TestComponent and the no-op wrapper to
simplify the test: locate the TestComponent declaration (const TestComponent =
() => { ... }) and the wrapper constant (const wrapper: FC =
({ children }) => <>{children}</>), delete both definitions and any references
to them in the test file, and run the tests to ensure nothing else depends on
these symbols; if the test actually intended to render TestComponent, instead
replace the removal by rendering it via your testing utility rather than keeping
an unused definition.In
@tests/TreatmentFunction.test.tsx:
- Around line 179-204: The test's mockedUseOptionalABSmartly return value uses
resetContext that currently returns undefined but must return a Promise
per the ABSmartly type; update the mock in this test so resetContext is an async
function or a function that returns a resolved Promise (e.g., async () => {} or
() => Promise.resolve()), keeping the rest of the mocked object (context, sdk)
unchanged and ensuring any other tests that rely on this mock are updated
similarly; target the mockedUseOptionalABSmartly mock return in this spec to
make resetContext return a Promise.- Around line 104-107: Remove the unused local variable declaration "attributes"
from the test to eliminate the unused-variable warning; locate the const
attributes = { attr1: 15, attr2: 50 } in the TreatmentFunction.test.tsx test and
either delete that declaration entirely or (if intended) replace its removal by
actually using "attributes" in the relevant test setup or function call (e.g.,
pass it into the function under test or mock setup) so the variable is
referenced.- Line 83: Remove the unused const config declarations (variable name: config)
introduced in the test file—either delete both unused declarations or use the
variable where intended; search for the two occurrences of "const config = { a:
1, b: 2 }" in the test and remove them if they are vestigial after refactor, or
replace them with the actual fixture/argument the tests expect.In
@tests/utils.test.ts:
- Around line 57-71: Add tests for invalid input handling for
convertLetterToNumber: create a new "invalid input handling" describe block that
asserts multi-character strings (e.g., "ab"), special characters (e.g., "!",
"@"), and empty string all return 0 when passed to convertLetterToNumber, and
optionally spy on/expect console.warn to have been called for those cases to
verify the warning behavior; reference the function name convertLetterToNumber
and add these tests alongside the existing "edge cases" tests in
tests/utils.test.ts.In
@tests/withABSmartly.test.tsx:
- Line 2: Remove the unused forwardRef import from the React import line in the
tests/withABSmartly.test.tsx file (leave React and ComponentType intact); locate
the import statement that currently reads something like "import React, {
forwardRef, ComponentType } from 'react'" and delete only forwardRef to clean up
unused imports.- Around line 32-33: The test mock in withABSmartly.test.tsx exposes internal
SDK properties _opts and _sdk which couples tests to implementation details;
remove or replace them with only the public API the test uses (e.g., methods or
return values the code under test calls) by updating the mock setup used in the
test to provide public functions/properties instead of _opts and _sdk, or stub
the module exports the component relies on, ensuring references to _opts/_sdk
(the private fields) are removed from the mock and any assertions so the test
only depends on the SDK's public surface.In
@tsconfig.json:
- Around line 3-23: Remove the redundant explicit strict-mode flags since
"strict": true already enables them; delete the entries "strictNullChecks",
"strictFunctionTypes", "strictBindCallApply", "strictPropertyInitialization",
and "alwaysStrict" from the tsconfig so only "strict": true remains as the
single source of truth for those behaviors.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 39bcfe5ecd01d36dcad5587170c8b345676d1bbd and 676203e486e5cf667aeb32612ab9fccc52e312b6. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` * `yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (24)</summary> * `.gitignore` * `README.md` * `package.json` * `src/components/ErrorBoundary/ErrorBoundary.tsx` * `src/components/ErrorBoundary/index.ts` * `src/components/SDKProvider/SDKProvider.tsx` * `src/components/Treatment/Treatment.tsx` * `src/components/Treatment/TreatmentFunction.tsx` * `src/hooks/useABSmartly.ts` * `src/hooks/useContextReady.ts` * `src/hooks/useTreatment.ts` * `src/index.ts` * `src/types.ts` * `src/utils/convertLetterToNumber.ts` * `tests/SDKProvider.test.tsx` * `tests/Treatment.test.tsx` * `tests/TreatmentFunction.test.tsx` * `tests/integration.test.tsx` * `tests/mocks.ts` * `tests/ssr.test.tsx` * `tests/useTreatment.test.ts` * `tests/utils.test.ts` * `tests/withABSmartly.test.tsx` * `tsconfig.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+46
to
+53
| render(): ReactNode { | ||
| if (this.state.hasError && this.state.error) { | ||
| if (this.props.fallback) { | ||
| if (typeof this.props.fallback === "function") { | ||
| return this.props.fallback(this.state.error, this.state.errorInfo!); | ||
| } | ||
| return this.props.fallback; | ||
| } |
There was a problem hiding this comment.
Potential null value passed to functional fallback.
When getDerivedStateFromError runs, it sets hasError and error, but errorInfo is only set later in componentDidCatch. During the initial error render, this.state.errorInfo may still be null, yet the non-null assertion (!) on line 50 asserts otherwise.
Consider handling the null case gracefully:
Proposed fix
if (typeof this.props.fallback === "function") {
- return this.props.fallback(this.state.error, this.state.errorInfo!);
+ return this.props.fallback(this.state.error, this.state.errorInfo ?? { componentStack: "" } as ErrorInfo);
}Alternatively, update the fallback type signature to accept ErrorInfo | null:
- fallback?: ReactNode | ((error: Error, errorInfo: ErrorInfo) => ReactNode);
+ fallback?: ReactNode | ((error: Error, errorInfo: ErrorInfo | null) => ReactNode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| render(): ReactNode { | |
| if (this.state.hasError && this.state.error) { | |
| if (this.props.fallback) { | |
| if (typeof this.props.fallback === "function") { | |
| return this.props.fallback(this.state.error, this.state.errorInfo!); | |
| } | |
| return this.props.fallback; | |
| } | |
| render(): ReactNode { | |
| if (this.state.hasError && this.state.error) { | |
| if (this.props.fallback) { | |
| if (typeof this.props.fallback === "function") { | |
| return this.props.fallback(this.state.error, this.state.errorInfo ?? { componentStack: "" } as ErrorInfo); | |
| } | |
| return this.props.fallback; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ErrorBoundary/ErrorBoundary.tsx` around lines 46 - 53, The
render path in ErrorBoundary may call the functional fallback with a null
errorInfo because getDerivedStateFromError sets hasError and error before
componentDidCatch populates errorInfo; remove the non-null assertion on
this.state.errorInfo and handle the nullable case by either (a) passing
this.state.errorInfo as possibly null/undefined to the fallback (update the
invocation in render inside the hasError branch where props.fallback is a
function), or (b) change the fallback prop type to accept ErrorInfo | null so
the function signature matches; ensure references to getDerivedStateFromError,
componentDidCatch, render, props.fallback and this.state.errorInfo are updated
accordingly.
Comment on lines
+86
to
+88
| const handleContextReady = useCallback(() => { | ||
| setSelectedTreatment(getSelectedChildIndex(ensuredContext)); | ||
| }, [ensuredContext, name]); |
There was a problem hiding this comment.
Potential stale closure in handleContextReady.
The callback depends on getSelectedChildIndex, which captures childrenInfo from the component's closure. However, childrenInfo is derived from children and isn't listed in the dependency array. If children change while the context is becoming ready, the callback may use stale child information.
Consider adding children or childrenInfo to the dependency array, or memoising getSelectedChildIndex.
🔧 Suggested fix
const handleContextReady = useCallback(() => {
setSelectedTreatment(getSelectedChildIndex(ensuredContext));
-}, [ensuredContext, name]);
+}, [ensuredContext, name, children]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleContextReady = useCallback(() => { | |
| setSelectedTreatment(getSelectedChildIndex(ensuredContext)); | |
| }, [ensuredContext, name]); | |
| const handleContextReady = useCallback(() => { | |
| setSelectedTreatment(getSelectedChildIndex(ensuredContext)); | |
| }, [ensuredContext, name, children]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Treatment/Treatment.tsx` around lines 86 - 88,
handleContextReady currently reads getSelectedChildIndex which closes over
childrenInfo (derived from children) but children/childrenInfo aren't in the
dependency list; update the useCallback dependency array to include children or
childrenInfo (e.g., [ensuredContext, name, childrenInfo] or [ensuredContext,
name, children]) so the callback sees the latest child data, or alternatively
memoize getSelectedChildIndex with useCallback/useMemo and then add that
memoized function to handleContextReady's dependencies; reference
handleContextReady, getSelectedChildIndex, childrenInfo, children,
ensuredContext, and setSelectedTreatment when making the change.
Comment on lines
+35
to
+40
| if (isContextReady) { | ||
| if (!initiallyReady && onReady) { | ||
| onReady(context); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
onReady callback is never invoked when context is initially ready.
When initiallyReady is true (context was ready on first render), the condition !initiallyReady && onReady on line 36 prevents onReady from being called. This means SSR-hydrated components or any scenario where the context is already ready will never receive the onReady callback.
If this is intentional for SSR, consider documenting this behaviour. Otherwise, you may want to invoke onReady when the context is initially ready:
🔧 Potential fix if onReady should always fire
if (isContextReady) {
- if (!initiallyReady && onReady) {
+ if (onReady) {
onReady(context);
}
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isContextReady) { | |
| if (!initiallyReady && onReady) { | |
| onReady(context); | |
| } | |
| return; | |
| } | |
| if (isContextReady) { | |
| if (onReady) { | |
| onReady(context); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useContextReady.ts` around lines 35 - 40, The onReady callback is
skipped when the context is already ready on first render because the code
checks !initiallyReady before calling onReady; update the logic in
useContextReady so that when isContextReady is true you call onReady(context) if
onReady is provided regardless of initiallyReady (i.e., remove the
initiallyReady gate), and ensure you don't introduce a duplicate call elsewhere
by keeping the place that later triggers onReady only when initiallyReady is
false or by tracking that onReady has been invoked; reference
variables/functions: useContextReady, isContextReady, initiallyReady, onReady,
context.
Comment on lines
+10
to
+15
| const [variant, setVariant] = useState<number | null>(() => { | ||
| if (isContextReady) { | ||
| return peek ? context.peek(name) : context.treatment(name); | ||
| } | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
Inconsistent fallback for undefined treatment value.
The initial state (lines 11-12) returns the raw result from context.peek(name) or context.treatment(name) which could be undefined. However, the async fetch path (line 29) applies a fallback: treatment ?? 0.
This inconsistency means SSR-rendered components could have variant as undefined, whilst client-side hydrated components would get 0.
🔧 Suggested fix for consistency
const [variant, setVariant] = useState<number | null>(() => {
if (isContextReady) {
- return peek ? context.peek(name) : context.treatment(name);
+ const treatment = peek ? context.peek(name) : context.treatment(name);
+ return treatment ?? 0;
}
return null;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [variant, setVariant] = useState<number | null>(() => { | |
| if (isContextReady) { | |
| return peek ? context.peek(name) : context.treatment(name); | |
| } | |
| return null; | |
| }); | |
| const [variant, setVariant] = useState<number | null>(() => { | |
| if (isContextReady) { | |
| const treatment = peek ? context.peek(name) : context.treatment(name); | |
| return treatment ?? 0; | |
| } | |
| return null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useTreatment.ts` around lines 10 - 15, The initial state in the
useTreatment hook uses the raw result of context.peek(name) /
context.treatment(name) which can be undefined, but the async fetch path uses
the fallback treatment ?? 0; make the initial state consistent by applying the
same fallback (coerce undefined to 0) when computing the initial variant in the
useState initializer so both SSR and client hydration produce a numeric variant;
update the initializer inside useTreatment (where peek and treatment are called)
to return (peek ? context.peek(name) : context.treatment(name)) ?? 0.
Comment on lines
+1
to
+8
| import type { Context, SDK } from "@absmartly/javascript-sdk"; | ||
|
|
||
| declare module "@absmartly/javascript-sdk" { | ||
| interface Context { | ||
| getSDK(): SDK; | ||
| getOptions(): { publishDelay: number; refreshPeriod: number }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@absmartly/javascript-sdk version 1.13.0 Context interface getSDK getOptions methods documentation
💡 Result:
In @absmartly/javascript-sdk v1.13.0, the exported Context class/interface does not define getSDK() or getOptions() methods (they don’t appear in the public TypeScript definitions for Context). Instead, Context exposes methods like publisher(), provider(), and eventLogger(), plus the usual experimentation APIs (treatment(), peek(), track(), refresh(), publish(), etc.). [1]
Where “options” live instead
- Context-level options are provided when the context is created (the SDK docs show passing options to
createContext(...), e.g.refreshInterval). [2] - The actual
ContextOptionstype includes fields likepublisher,dataProvider,eventLogger,refreshPeriod, andpublishDelay, but they aren’t exposed via agetOptions()getter onContext. [1]
Note on the v1.13.0 release
- The v1.13.0 change note mentions “Implement all the web console methods,” but that doesn’t correspond to adding
Context.getSDK()/Context.getOptions()in the public API. [3]
Sources
ContextTypeScript definitions (nogetSDK/getOptions, shows available methods andContextOptions). [1]- ABsmartly JavaScript SDK “Getting Started” (creating a context + passing context options like refresh interval). [2]
- v1.13.0 release note. [3]
Remove the invalid module augmentation for getSDK() and getOptions() methods.
The @absmartly/javascript-sdk v1.13.0 Context interface does not define getSDK() or getOptions() methods. The module augmentation in lines 3–7 declares methods that do not exist in the SDK's public TypeScript definitions. Context options are passed during context creation, not exposed via a getter. Calling these non-existent methods at runtime will fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types.ts` around lines 1 - 8, Remove the invalid module augmentation that
adds getSDK() and getOptions() to the SDK Context: delete the declare module
block (the augmentation of Context that defines getSDK() and getOptions()) in
src/types.ts, and rely on the SDK's native Context type instead; if you need
access to SDK or options, refactor callers to obtain them via the context
creation return values or explicit parameters rather than adding non-existent
methods to Context.
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
Test Results
Test plan
npx vitest run)Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation