Add embeddable admin UI plan: Leptos vs React analysis and implementation roadmap#918
Add embeddable admin UI plan: Leptos vs React analysis and implementation roadmap#918
Conversation
- Phase 1: TypeScript API client for all Superposition REST endpoints (dimensions, configs, overrides, experiments, resolve, audit) - Phase 2: Core React components (Table, Modal, Pagination, FormField, AlertBar, ConditionBadges, StatusBadge, JsonViewer) - Phase 3: Feature pages (ConfigManager, DimensionManager, OverrideManager, ExperimentManager, ResolveView, AuditLog, SuperpositionAdmin) - Phase 4: Context scoping - filter overrides/experiments by scoped context, auto-merge scoped dimensions on create, locked dimension indicators - Full test suite: 57 tests across 10 files (API client, context filtering, providers, components, pages) - TypeScript strict mode, Vitest, React Testing Library https://claude.ai/code/session_01MQDdbiwrn4RSwde6qAoNGE
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a complete React-based embeddable admin UI package for Superposition, including an HTTP API client, context providers for state management, reusable UI components, feature pages for managing configurations/dimensions/experiments/overrides/audits, comprehensive TypeScript typing, utility functions, and full test coverage with configuration for build and testing infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as Manager Page
participant Hook as useApi/useMutation
participant Provider as SuperpositionProvider
participant Client as SuperpositionClient
participant API as API Module
participant Server
User->>Page: Interact (create/fetch/delete)
Page->>Hook: Call useApi/useMutation
Hook->>Provider: useSuperposition()
Provider-->>Hook: Return client + API modules
Hook->>API: Call module method
API->>Client: request(method, path, body/query)
Client->>Client: Build headers, query string
Client->>Server: HTTP Request
Server-->>Client: Response
Client->>API: Resolve/throw error
API-->>Hook: Return typed data
Hook->>Page: Update state (data/loading/error)
Page->>User: Render UI + alerts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Suggested labels
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)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
clients/admin-ui/src/components/Pagination.tsx-39-43 (1)
39-43:⚠️ Potential issue | 🟡 MinorSet explicit
type="button"on pagination controls.Without explicit type, these buttons default to submit inside forms and can trigger accidental form submission.
🔧 Suggested fix
<button + type="button" style={btnStyle} disabled={currentPage <= 1} onClick={() => onPageChange(currentPage - 1)} > @@ <button + type="button" key={p} style={p === currentPage ? activeBtnStyle : btnStyle} onClick={() => onPageChange(p)} > @@ <button + type="button" style={btnStyle} disabled={currentPage >= totalPages} onClick={() => onPageChange(currentPage + 1)} >Also applies to: 47-51, 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/Pagination.tsx` around lines 39 - 43, The pagination button elements in the Pagination component default to type="submit" when rendered inside a form; update each button (the previous/next/page number buttons that use props/currentPage and the onPageChange handler and btnStyle) to explicitly include type="button" to prevent accidental form submission—add type="button" to the button that calls onPageChange(currentPage - 1), the one that calls onPageChange(currentPage + 1), and any page-number buttons that invoke onPageChange.clients/admin-ui/src/components/AlertBar.tsx-35-39 (1)
35-39:⚠️ Potential issue | 🟡 MinorSet
type="button"on the dismiss control.This avoids accidental form submission when the component is rendered inside a form subtree.
🔧 Suggested fix
<button + type="button" onClick={() => removeAlert(alert.id)} style={{ background: "none", border: "none", cursor: "pointer", padding: "0 0 0 12px", fontSize: 16 }} aria-label="Dismiss" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/AlertBar.tsx` around lines 35 - 39, The dismiss button in AlertBar is missing an explicit type which can cause accidental form submission when used inside a form; update the button element (the control that calls removeAlert(alert.id) in the AlertBar component) to include type="button" so it behaves as a non-submit button and preserves the existing onClick, styles, aria-label, and removeAlert usage.PLAN-embeddable-admin-ui.md-46-71 (1)
46-71:⚠️ Potential issue | 🟡 MinorSpecify a language for the fenced code block.
The architecture tree block should declare a fence language to satisfy markdown linting (MD040).
📝 Suggested fix
-``` +```text `@anthropic/superposition-admin-ui` (npm package) ... -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN-embeddable-admin-ui.md` around lines 46 - 71, Add a language identifier to the fenced code block that starts with "@anthropic/superposition-admin-ui (npm package)" in PLAN-embeddable-admin-ui.md (e.g., change ``` to ```text) so the markdown linter MD040 passes; update the opening fence only and leave the block contents intact.clients/admin-ui/src/components/JsonViewer.tsx-11-13 (1)
11-13:⚠️ Potential issue | 🟡 MinorHandle potential
JSON.stringifyerrors for circular references or BigInt.
JSON.stringifythrows aTypeErrorfor circular references orBigIntvalues. Ifdatacontains such values, this component will crash.🛡️ Proposed defensive fix
+function safeStringify(data: unknown, space?: number): string { + try { + return JSON.stringify(data, null, space); + } catch { + return "[Unable to display]"; + } +} + export function JsonViewer({ data, collapsed = true }: JsonViewerProps) { const [expanded, setExpanded] = useState(!collapsed); - const formatted = JSON.stringify(data, null, 2); - const preview = JSON.stringify(data); + const formatted = safeStringify(data, 2); + const preview = safeStringify(data); const isLong = preview.length > 60;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/JsonViewer.tsx` around lines 11 - 13, The JSON.stringify calls for formatted and preview can throw on circular refs or BigInt; update the JsonViewer component to use a safe stringify strategy: wrap the current const formatted, preview, and isLong logic in a try/catch and on error fall back to a safe serializer (e.g., a replacer that converts BigInt to string and a WeakSet-based circular-reference handler) or a simple toString fallback, then compute isLong from the safe preview; ensure the new logic replaces the existing `formatted`, `preview`, and `isLong` definitions so the component no longer crashes when `data` contains BigInt or circular references.clients/admin-ui/__tests__/pages/OverrideManager.test.tsx-57-71 (1)
57-71:⚠️ Potential issue | 🟡 MinorTest name and assertion mismatch: list rendering is not actually validated.
Line 57 through Line 71 claims to verify override list rendering, but it only checks the static heading
"Context Overrides". Please assert at least one row-level value frommockOverrides.dataso the test fails when list rendering breaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/pages/OverrideManager.test.tsx` around lines 57 - 71, The test "renders override list" currently only checks the static heading; update the test for OverrideManager to assert a row-level value from mockOverrides.data (e.g., the first override's name or id) so list rendering is actually validated—use the same SuperpositionProvider/AlertProvider render and then await screen.findByText or waitFor + expect(screen.getByText(<first mockOverrides.data item>)).toBeInTheDocument() to ensure a specific override row is present (and update the test name if desired to reflect that it verifies list rows).clients/admin-ui/src/pages/ConfigManager.tsx-67-85 (1)
67-85:⚠️ Potential issue | 🟡 MinorUse the caught exception for error alerts.
Current catches ignore
errand rely on mutation state, which can be stale and also masks JSON parse errors fromJSON.parse.Suggested fix
- } catch { - addAlert("error", createMutation.error || "Failed to create config"); + } catch (err) { + addAlert("error", err instanceof Error ? err.message : String(err)); @@ - } catch { - addAlert("error", deleteMutation.error || "Failed to delete config"); + } catch (err) { + addAlert("error", err instanceof Error ? err.message : String(err)); }Also applies to: 88-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/ConfigManager.tsx` around lines 67 - 85, The try/catch currently ignores the caught exception and uses mutation state which can be stale and misses JSON.parse errors; update handleCreate (and the analogous handleUpdate) to (1) accept the caught error parameter (e.g., catch (err)), (2) call createMutation.mutateAsync(...) instead of createMutation.mutate so thrown errors propagate to the try/catch, and (3) pass the actual error message/object to addAlert (e.g., addAlert("error", createMutation.error || err?.message || String(err))). Ensure the JSON.parse calls remain inside the try so parse errors are caught and surfaced via the alert.clients/admin-ui/src/pages/ResolveView.tsx-24-37 (1)
24-37:⚠️ Potential issue | 🟡 MinorClear stale result state on new/failed resolves.
If resolve fails, previous successful data remains visible, which is misleading.
Suggested fix
const handleResolve = async () => { + setResult(null); setLoading(true); @@ } catch (err) { + setResult(null); addAlert("error", err instanceof Error ? err.message : String(err));Also applies to: 73-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/ResolveView.tsx` around lines 24 - 37, Before starting a new resolve attempt, clear any previous result and ensure failures also clear it so stale data isn't shown; in handleResolve setResult(undefined) immediately before setLoading(true) (or right after) and also setResult(undefined) inside the catch block when an error occurs, and apply the same changes to the other resolve handler in this file that calls resolve.resolve so both new attempts and failed resolves clear prior results.clients/admin-ui/src/pages/ResolveView.tsx-27-31 (1)
27-31:⚠️ Potential issue | 🟡 MinorValidate Additional Context is a JSON object before merge.
JSON.parseaccepts primitives/arrays, but this flow expects a key-value object context.Suggested fix
- const ctx = mergeScopedContext( - JSON.parse(contextInput), - scopedContext, - ); + const parsed = JSON.parse(contextInput); + if (!parsed || Array.isArray(parsed) || typeof parsed !== "object") { + throw new Error("Additional Context must be a JSON object"); + } + const ctx = mergeScopedContext(parsed, scopedContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/ResolveView.tsx` around lines 27 - 31, Before calling mergeScopedContext and resolve.resolve, ensure the parsed contextInput yields a plain object: parse contextInput, verify the result is non-null, typeof === "object" and not Array.isArray(result); if the check fails, surface a clear validation error (or fallback to an empty object) rather than passing primitives/arrays into mergeScopedContext. Update the code around contextInput, mergeScopedContext, scopedContext and the use before calling resolve.resolve to perform this validation and handle the invalid-case path.clients/admin-ui/src/utils/format.ts-4-9 (1)
4-9:⚠️ Potential issue | 🟡 MinorInvalid datetime inputs bypass the intended fallback.
new Date(invalid).toLocaleString()does not throw; it returns"Invalid Date", socatchnever runs.Suggested fix
export function formatDateTime(iso: string): string { - try { - return new Date(iso).toLocaleString(); - } catch { - return iso; - } + const d = new Date(iso); + return Number.isNaN(d.getTime()) ? iso : d.toLocaleString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/utils/format.ts` around lines 4 - 9, formatDateTime currently assumes new Date(iso).toLocaleString() will throw on bad input, but Date returns an "Invalid Date" object instead; update formatDateTime to instantiate the Date (use new Date(iso)), check validity (e.g., isNaN(date.getTime()) or date.toString() === "Invalid Date") and if invalid return the original iso string, otherwise return date.toLocaleString(); keep the try/catch only if you still want to guard against unexpected errors but ensure the explicit validity check is used (reference function name: formatDateTime).clients/admin-ui/src/pages/OverrideManager.tsx-108-144 (1)
108-144:⚠️ Potential issue | 🟡 MinorStale closure:
handleDeleteis not in the dependency array.The
columnsmemoization useshandleDeletein the render function (line 134) but the dependency array only includes[lockedDims]. This can lead to stale closures wherehandleDeletecaptures outdated values ofdeleteMutationorrefetch.🔧 Proposed fix
+ const handleDelete = useCallback(async (id: string) => { + if (!confirm("Delete this override?")) return; + try { + await deleteMutation.mutate(id); + refetch(); + } catch { + addAlert("error", deleteMutation.error || "Failed to delete"); + } + }, [deleteMutation, refetch, addAlert]); const columns: Column<ContextOverride>[] = useMemo( () => [ // ... columns ], - // eslint-disable-next-line react-hooks/exhaustive-deps - [lockedDims], + [lockedDims, handleDelete], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/OverrideManager.tsx` around lines 108 - 144, The useMemo for columns creates a stale closure because the render uses handleDelete but the dependency array only lists lockedDims; update the memoization so columns sees the latest handleDelete by either adding handleDelete to the useMemo dependency array or make handleDelete stable (wrap delete logic that uses deleteMutation/refetch in a useCallback) and then include that stable function in the dependency list for columns; reference the columns constant, the handleDelete function, and the deleteMutation/refetch callers when changing dependencies.clients/admin-ui/src/pages/ExperimentManager.tsx-141-196 (1)
141-196:⚠️ Potential issue | 🟡 MinorStale closure:
handleRampis not in the dependency array.The
columnsmemoization useshandleRampin the render function (line 186) but the dependency array only includes[lockedDims]. SincehandleRampreferencesrampMutationandrefetch, it could become stale. Consider wrappinghandleRampinuseCallbackand adding it to the dependencies, or moving the handler outside the memoized columns.🔧 Proposed fix
+ const handleRamp = useCallback(async (id: string) => { + const input = prompt("Enter traffic percentage (0-100):"); + if (input === null) return; + const pct = parseInt(input, 10); + if (isNaN(pct) || pct < 0 || pct > 100) { + addAlert("error", "Invalid percentage"); + return; + } + try { + await rampMutation.mutate(id, pct); + refetch(); + } catch { + addAlert("error", rampMutation.error || "Failed to ramp"); + } + }, [rampMutation, refetch, addAlert]); const columns: Column<Experiment>[] = useMemo( () => [ // ... columns ], - // eslint-disable-next-line react-hooks/exhaustive-deps - [lockedDims], + [lockedDims, handleRamp], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/ExperimentManager.tsx` around lines 141 - 196, The columns useMemo captures handleRamp in its render but doesn't list it in the dependency array, which can cause a stale closure because handleRamp depends on rampMutation and refetch; fix by either wrapping handleRamp in useCallback (including rampMutation and refetch in its dependencies) and then adding that memoized handleRamp to the columns dependency array, or move the onClick handler logic out of the columns definition so columns only depend on lockedDims; update the columns useMemo dependency array accordingly to include handleRamp if you choose the useCallback approach.clients/admin-ui/src/pages/DimensionManager.tsx-67-86 (1)
67-86:⚠️ Potential issue | 🟡 MinorJSON.parse error will produce a confusing error message.
When
JSON.parse(newSchema)fails on Line 72, the error is caught but Line 84 usescreateMutation.errorwhich may benull(since the JSON parsing failure happens before the mutation runs). This results in the generic "Failed to create dimension" message rather than indicating the actual JSON syntax error.🛠️ Proposed fix: catch and display JSON parse errors explicitly
const handleCreate = async () => { + let schema; + try { + schema = JSON.parse(newSchema); + } catch (e) { + addAlert("error", "Invalid JSON schema"); + return; + } try { await createMutation.mutate({ dimension: newName, position: parseInt(newPosition, 10), - schema: JSON.parse(newSchema), + schema, description: newDesc || "No description", change_reason: newReason || "Created via admin UI", }); setShowCreate(false); // ... rest of reset logic refetch(); } catch { addAlert("error", createMutation.error || "Failed to create dimension"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/DimensionManager.tsx` around lines 67 - 86, The handleCreate function currently calls JSON.parse(newSchema) inline so a syntax error is caught by the outer try but the alert uses createMutation.error (which may be null), producing a misleading message; fix by parsing the schema into a variable before invoking createMutation and explicitly catching JSON.parse errors: attempt JSON.parse(newSchema) in its own try/catch (or validate), and if it throws call addAlert("error", parseError.message) and return early; only after successful parse call createMutation.mutate(...) and keep the existing catch to report createMutation.error or a fallback message for mutation failures.clients/admin-ui/src/pages/SuperpositionAdmin.tsx-38-46 (1)
38-46:⚠️ Potential issue | 🟡 MinorEdge case: empty
featuresarray results in rendering hidden content.If
config.featuresis set to an empty array[],visibleTabswill be empty and no tabs will render. However,activeTabfalls back to"config"andActiveComponentwill still renderConfigManager, creating a disconnected UI state.Consider handling the empty tabs case explicitly:
🛠️ Proposed fix: handle empty visible tabs
const visibleTabs = features ? allTabs.filter((t) => features.includes(t.id)) : allTabs; + if (visibleTabs.length === 0) { + return ( + <div style={{ padding: 20, color: "#6b7280" }}> + No features enabled. Check your configuration. + </div> + ); + } + const [activeTab, setActiveTab] = useState<Tab>( - visibleTabs.find((t) => t.id === defaultTab)?.id ?? visibleTabs[0]?.id ?? "config", + visibleTabs.find((t) => t.id === defaultTab)?.id ?? visibleTabs[0].id, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/SuperpositionAdmin.tsx` around lines 38 - 46, visibleTabs can be empty when features is an empty array, but activeTab currently defaults to "config" and ActiveComponent renders regardless, producing a disconnected UI; update the logic so that visibleTabs being empty is handled explicitly: compute visibleTabs as now, but set activeTab to visibleTabs[0]?.id ?? null (and update the generic type if needed), add an effect that resets setActiveTab whenever visibleTabs changes (e.g., to visibleTabs[0]?.id ?? null), and guard rendering of ActiveComponent (from tabComponents[activeTab]) so it only renders when activeTab is non-null and exists in tabComponents; reference visibleTabs, activeTab, setActiveTab, ActiveComponent, and tabComponents when making these changes.
🧹 Nitpick comments (24)
clients/admin-ui/tsconfig.json (1)
12-12: Avoid emitting test declarations from the library tsconfig.With
declaration: trueandinclude: ["src", "__tests__"],tsccan emit test.d.tsfiles intodist. Prefer keeping this config build-focused (srconly) and moving test typing to a separate test tsconfig.♻️ Minimal change
- "include": ["src", "__tests__"], + "include": ["src"],Also applies to: 26-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/tsconfig.json` at line 12, The library tsconfig enables "declaration": true while also including tests, causing .d.ts for tests to be emitted; update the tsconfig used for building the admin-ui library so that the "include" (or "files") only contains "src" (remove "__tests__") or move test sources into a separate test tsconfig (e.g., tsconfig.test.json) and leave "declaration": true only on the build tsconfig; target the "declaration" setting in clients/admin-ui/tsconfig.json and ensure test typing is produced by the separate test config instead of the main library config.clients/admin-ui/src/providers/AlertProvider.tsx (1)
32-38: Clean up auto-dismiss timers on provider unmount.
setTimeoutcallbacks are not tracked/cleared, so timers can outlive the provider. Add cleanup for better reliability.♻️ Suggested approach
-import React, { createContext, useCallback, useContext, useState } from "react"; +import React, { createContext, useCallback, useContext, useEffect, useRef, useState } from "react"; @@ export function AlertProvider({ children }: { children: React.ReactNode }) { const [alerts, setAlerts] = useState<Alert[]>([]); + const timeoutsRef = useRef<number[]>([]); @@ - setTimeout(() => { + const timeoutId = window.setTimeout(() => { setAlerts((prev) => prev.filter((a) => a.id !== id)); }, 5000); + timeoutsRef.current.push(timeoutId); }, []); + + useEffect(() => { + return () => { + timeoutsRef.current.forEach((id) => clearTimeout(id)); + timeoutsRef.current = []; + }; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/providers/AlertProvider.tsx` around lines 32 - 38, The addAlert implementation schedules timeouts that aren’t cleared on unmount; update the AlertProvider to track timer IDs (e.g., a useRef holding a Set or Map of timeout IDs keyed by alert id) when addAlert calls setTimeout, store the returned timer ID, and clear that specific timeout if the alert is removed early (in the same removal callback) and also clear all remaining timers in a cleanup returned from useEffect when AlertProvider unmounts; update references to addAlert, alertCounter, and setAlerts accordingly so every scheduled timeout is cancelled on unmount or when its alert is dismissed.clients/admin-ui/__tests__/providers/AlertProvider.test.tsx (1)
70-72: Consider referencing the timeout constant to avoid test brittleness.The test uses a hardcoded
5100ms value that's slightly more than the actual5000ms timeout inAlertProvider.tsx. If the timeout constant changes, this test could silently pass for any value ≤ 5100ms. Consider either:
- Exporting the timeout constant from
AlertProvider.tsxand using it here- Adding a comment explaining the relationship to the source timeout
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/providers/AlertProvider.test.tsx` around lines 70 - 72, The test advances timers by a hardcoded 5100ms which mirrors the hide timeout in AlertProvider.tsx and is brittle; export the timeout constant (e.g., ALERT_HIDE_TIMEOUT or similar) from AlertProvider (where the 5000ms is defined) and import it into AlertProvider.test.tsx, then replace the literal 5100 with a value derived from that exported constant (e.g., advanceTimersByTime(ALERT_HIDE_TIMEOUT + someGraceMs)) or add an inline comment referencing the original constant if you prefer not to export it; update references to AlertProvider and the timeout constant names accordingly so the test tracks future changes to the source timeout.clients/admin-ui/src/components/JsonViewer.tsx (1)
45-58: Consider adding an accessible label for screen readers.The expand/collapse button only has visible text. Adding an
aria-expandedattribute would improve accessibility.♿ Accessibility improvement
<button onClick={() => setExpanded(!expanded)} + aria-expanded={expanded} + aria-label={expanded ? "Collapse JSON" : "Expand JSON"} style={{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/JsonViewer.tsx` around lines 45 - 58, The expand/collapse button in JsonViewer.tsx lacks accessible state; update the button rendered where setExpanded and expanded are used to include aria-expanded={expanded} and an appropriate aria-label (e.g., "Expand JSON" / "Collapse JSON" or a single label that reflects the current state) so screen readers receive state info; ensure the aria-expanded value is wired to the expanded boolean and the onClick still toggles via setExpanded(!expanded).clients/admin-ui/__tests__/api/client.test.ts (1)
100-104: Avoid making two fetch calls for the same assertion.Lines 100 and 101 each call
client.get("/missing"), resulting in two separate fetch calls. Capture the promise once:♻️ Consolidated assertion
- await expect(client.get("/missing")).rejects.toThrow(SuperpositionApiError); - await expect(client.get("/missing")).rejects.toMatchObject({ - status: 404, - body: "Not found", - }); + const promise = client.get("/missing"); + await expect(promise).rejects.toThrow(SuperpositionApiError); + await expect(promise).rejects.toMatchObject({ + status: 404, + body: "Not found", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/api/client.test.ts` around lines 100 - 104, The test currently calls client.get("/missing") twice causing two fetches; change it to call client.get("/missing") once, store the returned promise in a variable (e.g., const promise = client.get("/missing")), then use that single promise in both assertions (expect(promise).rejects.toThrow(SuperpositionApiError) and expect(promise).rejects.toMatchObject({...})), ensuring you reference the same promise for both checks and keep the assertions on the single promise returned by client.get.clients/admin-ui/src/hooks/useApi.ts (1)
64-80: Potential stale closure ifmutationFnis not memoized by caller.The
mutatecallback depends onmutationFn(line 79), but if the caller passes an inline function,mutatewill be recreated on every render. This could cause issues if components depend on stablemutatereferences.Consider using a ref pattern similar to
useApi'sfetcherRef:♻️ Proposed fix using ref pattern
export function useMutation<TArgs extends unknown[], TResult>( mutationFn: (...args: TArgs) => Promise<TResult>, ): { mutate: (...args: TArgs) => Promise<TResult>; loading: boolean; error: string | null; } { const [loading, setLoading] = useState(false); const [error, setError] = useState<string | null>(null); + const mutationFnRef = useRef(mutationFn); + mutationFnRef.current = mutationFn; const mutate = useCallback( async (...args: TArgs): Promise<TResult> => { setLoading(true); setError(null); try { - const result = await mutationFn(...args); + const result = await mutationFnRef.current(...args); return result; } catch (err) { const msg = err instanceof Error ? err.message : String(err); setError(msg); throw err; } finally { setLoading(false); } }, - [mutationFn], + [], ); return { mutate, loading, error }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/hooks/useApi.ts` around lines 64 - 80, The mutate callback captures mutationFn and can cause stale/unstable references if callers pass inline functions; change to the ref pattern used by fetcherRef: create a mutationFnRef that is updated whenever mutationFn changes (e.g., in an effect or assignment), then have mutate (created with useCallback and an empty/depsafe array) call mutationFnRef.current(...args); keep the existing setLoading/setError logic and ensure mutationFnRef is updated whenever mutationFn changes so mutate remains stable even if callers don't memoize their function.clients/admin-ui/__tests__/components/Pagination.test.tsx (2)
6-11: Improve the "renders nothing" assertion.
container.innerHTMLbeing falsy is fragile — an empty container might still have whitespace or an empty wrapper. Consider checkingcontainer.firstChild:♻️ More robust empty check
it("renders nothing for single page", () => { const { container } = render( <Pagination currentPage={1} totalPages={1} onPageChange={() => {}} />, ); - expect(container.innerHTML).toBeFalsy(); + expect(container.firstChild).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/components/Pagination.test.tsx` around lines 6 - 11, Update the "renders nothing for single page" test for the Pagination component so it asserts the container is truly empty by checking container.firstChild instead of relying on container.innerHTML being falsy; in the test that renders <Pagination currentPage={1} totalPages={1} ... />, replace the fragile expect(container.innerHTML).toBeFalsy() with a robust assertion such as expect(container.firstChild).toBeNull() to ensure no DOM nodes are present.
18-22: Consider usingtoBeInTheDocument()for clearer assertions.Using
toBeDefined()withgetByTextis redundant sincegetByTextthrows if the element isn't found. ThetoBeInTheDocument()matcher from@testing-library/jest-domis more idiomatic and self-documenting.♻️ More idiomatic assertions
- expect(screen.getByText("1")).toBeDefined(); - expect(screen.getByText("2")).toBeDefined(); - expect(screen.getByText("3")).toBeDefined(); - expect(screen.getByText("Previous")).toBeDefined(); - expect(screen.getByText("Next")).toBeDefined(); + expect(screen.getByText("1")).toBeInTheDocument(); + expect(screen.getByText("2")).toBeInTheDocument(); + expect(screen.getByText("3")).toBeInTheDocument(); + expect(screen.getByText("Previous")).toBeInTheDocument(); + expect(screen.getByText("Next")).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/components/Pagination.test.tsx` around lines 18 - 22, Replace the redundant toBeDefined() assertions in Pagination.test.tsx with the more idiomatic toBeInTheDocument() matcher (e.g., change expect(screen.getByText("1")).toBeDefined() to expect(screen.getByText("1")).toBeInTheDocument()) for each occurrence ("1","2","3","Previous","Next"); also ensure `@testing-library/jest-dom` is imported/setup so toBeInTheDocument() is available (or add import '@testing-library/jest-dom' to the test setup if missing).clients/admin-ui/src/api/client.ts (2)
82-86: Type safety concern:undefined as Tmay mislead callers.When a 204 or zero-content response is returned,
undefined as Ttells TypeScript the return is of typeT, but it's actuallyundefined. Callers expectingTmay encounter runtime errors.Consider:
- Changing the return type to
Promise<T | undefined>for methods that can return 204- Or creating a separate
deleteVoidmethod that explicitly returnsPromise<void>♻️ Option: Explicit void method for deletions
- delete<T>(path: string): Promise<T> { - return this.request<T>("DELETE", path); + delete(path: string): Promise<void> { + return this.request<void>("DELETE", path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/api/client.ts` around lines 82 - 86, The current response-handling branch returns "undefined as T" when response.status === 204 or content-length is "0", which misleads TypeScript callers; update the API surface by either (A) changing the function signature that contains this branch to return Promise<T | undefined> and propagate that union through callers, or (B) implement a separate explicit deletion/void variant (e.g., deleteVoid or fetchVoid) that returns Promise<void> and use that for endpoints that can yield 204; modify the branch so it returns undefined (for the Promise<T | undefined> approach) or returns void for the new deleteVoid method, and update call sites to use the new signature/method accordingly (look for the function that contains the response.status check and the return response.json() line to make the changes).
85-87: Consider handling JSON parse errors gracefully.If the server returns a 200 with malformed JSON,
response.json()will throw an uncaught error. Consider wrapping this in a try/catch to provide a more descriptive error message.🛡️ Defensive JSON parsing
- return response.json(); + try { + return await response.json(); + } catch { + throw new SuperpositionApiError( + response.status, + "Invalid JSON response", + url, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/api/client.ts` around lines 85 - 87, Wrap the direct call to response.json() in a try/catch in the function that currently does "return response.json()": attempt to parse with response.json(), and on failure call await response.text() to capture the raw body and then throw a new, descriptive Error that includes the HTTP status and the raw body (and optionally attach the original parse error as the cause) so callers get a clear message for malformed JSON responses; use response.text() and include response.status/response.statusText in the error details.clients/admin-ui/__tests__/api/dimensions.test.ts (1)
76-83: Add a test for theupdateoperation.The test suite covers
list,get,create, anddelete, but theupdatemethod is not tested. Consider adding a test case forapi.update()to ensure consistent coverage across all API operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/api/dimensions.test.ts` around lines 76 - 83, Add a new test case covering api.update similar to the existing create/get/delete tests: mockFetch.mockResolvedValue with an appropriate success response, call api.update("dim-id", payload) (use the same payload shape used by api.create tests), then assert mockFetch was called with a URL containing "/dimension/dim-id", that init.method is the expected HTTP verb used by the implementation (e.g., "PUT" or "PATCH"), and that init.body equals JSON.stringify(payload) and headers include the JSON content-type; reference the api.update method and mockFetch.mock.calls to locate where to add the test.clients/admin-ui/__tests__/pages/ConfigManager.test.tsx (1)
57-99: Extract a shared render helper to remove repeated provider boilerplate.Line 57 through Line 99 repeats the same provider composition in every test, which makes future test updates noisy. Consider centralizing it into one helper.
♻️ Suggested refactor
+function renderConfigManager(configOverrides: Partial<{ + host: string; + orgId: string; + workspace: string; +}> = {}) { + return render( + <SuperpositionProvider + config={{ host: "https://test.com", orgId: "org", workspace: "ws", ...configOverrides }} + > + <AlertProvider> + <ConfigManager /> + </AlertProvider> + </SuperpositionProvider>, + ); +} + it("renders config list", async () => { - render( - <SuperpositionProvider - config={{ host: "https://test.com", orgId: "org", workspace: "ws" }} - > - <AlertProvider> - <ConfigManager /> - </AlertProvider> - </SuperpositionProvider>, - ); + renderConfigManager(); ... }); it("shows create button", async () => { - render( - <SuperpositionProvider - config={{ host: "https://test.com", orgId: "org", workspace: "ws" }} - > - <AlertProvider> - <ConfigManager /> - </AlertProvider> - </SuperpositionProvider>, - ); + renderConfigManager(); ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/pages/ConfigManager.test.tsx` around lines 57 - 99, Extract a shared test render helper that wraps components with the repeated SuperpositionProvider and AlertProvider usage to remove boilerplate; create a function (e.g., renderWithProviders or renderConfigManager) that takes a React node or props and calls the test renderer inside the shared provider composition, then replace the inline render(...) calls in the tests that render ConfigManager with this helper (references: ConfigManager, SuperpositionProvider, AlertProvider, and the existing test cases "renders config list", "shows create button", "shows heading").clients/admin-ui/__tests__/providers/SuperpositionProvider.test.tsx (1)
42-47: Strengthen provider wiring assertions beyond truthiness.Line 42 through Line 47 only verifies objects exist. A broken wiring that returns empty objects would still pass. Assert key method bindings (e.g.,
dimensions.list,defaultConfigs.list,overrides.list,experiments.list) to catch regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/__tests__/providers/SuperpositionProvider.test.tsx` around lines 42 - 47, The tests currently only assert truthiness for provider props; update the assertions in SuperpositionProvider.test.tsx to verify key methods are bound and callable by checking that dimensions.list, defaultConfigs.list, overrides.list, and experiments.list are functions (or have expected behavior) and optionally call them (mocking return values) to assert they return expected results; locate the test expectations referencing "has-client", "has-dims", "has-configs", "has-overrides", and "has-experiments" and replace or augment them with assertions that screen.getByTestId("has-dims").textContent (or the underlying prop) exposes a callable dimensions.list, similarly assert defaultConfigs.list, overrides.list, and experiments.list so empty objects no longer pass.clients/admin-ui/src/api/overrides.ts (1)
44-46: Consider stronger typing forbulkOperations.The
unknown[]type for operations provides flexibility but loses type safety. If the bulk operations schema is well-defined, consider creating a dedicated type (e.g.,BulkOperation) to improve type safety and self-documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/api/overrides.ts` around lines 44 - 46, The bulkOperations method currently accepts unknown[] which loses type safety; define a dedicated interface/type (e.g., BulkOperation) that models the expected shape of each operation, update the bulkOperations signature to accept BulkOperation[] (or Array<BulkOperation> | ReadonlyArray<BulkOperation> as appropriate), and update any callsites and tests to use the new type; keep the client.put call in bulkOperations as-is but ensure its payload is typed accordingly so the compiler enforces correct operation structure.clients/admin-ui/src/pages/AuditLog.tsx (1)
95-99: Consider resetting page when filter changes.When
actionFilterchanges, the user might be on a page that no longer exists in the filtered result set. Consider resettingpageto 1 when the filter is toggled to avoid showing an empty or out-of-bounds page.♻️ Proposed fix
onClick={() => - setActionFilter((prev) => - prev.includes(a) ? prev.filter((x) => x !== a) : [...prev, a], - ) + setActionFilter((prev) => { + setPage(1); + return prev.includes(a) ? prev.filter((x) => x !== a) : [...prev, a]; + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/AuditLog.tsx` around lines 95 - 99, Toggling the action filter via the onClick handler that calls setActionFilter can leave the current pagination page out-of-range; update the handler so that whenever actionFilter is changed you also reset the page state (e.g., call setPage(1)) — modify the onClick logic that updates setActionFilter to also call setPage(1) (or include page reset in a combined state updater) so filtering always returns the user to the first page.clients/admin-ui/src/components/Modal.tsx (1)
33-43: Consider adding focus management for better accessibility.The modal has good ARIA attributes but lacks focus trapping (Tab can escape to background elements) and doesn't restore focus to the triggering element on close. The
refon line 34 is unused and could be leveraged for focus management.For an admin UI this may be acceptable, but consider adding focus trapping for keyboard-only users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/Modal.tsx` around lines 33 - 43, The Modal currently adds Escape handling but doesn't manage focus; use the existing ref (ref) and the Modal component to implement focus trapping and restore focus: on open, save document.activeElement, focus the modal or its first focusable child, and add a keydown handler (in the same useEffect or a new one tied to open) that intercepts Tab/Shift+Tab to keep focus inside the modal container (using ref.current.querySelectorAll to find focusable elements), and on close restore focus to the previously focused element and remove the keydown listener; update the cleanup to remove listeners and clear any saved state.clients/admin-ui/src/pages/OverrideManager.tsx (1)
78-96: JSON parse errors will show generic mutation error message.Similar to ExperimentManager, when
JSON.parse(newContext)orJSON.parse(newOverride)throws, the catch block showscreateMutation.errorwhich won't contain the actual parse error. Consider providing specific feedback for JSON syntax errors.♻️ Proposed fix
const handleCreate = async () => { try { - const ctx = mergeScopedContext(JSON.parse(newContext), scopedContext); + let ctx: Record<string, unknown>; + let override: Record<string, unknown>; + try { + ctx = mergeScopedContext(JSON.parse(newContext), scopedContext); + } catch { + addAlert("error", "Invalid JSON in context field"); + return; + } + try { + override = JSON.parse(newOverride); + } catch { + addAlert("error", "Invalid JSON in override field"); + return; + } await createMutation.mutate({ context: ctx, - override: JSON.parse(newOverride), + override, description: newDesc || undefined, change_reason: newReason || "Created via admin UI", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/OverrideManager.tsx` around lines 78 - 96, handleCreate currently parses newContext and newOverride inline so a JSON.parse SyntaxError ends up swallowed and addAlert shows createMutation.error; change it to validate/parse newContext and newOverride before calling createMutation (e.g., try parsing each into ctx and overrideVars with JSON.parse and if a SyntaxError occurs call addAlert("error", `Invalid JSON: ${error.message}`) and return), then call mergeScopedContext and createMutation.mutate with the parsed objects as before; reference functions/variables: handleCreate, mergeScopedContext, createMutation, newContext, newOverride, addAlert.clients/admin-ui/src/pages/ExperimentManager.tsx (1)
88-109: JSON parse errors will show generic mutation error message.When
JSON.parse(newContext)orJSON.parse(newVariants)throws aSyntaxError, the catch block showscreateMutation.errorwhich won't contain the parse error details. Consider providing more specific feedback for JSON syntax errors.♻️ Proposed fix
const handleCreate = async () => { try { - const ctx = scopedContext - ? { ...JSON.parse(newContext), ...scopedContext } - : JSON.parse(newContext); + let ctx: Record<string, unknown>; + let variants: unknown[]; + try { + ctx = scopedContext + ? { ...JSON.parse(newContext), ...scopedContext } + : JSON.parse(newContext); + } catch { + addAlert("error", "Invalid JSON in context field"); + return; + } + try { + variants = JSON.parse(newVariants); + } catch { + addAlert("error", "Invalid JSON in variants field"); + return; + } await createMutation.mutate({ name: newName, context: ctx, - variants: JSON.parse(newVariants), + variants, description: newDesc || "No description", change_reason: newReason || "Created via admin UI", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/ExperimentManager.tsx` around lines 88 - 109, The handleCreate function currently parses newContext and newVariants inline so a JSON.parse SyntaxError is swallowed by the outer catch that only shows createMutation.error; change the flow to first attempt parsing newContext and newVariants inside their own try/catch (or validate upfront) and if parsing fails call addAlert("error", `Invalid JSON in context: ${err.message}`) or similar, then proceed to call createMutation.mutate in a separate try/catch that uses addAlert("error", createMutation.error || err.message || "Failed to create experiment") on mutation failure; update references: handleCreate, newContext, newVariants, createMutation, addAlert to implement this split-error handling so JSON syntax errors are surfaced separately from server/mutation errors.clients/admin-ui/src/components/Table.tsx (1)
3-8: Consider typingkeyaskeyof Tfor better type safety.Currently,
keyisstringwhich allows any value, but the default cell renderer accessesrow[col.key]. Typing it askeyof T & stringwould catch mismatches at compile time when not using a customrenderfunction.💡 Alternative with stricter typing
-export interface Column<T> { - key: string; +export interface Column<T, K extends keyof T = keyof T> { + key: K | string; // string allows action columns that don't map to data header: string; render?: (row: T) => React.ReactNode; width?: string; }Note: Keeping
| stringallows "virtual" columns like"actions"that don't correspond to actual data fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/components/Table.tsx` around lines 3 - 8, The Column<T> interface types the column "key" as string which permits invalid keys when the default renderer accesses row[col.key]; change the "key" type to constrain it to actual properties of T by using keyof T & string (or keyof T | string if you need to allow virtual keys like "actions") so TypeScript catches mismatches; update usages of Column<T>, and ensure the default cell renderer that reads row[col.key] compiles against the new keyof typing and adjust any places passing arbitrary strings to either use a custom render or the allowed union form.clients/admin-ui/src/pages/DimensionManager.tsx (2)
98-134: Stale closure risk in columns definition.The
columnsuseMemo has an empty dependency array but captureshandleDeletewhich referencesdeleteMutation. IfdeleteMutationis not referentially stable across renders, the delete action could use stale state.Either add
handleDeleteto the dependency array or verify thatuseMutationreturns stable function references.♻️ Proposed fix: include handleDelete in dependencies
], - // eslint-disable-next-line react-hooks/exhaustive-deps - [], + [handleDelete], );This requires wrapping
handleDeleteinuseCallback:- const handleDelete = async (name: string) => { + const handleDelete = useCallback(async (name: string) => { if (!confirm(`Delete dimension "${name}"?`)) return; try { await deleteMutation.mutate(name); refetch(); } catch { addAlert("error", deleteMutation.error || "Failed to delete"); } - }; + }, [deleteMutation, refetch, addAlert]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/DimensionManager.tsx` around lines 98 - 134, The columns useMemo captures handleDelete causing a stale-closure risk; to fix, make handleDelete referentially stable (wrap the existing handleDelete in useCallback) and include that handleDelete in the dependency array of the columns useMemo (or alternatively add deleteMutation if you prefer to depend on the mutation directly). Ensure you reference the same handleDelete name used in the render function and update the useMemo dependency list rather than disabling eslint for exhaustive-deps.
88-96: Consider replacingconfirm()with a confirmation modal.Using
window.confirm()works but has limitations:
- Not testable without mocking globals
- Cannot be styled to match the UI
- Blocks the main thread
The codebase already has a
Modalcomponent that could be adapted for confirmation dialogs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/pages/DimensionManager.tsx` around lines 88 - 96, Replace the blocking window.confirm call in handleDelete with a non-blocking Modal flow: add local state (e.g., confirmOpen:boolean and pendingDeleteName:string) and render the existing Modal (or a small ConfirmModal variant) that shows the message and exposes onConfirm/onCancel callbacks; when user clicks delete in the UI, set pendingDeleteName and open the modal, and in the modal's onConfirm call perform deleteMutation.mutate(pendingDeleteName) then refetch() and addAlert on error (using deleteMutation.error || "Failed to delete"), and onCancel just close the modal—update handleDelete to only open the modal and move the actual deletion logic into the modal confirm handler.clients/admin-ui/src/providers/SuperpositionProvider.tsx (1)
41-59: Consider stabilizing useMemo dependencies to prevent unnecessary re-instantiation.The
useMemodepends on the entireconfigobject reference. If the parent component re-renders and passes a new object reference (even with identical values), the client and all API modules will be recreated. This could cause unnecessary work and potentially disrupt in-flight requests.Options to consider:
- Document that consumers should memoize the config object
- Use a deep comparison or serialize config fields as dependencies
💡 Alternative: Destructure stable dependencies
export function SuperpositionProvider({ config, children, }: SuperpositionProviderProps) { + const { host, orgId, workspace, auth } = config; const value = useMemo<SuperpositionContextValue>(() => { const client = new SuperpositionClient({ - host: config.host, - orgId: config.orgId, - workspace: config.workspace, - auth: config.auth, + host, + orgId, + workspace, + auth, }); return { config, client, dimensions: dimensionsApi(client), defaultConfigs: defaultConfigsApi(client), overrides: overridesApi(client), experiments: experimentsApi(client), resolve: resolveApi(client), audit: auditApi(client), }; - }, [config]); + }, [config, host, orgId, workspace, auth]);Note: The
authobject reference would still need stable identity from the consumer, or you could serialize it (e.g.,JSON.stringify(auth)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/providers/SuperpositionProvider.tsx` around lines 41 - 59, The useMemo in SuperpositionProvider currently depends on the whole config object which causes SuperpositionClient and all APIs to be re-created when a new config reference is passed; update the dependency list for the useMemo that builds the SuperpositionContextValue to use stable primitives (e.g., config.host, config.orgId, config.workspace) and a stable representation of auth (either require consumers to memoize auth or use a serialized form like JSON.stringify(config.auth)) so SuperpositionClient and the API factories (dimensionsApi, defaultConfigsApi, overridesApi, experimentsApi, resolveApi, auditApi) only re-instantiate when actual config values change.clients/admin-ui/src/utils/context-filter.ts (1)
63-68: Always return a fresh context object frommergeScopedContext.Line 67 currently returns the original
userContextreference whenscopedContextis absent, even though this helper is documented as producing a new merged context. That makes the no-scope path behave differently and can leak mutations back into caller state.Suggested fix
export function mergeScopedContext( userContext: Condition, scopedContext?: ScopedContext, ): Condition { - if (!scopedContext) return userContext; - return { ...userContext, ...scopedContext }; + return { ...userContext, ...(scopedContext ?? {}) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/utils/context-filter.ts` around lines 63 - 68, mergeScopedContext currently returns the original userContext reference when scopedContext is undefined, which can leak mutations; change the no-scope path in mergeScopedContext to return a fresh shallow copy of userContext (e.g., construct and return a new object with the same entries) so the function always returns a new Condition object and preserves immutability between callers and the helper.clients/admin-ui/src/index.ts (1)
27-34: Re-exportScopedContextwith the scoped helpers.These helpers are part of the root package API, but the shared
ScopedContexttype fromclients/admin-ui/src/utils/context-filter.tsis not. Consumers will otherwise need a deep import just to type the same object they pass intocontextMatchesScopeand thefilter*ByScopehelpers.Suggested fix
export { contextMatchesScope, filterOverridesByScope, filterExperimentsByScope, mergeScopedContext, getLockedDimensions, } from "./utils"; +export type { ScopedContext } from "./utils/context-filter";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/index.ts` around lines 27 - 34, Re-export the Shared ScopedContext type from the utils module alongside the scoped helper functions so consumers can import the type without a deep import; specifically add an export for ScopedContext (the type defined in utils/context-filter.ts) to the same export block that currently re-exports contextMatchesScope, filterOverridesByScope, filterExperimentsByScope, mergeScopedContext, and getLockedDimensions (i.e., ensure ScopedContext is exported from the package entry so callers can type values passed to contextMatchesScope and the filter*ByScope helpers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbaac5e9-72f5-4eb7-a660-6c4875a4425d
⛔ Files ignored due to path filters (1)
clients/admin-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
PLAN-embeddable-admin-ui.mdclients/admin-ui/.prettierrcclients/admin-ui/__tests__/api/client.test.tsclients/admin-ui/__tests__/api/context-filter.test.tsclients/admin-ui/__tests__/api/dimensions.test.tsclients/admin-ui/__tests__/components/Modal.test.tsxclients/admin-ui/__tests__/components/Pagination.test.tsxclients/admin-ui/__tests__/components/Table.test.tsxclients/admin-ui/__tests__/pages/ConfigManager.test.tsxclients/admin-ui/__tests__/pages/OverrideManager.test.tsxclients/admin-ui/__tests__/providers/AlertProvider.test.tsxclients/admin-ui/__tests__/providers/SuperpositionProvider.test.tsxclients/admin-ui/__tests__/setup.tsclients/admin-ui/package.jsonclients/admin-ui/src/api/audit.tsclients/admin-ui/src/api/client.tsclients/admin-ui/src/api/default-configs.tsclients/admin-ui/src/api/dimensions.tsclients/admin-ui/src/api/experiments.tsclients/admin-ui/src/api/index.tsclients/admin-ui/src/api/overrides.tsclients/admin-ui/src/api/resolve.tsclients/admin-ui/src/components/AlertBar.tsxclients/admin-ui/src/components/ConditionBadges.tsxclients/admin-ui/src/components/FormField.tsxclients/admin-ui/src/components/JsonViewer.tsxclients/admin-ui/src/components/Modal.tsxclients/admin-ui/src/components/Pagination.tsxclients/admin-ui/src/components/StatusBadge.tsxclients/admin-ui/src/components/Table.tsxclients/admin-ui/src/components/index.tsclients/admin-ui/src/hooks/index.tsclients/admin-ui/src/hooks/useApi.tsclients/admin-ui/src/index.tsclients/admin-ui/src/pages/AuditLog.tsxclients/admin-ui/src/pages/ConfigManager.tsxclients/admin-ui/src/pages/DimensionManager.tsxclients/admin-ui/src/pages/ExperimentManager.tsxclients/admin-ui/src/pages/OverrideManager.tsxclients/admin-ui/src/pages/ResolveView.tsxclients/admin-ui/src/pages/SuperpositionAdmin.tsxclients/admin-ui/src/pages/index.tsclients/admin-ui/src/providers/AlertProvider.tsxclients/admin-ui/src/providers/SuperpositionProvider.tsxclients/admin-ui/src/providers/index.tsclients/admin-ui/src/types/api.tsclients/admin-ui/src/types/index.tsclients/admin-ui/src/utils/context-filter.tsclients/admin-ui/src/utils/format.tsclients/admin-ui/src/utils/index.tsclients/admin-ui/tsconfig.jsonclients/admin-ui/vite.config.tsclients/admin-ui/vitest.config.ts
| it("filters overrides by scoped context", async () => { | ||
| render( | ||
| <SuperpositionProvider | ||
| config={{ | ||
| host: "https://test.com", | ||
| orgId: "org", | ||
| workspace: "ws", | ||
| context: { region: "us-east-1" }, | ||
| }} | ||
| > | ||
| <AlertProvider> | ||
| <OverrideManager /> | ||
| </AlertProvider> | ||
| </SuperpositionProvider>, | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| // Should show US override (region matches) and EU override (region doesn't conflict because us-east-1 ≠ eu-west-1) | ||
| // Actually: ctx-2 has region: eu-west-1 but scoped is region: us-east-1 | ||
| // contextMatchesScope: condition[region] = eu-west-1 !== us-east-1 → no match | ||
| expect(screen.getByText("Context Overrides")).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Scoped filtering behavior is untested (false-positive test).
Line 73 through Line 94 should verify filtering by config.context, but it currently only checks the heading, so filtering regressions won’t be caught.
✅ Suggested assertion upgrade
it("filters overrides by scoped context", async () => {
render(
<SuperpositionProvider
config={{
host: "https://test.com",
orgId: "org",
workspace: "ws",
context: { region: "us-east-1" },
}}
>
<AlertProvider>
<OverrideManager />
</AlertProvider>
</SuperpositionProvider>,
);
await waitFor(() => {
- // Should show US override (region matches) and EU override (region doesn't conflict because us-east-1 ≠ eu-west-1)
- // Actually: ctx-2 has region: eu-west-1 but scoped is region: us-east-1
- // contextMatchesScope: condition[region] = eu-west-1 !== us-east-1 → no match
- expect(screen.getByText("Context Overrides")).toBeDefined();
+ expect(screen.getByText("us-east-1")).toBeDefined();
+ expect(screen.queryByText("eu-west-1")).toBeNull();
});
});📝 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.
| it("filters overrides by scoped context", async () => { | |
| render( | |
| <SuperpositionProvider | |
| config={{ | |
| host: "https://test.com", | |
| orgId: "org", | |
| workspace: "ws", | |
| context: { region: "us-east-1" }, | |
| }} | |
| > | |
| <AlertProvider> | |
| <OverrideManager /> | |
| </AlertProvider> | |
| </SuperpositionProvider>, | |
| ); | |
| await waitFor(() => { | |
| // Should show US override (region matches) and EU override (region doesn't conflict because us-east-1 ≠ eu-west-1) | |
| // Actually: ctx-2 has region: eu-west-1 but scoped is region: us-east-1 | |
| // contextMatchesScope: condition[region] = eu-west-1 !== us-east-1 → no match | |
| expect(screen.getByText("Context Overrides")).toBeDefined(); | |
| }); | |
| it("filters overrides by scoped context", async () => { | |
| render( | |
| <SuperpositionProvider | |
| config={{ | |
| host: "https://test.com", | |
| orgId: "org", | |
| workspace: "ws", | |
| context: { region: "us-east-1" }, | |
| }} | |
| > | |
| <AlertProvider> | |
| <OverrideManager /> | |
| </AlertProvider> | |
| </SuperpositionProvider>, | |
| ); | |
| await waitFor(() => { | |
| expect(screen.getByText("us-east-1")).toBeDefined(); | |
| expect(screen.queryByText("eu-west-1")).toBeNull(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/__tests__/pages/OverrideManager.test.tsx` around lines 73 -
94, The test "filters overrides by scoped context" currently only asserts the
heading and misses validating actual filtering; update the test that renders
OverrideManager inside SuperpositionProvider (with config.context) to assert
that overrides whose scope matches config.context (e.g., the US override) are
present and that non-matching scoped overrides (e.g., the EU override / ctx-2)
are not rendered; use the existing render/waitFor and screen matchers
(screen.getByText / queryByText or getByRole) to explicitly expect presence of
the matching override label and absence (null) of the non-matching override so
the test will fail if contextMatchesScope logic in OverrideManager breaks.
| @@ -0,0 +1,56 @@ | |||
| { | |||
| "name": "@anthropic/superposition-admin-ui", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json, pathlib
for p in pathlib.Path(".").rglob("package.json"):
try:
data = json.loads(p.read_text())
except Exception:
continue
if "name" in data:
print(f"{p}: {data['name']}")
PYRepository: juspay/superposition
Length of output: 834
Fix package scope mismatch before publishing.
The @anthropic scope at line 2 is inconsistent with the repository ownership (juspay/superposition) and conflicts with all other packages in the project. A scan of all package.json files shows this is the only package using the @anthropic scope; all others use unscoped names (e.g., superposition-client, superposition-sdk, superposition_browser_client). Publishing under a scope you do not own will fail or create namespace conflicts. Update the package name to match the repository convention, such as @juspay/superposition-admin-ui or superposition-admin-ui.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/package.json` at line 2, The package name currently uses the
incorrect scope "@anthropic/superposition-admin-ui" in the package.json "name"
field; change that value to match repository conventions (either an unscoped
"superposition-admin-ui" or the org-scoped "@juspay/superposition-admin-ui") so
it aligns with other packages and avoids publishing namespace conflicts—update
the "name" entry accordingly and verify npm publish/tagging workflows if any
rely on the package name.
| "files": [ | ||
| "dist" | ||
| ], | ||
| "scripts": { | ||
| "dev": "vite", | ||
| "build": "tsc && vite build", | ||
| "test": "vitest run", |
There was a problem hiding this comment.
build currently risks publishing compiled test artifacts.
Line 22 runs tsc, and clients/admin-ui/tsconfig.json includes __tests__. With "files": ["dist"] (Lines 17-19), test outputs can be shipped in the package.
📦 Suggested packaging fix
"scripts": {
- "build": "tsc && vite build",
+ "build": "tsc -p tsconfig.build.json --emitDeclarationOnly && vite build",// tsconfig.build.json (new file)
{
"extends": "./tsconfig.json",
"include": ["src"],
"exclude": ["__tests__", "dist", "node_modules"]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/package.json` around lines 17 - 23, The build script
currently runs plain tsc which compiles __tests__ per
clients/admin-ui/tsconfig.json; create a new tsconfig.build.json that extends
tsconfig.json and restricts compilation to source (include "src" and exclude
"__tests__", "dist", "node_modules"), then update the package.json "build"
script to invoke tsc with that project file (e.g., change the "build" command to
run tsc -p tsconfig.build.json && vite build) so only intended source files are
emitted to dist.
| "peerDependencies": { | ||
| "react": "^18.0.0 || ^19.0.0", | ||
| "react-dom": "^18.0.0 || ^19.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@testing-library/jest-dom": "^6.6.3", | ||
| "@testing-library/react": "^16.1.0", | ||
| "@testing-library/user-event": "^14.5.2", | ||
| "@types/react": "^18.3.12", | ||
| "@types/react-dom": "^18.3.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For a library package, what validation is recommended before declaring peerDependencies as "^18 || ^19" for react/react-dom, and are there known React 19 type/runtime compatibility checks maintainers should run?
💡 Result:
Before you declare peerDependencies: { react: "^18 || ^19", react-dom: "^18 || ^19" }, the most defensible validation is to prove your published artifacts work in both “React 18 consumer” and “React 19 consumer” environments, at both runtime and type-level, because React 19 includes removals/deprecations and TypeScript type changes that can break libraries even when runtime is fine.
Recommended validation for ^18 || ^19 peer deps
-
CI runtime matrix (build + tests)
- Run your test suite against React 18 + ReactDOM 18 and React 19 + ReactDOM 19.
- Make sure you exercise any renderer-specific paths you support (client render, hydration/SSR if applicable).
- Specifically ensure you are not calling APIs removed in React 19, such as:
ReactDOM.unmountComponentAtNode(removed) [1]findDOMNodeusage (removed from supported patterns; migrate to refs) [1]- other removed
react-domunstable APIs listed in the upgrade guide [1]
-
CI type matrix (your library’s public types, not just its build)
- Typecheck your library (and ideally a tiny “consumer” project) with:
typescript@<your supported range>+@types/react@18+@types/react-dom@18typescript@<your supported range>+@types/react@19+@types/react-dom@19[1]
- If you publish
.d.ts, prefer an actual consumer-style check (e.g.,tsd) so you catch breaking exported types, not just internal compilation.
- Typecheck your library (and ideally a tiny “consumer” project) with:
-
No React internals
- Audit for any reliance on React internals (e.g.
SECRET_INTERNALS...), which React explicitly warns can block upgrades [1].
- Audit for any reliance on React internals (e.g.
-
Testing strategy sanity
- If your tests (or docs) rely on
react-test-renderer, expect React 19 deprecation warnings and behavior differences; React recommends migrating to modern testing libraries (e.g. Testing Library) [1].
- If your tests (or docs) rely on
Known React 19 type/runtime compatibility checks maintainers should run
Type-level gotchas (common library-breakers)
- JSX namespace / augmentation changes: if your library augments
JSX(custom elements, intrinsic elements), React 19’s guidance is to scope it viadeclare module "react" { namespace JSX { ... } }(exact module depends on JSX runtime) [1]. - Ref callback typing is stricter: callback refs must not implicitly return a value (because ref cleanup functions exist); this often breaks library typings until fixed [1].
useRefnow requires an initial value in the types (can break code and examples) [1].ReactNode/ renderable types changed in React 19 types: React 19 types were updated to reflect runtime changes like renderingPromise(async rendering patterns), which can create cross-version friction if your public API exposes React 19-only renderable types to React 18 consumers (e.g.,ReactNode | Promise<ReactNode>not assignable toReactNode) [2].
Runtime/API removals to smoke-test
- Anything that still uses legacy root APIs or removed DOM APIs should be validated (or eliminated) following the React 19 upgrade guide (e.g.,
unmountComponentAtNode,findDOMNode, other removedreact-domAPIs) [1].
If you support / mention React Compiler
- React’s guidance for library maintainers is to compile and test your library with the compiler and ship compiled output; if you need to support apps not yet on React 19, you may need to set a minimum compiler
targetand depend onreact-compiler-runtime[3].
Practical rule: don’t ship ^18 || ^19 until you have (1) a React 18/19 runtime CI matrix, and (2) an @types/react@18 + @types/react@19 consumer type-test matrix that covers your exported surface area (especially anything involving JSX, ref, ReactNode, or component return types).
Sources: React 19 upgrade guidance (API removals, ref/test-renderer deprecations, TS changes) [1]; React 19 type-change tracking (including Promise renderability) [2]; React Compiler guidance for library maintainers [3].
[1] https://react.dev/blog/2024/04/25/react-19-upgrade-guide
[2] DefinitelyTyped/DefinitelyTyped#64451
[3] https://react.dev/blog/2024/10/21/react-compiler-beta-release
🏁 Script executed:
cd clients/admin-ui && cat -n package.json | head -50Repository: juspay/superposition
Length of output: 1956
🏁 Script executed:
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name ".github" \) | head -20Repository: juspay/superposition
Length of output: 678
🏁 Script executed:
rg -l "react.*19|React.*19" --type json --type md --type ymlRepository: juspay/superposition
Length of output: 91
🏁 Script executed:
cat -n ./.github/workflows/ci_check_pr.yamlRepository: juspay/superposition
Length of output: 19030
🏁 Script executed:
rg "19" clients/admin-ui/ --type jsonRepository: juspay/superposition
Length of output: 2659
🏁 Script executed:
rg -i "react.*19|react 19|version.*19" clients/admin-ui/ -A 2 -B 2Repository: juspay/superposition
Length of output: 2159
🏁 Script executed:
cat -n clients/admin-ui/README.md 2>/dev/null | head -50Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
find clients/admin-ui/src -name "*.tsx" -o -name "*.ts" | head -5 | xargs grep -l "unmountComponentAtNode\|findDOMNode" 2>/dev/nullRepository: juspay/superposition
Length of output: 48
🏁 Script executed:
rg "react" clients/admin-ui/README.md -i -A 3 -B 1 2>/dev/null || echo "No README or grep found React references"Repository: juspay/superposition
Length of output: 104
Add React 19 validation to CI or narrow peerDependencies to React 18 only.
This package declares react/react-dom as ^18.0.0 || ^19.0.0 but all dev/type dependencies are pinned to React 18.x (@types/react@^18.3.12, react@^18.3.1, etc.) and CI does not test against React 19. Before shipping dual-version support to consumers, validate both runtime (test suite against React 19) and types (typecheck with @types/react@19) in CI. Otherwise, narrow peerDependencies to ^18.0.0 until React 19 compatibility is confirmed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/package.json` around lines 31 - 40, The package.json
declares peerDependencies "react" and "react-dom" as "^18.0.0 || ^19.0.0" while
devDependencies and types (e.g., "@types/react": "^18.3.12") are pinned to React
18; either update CI to validate React 19 or narrow the peerDependencies to
React 18 only. To fix, either (A) add React 19 job(s) to the CI matrix that
install react@^19 and react-dom@^19, run the test suite and run typechecking
with `@types/react`@^19 (or install `@types/react`@^19 in devDependencies for that
job), or (B) change the package.json peerDependencies entries "react" and
"react-dom" to "^18.0.0" to match current dev/type deps until full React 19
validation is added.
| getConfig( | ||
| context: Record<string, JsonValue> = {}, | ||
| ): Promise<Config> { | ||
| return client.get("/config", context); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect query serialization behavior in client
rg -n -C4 'get<T>\(|request<T>\(|URLSearchParams|query' clients/admin-ui/src/api/client.tsRepository: juspay/superposition
Length of output: 1328
🏁 Script executed:
# View the full buildQueryString implementation
sed -n '40,80p' clients/admin-ui/src/api/client.tsRepository: juspay/superposition
Length of output: 1345
🏁 Script executed:
# Find the JsonValue type definition
rg -n 'type JsonValue|interface JsonValue' clients/admin-ui/srcRepository: juspay/superposition
Length of output: 123
🏁 Script executed:
# View the full JsonValue type definition
sed -n '3,20p' clients/admin-ui/src/types/api.tsRepository: juspay/superposition
Length of output: 503
🏁 Script executed:
# Find usages of getConfig
rg -n 'getConfig\(' clients/admin-ui/src --type ts --type tsxRepository: juspay/superposition
Length of output: 91
🏁 Script executed:
# Find usages of getConfig without type filter
rg -n 'getConfig\(' clients/admin-ui/srcRepository: juspay/superposition
Length of output: 116
🏁 Script executed:
# Search for where the resolve API is imported and used
rg -n 'from.*resolve|import.*resolve' clients/admin-ui/src -A 3Repository: juspay/superposition
Length of output: 591
🏁 Script executed:
# Find where resolveApi.getConfig is called
rg -n 'resolveApi\.getConfig\|\.getConfig\(' clients/admin-ui/src -B 2 -A 5Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Search more broadly for getConfig usage
rg -n 'getConfig' clients/admin-ui/src -B 2 -A 5Repository: juspay/superposition
Length of output: 578
🏁 Script executed:
# Search in SuperpositionProvider for how getConfig is called
sed -n '1,100p' clients/admin-ui/src/providers/SuperpositionProvider.tsx | cat -nRepository: juspay/superposition
Length of output: 2561
🏁 Script executed:
# View the complete resolve.ts file to understand the full context
cat -n clients/admin-ui/src/api/resolve.tsRepository: juspay/superposition
Length of output: 798
🏁 Script executed:
# Check if there are server-side handlers or tests that show how context should be passed
rg -n 'context' clients/admin-ui/src/api/resolve.ts -B 2 -A 2Repository: juspay/superposition
Length of output: 507
Fix unsafe query parameter serialization in getConfig().
The context is passed as query parameters using buildQueryString(), which converts nested objects to "[object Object]" instead of proper JSON serialization. This causes incorrect server-side context matching.
Consider passing context as JSON in the request body (similar to the resolve() method in the same file) or explicitly serializing it:
Possible fix direction
- getConfig(
- context: Record<string, JsonValue> = {},
- ): Promise<Config> {
- return client.get("/config", context);
+ getConfig(
+ context: Record<string, JsonValue> = {},
+ ): Promise<Config> {
+ return Object.keys(context).length === 0
+ ? client.get("/config")
+ : client.get("/config", { context: JSON.stringify(context) });
},📝 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.
| getConfig( | |
| context: Record<string, JsonValue> = {}, | |
| ): Promise<Config> { | |
| return client.get("/config", context); | |
| }, | |
| getConfig( | |
| context: Record<string, JsonValue> = {}, | |
| ): Promise<Config> { | |
| return Object.keys(context).length === 0 | |
| ? client.get("/config") | |
| : client.get("/config", { context: JSON.stringify(context) }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/api/resolve.ts` around lines 6 - 10, getConfig currently
passes the context object directly to client.get which relies on
buildQueryString and turns nested objects into "[object Object]"; change
getConfig to send the context as JSON like the resolve() method does (e.g., post
the context in the request body) or explicitly serialize context with
JSON.stringify before sending so nested objects are preserved. Update the
getConfig function signature/implementation to mirror resolve()'s approach (use
the same HTTP verb and body shape or ensure the client call sends a serialized
context payload) so server-side matching receives correct JSON context.
| <label | ||
| style={{ | ||
| display: "block", | ||
| marginBottom: 4, | ||
| fontSize: 13, | ||
| fontWeight: 500, | ||
| color: "#374151", | ||
| }} | ||
| > | ||
| {label} | ||
| {required && <span style={{ color: "#ef4444", marginLeft: 2 }}>*</span>} | ||
| </label> | ||
| {children} |
There was a problem hiding this comment.
Associate labels with inputs.
<label> is currently unbound to a control, so click-to-focus and screen-reader association are lost.
Suggested fix
export interface FormFieldProps {
label: string;
+ htmlFor?: string;
@@
export function FormField({
label,
+ htmlFor,
@@
- <label
+ <label
+ htmlFor={htmlFor}
style={{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/components/FormField.tsx` around lines 20 - 32,
FormField's <label> is not associated with the input, breaking click-to-focus
and screen-reader binding; update the FormField component to accept or generate
an input id and associate it with the label via htmlFor (or wrap the input) and
ensure the child input receives that id (use props.id or generate one and pass
it to children via React.cloneElement when children is a React element).
Specifically modify the FormField component (props: label, required, children)
to produce/propagate a unique id and set <label htmlFor={id}> and give the input
element the same id so the label correctly targets the control.
| const columns: Column<DefaultConfig>[] = useMemo( | ||
| () => [ | ||
| { key: "key", header: "Key", width: "25%" }, | ||
| { | ||
| key: "value", | ||
| header: "Value", | ||
| width: "25%", | ||
| render: (row) => <JsonViewer data={row.value} />, | ||
| }, | ||
| { | ||
| key: "schema", | ||
| header: "Schema", | ||
| width: "20%", | ||
| render: (row) => <JsonViewer data={row.schema} />, | ||
| }, | ||
| { key: "description", header: "Description", width: "20%" }, | ||
| { | ||
| key: "actions", | ||
| header: "", | ||
| width: "10%", | ||
| render: (row) => ( | ||
| <button | ||
| style={buttonDanger} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| handleDelete(row.key); | ||
| }} | ||
| > | ||
| Delete | ||
| </button> | ||
| ), | ||
| }, | ||
| ], | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify stale-closure risk pattern in this file
rg -n -C3 'useMemo\(|eslint-disable-next-line react-hooks/exhaustive-deps|handleDelete' clients/admin-ui/src/pages/ConfigManager.tsxRepository: juspay/superposition
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Get broader context around handleDelete definition and useMemo
head -n 150 clients/admin-ui/src/pages/ConfigManager.tsx | tail -n +60Repository: juspay/superposition
Length of output: 2558
🏁 Script executed:
#!/bin/bash
# Check for the definition of deleteMutation, refetch, and addAlert
rg -n 'deleteMutation|const refetch|const addAlert|useAlert|useMutation' clients/admin-ui/src/pages/ConfigManager.tsx | head -30Repository: juspay/superposition
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Find refetch definition and broader hook context
rg -n 'refetch|useQuery|const.*=.*use' clients/admin-ui/src/pages/ConfigManager.tsx | head -20Repository: juspay/superposition
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Check if useCallback is used elsewhere in the file to understand team patterns
rg -n 'useCallback' clients/admin-ui/src/pages/ConfigManager.tsxRepository: juspay/superposition
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Get full context of handleDelete and surrounding code
sed -n '46,96p' clients/admin-ui/src/pages/ConfigManager.tsxRepository: juspay/superposition
Length of output: 1460
Wrap handleDelete in useCallback to stabilize columns memoization.
columns is frozen with [] while its onClick handler calls handleDelete, which references deleteMutation, refetch, and addAlert—all hook-derived values that may change on each render. This creates a stale-closure risk where the memoized callback holds stale references. Wrap handleDelete in useCallback with proper dependencies and include it in the useMemo deps instead of suppressing the lint warning.
Suggested refactor
- const handleDelete = async (key: string) => {
+ const handleDelete = useCallback(async (key: string) => {
if (!confirm(`Delete config "${key}"?`)) return;
try {
await deleteMutation.mutate(key);
refetch();
} catch {
addAlert("error", deleteMutation.error || "Failed to delete config");
}
- };
+ }, [deleteMutation, refetch, addAlert]);
const columns: Column<DefaultConfig>[] = useMemo(
@@
- // eslint-disable-next-line react-hooks/exhaustive-deps
- [],
+ [handleDelete],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/pages/ConfigManager.tsx` around lines 98 - 133, The
columns memo captures a mutable handleDelete closure (which uses deleteMutation,
refetch, addAlert) while being memoized with an empty deps array; wrap
handleDelete in useCallback with the correct dependencies (deleteMutation,
refetch, addAlert, and any other hooks it references) so it always has fresh
references, then include that handleDelete in the useMemo dependency array for
columns (remove the eslint-disable for exhaustive-deps) so the column render's
onClick uses a stable, non-stale function.
| export interface ContextOverride { | ||
| id: string; | ||
| value: Condition; | ||
| override_id: string; | ||
| created_at: string; | ||
| created_by: string; | ||
| override_: Overrides; | ||
| last_modified_at: string; | ||
| last_modified_by: string; | ||
| weight: string; | ||
| description: string; | ||
| change_reason: string; | ||
| } |
There was a problem hiding this comment.
Use the wire name override, not override_.
Line 122 is typed as override_, but the backend model in crates/superposition_types/src/database/models/cac.rs:32-53 serializes this field as override, and this file already uses override in PutContextRequest on Line 132. Unless the client explicitly remaps the field, downstream code will read a property that is never present on the wire.
Suggested fix
export interface ContextOverride {
id: string;
value: Condition;
override_id: string;
created_at: string;
created_by: string;
- override_: Overrides;
+ override: Overrides;
last_modified_at: string;
last_modified_by: string;
weight: string;
description: string;
change_reason: string;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/types/api.ts` around lines 116 - 128, The
ContextOverride interface defines the property as override_ but the backend wire
name is override; update the ContextOverride type to use override: Overrides
(matching the existing PutContextRequest usage) so the client deserializes the
same field the server sends; locate the ContextOverride interface and replace
the override_ property with override (type Overrides) and ensure any
usages/serializations reference ContextOverride.override.
| export interface Experiment { | ||
| id: string; | ||
| created_at: string; | ||
| created_by: string; | ||
| last_modified: string; | ||
| name: string; | ||
| experiment_type: ExperimentType; | ||
| override_keys: string[]; | ||
| status: ExperimentStatus; | ||
| traffic_percentage: number; | ||
| started_at: string | null; | ||
| started_by: string | null; | ||
| context: Condition; | ||
| variants: Variant[]; | ||
| last_modified_by: string; | ||
| chosen_variant: string | null; | ||
| description: string; | ||
| change_reason: string; | ||
| metrics: Record<string, JsonValue>; | ||
| metrics_url?: string; | ||
| experiment_group_id: string | null; |
There was a problem hiding this comment.
Align experiment identifiers with the backend's numeric contract.
The backend Experiment model in crates/superposition_types/src/database/models/experimentation.rs:275-303 exposes id: i64 and experiment_group_id: Option<i64>, so the JSON payload is numeric. Typing Line 186 and Line 205 as strings drifts from that contract and will leak string/number casts into every consumer unless the client normalizes these values before exposure.
Suggested fix
export interface Experiment {
- id: string;
+ id: number;
created_at: string;
created_by: string;
last_modified: string;
name: string;
experiment_type: ExperimentType;
override_keys: string[];
status: ExperimentStatus;
traffic_percentage: number;
started_at: string | null;
started_by: string | null;
context: Condition;
variants: Variant[];
last_modified_by: string;
chosen_variant: string | null;
description: string;
change_reason: string;
metrics: Record<string, JsonValue>;
metrics_url?: string;
- experiment_group_id: string | null;
+ experiment_group_id: number | null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/types/api.ts` around lines 185 - 205, The Experiment
interface incorrectly types the identifiers as strings; update the Experiment.id
from string to number and Experiment.experiment_group_id from string | null to
number | null to match the backend i64/Option<i64> contract (locate these in the
Experiment interface declaration), and ensure any client code that assumes
string IDs is adjusted to accept numeric IDs (or normalize at the API boundary)
so consumers no longer need to cast between string/number.
| export function formatJsonPreview(value: unknown, maxLength = 80): string { | ||
| const str = JSON.stringify(value); | ||
| if (str.length <= maxLength) return str; | ||
| return str.slice(0, maxLength - 3) + "..."; |
There was a problem hiding this comment.
formatJsonPreview can throw for valid unknown inputs.
JSON.stringify(undefined) returns undefined, so str.length crashes at runtime.
Suggested fix
export function formatJsonPreview(value: unknown, maxLength = 80): string {
- const str = JSON.stringify(value);
+ const str = JSON.stringify(value) ?? String(value);
if (str.length <= maxLength) return str;
return str.slice(0, maxLength - 3) + "...";
}📝 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.
| export function formatJsonPreview(value: unknown, maxLength = 80): string { | |
| const str = JSON.stringify(value); | |
| if (str.length <= maxLength) return str; | |
| return str.slice(0, maxLength - 3) + "..."; | |
| export function formatJsonPreview(value: unknown, maxLength = 80): string { | |
| const str = JSON.stringify(value) ?? String(value); | |
| if (str.length <= maxLength) return str; | |
| return str.slice(0, maxLength - 3) + "..."; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/utils/format.ts` around lines 15 - 18, formatJsonPreview
can throw because JSON.stringify(value) may return undefined (e.g., for
undefined) so accessing .length crashes; update formatJsonPreview to coerce the
stringify result into a safe string (for example use const str =
JSON.stringify(value) ?? String(value) or String(value)) before checking length
and slicing, then continue to apply the maxLength logic; modify the
formatJsonPreview function to use that safe string variable so .length and
.slice are never called on undefined.
Adds demo/index.html with a toolbar to configure host, org, workspace, auth token, and feature subset, then renders the embeddable admin UI against a local Superposition instance. Run with `npm run demo`. https://claude.ai/code/session_01MQDdbiwrn4RSwde6qAoNGE
Requests from the demo page to /api/* are proxied to localhost:8080, so the browser never makes cross-origin requests. The host field now defaults to /api. https://claude.ai/code/session_01MQDdbiwrn4RSwde6qAoNGE
2 participants
https://claude.ai/code/session_01MQDdbiwrn4RSwde6qAoNGE
Summary by CodeRabbit