Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ic loading work Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds foundational runtime extensibility and configuration loading to ObjectUI: a widget manifest/registry system (types + core) and a useDynamicApp hook (react) to load app definitions from server metadata with cache invalidation support, alongside expanded auth package test coverage and ROADMAP updates.
Changes:
- Introduces
WidgetManifest/ widget registry type system in@object-ui/typesand exports them from the package entrypoint. - Adds
WidgetRegistryin@object-ui/corewith async widget loading (inline/registry/module), dependency resolution, registry sync, events, and tests. - Adds
useDynamicApphook in@object-ui/reactwith tests, plus additional auth unit tests and ROADMAP checkbox updates.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/widget.ts | Defines widget manifest/source/capability/event types for runtime widget loading/registration. |
| packages/types/src/index.ts | Re-exports widget system types from @object-ui/types. |
| packages/core/src/registry/WidgetRegistry.ts | Implements runtime widget registry with loading strategies, dependency handling, eventing, and component registry sync. |
| packages/core/src/registry/tests/WidgetRegistry.test.ts | Adds unit tests covering WidgetRegistry registration/loading/events/stats behaviors. |
| packages/core/src/index.ts | Exposes WidgetRegistry from @object-ui/core public API. |
| packages/react/src/hooks/useDynamicApp.ts | Adds hook to fetch app config via adapter getApp() with fallback + refresh cache invalidation. |
| packages/react/src/hooks/index.ts | Exports useDynamicApp from hooks barrel. |
| packages/react/src/hooks/tests/useDynamicApp.test.ts | Adds unit tests for useDynamicApp loading/fallback/refresh behaviors. |
| packages/auth/src/tests/types.test.ts | Adds tests for getUserInitials. |
| packages/auth/src/tests/createAuthenticatedFetch.test.ts | Adds tests for auth header injection behavior. |
| packages/auth/src/tests/createAuthClient.test.ts | Adds tests for createAuthClient request construction + error handling. |
| packages/auth/src/tests/AuthProvider.test.tsx | Adds tests for AuthProvider/useAuth/AuthGuard integration behavior. |
| ROADMAP.md | Updates roadmap checkboxes to reflect completion of auth + widget/dynamic app loading milestones. |
| // Resolve dependencies first | ||
| if (manifest.dependencies) { | ||
| for (const dep of manifest.dependencies) { | ||
| if (!this.isLoaded(dep)) { | ||
| await this.load(dep); |
There was a problem hiding this comment.
Dependency resolution recurses with await this.load(dep) but has no cycle detection. A cyclic dependency graph (A -> B -> A) will cause infinite recursion / stack overflow. Consider tracking a loading/visiting set per call and throwing a clear error when a cycle is detected.
| // Sync to component registry if available | ||
| if (this.componentRegistry) { | ||
| this.componentRegistry.register(manifest.type, component as any, { | ||
| label: manifest.label, | ||
| icon: manifest.icon, |
There was a problem hiding this comment.
When syncing into Registry, this registers manifest.type without a namespace. Registry.register() currently emits a deprecation console.warn for non-namespaced registrations, so loading widgets will spam warnings at runtime. Consider providing a namespace in the meta (while still allowing the short-name fallback) to avoid the warning.
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; |
There was a problem hiding this comment.
Unused import: beforeEach is imported from vitest but never used in this test file.
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |
| import { describe, it, expect, vi } from 'vitest'; |
| try { | ||
| const serverConfig = await adapter.getApp(appId); | ||
|
|
||
| if (!mountedRef.current) return; | ||
|
|
There was a problem hiding this comment.
loadConfig only guards against unmounts via mountedRef, but it does not guard against stale in-flight requests. If appId (or adapter/staticConfig/enabled) changes while a previous getApp call is still pending, the older request can resolve later and overwrite state with the wrong app config. Consider tracking a monotonically increasing request id (or AbortController) and only applying results for the latest request.
| async load(name: string): Promise<ResolvedWidget> { | ||
| // Return cached if already loaded | ||
| const cached = this.resolved.get(name); | ||
| if (cached) return cached; |
There was a problem hiding this comment.
load() only memoizes completed loads in this.resolved. If load(name) is called concurrently before the first call finishes, the widget may be resolved/imported multiple times and events may emit multiple times. Consider caching an in-flight Promise per name to dedupe concurrent loads.
| unregister(name: string): boolean { | ||
| const existed = this.manifests.delete(name); | ||
| this.resolved.delete(name); | ||
| if (existed) { | ||
| this.emit({ type: 'widget:unregistered', name }); |
There was a problem hiding this comment.
unregister() removes the widget from manifests/resolved, but if the widget was previously loaded it remains registered in componentRegistry (there is no corresponding unregister). That can make unregister() appear to succeed while schema rendering still finds the component via the registry. Consider either adding/removing a registry entry (if supported) or documenting this behavior explicitly / adding a syncToComponentRegistry option.
| case 'module': { | ||
| const mod = await import(/* @vite-ignore */ source.url); | ||
| const exportName = source.exportName ?? 'default'; | ||
| const component = mod[exportName]; | ||
| if (!component) { |
There was a problem hiding this comment.
Runtime module loading uses import(/* @vite-ignore */ source.url), which bakes a Vite-specific directive into @object-ui/core and may not behave correctly in non-Vite bundlers/environments. Consider making the module loader injectable (e.g., via WidgetRegistryOptions) and validating/normalizing URLs before executing dynamic imports.
| * Tests for useDynamicApp hook | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; |
There was a problem hiding this comment.
Unused import: beforeEach is imported from vitest but never used in this test file.
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |
| import { describe, it, expect, vi } from 'vitest'; |
| it('loads a widget from the component registry', async () => { | ||
| const componentRegistry = new Registry(); | ||
| const component = () => null; | ||
| componentRegistry.register('existing-type', component); |
There was a problem hiding this comment.
Registry.register() emits a deprecation console.warn when called without a namespace. This test registers existing-type without meta, which can add noisy warnings to test output. Consider passing a namespace in the meta argument (or using a helper) to keep test logs clean.
| componentRegistry.register('existing-type', component); | |
| componentRegistry.register('existing-type', component, { namespace: 'test' }); |
Delivers the remaining items from ROADMAP §1.5 (auth test coverage) and §1.6 (widget system + dynamic app loading).
WidgetManifest type system (
@object-ui/types)WidgetManifestinterface with three source strategies:module(ES module URL),inline(pre-loaded component),registry(existing component registry key)WidgetInput,WidgetCapabilities,ResolvedWidget,WidgetRegistryEventWidgetRegistry (
@object-ui/core)useDynamicApphook (@object-ui/react)adapter.getApp(appId), falls back to static configrefresh()invalidates MetadataCache and re-fetches for schema hot-reloadAuth package test coverage
createAuthClient,createAuthenticatedFetch,AuthProvider,useAuth,AuthGuard, andgetUserInitialsROADMAP
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.