feat(api): abort and tanstack query#227
Conversation
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
from
43d9863 to
ba91f79
Compare
|
Important Review skippedToo many files! This PR contains 178 files, which is 28 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (178)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
2 times, most recently
from
3e018a1 to
4e64327
Compare
|
please target develop branch instead of main |
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
2 times, most recently
from
9875129 to
22fea10
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/react/llms.txt (1)
66-79:⚠️ Potential issue | 🟡 MinorExample missing
registryvariable definition.The
ExtensionDomainSlotexample referencesregistry={registry}butregistryis not defined anywhere in the code snippet. Consider showing how to obtain the registry (e.g., viauseFrontX().screensetsRegistry) for a complete example.📝 Suggested improvement
```tsx -import { ExtensionDomainSlot } from '@cyberfabric/react'; +import { ExtensionDomainSlot, useFrontX } from '@cyberfabric/react'; function LayoutScreen() { + const app = useFrontX(); + const registry = app.screensetsRegistry; + return ( <ExtensionDomainSlot registry={registry} domainId="screen" extensionId="home" loadingComponent={<Loading />} /> ); } ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/llms.txt` around lines 66 - 79, The example for LayoutScreen uses registry but never defines it; inside the LayoutScreen component import and call useFrontX from '@cyberfabric/react' (add useFrontX to the import alongside ExtensionDomainSlot), then obtain the registry via const app = useFrontX(); const registry = app.screensetsRegistry; and pass that registry into the ExtensionDomainSlot call so registry is defined for ExtensionDomainSlot (referencing the LayoutScreen, ExtensionDomainSlot, useFrontX and screensetsRegistry symbols).packages/api/src/__tests__/restPlugins.integration.test.ts (1)
113-143:⚠️ Potential issue | 🟡 MinorAwait the simulated
onResponsechain.
onResponseis async, but line 139 usesforEachwithout awaiting the returned promises. This only observes synchronous side effects and would miss ordering bugs if a hook awaits before mutating state.🧪 Suggested fix
- it('should execute onResponse hooks in reverse order (LIFO)', () => { + it('should execute onResponse hooks in reverse order (LIFO)', async () => { const executionOrder: string[] = []; @@ - plugins.forEach((p) => { + for (const p of plugins) { if (p.onResponse) { const ctx: RestResponseContext = { status: 200, headers: {}, data: {} }; - p.onResponse(ctx); + await p.onResponse(ctx); } - }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/__tests__/restPlugins.integration.test.ts` around lines 113 - 143, The test currently calls async onResponse hooks with plugins.forEach without awaiting, so make the test async and execute the simulated onResponse chain sequentially by replacing plugins.forEach(...) with a for...of loop that awaits each p.onResponse(ctx) (keeping the same RestResponseContext creation inside the loop) so that awaiting hooks that mutate executionOrder are observed; refer to the test case name and the symbols RestProtocol.getPluginsInOrder, plugins (the array), and onResponse when making this change.packages/api/src/apiRegistry.ts (1)
193-232: 🛠️ Refactor suggestion | 🟠 MajorUse thin wrapper functions instead of bind() to preserve generic types.
Lines 193-232 bind generic methods from
protocolPluginRegistry, which causes TypeScript to lose the generic type signatures. This is why tests resort toas nevertype casts when callingapiRegistry.plugins.has()andapiRegistry.plugins.remove(). Wrap each method in an arrow function to preserve the generic protocol parameter.♻️ Suggested refactor
- public readonly plugins = { - add: protocolPluginRegistry.add.bind(protocolPluginRegistry), - remove: protocolPluginRegistry.remove.bind(protocolPluginRegistry), - has: protocolPluginRegistry.has.bind(protocolPluginRegistry), - getAll: protocolPluginRegistry.getAll.bind(protocolPluginRegistry), - clear: protocolPluginRegistry.clear.bind(protocolPluginRegistry), - }; + public readonly plugins = { + add: <T extends ApiProtocol>( + protocolClass: new (...args: never[]) => T, + plugin: ProtocolPluginType<T> + ) => protocolPluginRegistry.add(protocolClass, plugin), + remove: <T extends ApiProtocol>( + protocolClass: new (...args: never[]) => T, + pluginClass: abstract new (...args: never[]) => unknown + ) => protocolPluginRegistry.remove(protocolClass, pluginClass), + has: <T extends ApiProtocol>( + protocolClass: new (...args: never[]) => T, + pluginClass: abstract new (...args: never[]) => unknown + ) => protocolPluginRegistry.has(protocolClass, pluginClass), + getAll: <T extends ApiProtocol>( + protocolClass: new (...args: never[]) => T + ) => protocolPluginRegistry.getAll(protocolClass), + clear: <T extends ApiProtocol>( + protocolClass: new (...args: never[]) => T + ) => protocolPluginRegistry.clear(protocolClass), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/apiRegistry.ts` around lines 193 - 232, The bound methods (protocolPluginRegistry.add/remove/has/getAll/clear) lose generic type information when exported on apiRegistry.plugins, causing callers/tests to need unsafe casts; replace each .bind(...) with a thin generic wrapper arrow function that forwards arguments to the underlying method (e.g. add: <T>(protocolClass: Constructor<T>, plugin: PluginClass<T>) => protocolPluginRegistry.add(protocolClass, plugin)) so the generic parameter is preserved for add, remove, has, getAll and clear; update all five entries accordingly and remove the .bind usages so callers of apiRegistry.plugins can infer the protocol generic without as never casts.packages/react/src/index.ts (1)
23-44: 🛠️ Refactor suggestion | 🟠 MajorRe-export the
queryCacheframework plugin from this barrel.These new query hooks/types are public from
@cyberfabric/react, but the matching L2queryCache()plugin is still missing from the convenience exports below. That leaves host apps unable to stay on the single-package import path this package promises for the new data layer.Based on learnings, "The hai3/react package should re-export everything from hai3/framework for user convenience (SDK primitives, plugins, registries, types, and MFE actions)."📦 Suggested export
export { screensets, themes, layout, i18n, effects, + queryCache, // Registries createThemeRegistry,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/index.ts` around lines 23 - 44, Add a re-export for the framework plugin so host apps can import the L2 plugin from this barrel: export the queryCache plugin (symbol queryCache) from the framework module in packages/react/src/index.ts (i.e., add an export line that re-exports queryCache from the framework package), and consider re-exporting the rest of the public framework primitives/plugins/registries/types so the package continues to provide a single-package import surface for the new data layer.
🟠 Major comments (19)
src/app/layout/Menu.tsx-48-57 (1)
48-57:⚠️ Potential issue | 🟠 Major
visiblePackagefallback can lock menu to a stale package.Line [56] keeps the previous package when
activePackagebecomesundefined, so Lines [63]-[65] continue filtering by an old package and may prevent the domain fallback path from ever being used.💡 Suggested fix
- const [lastActivePackage, setLastActivePackage] = useState<string | undefined>(); - - useEffect(() => { - if (activePackage) { - setLastActivePackage(activePackage); - } - }, [activePackage]); - - const visiblePackage = activePackage ?? lastActivePackage; + const visiblePackage = activePackage;Also applies to: 63-65, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/layout/Menu.tsx` around lines 48 - 57, The current logic stores a stale package in lastActivePackage and computes visiblePackage = activePackage ?? lastActivePackage, which preserves an old package when activePackage becomes undefined and prevents the domain fallback from ever being used; update the effect that sets lastActivePackage (the useEffect referencing activePackage and setLastActivePackage) so it clears lastActivePackage when activePackage is undefined (or remove lastActivePackage entirely and compute visiblePackage directly from activePackage with the proper domain fallback), ensuring visiblePackage no longer falls back to a stale value and allows the domain fallback path to run.packages/cli/template-sources/project/tsconfig.json-10-27 (1)
10-27:⚠️ Potential issue | 🟠 MajorMonorepo-relative
@hai3/*path aliases in tsconfig.json are copied verbatim and not rewritten during scaffolding.Lines 10-27 define hardcoded source paths like
../../../state/src/*. The project generator copies this template file without transformation (unlike main.tsx and App.tsx which receive sanitization). While package.json dependencies are rewritten to file: references when scaffolding within a monorepo, tsconfig.json paths remain unchanged, causing TypeScript resolution failures for projects generated or used outside the monorepo context.Update the scaffolding logic to either:
- Conditionally rewrite
@hai3/*paths to package names whenuseLocalPackagesis false, or- Apply path alias rewriting consistent with how package.json dependencies are handled (via the existing
toLocalRefspattern)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/template-sources/project/tsconfig.json` around lines 10 - 27, The tsconfig.json template contains hardcoded monorepo-relative "@hai3/*" path aliases that aren't rewritten during scaffolding; update the scaffolding logic that currently applies to package.json to also transform tsconfig.json paths when useLocalPackages is false by reusing the existing toLocalRefs pattern (or add a similar helper) so that entries like "@hai3/state": ["../../../state/src/*"] are rewritten to the package names/installed paths; locate the code that handles scaffolding and package rewriting (the toLocalRefs usage and the useLocalPackages flag) and apply the same conditional rewrite to the tsconfig.json content before writing the scaffolded project.packages/screensets/src/mfe/handler/types.ts-74-90 (1)
74-90: 🛠️ Refactor suggestion | 🟠 MajorMove TanStack-specific adapter types out of SDK contract.
QueryClientLikeand the mount context semantics are already exported frompackages/screensets/src/index.ts(lines 65–66), widening the L1 surface with library-specific abstractions. These types are only used bypackages/react/src/mfe/ThemeAwareReactLifecycle.tsxfor TanStack Query adaptation. Keep the SDK mount context opaque (e.g.,unknownforqueryClient) and move the TanStack type guard and adaptation logic entirely into the React layer. This preserves the swap boundary and keeps the core screensets package library-agnostic.Also apply to lines 106–112 where
MfeEntryLifecycle.mount()acceptsmountContext?: MfeMountContext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/screensets/src/mfe/handler/types.ts` around lines 74 - 90, Remove the TanStack-specific type QueryClientLike from the SDK surface and make the MfeMountContext opaque by changing the queryClient field to unknown (e.g., readonly queryClient?: unknown) and keeping extensionId/domainId as-is; also update MfeEntryLifecycle.mount() to accept mountContext?: MfeMountContext with the opaque queryClient type and remove any exports of QueryClientLike from the package index, then move the existing TanStack type guard and adaptation logic into the React layer (e.g., ThemeAwareReactLifecycle.tsx) where you can reintroduce QueryClientLike and perform the runtime checks and defaultQueryOptions/mutation/getQueryCache handling.packages/framework/src/plugins/microfrontends/actions.ts-155-162 (1)
155-162:⚠️ Potential issue | 🟠 MajorTreat a missing domain as an error, not swap semantics.
resolveDomainId()only proves the extension exists. IfgetDomain(domainId)returnsundefined, this branch now logs a misleading warning and skips the actual unmount, which can leave the extension mounted. Only take the no-op path after you've confirmed the domain is present and explicitly does not supportHAI3_ACTION_UNMOUNT_EXT.Suggested fix
export function unmountExtension(extensionId: string): void { const domainId = resolveDomainId(extensionId); - const supportsUnmount = screensetsRegistry?.getDomain(domainId)?.actions.includes(HAI3_ACTION_UNMOUNT_EXT) ?? false; + const domain = screensetsRegistry!.getDomain(domainId); + if (!domain) { + throw new Error(`Domain '${domainId}' is not registered for extension '${extensionId}'.`); + } + const supportsUnmount = domain.actions?.includes(HAI3_ACTION_UNMOUNT_EXT) ?? false; if (!supportsUnmount) { console.warn( `[MFE] Skipping unmount for ${extensionId}: domain '${domainId}' uses swap semantics and does not support ${HAI3_ACTION_UNMOUNT_EXT}.`Based on learnings: Extension registration must include ExtensionDomain definition with containerProvider - register domain before registering related extensions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/plugins/microfrontends/actions.ts` around lines 155 - 162, The current code treats a missing domain as "no unmount support" and skips unmount; change it to first retrieve the domain from screensetsRegistry (e.g., const domain = screensetsRegistry?.getDomain(domainId)) and if domain is undefined, surface an error (throw or processLogger.error and fail the operation) referencing domainId and extensionId; only after confirming domain exists should you check domain.actions.includes(HAI3_ACTION_UNMOUNT_EXT) (the supportsUnmount logic) and take the no-op path when the domain explicitly does not support HAI3_ACTION_UNMOUNT_EXT.packages/screensets/src/mfe/runtime/DefaultScreensetsRegistry.ts-201-202 (1)
201-202:⚠️ Potential issue | 🟠 MajorThis mount-context handoff is racey for concurrent mounts.
Line 369 writes a shared per-extension slot outside
OperationSerializer, and Line 201 reads that slot later inside the mount path. If two mount chains for the same extension overlap, the latersetExtensionMountContext()can overwrite the earlier context, and the earlier chain's cleanup can clear the later one before it mounts. Carry the context through the serialized mount operation itself, or key it by an operation token instead of a mutable per-extension slot.Also applies to: 368-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/screensets/src/mfe/runtime/DefaultScreensetsRegistry.ts` around lines 201 - 202, The current handoff of mount context via the shared per-extension slot is racey: setExtensionMountContext (the setter referenced around lines 368-378) writes a mutable slot outside OperationSerializer while resolveMountContext calls this.extensionManager.getMountContext inside the mount flow, allowing overlapping mount chains to clobber each other; fix by threading the mount context through the serialized mount operation itself (pass the context object/token through the OperationSerializer-managed mount call) or switch the shared slot to be keyed by an operation token (generate a unique mountOperationId per mount, store contexts under that key, and have resolveMountContext look up by mountOperationId) so resolveMountContext and cleanup use the operation-scoped context instead of a single mutable per-extension slot.packages/cli/template-sources/project/src/app/main.custom-uikit.tsx-9-10 (1)
9-10:⚠️ Potential issue | 🟠 MajorRemove
@ts-expect-errorfrom the template source code.Line 9's
@ts-expect-errorsuppresses the missing barrel intemplate-sources, but once scaffolding generates@/app/themes, this directive becomes unused. TypeScript will emit a TS2578 error on the generated project. Use a template-only stub or module declaration instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/template-sources/project/src/app/main.custom-uikit.tsx` around lines 9 - 10, Remove the inline `@ts-expect-error` on the import in main.custom-uikit.tsx and replace it by adding a template-only TypeScript stub/module declaration that exports the same symbols (hai3Themes and DEFAULT_THEME_ID) so the template compiles but does not conflict after scaffolding; locate the import statement that references '@/app/themes' and either add a declaration file under the template-sources (e.g., a .d.ts stub) or a small template-only module that exports the expected names (hai3Themes, DEFAULT_THEME_ID) to avoid TS2578 in generated projects..ai/targets/REACT.md-41-49 (1)
41-49:⚠️ Potential issue | 🟠 MajorAdd
queryCache()to the canonical pre-built app example.The same file now recommends
useApiQuery/useApiMutation, but this example still builds an app without a query client. Following it produces a provider tree where those hooks fail unless the caller separately injectssharedQueryClient, which makes the canonical snippet misleading.🛠️ Suggested edit
-const app = createHAI3().use(screensets()).build(); +const app = createHAI3().use(queryCache()).use(screensets()).build(); <HAI3Provider app={app}> <AppRouter fallback={<Loading />} /> </HAI3Provider>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ai/targets/REACT.md around lines 41 - 49, The example builds an app without attaching a QueryClient so useApiQuery/useApiMutation will fail; update the pre-built app creation to include the query cache step (call queryCache on the builder) before build so the produced app contains a QueryClient, and keep HAI3Provider/HAI3Provider app={app} and optional sharedQueryClient usage as shown; target the createHAI3 builder (createHAI3, .use(screensets()), .queryCache(), .build()) and ensure HAI3Provider/ useApiQuery and useApiMutation now resolve against the embedded QueryClient.architecture/ADR/0017-tanstack-query-data-management.md-72-73 (1)
72-73:⚠️ Potential issue | 🟠 MajorRefocus RTK Query rejection on architectural mismatch, not outdated capability claims.
Lines 72–73 (and 209–211) cite missing features that RTK Query now supports or offers differently:
- Infinite queries: Supported since Redux Toolkit v2.6.0 (Feb 2025) via
build.infiniteQuery()anduseInfiniteQuery.- Structural sharing: Enabled automatically via
copyWithStructuralSharing; can be disabled per-endpoint withstructuralSharing: false.- Request cancellation: RTK Query provides
AbortSignalsupport in baseQuery and manual.abort()on mutations/lazy queries. It does not automatically cancel on unmount by design (maintainers argue post-send cancellation adds little value), unlike TanStack Query's automatic behavior.If TanStack Query is the right choice, the ADR should emphasize FrontX's dynamic
apiRegistryand isolated service instances as incompatible with RTK Query's staticcreateApiendpoint definition, rather than feature gaps that no longer apply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/ADR/0017-tanstack-query-data-management.md` around lines 72 - 73, Update the ADR text to shift the RTK Query rejection rationale from outdated feature claims to an architectural mismatch: remove or reword lines referencing lack of "infinite queries", "structural sharing", and "request cancellation" and instead state that RTK Query's static createApi endpoint model conflicts with FrontX's dynamic per-MFE apiRegistry and isolated service instances; mention that RTK Query does offer infinite queries (build.infiniteQuery/useInfiniteQuery), structural sharing via copyWithStructuralSharing (or per-endpoint structuralSharing: false), and AbortSignal/.abort() support for cancellations, but that automatic unmount cancellation behavior differs by design—emphasize createApi and apiRegistry as the primary incompatibility.packages/framework/src/plugins/queryCache.ts-85-87 (1)
85-87:⚠️ Potential issue | 🟠 MajorCancel active queries before clearing the client.
On mock toggle (line 86) and destroy (line 171),
queryClient.clear()is called without first canceling in-flight requests. TanStack Query'sclear()removes cached data but does not abort in-flight requests—those can still settle after the cache is cleared and repopulate it with stale responses from the previous mode. CallqueryClient.cancelQueries()before eachclear()to prevent requests started under the old mode from contaminating the fresh cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/plugins/queryCache.ts` around lines 85 - 87, On MockEvents.Toggle (the mockToggleSub callback where queryClient.clear() is invoked) and in the teardown/destroy path where queryClient.clear() is used, call queryClient.cancelQueries() first to abort any in-flight requests before clearing the cache; update the callbacks that reference queryClient.clear() (e.g., the handler registered via eventBus.on(MockEvents.Toggle) and the destroy/cleanup routine) to invoke queryClient.cancelQueries() and await it (or handle the promise) prior to calling queryClient.clear() so old requests cannot repopulate the cache.packages/react/src/HAI3Provider.tsx-94-101 (1)
94-101:⚠️ Potential issue | 🟠 MajorMutating
configRefduring render is an anti-pattern that violates React's purity requirement.The code writes
configRef.currentconditionally on lines 95–99 during the render phase. React explicitly advises against this—render must remain a pure calculation without side effects on refs. In Strict Mode, renders invoke twice, so this ref mutation may execute zero, one, or twice unpredictably. In concurrent rendering, a discarded render still mutates the ref, causingstableConfigon line 101 to reflect uncommitted state, potentially based on an aborted render cycle.The intent (memoizing config to avoid unnecessary app recreation) is valid, but the solution violates React principles. Better approaches: have callers wrap inline config in
useMemo, or refactor to useuseStatewith auseEffectguard to track config changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/HAI3Provider.tsx` around lines 94 - 101, The current render mutates configRef.current (via configRef, config and stableConfig using shallowEqual) which is an anti-pattern; instead, stop writing the ref during render and either (A) require callers to memoize inline config (document that config passed to HAI3Provider must be wrapped in useMemo) or (B) implement a safe updater: replace the in-render mutation with a state/ref update inside an effect (useEffect) that compares config with shallowEqual and updates a stateful stableConfig (useState) or updates configRef.current from inside useEffect; ensure all comparisons still use shallowEqual and only update when values differ so app recreation remains minimized.packages/react/src/hooks/useApiStream.ts-78-84 (1)
78-84:⚠️ Potential issue | 🟠 Major
disconnect()doesn't cancel an in-flight connect.If a caller disconnects during
'connecting',connectionIdRef.currentis still null, so nothing happens and Lines 120-123 still flip the hook back to'connected'when the promise resolves. Track a disconnect request and close the resolved id in thethen()path.Suggested fix
// Tracks the resolved connectionId for the manual disconnect() callback. const connectionIdRef = useRef<string | null>(null); + const disconnectRequestedRef = useRef(false); @@ const disconnect = useCallback(() => { + disconnectRequestedRef.current = true; if (connectionIdRef.current) { descriptorRef.current.disconnect(connectionIdRef.current); connectionIdRef.current = null; - setStatus('disconnected'); } + setStatus('disconnected'); }, []); @@ let cancelled = false; + disconnectRequestedRef.current = false; @@ connectPromise .then((id) => { if (cancelled) return; + if (disconnectRequestedRef.current) { + d.disconnect(id); + return; + } connectionIdRef.current = id; setStatus('connected'); })Also applies to: 119-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/useApiStream.ts` around lines 78 - 84, The disconnect() logic fails to cancel an in-flight connect because connectionIdRef.current is null while status is 'connecting'; add a "disconnect requested" flag (e.g., disconnectRequestedRef) that disconnect() sets to true and clears connectionIdRef/status if already connected, and update the connect promise resolution logic (the then() handler that currently assigns connectionIdRef.current and calls setStatus('connected')) to check disconnectRequestedRef: if true, immediately call descriptorRef.current.disconnect(newConnectionId), clear disconnectRequestedRef, keep connectionIdRef null and setStatus('disconnected'; otherwise proceed as before. Ensure both disconnect() and the connect resolution clear the flag appropriately and reference disconnectRequestedRef, connectionIdRef, descriptorRef, and setStatus.src/mfe_packages/demo-mfe/src/screens/profile/components/ProfileDetailsCard.tsx-83-92 (1)
83-92:⚠️ Potential issue | 🟠 MajorCatch save failures inside the submit handler.
If
onSubmit()rejects, this async form handler leaks a rejected promise out of React. Keep the form open and letsaveErrorMessagesurface the failure instead of letting the rejection escape.Suggested fix
const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => { event.preventDefault(); @@ - await onSubmit(normalizedValues); - setIsEditing(false); + try { + await onSubmit(normalizedValues); + setIsEditing(false); + } catch { + // Parent surfaces the error via saveErrorMessage; keep editing open. + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mfe_packages/demo-mfe/src/screens/profile/components/ProfileDetailsCard.tsx` around lines 83 - 92, The handleSubmit async handler currently awaits onSubmit(normalizedValues) without catching rejections; wrap the call in a try/catch so rejections don’t leak to React: call await onSubmit(normalizedValues) inside try, on success clear any existing save error (e.g. via setSaveErrorMessage(null)) and then setIsEditing(false), and in catch set the save error state (setSaveErrorMessage with a user-facing message or error.message) and do not call setIsEditing(false) or rethrow the error; keep the rest of the handler (event.preventDefault, isDirty/isFormValid checks) unchanged.packages/react/src/mfe/bootstrapMfeCore.ts-67-69 (1)
67-69:⚠️ Potential issue | 🟠 MajorDon't seed shared language with a constant.
Line 69 always publishes
'en', so MFEs mounted under a non-English host start with the wrong shared language. This should come from the app's current i18n state and only fall back to'en'when that state is unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/mfe/bootstrapMfeCore.ts` around lines 67 - 69, Replace the hardcoded 'en' when calling screensetsRegistry.updateSharedProperty for HAI3_SHARED_PROPERTY_LANGUAGE with the app's current i18n value (e.g., derive from app.i18n or app.getI18n()/app.i18n?.getCurrent() — such as app.i18n?.language or app.i18n?.getCurrent()?.locale) and fall back to 'en' if that value is undefined; update the call to screensetsRegistry.updateSharedProperty(HAI3_SHARED_PROPERTY_LANGUAGE, derivedLanguage ?? 'en') so MFEs inherit the host's actual language.packages/react/src/hooks/useApiStream.ts-74-76 (1)
74-76:⚠️ Potential issue | 🟠 MajorDescriptor changes leak stale stream state while the hook is disabled.
The
enabled === falseearly return skips any reset, so swappingdescriptorKeyormodewhile disabled keeps exposing the previous stream'sdata,events, anderror. The reset should be keyed to descriptor identity, not only to the active connection path.Suggested fix
// Stable identity derived from descriptor key — used as effect dependency. // JSON.stringify avoids join('/') collisions when a segment contains '/'. const descriptorKey = useMemo(() => JSON.stringify(descriptor.key), [descriptor.key]); + + useEffect(() => { + setData(undefined); + setEvents([]); + setError(null); + }, [descriptorKey, mode]); @@ useEffect(() => { if (!enabled) { setStatus('idle'); return; } @@ - setData(undefined); - setEvents([]); setStatus('connecting'); - setError(null);Also applies to: 86-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/useApiStream.ts` around lines 74 - 76, The hook useApiStream currently returns early when enabled === false without clearing prior stream state, so changes to descriptor.key (descriptorKey) or mode can leak stale data/events/error; update the hook to reset the stream state whenever descriptorKey or mode changes even if enabled is false — e.g., add an effect or move the reset logic that clears data, events and error (the state setters used in useApiStream) to run on descriptorKey and mode changes before the early return so the previous stream's data/events/error are cleared whenever descriptorKey or mode updates while disabled.packages/react/src/mfe/bootstrapMfeCore.ts-72-83 (1)
72-83:⚠️ Potential issue | 🟠 MajorFall back to
app.queryClientwhen no explicit client is passed.A host that installs
queryCache()but calls this helper without the third arg still routes out-of-slotmount_extactions through the unshared path. Defaulting toqueryClient ?? app.queryClientkeeps the shared-cache behavior aligned withExtensionDomainSlot.Suggested fix
- if (queryClient) { + const sharedQueryClient = queryClient ?? app.queryClient; + + if (sharedQueryClient) { @@ await executeActionsChainWithMountContext( screensetsRegistry, chain, - queryClient, + sharedQueryClient, origExecuteActionsChain, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/mfe/bootstrapMfeCore.ts` around lines 72 - 83, The current override of screensetsRegistry.executeActionsChain uses the local queryClient but doesn't fall back to app.queryClient when queryClient is undefined; update the wrapped call to pass (queryClient ?? app.queryClient) into executeActionsChainWithMountContext so out-of-slot mount_ext actions join the host shared cache. Locate the override of screensetsRegistry.executeActionsChain and replace the passed queryClient argument with the nullish-coalesced value, ensuring executeActionsChainWithMountContext still receives screensetsRegistry, chain, the resolved client, and origExecuteActionsChain.packages/react/src/mfe/components/ExtensionDomainSlot.tsx-15-22 (1)
15-22:⚠️ Potential issue | 🟠 MajorDon't hardcode screen domains out of the unmount path.
The registry already advertises whether a domain supports
HAI3_ACTION_UNMOUNT_EXT, but Line 146 forces everyscreenDomaindown the no-cleanup path anyway. That makes teardown impossible to opt into for screen domains and leaves cleanup dependent on some later swap.Based on learnings: Extensions follow a strict 5-stage lifecycle: Register → Load → Mount → Unmount → Unregister, with lifecycle hooks and actions chains supporting success/fallback branching.Suggested fix
import { type ActionsChain, type ParentMfeBridge, type ScreensetsRegistry, HAI3_ACTION_MOUNT_EXT, HAI3_ACTION_UNMOUNT_EXT, - screenDomain, } from '@cyberfabric/framework'; @@ - const domainSupportsUnmount = ( - domainId !== screenDomain.id - && (registry.getDomain(domainId)?.actions.includes(HAI3_ACTION_UNMOUNT_EXT) ?? false) - ); + const domainSupportsUnmount = + registry.getDomain(domainId)?.actions.includes(HAI3_ACTION_UNMOUNT_EXT) ?? false;Also applies to: 141-148, 191-204, 254-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/mfe/components/ExtensionDomainSlot.tsx` around lines 15 - 22, The code currently forces screenDomain into the "no-cleanup" path; instead determine whether to perform unmount/cleanup from the registry's advertised capability for HAI3_ACTION_UNMOUNT_EXT. In ExtensionDomainSlot (the mount/unmount logic that references HAI3_ACTION_UNMOUNT_EXT and screenDomain), remove the special-case hardcoding of screenDomain and replace it with a check like registry.supportsAction(domain, HAI3_ACTION_UNMOUNT_EXT) (or the registry method that advertises support) to decide whether to enqueue/dispatch the unmount action and run teardown; apply the same change to the other similar blocks that handle mount/unmount and action chains so unmount behavior is opt-in per domain rather than forced for screenDomain.packages/react/src/hooks/QueryCache.ts-41-45 (1)
41-45:⚠️ Potential issue | 🟠 MajorNormalize descriptor keys in
invalidateMany().Every other cache method accepts an
EndpointDescriptorand resolves.keybefore touching TanStack.invalidateMany()is the odd one out:queryKeyis typed as rawQueryKeyand forwarded untouched, soqueryCache.invalidateMany({ queryKey: accounts.currentUser })either fails the type-check or silently misses the intended query.🔧 Suggested fix
export type QueryCacheInvalidateFilters = { - queryKey?: QueryKey; + queryKey?: EndpointDescriptor<unknown> | QueryKey; exact?: boolean; refetchType?: 'active' | 'inactive' | 'all' | 'none'; }; @@ invalidateMany: (filters): Promise<void> => { - return queryClient.invalidateQueries(filters); + const { queryKey, ...rest } = filters; + return queryClient.invalidateQueries({ + ...rest, + ...(queryKey !== undefined + ? { queryKey: resolveKey(queryKey) as QueryKey } + : {}), + }); },Also applies to: 139-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/QueryCache.ts` around lines 41 - 45, invalidateMany() is inconsistent: QueryCacheInvalidateFilters currently types queryKey as a raw QueryKey and forwards it directly to queryCache.invalidateMany, so calls like queryCache.invalidateMany({ queryKey: accounts.currentUser }) fail or miss the intended query; change the filter to accept an EndpointDescriptor (or QueryKey | EndpointDescriptor) like other cache methods and normalize/resolve descriptor.key to a QueryKey before calling queryCache.invalidateMany. Update the QueryCacheInvalidateFilters type and the invalidateMany implementation to detect an EndpointDescriptor, extract/resolve its .key (same helper or logic used by other methods), and pass the resolved QueryKey into queryCache.invalidateMany.packages/react/src/hooks/useApiMutation.ts-78-105 (1)
78-105:⚠️ Potential issue | 🟠 MajorDon't default to aborting previous mutations.
This turns every repeat
mutate()into last-write-wins. For writes, aborting is only local; the first POST/PUT/PATCH/DELETE may already be committed server-side, while the client rolls back optimistic state or ignores its result as if it never happened. Please make superseding/unmount cancellation opt-in instead of the default for the generic mutation hook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/useApiMutation.ts` around lines 78 - 105, The hook currently always aborts previous mutations via fetchAbortRef and in the useEffect cleanup; make this behavior opt-in by adding a boolean option (e.g. cancelOnSupersede / abortOnUnmount) to the hook options and only call fetchAbortRef.current?.abort() in mutationFn and the useEffect cleanup when that option is true; update checks inside mutationFn (where it creates controller and attaches librarySignal) to consult latestRef.current.options.cancelOnSupersede before aborting a prior controller or linking signals, and use latestRef.current.options.abortOnUnmount in the effect cleanup so unmount cancellation is opt-in.packages/api/src/BaseApiService.ts-471-480 (1)
471-480:⚠️ Potential issue | 🟠 MajorNarrow
mutation()to the methods you actually support.
method: HttpMethodlets callers buildDELETE,HEAD, andOPTIONSdescriptors, but downstreamdispatchRequest()only handles POST/PUT/PATCH correctly:DELETEdropsvariables, andHEAD/OPTIONSthrow at runtime. That makes the exported descriptor contract unsound.If DELETE bodies are intended, plumb `data` through `RestProtocol.delete()` first and then add `'DELETE'` back to `MutationMethod`.🔧 Suggested fix
+type MutationMethod = 'POST' | 'PUT' | 'PATCH'; + protected mutation<TData, TVariables>( - method: HttpMethod, + method: MutationMethod, path: string ): MutationDescriptor<TData, TVariables> {Also applies to: 546-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/BaseApiService.ts` around lines 471 - 480, The mutation() signature is too broad (accepts HttpMethod) while dispatchRequest() only supports POST/PUT/PATCH (and DELETE needs special plumbing), so narrow the allowed methods to the mutation-specific union (e.g., MutationMethod / the set of 'POST'|'PUT'|'PATCH' and optionally 'DELETE') in the mutation<TData,TVariables> declaration and in callers that build descriptors; if DELETE bodies are required, route variables through RestProtocol.delete() inside dispatchRequest()/RestProtocol plumbing and then re-add 'DELETE' to the MutationMethod set so delete bodies are serialized correctly; update references to mutation(), dispatchRequest(), MutationMethod and RestProtocol.delete() accordingly to keep the contract sound.
🟡 Minor comments (10)
architecture/explorations/2026-03-17-tanstack-query-integration-research.md-330-334 (1)
330-334:⚠️ Potential issue | 🟡 MinorAdd a language to this fenced block.
markdownlint MD040 will keep flagging this section until the fence is annotated.
📝 Suggested fix
-``` +```text L1 EndpointDescriptor { key, fetch } — library-agnostic, zero deps L2 queryCache() plugin — owns QueryClient, event integration L3 useApiQuery(descriptor) — maps descriptor → TanStack hook</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@architecture/explorations/2026-03-17-tanstack-query-integration-research.md
around lines 330 - 334, Annotate the fenced code block with a language label
(e.g., "text") to satisfy markdownlint MD040; locate the fence that lists
EndpointDescriptor, queryCache(), and useApiQuery in the document and change the
opening triple-backticks to include the language token (for example: ```text) so
the block is properly recognized by the linter.</details> </blockquote></details> <details> <summary>packages/react/src/mfe/MfeProvider.tsx-7-17 (1)</summary><blockquote> `7-17`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove duplicate documentation paragraph.** Lines 13-17 are an exact duplicate of lines 7-11. This appears to be a copy-paste error. <details> <summary>📝 Proposed fix</summary> ```diff * MfeProvider does not create or own a QueryClient. When the host injects the * same QueryClient into each separately mounted MFE root via HAI3Provider, * overlapping queries (same query key) are deduplicated and cached once across * MFE boundaries. Each MFE still uses its own apiRegistry and service * instances in queryFn. * - * MfeProvider does not create or own a QueryClient. When the host injects the - * same QueryClient into each separately mounted MFE root via HAI3Provider, - * overlapping queries (same query key) are deduplicated and cached once across - * MFE boundaries. Each MFE still uses its own apiRegistry and service - * instances in queryFn. - * * React Layer: L3 (Depends on `@cyberfabric/framework`) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/react/src/mfe/MfeProvider.tsx` around lines 7 - 17, Remove the duplicated documentation block describing MfeProvider's QueryClient behavior: keep a single copy of the paragraph that explains that MfeProvider does not create or own a QueryClient and that the host can inject the same QueryClient via HAI3Provider (so overlapping queries are deduplicated while each MFE keeps its own apiRegistry/service instances in queryFn), and delete the second repeated paragraph so there is only one instance of that doc comment near the MfeProvider declaration. ``` </details> </blockquote></details> <details> <summary>packages/state/src/types.ts-10-12 (1)</summary><blockquote> `10-12`: _⚠️ Potential issue_ | _🟡 Minor_ **Add `redux` as an explicit peer dependency.** The `[Symbol.observable]` implementation is correct in `store.ts` (lines 82, 105), but `types.ts` line 2 imports `Observable` directly from `redux` without declaring it as a peer dependency. While `redux` is transitively available through `@reduxjs/toolkit`, it should be explicitly listed in `peerDependencies` to ensure proper type resolution for consumers and to avoid breaking strict package manager configurations. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/state/src/types.ts` around lines 10 - 12, The package currently imports Observable from redux in packages/state/src/types.ts (and implements [Symbol.observable] in store.ts at the referenced locations) but redux is not declared as a peer dependency; update package manifest to add "redux" as a peerDependency (with a compatible semver range that matches the `@reduxjs/toolkit` consumer expectations) so consumers can resolve the Observable type correctly and avoid strict package manager failures. ``` </details> </blockquote></details> <details> <summary>architecture/features/feature-react-bindings/FEATURE.md-409-409 (1)</summary><blockquote> `409-409`: _⚠️ Potential issue_ | _🟡 Minor_ **Sync this DoD sentence with the shipped provider behavior.** This still says `HAI3Provider` creates a local `QueryClient` when the prop is absent, but the implementation now only uses `app.queryClient` or an injected client and otherwise omits `QueryClientProvider`. The current sentence documents behavior that no longer exists. <details> <summary>🛠️ Suggested edit</summary> ```diff -`HAI3Provider` accepts `children`, optional `config`, optional pre-built `app`, optional injected `queryClient`, and optional `mfeBridge`. When `app` is not provided, it creates one via `createHAI3App(config)`. When `queryClient` is not provided, it creates one locally; when it is provided, separate React roots can share the same TanStack cache instance. The instance is memoized; internally created resources are cleaned up on unmount. The full context tree (`HAI3Context` → `ReduxProvider` → `QueryClientProvider` → optional `MfeProvider`) is assembled before children render. +`HAI3Provider` accepts `children`, optional `config`, optional pre-built `app`, optional injected `queryClient`, and optional `mfeBridge`. When `app` is not provided, it creates one via `createHAI3App(config)`. When `queryClient` is not provided, it uses `app.queryClient` when available; otherwise it renders without `QueryClientProvider`. When a client is provided, separate React roots can share the same TanStack cache instance. The instance is memoized; internally created resources are cleaned up on unmount. The full context tree (`HAI3Context` → `ReduxProvider` → optional `QueryClientProvider` → optional `MfeProvider`) is assembled before children render. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@architecture/features/feature-react-bindings/FEATURE.md` at line 409, Update the DoD sentence to match current HAI3Provider behavior: state that HAI3Provider accepts children, optional config, optional pre-built app, optional injected queryClient, and optional mfeBridge; when app is not provided it creates one via createHAI3App(config); if an injected queryClient is passed or app.queryClient exists it wraps children with QueryClientProvider, otherwise it does not render a QueryClientProvider (i.e., it no longer creates a local QueryClient); note that instances are memoized and internally created resources are cleaned up on unmount and that the full context tree (HAI3Context → ReduxProvider → optional QueryClientProvider → optional MfeProvider) is assembled before children render. ``` </details> </blockquote></details> <details> <summary>architecture/ADR/0017-tanstack-query-data-management.md-255-271 (1)</summary><blockquote> `255-271`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language tag to this fenced block.** This trips `markdownlint` `MD040`. <details> <summary>🛠️ Suggested edit</summary> ```diff -``` +```text L1 `@cyberfabric/api` EndpointDescriptor { key, fetch, staleTime?, gcTime? } StreamDescriptor { key, connect, disconnect } BaseApiService.query() / queryWith() / mutation() / stream() No caching library dependency L2 `@cyberfabric/framework` queryCache() plugin — owns QueryClient lifecycle `@tanstack/query-core` as peer dependency Event-driven cache invalidation + mock mode integration Exposes app.queryClient for non-React access L3 `@cyberfabric/react` useApiQuery(descriptor) / useApiMutation({ endpoint }) useApiStream(descriptor) — SSE lifecycle management `@tanstack/react-query` as peer dependency Maps descriptors → TanStack hooks using plugin's QueryClient HAI3Provider reads app.queryClient instead of creating its own -``` +``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@architecture/ADR/0017-tanstack-query-data-management.mdaround lines 255 -
271, The fenced code block containing the L1/L2/L3 description is missing a
language tag and triggers markdownlint MD040; update that triple-backtick fence
to include a language tag such as "text" (i.e., change the opening ``` to`@cyberfabric/api`, `@cyberfabric/framework`, and `@cyberfabric/react` and add the tag to the opening fence.architecture/features/feature-request-lifecycle/FEATURE.md-485-485 (1)
485-485:⚠️ Potential issue | 🟡 MinorAcceptance criteria still mention the removed L3 invalidation listener.
Line 485 says
cache/invalidateis handled by a synchronous listener inHAI3Provider, but Lines 643-645 say that listener moved intoqueryCache()at L2. Please align the acceptance criteria with the implemented architecture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/features/feature-request-lifecycle/FEATURE.md` at line 485, Update the acceptance criteria to reflect that the `cache/invalidate` listener is implemented in `queryCache()` at L2, not as a synchronous listener in `HAI3Provider` at L3: remove or change the line mentioning a synchronous listener in `HAI3Provider`, explicitly state that cross-feature mutations emit a `cache/invalidate` event handled by the `queryCache()` L2 listener for cache invalidation, and ensure any references to L2/L3 behaviour mention the listener relocation to `queryCache()` so the document is consistent.packages/api/CLAUDE.md-41-42 (1)
41-42:⚠️ Potential issue | 🟡 MinorUpdate the new descriptor examples to the published package names.
These snippets use
@hai3/api/@hai3/react, while the rest of this doc and the repo use@cyberfabric/*. Copy-pasting the new examples will currently fail for consumers.Also applies to: 67-67, 89-90, 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/CLAUDE.md` around lines 41 - 42, The example imports in CLAUDE.md use the unpublished package names `@hai3/api` and `@hai3/react`; update those example descriptor snippets to the published package names used across the repo (e.g., replace `import { BaseApiService, RestProtocol } from '@hai3/api'` and any `@hai3/react` usages with the corresponding `@cyberfabric/*` package names) so copy-pasted examples resolve for consumers; make the same replacements for the other example blocks that contain `@hai3/api`/`@hai3/react` (the import lines and any references to those package symbols) to ensure consistency.architecture/DESIGN.md-528-532 (1)
528-532:⚠️ Potential issue | 🟡 MinorThis still documents the old query model.
The bullets here say
HAI3Providermay create its ownQueryClientand recommend per-domain key factories plusqueryOptions(). PRD 5.20 moved both responsibilities out of the React layer:queryCache()owns the client, and service descriptors are the only sanctioned key source. Keeping both models here will misdirect follow-up implementation work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/DESIGN.md` around lines 528 - 532, Update the DESIGN.md bullets to replace the old React-owned query model: remove the statement that HAI3Provider may create or reuse a QueryClient and delete guidance around per-domain query key factories and queryOptions(); instead state that queryCache() is the sole owner of the TanStack client and that service descriptors are the only sanctioned source of query keys, while keeping the documented QueryCache API (get/getState/set/cancel/invalidate/invalidateMany/remove) and noting that useQueryClient is not exported; reference HAI3Provider, queryCache(), QueryCache, queryOptions(), and service descriptors when making these edits so the new single-source-of-truth model is clear.packages/react/__tests__/queryHooks.test.tsx-350-352 (1)
350-352:⚠️ Potential issue | 🟡 MinorThe
&&insidewaitFormay not behave as expected.The expression
expect(...).toBeUndefined() && expect(...).toBe(false)doesn't return a useful value forwaitFor. The firstexpectreturnsundefinedon success, making the&&short-circuit. This assertion works by accident because the real wait condition isisPendingbecomingfalse.Consider splitting for clarity:
Proposed fix
- await waitFor(() => expect(result.current.data).toBeUndefined() && expect(result.current.isPending).toBe(false)); + await waitFor(() => expect(result.current.isPending).toBe(false)); + expect(result.current.data).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/__tests__/queryHooks.test.tsx` around lines 350 - 352, The waitFor assertion uses a boolean expression combining two expect() calls which short-circuits because expect() returns undefined; split the checks so waitFor receives a single clear condition: call result.current.mutate(); then use await waitFor(() => expect(result.current.data).toBeUndefined()); and a separate await waitFor(() => expect(result.current.isPending).toBe(false)); or keep one waitFor for the pending flag and a separate expect for data — update the test around result.current.mutate(), waitFor(), result.current.data and result.current.isPending accordingly.packages/react/CLAUDE.md-54-56 (1)
54-56:⚠️ Potential issue | 🟡 MinorInconsistent package name in import example.
The import uses
@hai3/reactbut the rest of the documentation uses@cyberfabric/react. This should be consistent.Proposed fix
-import { useApiQuery, useApiMutation, apiRegistry } from '@hai3/react'; +import { useApiQuery, useApiMutation, apiRegistry } from '@cyberfabric/react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/CLAUDE.md` around lines 54 - 56, The import example is using the wrong package name; update the import statement that brings in useApiQuery, useApiMutation, and apiRegistry so it references the documented package name '@cyberfabric/react' instead of '@hai3/react' (the AccountsApiService import can remain unchanged); ensure the example uses the same package name used throughout the docs so symbols useApiQuery, useApiMutation, and apiRegistry resolve consistently.
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
from
c8903fd to
30c0e15
Compare
GeraBart
left a comment
GeraBart
left a comment
There was a problem hiding this comment.
Architecture & Code Review — 5 Key Findings
Verdict: REQUEST CHANGES — The endpoint descriptor abstraction and layer separation (L1/L2/L3) are well designed. Five issues should be addressed.
1. [BLOCKING] Descriptor factories on BaseApiService violate protocol-separated design (ADR-0010)
query(), queryWith(), mutation() hardcode this.protocol(RestProtocol) via dispatchRequest(). stream() hardcodes SseProtocol. The base class is now a protocol router, not a protocol abstraction.
- SRP: BaseApiService manages protocols AND routes protocol-specific requests
- OCP: Adding a third protocol (WebSocket, GraphQL) requires modifying the base class
- DIP: Base class depends on concrete implementations
Fix: Move factories to their respective protocols. Each protocol already has config.baseURL after initialize():
// Service becomes explicit about protocol choice:
readonly getCurrentUser = this.protocol(RestProtocol).query<User>('/user/current');
readonly messageStream = this.protocol(SseProtocol).stream<Message>('/stream');This also eliminates dispatchRequest and anchorDescriptorFactories.
2. [BLOCKING] bootstrapMfeDomains double-wrap not idempotent
bootstrapMfeCore.ts monkey-patches screensetsRegistry.executeActionsChain without a guard. If called twice (hot reload, domain re-init), it wraps the already-wrapped method — injecting QueryClient context multiple times per mount_ext.
Fix: Add a Symbol marker on the patched function and check before patching.
3. [MEDIUM] useStableAdapter declared inside useApiMutation
useApiMutation.ts declares useStableAdapter as a function inside the hook body that calls useRef and useCallback. While functionally correct (called unconditionally 4 times), this may violate rules-of-hooks in stricter ESLint configurations.
Fix: Extract to a module-level hook.
4. [MEDIUM] onDestroy async teardown is fire-and-forget
queryCache.ts — cancelQueriesThenClear fires void queryClient.cancelQueries().then(() => queryClient.clear()). If the framework tears down synchronously after onDestroy, the microtask-queued clear() may not execute.
Fix: Call clear() synchronously or await the cancel.
5. [MEDIUM] Missing test coverage for mutation abort paths
cancelOnSupersede (rapidly firing mutations should abort the previous one) and abortOnUnmount (cleanup on component unmount) have no dedicated test cases. These are critical lifecycle paths.
Refinement on Finding #1 — Descriptor factories are not protocol methodsTo clarify: the fix is not to move
A descriptor factory composes over a protocol — it's not a protocol operation. Putting both on The natural design is a separate composition layer that takes a protocol and config: class AccountsService extends BaseApiService {
private rest = new RestEndpoints(this.protocol(RestProtocol), this.config);
private sse = new SseEndpoints(this.protocol(SseProtocol), this.config);
readonly getCurrentUser = this.rest.query<User>('/user/current');
readonly updateProfile = this.rest.mutation<User, ProfileUpdate>('PUT', '/profile');
readonly messageStream = this.sse.stream<Message>('/stream');
}Each layer does one thing:
Naming of the builder classes is up to the author — the point is the separation of concerns, not specific class names. |
Correction on Finding #1 — separate protocol class, not a builder layerPrevious comment introduced an "endpoint builder" wrapper — that's not the right pattern here. The point is simpler:
The service registers whichever contract it needs via the existing class AccountsService extends BaseApiService {
constructor() {
super(
{ baseURL: '/api/accounts' },
new RestProtocol(), // imperative: get/post/put
new RestQueryProtocol(), // declarative: query/mutation (name TBD)
);
}
readonly getCurrentUser = this.protocol(RestQueryProtocol).query<User>('/user/current');
}This way:
Naming is up to the author — the architectural point is that competing methods for the same functionality don't belong on the same class. |
Finding #6: Cache sharing mechanism — action chain payload injection vs. protocol-level sharingTwo issues with the current QueryClient sharing approach: 6a. Hidden payload field bypasses GTS validation
If payload validation were strict, this approach would break. The field is typed in TypeScript ( 6b. Cache sharing should be an implicit protocol-level optimization, not an action chain concern The current implementation requires:
None of this is necessary. Blob URL isolation creates separate module scopes but shares const CACHE_KEY = Symbol.for('hai3:query-cache');
// Inside the protocol — first instance creates, all others reuse
get queryClient(): QueryClient {
if (!(globalThis as any)[CACHE_KEY]) {
(globalThis as any)[CACHE_KEY] = new QueryClient({ ... });
}
return (globalThis as any)[CACHE_KEY];
}Same QueryClient instance across all MFEs. Same observers, same staleness, same deduplication, same invalidation. All features preserved. This approach:
|
In this architecture, packages/api is descriptor-only, while queryCache() in packages/framework owns QueryClient creation, configuration, invalidation, mock-toggle clearing, and destroy cleanup. |
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
2 times, most recently
from
38057a5 to
1891d80
Compare
Finding #7: Cache sharing is a tooling-level optimization, not a system-level contractTwo issues with 1. Wrong layer. 2. Wrong contract level. Cache sharing across MFEs is an optional performance optimization, not a core architectural concern. The MFE default handling approach uses explicit-only, thin contracts — optimizations should be implicit at the tooling level, not promoted to abstract methods on the system's central orchestration interface. Adding it here means every Alternative: |
I would not use globalThis + Symbol.for() for this. It assumes a specific runtime/isolation model. The screenset's runtime is handler-based and meant to abstract over different loading/isolation strategies, so relying on realm-global lookup couples the behavior to today’s implementation details in a way an explicit mount contract does not. Also, test isolation is worse. Instead of constructing the unit with explicit dependencies, the unit silently reaches out to the ambient state, so tests must mock the environment rather than just pass inputs. When a test fails, it is harder to tell whether the bug is in the unit itself or in leftover global state from another test. With dependency injection, each test gets a fresh instance naturally. With globalThis, you have to remember to tear down and restore the state explicitly. Cache sharing is an application/runtime concern, not a protocol concern. The API package should know how to define or execute requests. Deciding whether multiple app roots, modules, or MFEs share one cache is a host composition decision. Different hosts may want separate caches, shared caches, scoped caches per tenant, per tab, per subtree, or no cache at all. That flexibility is easier when the host/runtime injects the cache explicitly rather than the API package assuming one global shared instance. |
Architectural feedback: Cache sharing mechanismAfter thorough research (4 explorations, all 9 implementation challenges validated), we're requesting a different approach to cross-MFE cache sharing. SOLID violations in the current implementation
Why contracts must stay cleanCache sharing across MFEs is an optional performance optimization, not a core architectural concern. The MFE default handling approach uses explicit-only, thin contracts for system evolution flexibility. Promoting an optimization to an abstract method on the system's central orchestration interface:
Proposed alternative: protocol-level shared fetch cacheMove cache sharing entirely into // Inside RestEndpointProtocol.query().fetch():
const cache = globalThis[Symbol.for('hai3:fetch-cache')];
if (cache) {
return cache.getOrFetch(cacheKey, () => this.rest.get(path));
}
return this.rest.get(path, { signal });Each MFE has its own independent TanStack What changes:
What is removed:
What stays unchanged:
Feasibility validationEach concern was validated with implementation sketches and source-level evidence:
Isolation strategy supportThe protocol doesn't know which isolation model it runs in. It feature-detects:
|
|
@cyberfabric/api got sharedFetchCache, and RestEndpointProtocol uses it for descriptor GETs. That covers transport-level reuse: deduplicating the same request, reusing warm fetch results, handling aborts, and invalidating transport entries by descriptor key. I did not put the whole solution into L1 because cross-MFE cache sharing is not only a transport problem. We also need shared query-state behavior: one root must be able to see cached data produced by another root, optimistic set/remove operations need to propagate across roots, and invalidation needs to affect both the query runtime and the transport cache. That logic sits above the protocol layer. To keep that replaceable, I introduced a FrontX-owned server-state contract in L2/L3 and kept TanStack behind it. framework exposes ServerStateRuntime / ServerStateCache, and react resolves a React adapter by adapterId instead of binding app code directly to TanStack. The current implementation uses TanStack under the hood, but the public surface is our runtime + adapter contract, not QueryClient APIs. So the split is: api owns request reuse |
Alternative architecture proposal: Shared QueryClient via MFE App FactoryAfter studying this PR, the review thread, and the latest Design requirements this proposal is built onCache:
Cache sharing across MFEs:
Framework layers and abstractions:
Where the cache lives: single-layer vs. dual-layerThe current
Two caches require synchronization — invalidation must propagate through both layers, mock-mode clearing must hit both, and the broadcast protocol must prevent echo between runtimes. The alternative: a single-layer cache — one shared TanStack QueryClient at L2. L1 has no cache, only descriptors.
Why L1 doesn't need a cache when QueryClient is shared: With one shared QueryClient, TanStack natively provides:
No synchronization needed — there's only one cache. What the dual-layer approach costsThe
All of this is necessary only if each MFE has its own QueryClient. With a shared QueryClient, estimated new code: ~230 lines (queryCache plugin + thin hook wrappers). Core idea: MFE App FactoryInstead of each MFE creating its own // L2 (microfrontends plugin) prepares a factory during mount:
const createMfeApp = (config?: Partial<HAI3Config>) =>
createHAI3App(config)
.use(queryCache({ queryClient: hostApp.queryClient })) // shared!
.use(themes())
.use(i18n())
.build();
// MFE lifecycle (L3) receives and calls the factory:
mount(container, bridge, mountContext) {
const app = mountContext.appFactory?.() ?? createHAI3App().build();
// app.queryClient IS the shared QueryClient — all TanStack features work
}The MFE developer can use the factory (zero config, shared cache) or ignore it and create their own app (full autonomy, isolated cache). The factory is opt-in, not imposed. Integration spectrumMFEs choose their level of integration — from full shared cache to fully independent:
Partial integration works because the factory returns
Framework-agnostic by designThe architecture has zero framework coupling at L1 and L2:
If an MFE chooses a non-TanStack cache library (SWR, Apollo, RTK Query), it can still use L1 descriptors as its transport contract — // SWR MFE — uses descriptors, own cache:
useSWR(descriptor.key, () => descriptor.fetch())
// Apollo MFE — adapts descriptor to GraphQL query:
useQuery(toApolloQuery(descriptor))The trade-off is explicit: a non-TanStack MFE gets its own isolated cache and loses cross-MFE dedup/invalidation. This is an informed choice, not a limitation — the MFE opts out of shared cache while retaining the descriptor contract for consistent API access. Delivery mechanism: two options for discussionThe factory is a function reference (not serializable). Two clean ways to deliver it from host to MFE: Option F1: // L1: MfeMountContext (generic addition)
interface MfeMountContext {
readonly extensionId?: string;
readonly domainId?: string;
readonly appFactory?: (...args: unknown[]) => unknown; // generic factory
}
// L2: microfrontends plugin sets it during mount action processing
// L3: ThemeAwareReactLifecycle reads it
Option F3: // L1: ChildMfeBridge (generic extension)
interface ChildMfeBridge {
// ...existing methods...
getHostResource<T>(key: string): T | undefined;
}
// L2: RuntimeBridgeFactory attaches resources during bridge creation
// bridge.setHostResource('appFactory', createMfeApp)
// L3: ThemeAwareReactLifecycle reads from bridge
Both options keep L1 screensets free of cache/QueryClient knowledge. Neither option extends Comparison
What I'd preserve from this PR (well-designed, no changes)
Questions for the team
Note on globalThis + Symbol.for()I'm not categorically opposed to the If the team prefers The key point is: regardless of the delivery mechanism (F1 / F3 / globalThis), sharing one QueryClient eliminates the need for SharedFetchCache, ServerStateRuntime, the adapter layer, and the token handoff — which together account for ~1450 lines. I'm interested to hear your thoughts |
|
This is directionally close to what we had in the previous commit. In it, MfeMountContext directly carried queryClient, and ScreensetsRegistry had setMountContextResolver(...). The new comment aims to preserve the old shared-client idea while removing the older L1 coupling. So it is more like:
The best point is the YAGNI critique: if one shared TanStack runtime already exists, the case for a second L1 transport cache plus sync machinery needs to be justified. L1 cache helps when we have separate runtimes or imperfect handoff boundaries. It benefits non-L2 consumers: imperative service calls, tests, scripts, SSR helpers, or any direct api usage. L2 helps once the data is cached in the runtime. L1 helps on the expensive edge case where many consumers miss at once and would otherwise stampede the backend. If we accept that queryClient is usually shared, and if it's not, the common cache doesn't work anymore, the L1 cache is harder to justify.
I didn't have it on the previous commit. Added after review. It's ready and nice to have, but it adds complexity.
If appFactory is added to MfeMountContext, that is a thicker contract than the current minimal identity-only shape, because L1 stops carrying just mount metadata and starts carrying a host capability. Even if it is “generic,” it is still more than pure context. I’d distinguish the options like this:
So if the goal is strictly “minimal contract,” then yes, appFactory makes it thicker. If the goal is “simpler overall architecture,” it may still be worth it, because it can reduce a lot of hidden machinery behind the current token side channel.
Note on globalThis + Symbol.for() globalThis is the choice if we strongly prioritize zero L1 contract changes over explicitness. |
Architectural feedback on the latest updateThree areas need revision. 1. Remove
|
| Item | Action |
|---|---|
ActionsChain.mountRuntimeToken |
Remove |
MfeMountContext.mountRuntimeToken |
Remove |
| Mount runtime token infrastructure | Remove (~250 lines) |
| QueryClient sharing | globalThis[Symbol.for()] inside plugin, queryCache() / queryCacheShared() split |
| ServerStateRuntime / ServerStateReactAdapter | Collapse into plugin and hook internals |
|
@gs-layer Good analysis. We align on several points and diverge on a few. Where we agree:
Where we diverge:
|
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
2 times, most recently
from
84db521 to
e3f9596
Compare
… updates, demo, use ExtensionDomainSlot, dedupe ThemeAwareLifecycle, update docs Signed-off-by: Vuk D. <davydowiktor@gmail.com>
tscbmstubp
force-pushed
the
feat/request-cancellation
branch
from
d9741ff to
6adbf07
Compare
|
4 participants
TanStack Query, abort, optimistic updates, restricted common cache. Profile editing in the demo and a new MFE to demonstrate common cache.
cyberfabric/cyberfabric-core#1101
cyberfabric/cyberfabric-core#1098
Will fix #236
Summary by CodeRabbit
New Features
useApiQuery,useApiMutation,useApiStream, anduseQueryCachefor declarative data fetching and cache management.queryCache()plugin for QueryClient lifecycle ownership and cache invalidation support.AbortSignalfor REST endpoints.Documentation
Refactor
Chores
@tanstack/react-queryand@tanstack/query-coredependencies.