Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a plugin system and custom-config registry, plugin-driven activities/sources/file-actions/icons/i18n, UI components to render plugin activities, and defers setCurrentActivity/setModalState execution to the next tick to align lifecycle timing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config as Config Block
participant PM as PluginManager
participant Plugin
participant Registry as Registry API
User->>Config: set config.plugins = [plugin]
Config->>PM: notify plugin config change
PM->>PM: _syncPlugins()
PM->>Plugin: setup(pluginApi, uploaderApi)
Plugin->>Registry: registerSource / registerActivity / registerConfig / registerIcon / registerFileAction
Registry-->>PM: store registrations
PM->>Config: publish custom config defs (configRegistry)
PM->>PM: notifySubscribers()
User->>Config: set config.plugins = []
Config->>PM: notify plugin config change
PM->>Plugin: dispose()
PM->>Registry: unregister all plugin registrations
PM->>PM: notifySubscribers()
sequenceDiagram
participant App
participant LitBlock as Component
participant Shared as SharedInstances
participant PM as PluginManager
participant Activity as Plugin Activity
App->>LitBlock: initialize component
LitBlock->>Shared: request pluginManager
Shared->>PM: provide PluginManager
App->>LitBlock: setCurrentActivity('plugin-act', params)
LitBlock->>PM: lookup activity registration
PM-->>LitBlock: return registration
LitBlock->>Activity: call render(hostEl, params)
Activity-->>LitBlock: returns dispose()
App->>LitBlock: plugin unregistered
LitBlock->>Activity: call dispose()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive plugin system for the file uploader, enabling extensibility through custom sources, activities, file actions, icons, i18n translations, and configuration options. The system is designed as a WIP (Work in Progress) foundation for modularizing uploader functionality.
Changes:
- Adds PluginManager as a SharedInstance that handles plugin registration, lifecycle management, and cleanup
- Implements a dynamic custom configuration system using runtime property descriptors and MutationObserver for attribute handling
- Refactors existing components (SourceList, SourceBtn, Icon, FileItem) to integrate with the plugin system
- Introduces PluginActivityRenderer for rendering plugin-provided activities in modal and inline modes
- Updates UploaderPublicApi with setTimeout workarounds for async activity registration timing issues
- Creates urlSourcePlugin as an example of migrating built-in functionality to the plugin architecture
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/abstract/managers/plugin/PluginManager.ts |
Core plugin lifecycle manager with async setup/teardown and registration tracking |
src/abstract/managers/plugin/PluginTypes.ts |
Type definitions for plugin API, including registry, config, and activity APIs |
src/abstract/customConfigOptions.ts |
Custom configuration registry for runtime-defined config options |
src/blocks/Config/Config.ts |
Extended to support dynamic plugin configs via property descriptors and MutationObserver |
src/blocks/PluginActivityRenderer/PluginActivityRenderer.ts |
New components for rendering plugin activities in modal or inline modes |
src/blocks/SourceList/SourceList.ts |
Refactored to merge built-in and plugin sources into unified source list |
src/blocks/SourceBtn/SourceBtn.ts |
Simplified to use SourceButtonConfig instead of type-based registration |
src/blocks/Icon/Icon.ts |
Extended to render plugin-registered SVG icons via unsafeSVG directive |
src/blocks/FileItem/FileItem.ts |
Integrated plugin file actions with conditional rendering based on shouldRender callbacks |
src/blocks/Modal/Modal.ts |
Minor updates for activity type handling and current activity clearing |
src/abstract/UploaderPublicApi.ts |
Added setTimeout workarounds for async activity registration timing |
src/abstract/managers/LocaleManager.ts |
Extended to apply plugin-registered i18n translations |
src/lit/shared-instances.ts |
Added pluginManager to shared instances and implemented when() helper |
src/lit/SharedState.ts |
Added pluginManager and sharedContextInstances to state type |
src/lit/LitBlock.ts |
Moved sharedContextInstances to shared state for cross-block access |
src/lit/LitActivityBlock.ts |
Updated activity type system to support plugin-registered activities |
src/plugins/urlSourcePlugin.ts |
Example plugin implementing URL source as external plugin |
tests/plugins/*.e2e.test.tsx |
Comprehensive test suite covering all plugin functionality |
demo/features/plugins.html |
Demo page showcasing plugin system capabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Give it time to settle, then verify callback is not called for new changes | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| callback.mockClear(); | ||
|
|
||
| // This should not trigger the old subscription's callback | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| expect(callback).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test doesn't actually verify that the subscription callback is not called after cleanup. It just waits 100ms twice and checks that the callback wasn't called, but no config value changes are attempted after unregistering the plugin. The test should actually try to change the config value to verify the subscription was cleaned up.
Add a line like config.cleanupOption = 'changed'; after line 152 to properly test that the subscription callback is not invoked after plugin unregistration.
| public override initCallback(): void { | ||
| this.activityType = (this.registration?.id as ActivityType) ?? null; | ||
| super.initCallback(); | ||
| this._ensureRegistered(); | ||
| } |
There was a problem hiding this comment.
The activityType is being set from registration.id in initCallback, but the registration property might not be available yet since properties are set asynchronously in Lit. If registration is set after initCallback runs, the activityType will be null and the activity won't be properly registered.
Consider moving the activityType assignment to willUpdate() or using a computed property pattern to ensure it's always in sync with the registration property.
| if (changedProperties.has('registration')) { | ||
| this._ensureRegistered(); | ||
| if (this.isActivityActive) { | ||
| this._disposeActivity(); | ||
| this._renderActivity(); | ||
| } | ||
| } |
There was a problem hiding this comment.
When the registration property changes, the code re-registers the activity but doesn't update the activityType. This could lead to inconsistencies where the activity is registered with a different ID than what's stored in activityType.
The activityType should be updated when registration changes: this.activityType = (this.registration?.id as ActivityType) ?? null; before calling _ensureRegistered().
| try { | ||
| await this._registerPlugin(plugin); | ||
| } catch (error) { | ||
| this._purgeRegistrations(plugin.id); |
There was a problem hiding this comment.
After a plugin setup fails and registrations are purged, _notifySubscribers() is called immediately, but the failed plugin is not removed from the _plugins map. This means the plugin state will be inconsistent - it remains in the map but has no registrations.
Add this._plugins.delete(plugin.id); after purging registrations to fully clean up the failed plugin state.
| this._purgeRegistrations(plugin.id); | |
| this._purgeRegistrations(plugin.id); | |
| this._plugins.delete(plugin.id); |
| this.set$({ | ||
| '*currentActivity': LitActivityBlock.activities.UPLOAD_LIST, | ||
| }); | ||
| this.modalManager?.open(LitActivityBlock.activities.UPLOAD_LIST); |
There was a problem hiding this comment.
The order of setting *currentActivity and calling modalManager.open() was changed. Previously, the modal was opened first, then the activity was set. Now the activity is set first, then the modal is opened. While this change aligns with the expected order, it might affect timing-sensitive code that depends on the previous ordering.
Verify that this order change doesn't break any existing behavior, especially around modal initialization and activity lifecycle hooks.
| this.set$({ | |
| '*currentActivity': LitActivityBlock.activities.UPLOAD_LIST, | |
| }); | |
| this.modalManager?.open(LitActivityBlock.activities.UPLOAD_LIST); | |
| this.modalManager?.open(LitActivityBlock.activities.UPLOAD_LIST); | |
| this.set$({ | |
| '*currentActivity': LitActivityBlock.activities.UPLOAD_LIST, | |
| }); |
src/blocks/SourceList/SourceList.ts
Outdated
| // case UploadSource.URL: | ||
| // return { | ||
| // id: type, | ||
| // label: 'src-type-from-url', | ||
| // icon: type, | ||
| // onClick: () => this._openActivity('url'), | ||
| // }; |
There was a problem hiding this comment.
The URL source handling is commented out. This appears to be intentional since the URL source is now being implemented as a plugin (urlSourcePlugin.ts), but the commented code should be removed to avoid confusion.
Either remove these commented lines entirely or add a comment explaining why URL source was moved to a plugin.
| // case UploadSource.URL: | |
| // return { | |
| // id: type, | |
| // label: 'src-type-from-url', | |
| // icon: type, | |
| // onClick: () => this._openActivity('url'), | |
| // }; |
| if (this._pluginSvg) { | ||
| return html`${this.yield('', html`${unsafeSVG(this._pluginSvg)}`)}`; |
There was a problem hiding this comment.
Using unsafeSVG with plugin-provided SVG content creates a potential XSS vulnerability. A malicious plugin could inject arbitrary HTML/JavaScript through the SVG string.
Consider sanitizing the SVG content before rendering it, or validate that it only contains safe SVG elements and attributes. You could use a library like DOMPurify or implement allowlist-based SVG validation.
src/abstract/UploaderPublicApi.ts
Outdated
| setTimeout(() => { | ||
| if (hasBlockInCtx(this._sharedInstancesBag.blocksRegistry, (b) => b.activityType === activityType)) { | ||
| this._ctx.pub('*currentActivityParams', params[0] ?? {}); | ||
| this._ctx.pub('*currentActivity', activityType); | ||
| return; | ||
| } | ||
| console.warn(`Activity type "${activityType}" not found in the context`); | ||
| }); |
There was a problem hiding this comment.
The setTimeout with no delay (defaults to 0) is used to defer execution, but there's no guarantee that a single tick is enough for Lit to render and register activities. In slow environments or with complex plugin activities, the activity might not be registered yet, leading to the warning being logged even for valid activities.
Consider using a more robust approach: either check multiple times with a timeout, use requestAnimationFrame, or better yet, implement an ActivityManager (as the TODO suggests) that provides proper async/await APIs for activity registration.
src/plugins/urlSourcePlugin.ts
Outdated
| registry.registerActivity({ | ||
| id: URL_ACTIVITY_ID, | ||
| icon: URL_ACTIVITY_ID, | ||
| render: (host) => { | ||
| const urlSource = document.createElement('uc-url-source'); | ||
| host.replaceChildren(urlSource); | ||
| return () => host.replaceChildren(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The urlSourcePlugin registers an activity with 'icon: URL_ACTIVITY_ID' but icon registration is not included in the activity registration type. The PluginActivityRegistration type only has 'id' and 'render' properties, so this icon property will be ignored.
If icons are needed for activities, either update the PluginActivityRegistration type to include an optional icon property, or remove the icon property from this registration.
| it('should keep plugin i18n overrides even after plugin is unregistered (current behavior)', async () => { | ||
| const plugin = createTestPlugin({ | ||
| id: 'i18n-persist', | ||
| setup: ({ pluginApi }) => { | ||
| pluginApi.registry.registerI18n({ | ||
| en: { | ||
| 'upload-files': 'Translated Upload', | ||
| }, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| const { config } = await renderUploader([plugin]); | ||
| const api = getApi(); | ||
|
|
||
| api.setModalState(true); | ||
| await expect.element(page.getByText('Translated Upload')).toBeVisible(); | ||
|
|
||
| config.plugins = []; | ||
|
|
||
| api.setModalState(true); | ||
| await expect.element(page.getByText('Translated Upload')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
The test title says "should keep plugin i18n overrides even after plugin is unregistered (current behavior)" which suggests this might be unintended behavior. When a plugin is unregistered, its i18n translations persist in the locale state, which could be confusing or cause conflicts if another plugin tries to register the same translation keys.
Consider whether plugin i18n should be cleaned up when plugins are unregistered, similar to how other plugin registrations are purged. If the current behavior is intended, document why i18n is special-cased.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/abstract/managers/LocaleManager.ts`:
- Around line 20-29: When pluginManager.onPluginsChange triggers and you add the
subscription via addSub, you currently call
_applyPluginLocales(this._localeName) but do not reapply user overrides; update
the callback passed to pluginManager.onPluginsChange to call
this._applyPluginLocales(this._localeName) and then immediately call
this._applyOverrides(this._localeName) (or this._applyOverrides() if it takes no
args) so user-provided localeDefinitionOverride values are reapplied and keep
precedence after plugins change; use the existing symbols
pluginManager.onPluginsChange, addSub, _applyPluginLocales, _applyOverrides and
_localeName to locate and modify the callback.
In `@src/blocks/FileItem/FileItem.ts`:
- Around line 303-318: The try-catch in _handlePluginFileAction currently won't
catch rejections from async plugin handlers; change handling so async rejections
are caught by either making _handlePluginFileAction async and awaiting
action.onClick(outputFileEntry) with a try/catch, or by wrapping the call in
Promise.resolve(action.onClick(outputFileEntry)).catch(...) to log the error
(referencing action.id for context); ensure PluginFileActionRegistration async
handlers are supported and preserve early returns when this.uid or
this.api.getOutputItem(this.uid) are missing.
In `@src/blocks/Icon/Icon.ts`:
- Around line 66-68: The code currently renders plugin-provided SVG via
unsafeSVG(this._pluginSvg) inside Icon.ts (the this._pluginSvg branch in the
render/yield logic), but there is no validation or documented trust boundary;
update the Icon rendering flow to either (a) validate/sanitize the SVG string
before passing it to unsafeSVG (e.g., strip scripts/on* attributes, disallow
foreignObject, or run through a trusted SVG sanitizer) or (b) explicitly
document the trust requirement in the plugin API/spec so contributors know
plugin SVGs must be trusted; ensure the change is applied where this._pluginSvg
is used and keep unsafeSVG only for sanitized/trusted input and add a clear
comment referencing the plugin registration contract.
In `@src/blocks/SourceList/SourceList.ts`:
- Around line 185-191: The plugin source config currently sets onClick to call
source.onSelect() directly in _makePluginSourceConfig (and similarly in
_makeBuiltInSourceConfig), which can throw or return a rejected promise; update
this to call a guarded wrapper that catches synchronous exceptions and
awaits/rejects promises, logging failures. Implement the guard either by
replacing the onClick assignment in
_makePluginSourceConfig/_makeBuiltInSourceConfig with an async wrapper that
try/catches around await source.onSelect(), or centralize the logic inside
SourceBtn.activate() so it wraps any provided onClick handler with try/catch and
logs errors consistent with the FileItem.ts file-action error-handling pattern.
Ensure logging includes the source id/label and the caught error.
In `@tests/plugins/utils.tsx`:
- Around line 40-48: Add missing type imports for Config and UploadCtxProvider
used in tests/plugins/utils.tsx: import the types from '@/index.ts' and use an
import type statement (e.g., import type { Config, UploadCtxProvider } from
'@/index.ts';) so the `as Config` and `as UploadCtxProvider` assertions and any
function signatures referencing `Config` compile correctly; place this at the
top of the file alongside other imports.
🧹 Nitpick comments (17)
src/solutions/file-uploader/minimal/FileUploaderMinimal.ts (1)
194-194: Minor: Inconsistent indentation.The
uc-plugin-activity-rendererelement has extra leading spaces compared to sibling elements in the template.🧹 Suggested fix
- <uc-plugin-activity-renderer mode="modal"></uc-plugin-activity-renderer> + <uc-plugin-activity-renderer mode="modal"></uc-plugin-activity-renderer>tests/plugins/icon-and-i18n.e2e.test.tsx (1)
60-82: Consider if i18n persistence after plugin unregister is intentional.The test documents that plugin i18n overrides persist even after the plugin is unregistered. While this is labeled as "current behavior", this could lead to stale translations if plugins are dynamically added/removed. If this is intentional (e.g., for performance), consider adding a brief code comment in
LocaleManager._applyPluginLocalesexplaining the rationale.tests/plugins/cleanup-edge-cases.e2e.test.tsx (2)
27-55: Test comment and assertions are misaligned.The comment on line 51 states "The final state should have the plugin registered", but the assertion on line 53 only verifies that
setupwas called at least once. This doesn't confirm the plugin remains registered in the final state. Consider adding an assertion that validates the plugin is actually present after the rapid toggling.💡 Suggested improvement
// The final state should have the plugin registered await vi.waitFor(() => { - expect(setup.mock.calls.length).toBeGreaterThanOrEqual(1); + expect(setup).toHaveBeenCalled(); }); + + // Verify plugin is actually registered in final state + // (e.g., by checking a registered source/action is present)
86-108: Consider wrapping the final assertion invi.waitForfor consistency.Line 107's assertion (
expect(setup2).not.toHaveBeenCalled()) is outside thewaitForblock, which could lead to flakiness if the plugin registration has any async timing. Though unlikely given the synchronous nature of setup, wrapping it would be more robust.tests/plugins/source-registration.e2e.test.tsx (1)
74-95: Consider usingvi.waitForfor the removal assertion.The assertion on line 94 (
not.toBeInTheDocument()) is executed immediately after settingconfig.plugins = []. Since plugin unregistration may involve async cleanup, wrapping this invi.waitForwould make the test more robust.💡 Suggested fix
config.plugins = []; - await expect.element(page.getByText('Removable Source')).not.toBeInTheDocument(); + await vi.waitFor(async () => { + await expect.element(page.getByText('Removable Source')).not.toBeInTheDocument(); + });src/abstract/managers/LocaleManager.ts (1)
70-94: Type casting is awkward but functional; consider a dedicated type.The inline type assertion on lines 78-80 is hard to read. Consider defining a more explicit type for the i18n snapshot entries to improve clarity.
💡 Suggested improvement
+type PluginI18nEntry = { + pluginId: string; +} & Record<string, Partial<LocaleDefinition>>; private _applyPluginLocales(localeName: string): void { // ... for (const entry of snapshot.i18n) { - const { pluginId: _pluginId, ...locales } = entry as unknown as Record<string, Partial<LocaleDefinition>> & { - pluginId?: string; - }; + const { pluginId: _pluginId, ...locales } = entry as unknown as PluginI18nEntry;tests/plugins/file-action-registration.e2e.test.tsx (1)
144-168: Consider usingvi.waitForfor the removal assertion.Similar to the source registration test, line 167's assertion is executed immediately after setting
config.plugins = []. Wrapping invi.waitForwould be more robust.💡 Suggested fix
config.plugins = []; - await expect.element(page.getByRole('button', { name: 'remove-file-action' })).not.toBeInTheDocument(); + await vi.waitFor(async () => { + await expect.element(page.getByRole('button', { name: 'remove-file-action' })).not.toBeInTheDocument(); + });tests/plugins/activity-registration.e2e.test.tsx (1)
156-183: Consider usingvi.waitForfor the host removal assertion.Line 182's assertion is executed immediately after
config.plugins = []. For consistency with async plugin cleanup, consider wrapping invi.waitFor.💡 Suggested fix
config.plugins = []; - await expect.element(page.getByText('Activity To Remove')).not.toBeInTheDocument(); + await vi.waitFor(async () => { + await expect.element(page.getByText('Activity To Remove')).not.toBeInTheDocument(); + });tests/plugins/activity-api.e2e.test.tsx (2)
6-30: Consider adding a guard for potential undefined access.The
activityApivariable on line 7 is accessed at line 27 insidewaitFor, but TypeScript cannot guarantee it's assigned since the assertion runs after an async boundary. While this works in practice (setup runs synchronously during plugin registration), adding a guard would improve robustness.💡 Suggested improvement
await vi.waitFor(() => { + expect(activityApi).toBeDefined(); const params = activityApi.getParams(); expect(params).toMatchObject({ key: 'value' }); });
65-94: Fixed delays may be flaky; consider more deterministic waits.Lines 85 and 92 use
setTimeout(resolve, 100)which could be flaky in slower CI environments. Consider usingvi.waitForwith a condition or increasing the timeout for more reliability.💡 Suggested improvement
- // Wait for cleanup to happen - await new Promise((resolve) => setTimeout(resolve, 100)); - paramsCallback.mockClear(); + // Wait for cleanup to propagate + await vi.waitFor(() => { + // Give time for async cleanup + }, { timeout: 500 }); + paramsCallback.mockClear();src/blocks/FileItem/FileItem.ts (1)
526-539: Consider using a localized label for accessibility.Using
action.idfortitleandaria-labelmay not provide a user-friendly description. If plugin actions have alabelproperty, consider using it for better accessibility.src/blocks/SourceBtn/SourceBtn.ts (1)
47-50: Consider adding error handling foronClickfailures.While using
voidto discard the Promise is acceptable, unhandled rejections fromonClickwill go to the global handler. Consider adding a.catch()for robustness.♻️ Proposed enhancement
public activate(): void { if (!this.source) return; - void this.source.onClick(); + void this.source.onClick().catch?.((error) => { + console.error(`Error in source onClick (${this.source?.id}):`, error); + }); }Note: The
?.is needed becauseonClickcan returnvoid(not a Promise).src/solutions/file-uploader/inline/FileUploaderInline.ts (1)
85-95: Consider preserving explicit type annotations in subscription callbacks.The change from explicit types (
val: string | null,list: unknown) to inferred types (val,list) slightly reduces code clarity, though functionality remains correct.tests/plugins/custom-config.e2e.test.tsx (1)
147-154: Fixed timeouts in cleanup test may cause flakiness.Using
setTimeoutwith fixed delays (100ms) for testing cleanup behavior can be flaky in CI environments with varying execution speeds. Consider usingvi.waitForwith a condition instead.♻️ Suggested improvement
// Unregister the plugin config.plugins = []; - // Give it time to settle, then verify callback is not called for new changes - await new Promise((resolve) => setTimeout(resolve, 100)); - callback.mockClear(); - - // This should not trigger the old subscription's callback - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(callback).not.toHaveBeenCalled(); + // Wait for unsubscription to complete + await vi.waitFor(() => { + // Give the cleanup some time, then verify callback is not called + expect(callback).not.toHaveBeenCalled(); + }, { timeout: 500 });src/abstract/customConfigOptions.ts (1)
70-76: Deleting from Map while iterating may cause issues.Modifying a Map during iteration with
for...ofcan skip entries or cause unexpected behavior in some JavaScript engines. Consider collecting keys first, then deleting.♻️ Safer iteration pattern
public unregisterByPlugin(pluginId: string): void { + const keysToDelete: string[] = []; for (const [name, definition] of this._definitions) { if (definition.pluginId === pluginId) { - this._definitions.delete(name); + keysToDelete.push(name); } } + for (const key of keysToDelete) { + this._definitions.delete(key); + } }src/abstract/managers/plugin/PluginManager.ts (1)
218-226: Silent error swallowing in subscriber notification may hide bugs.Errors in subscriber callbacks are silently caught and ignored. Consider at least logging these errors in debug mode to aid troubleshooting.
♻️ Log subscriber errors in debug mode
private _notifySubscribers(): void { for (const callback of this._subscribers) { try { callback(); - } catch { - // Ignore subscriber errors + } catch (error) { + // Log but don't propagate subscriber errors + this._debugPrint('Subscriber callback error', error); } } }src/abstract/UploaderPublicApi.ts (1)
370-381: Async deferral may surprise API consumers.The
setTimeoutdeferral meanssetCurrentActivityis now asynchronous, which could surprise callers expecting synchronous behavior. The TODO comment acknowledges this is a workaround.Consider returning a Promise to make the async nature explicit to API consumers:
♻️ Make async behavior explicit
public setCurrentActivity = <T extends RegisteredActivityType>( activityType: T, ...params: T extends keyof ActivityParamsMap ? [ActivityParamsMap[T]] extends [never] ? [] : [ActivityParamsMap[T]] : [] - ) => { + ): Promise<void> => { - setTimeout(() => { + return new Promise((resolve) => setTimeout(() => { if (hasBlockInCtx(this._sharedInstancesBag.blocksRegistry, (b) => b.activityType === activityType)) { this._ctx.pub('*currentActivityParams', params[0] ?? {}); this._ctx.pub('*currentActivity', activityType); - return; + resolve(); + return; } console.warn(`Activity type "${activityType}" not found in the context`); - }); + resolve(); + })); };
| const pluginManager = sharedInstancesBag.pluginManager; | ||
| if (pluginManager?.onPluginsChange) { | ||
| this.addSub( | ||
| pluginManager.onPluginsChange(() => { | ||
| if (this._localeName) { | ||
| this._applyPluginLocales(this._localeName); | ||
| } | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
User overrides may be lost when plugins change.
When onPluginsChange fires (line 23-27), only _applyPluginLocales is called. This could cause newly registered plugin locales to override user-provided localeDefinitionOverride values, since _applyOverrides isn't reapplied afterward.
Consider also calling _applyOverrides after _applyPluginLocales to maintain the correct precedence:
🔧 Proposed fix
this.addSub(
pluginManager.onPluginsChange(() => {
if (this._localeName) {
this._applyPluginLocales(this._localeName);
+ const overrides = this._cfg.localeDefinitionOverride?.[this._localeName];
+ if (overrides) {
+ this._applyOverrides(this._localeName, overrides);
+ }
}
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pluginManager = sharedInstancesBag.pluginManager; | |
| if (pluginManager?.onPluginsChange) { | |
| this.addSub( | |
| pluginManager.onPluginsChange(() => { | |
| if (this._localeName) { | |
| this._applyPluginLocales(this._localeName); | |
| } | |
| }), | |
| ); | |
| } | |
| const pluginManager = sharedInstancesBag.pluginManager; | |
| if (pluginManager?.onPluginsChange) { | |
| this.addSub( | |
| pluginManager.onPluginsChange(() => { | |
| if (this._localeName) { | |
| this._applyPluginLocales(this._localeName); | |
| const overrides = this._cfg.localeDefinitionOverride?.[this._localeName]; | |
| if (overrides) { | |
| this._applyOverrides(this._localeName, overrides); | |
| } | |
| } | |
| }), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/abstract/managers/LocaleManager.ts` around lines 20 - 29, When
pluginManager.onPluginsChange triggers and you add the subscription via addSub,
you currently call _applyPluginLocales(this._localeName) but do not reapply user
overrides; update the callback passed to pluginManager.onPluginsChange to call
this._applyPluginLocales(this._localeName) and then immediately call
this._applyOverrides(this._localeName) (or this._applyOverrides() if it takes no
args) so user-provided localeDefinitionOverride values are reapplied and keep
precedence after plugins change; use the existing symbols
pluginManager.onPluginsChange, addSub, _applyPluginLocales, _applyOverrides and
_localeName to locate and modify the callback.
| if (this._pluginSvg) { | ||
| return html`${this.yield('', html`${unsafeSVG(this._pluginSvg)}`)}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SVG validation/sanitization in plugin registration
rg -n -A5 'icons.*svg|registerIcon|PluginIconRegistration' --type=tsRepository: uploadcare/file-uploader
Length of output: 5206
🏁 Script executed:
#!/bin/bash
# Search for plugin registration interface/type definitions
rg -n 'PluginIconRegistration|plugin.*icon|icon.*plugin' --type=ts | head -50Repository: uploadcare/file-uploader
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Search for any sanitization utilities or security-related code
rg -n 'sanitize|sanitizer|xss|validate.*svg' --type=tsRepository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for documentation or comments about plugin trust/security
rg -n 'trust|trusted.*plugin|plugin.*trust|security.*plugin' --type=tsRepository: uploadcare/file-uploader
Length of output: 50
Document or enforce SVG validation for plugin-provided icons.
The plugin registration process accepts SVG strings without any validation or sanitization. Using unsafeSVG() to render this content assumes plugins are trusted sources, but this assumption is not documented in the code. If plugins can be contributed by untrusted parties, add validation/sanitization to prevent XSS vulnerabilities. At minimum, document the trust assumptions for plugin SVG content in the plugin API specification.
🤖 Prompt for AI Agents
In `@src/blocks/Icon/Icon.ts` around lines 66 - 68, The code currently renders
plugin-provided SVG via unsafeSVG(this._pluginSvg) inside Icon.ts (the
this._pluginSvg branch in the render/yield logic), but there is no validation or
documented trust boundary; update the Icon rendering flow to either (a)
validate/sanitize the SVG string before passing it to unsafeSVG (e.g., strip
scripts/on* attributes, disallow foreignObject, or run through a trusted SVG
sanitizer) or (b) explicitly document the trust requirement in the plugin
API/spec so contributors know plugin SVGs must be trusted; ensure the change is
applied where this._pluginSvg is used and keep unsafeSVG only for
sanitized/trusted input and add a clear comment referencing the plugin
registration contract.
| private _makePluginSourceConfig(source: PluginSourceRegistration): SourceButtonConfig { | ||
| return { | ||
| id: source.id, | ||
| label: source.label, | ||
| icon: source.icon, | ||
| onClick: () => source.onSelect(), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find any existing plugin error handling utilities
rg -n "handlePluginError|pluginError" -g'*.ts'Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Find all onSelect invocations to check for consistent error handling
rg -n "\.onSelect\(" -g'*.ts' -A 2Repository: uploadcare/file-uploader
Length of output: 238
🏁 Script executed:
# Examine the SourceList.ts file to understand the full context
cat -n src/blocks/SourceList/SourceList.ts | head -200Repository: uploadcare/file-uploader
Length of output: 7621
🏁 Script executed:
# Search for PluginSourceRegistration to understand its type
rg -n "PluginSourceRegistration" -g'*.ts' -A 5 | head -50Repository: uploadcare/file-uploader
Length of output: 4188
🏁 Script executed:
# Look for similar error handling patterns in the codebase
rg -n "Promise\.resolve.*catch" -g'*.ts' | head -20Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Find SourceBtn component and how it handles onClick
cat -n src/blocks/SourceBtn/SourceBtn.ts | head -150Repository: uploadcare/file-uploader
Length of output: 2053
🏁 Script executed:
# Search for global error handling or addEventListener patterns
rg -n "addEventListener|onerror|catch|error.*handler" -g'*.ts' src/blocks/SourceBtn/ -A 2Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Check if there's a pattern for handling onClick errors elsewhere
rg -n "onClick.*try|onClick.*catch" -g'*.ts' -B 2 -A 4Repository: uploadcare/file-uploader
Length of output: 1074
🏁 Script executed:
# Examine how the SourceButtonConfig is defined and used
rg -n "SourceButtonConfig" -g'*.ts' -B 2 -A 5 | head -80Repository: uploadcare/file-uploader
Length of output: 4332
🏁 Script executed:
# Search for activate() calls and other direct onClick invocations
rg -n "\.activate\(\)|\.onClick\(" -g'*.ts' -B 2 -A 2 | head -50Repository: uploadcare/file-uploader
Length of output: 1187
🏁 Script executed:
# Check if there are any other places where SourceButtonConfig onClick is called
rg -n "source\.onClick|config\.onClick" -g'*.ts' -B 2 -A 2Repository: uploadcare/file-uploader
Length of output: 347
Guard plugin onSelect against thrown errors and rejected promises.
source.onSelect() is invoked without error handling in SourceBtn.activate(). Unhandled promise rejections or sync throws will bubble to the window. Wrap the invocation and log failures.
🛡️ Suggested fix
- onClick: () => source.onSelect(),
+ onClick: () => {
+ try {
+ Promise.resolve(source.onSelect()).catch((err) => {
+ console.error('Plugin source onSelect failed', source.id, err);
+ });
+ } catch (err) {
+ console.error('Plugin source onSelect failed', source.id, err);
+ }
+ },Note: This same pattern applies to built-in sources created in _makeBuiltInSourceConfig(). Consider centralizing error handling in SourceBtn.activate() for consistency with the file action handling pattern in FileItem.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _makePluginSourceConfig(source: PluginSourceRegistration): SourceButtonConfig { | |
| return { | |
| id: source.id, | |
| label: source.label, | |
| icon: source.icon, | |
| onClick: () => source.onSelect(), | |
| }; | |
| private _makePluginSourceConfig(source: PluginSourceRegistration): SourceButtonConfig { | |
| return { | |
| id: source.id, | |
| label: source.label, | |
| icon: source.icon, | |
| onClick: () => { | |
| try { | |
| Promise.resolve(source.onSelect()).catch((err) => { | |
| console.error('Plugin source onSelect failed', source.id, err); | |
| }); | |
| } catch (err) { | |
| console.error('Plugin source onSelect failed', source.id, err); | |
| } | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/blocks/SourceList/SourceList.ts` around lines 185 - 191, The plugin
source config currently sets onClick to call source.onSelect() directly in
_makePluginSourceConfig (and similarly in _makeBuiltInSourceConfig), which can
throw or return a rejected promise; update this to call a guarded wrapper that
catches synchronous exceptions and awaits/rejects promises, logging failures.
Implement the guard either by replacing the onClick assignment in
_makePluginSourceConfig/_makeBuiltInSourceConfig with an async wrapper that
try/catches around await source.onSelect(), or centralize the logic inside
SourceBtn.activate() so it wraps any provided onClick handler with try/catch and
logs errors consistent with the FileItem.ts file-action error-handling pattern.
Ensure logging includes the source id/label and the caught error.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/plugins/activity-api.e2e.test.tsx (1)
65-94: Fixed timeouts may cause test flakiness.Lines 85 and 92 use hardcoded
setTimeout(resolve, 100)for cleanup verification. Consider usingvi.waitFor()with a condition or increasing the timeout to reduce flakiness in slower CI environments.♻️ Suggested refactor using vi.waitFor for more robust timing
paramsCallback.mockClear(); config.plugins = []; - // Wait for cleanup to happen - await new Promise((resolve) => setTimeout(resolve, 100)); - paramsCallback.mockClear(); + // Wait for cleanup to happen - use vi.waitFor to allow for timing variations + await vi.waitFor(() => { + // Cleanup should have processed + }, { timeout: 500 }); + paramsCallback.mockClear(); // Changes should not trigger the old subscription const api = getApi(); api.setCurrentActivity('some-activity', { data: 'test' }); - await new Promise((resolve) => setTimeout(resolve, 100)); + // Give enough time for any potential (incorrect) callback invocation + await new Promise((resolve) => setTimeout(resolve, 200)); expect(paramsCallback).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/activity-api.e2e.test.tsx` around lines 65 - 94, Replace the two fixed setTimeout waits used to await plugin cleanup with a deterministic wait using vi.waitFor: after clearing paramsCallback and setting config.plugins = [], wait with vi.waitFor until either paramsCallback has not been called for a short period or until the plugin’s internal subscription list is empty (use available API/getters), then proceed to call getApi().setCurrentActivity and assert paramsCallback was not invoked; target the paramsCallback spy and the createTestPlugin/setup plugin teardown flow so you remove the hardcoded setTimeout(resolve, 100) waits and use vi.waitFor(...) with an explicit condition and a reasonable timeout instead.src/abstract/UploaderPublicApi.ts (2)
357-376: AsyncsetCurrentActivitymay cause subtle issues for callers expecting synchronous behavior.Wrapping the implementation in
setTimeoutmakes this method fire-and-forget with no way for callers to:
- Know when the activity change has completed
- Catch errors (the warning at line 374 won't propagate)
- Guarantee ordering if called multiple times rapidly
The TODO comment acknowledges this needs a better solution. For now, consider returning a
Promiseor documenting the async behavior prominently so callers know to useawaitpatterns (likevi.waitForin tests) when depending on state changes.♻️ Consider returning a Promise for better caller control
public setCurrentActivity = <T extends ActivityType>( activityType: T, ...params: T extends keyof ActivityParamsMap ? [ActivityParamsMap[T]] extends [never] ? [] : [ActivityParamsMap[T]] : [] - ) => { + ): Promise<void> => { + return new Promise((resolve) => { setTimeout(() => { if (hasBlockInCtx(this._sharedInstancesBag.blocksRegistry, (b) => b.activityType === activityType)) { this._ctx.pub('*currentActivityParams', params[0] ?? {}); this._ctx.pub('*currentActivity', activityType); + resolve(); return; } console.warn(`Activity type "${activityType}" not found in the context`); + resolve(); }); + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abstract/UploaderPublicApi.ts` around lines 357 - 376, The setCurrentActivity method currently uses setTimeout which makes it fire-and-forget; change it to return a Promise so callers can await completion and catch errors: inside setCurrentActivity (referencing the function name setCurrentActivity, the helper hasBlockInCtx, the registry this._sharedInstancesBag.blocksRegistry, and publishing via this._ctx.pub) replace the setTimeout pattern with a Promise that waits a tick (e.g., Promise.resolve().then(...) or requestAnimationFrame) then checks hasBlockInCtx and either resolves after calling this._ctx.pub('*currentActivityParams', ...) and this._ctx.pub('*currentActivity', ...) or rejects with an Error (instead of only console.warn) so callers can await and handle ordering/errors.
382-397:setModalStateshould align withsetCurrentActivityasync pattern.If
setCurrentActivityreturns a Promise (as suggested),setModalStateshould also return a Promise for consistency and to allow callers to chain these operations reliably.The warning at line 386 is helpful, but callers have no way to handle it programmatically.
♻️ Consider returning a Promise for consistency
- public setModalState = (opened: boolean): void => { + public setModalState = (opened: boolean): Promise<void> => { + return new Promise((resolve) => { setTimeout(() => { if (opened && !this._ctx.read('*currentActivity')) { console.warn(`Can't open modal without current activity. Please use "setCurrentActivity" method first.`); + resolve(); return; } if (opened) { this._sharedInstancesBag.modalManager?.open(this._ctx.read('*currentActivity')); } else { this._sharedInstancesBag.modalManager?.close(this._ctx.read('*currentActivity')); this._ctx.pub('*currentActivity', null); } + resolve(); }); + }); };tests/plugins/uploader-api.e2e.test.tsx (1)
115-138: Consider adding a test fordoneFlow()to match the test name.The test is named
'should allow calling initFlow() / doneFlow()'but only testsinitFlow(). Either rename the test to reflect actual coverage or add assertions fordoneFlow()behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/uploader-api.e2e.test.tsx` around lines 115 - 138, The test name claims it covers initFlow() and doneFlow() but only calls initFlow(); modify the test to actually exercise doneFlow() by invoking uploaderApi.doneFlow() from the plugin (same setup block where initFlow() is called) or by triggering it from the test after clicking "Start Flow", then add an assertion that the flow is completed/teardown occurs (e.g., check that page.getByTestId('uc-upload-list') is no longer visible or that the flow-specific UI/state is cleared) so the test verifies both initFlow and doneFlow behavior; update symbols referenced: createTestPlugin, setup, uploaderApi.initFlow, uploaderApi.doneFlow, addSource, openModal, and the expectation on page.getByTestId('uc-upload-list').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/abstract/UploaderPublicApi.ts`:
- Around line 357-376: The setCurrentActivity method currently uses setTimeout
which makes it fire-and-forget; change it to return a Promise so callers can
await completion and catch errors: inside setCurrentActivity (referencing the
function name setCurrentActivity, the helper hasBlockInCtx, the registry
this._sharedInstancesBag.blocksRegistry, and publishing via this._ctx.pub)
replace the setTimeout pattern with a Promise that waits a tick (e.g.,
Promise.resolve().then(...) or requestAnimationFrame) then checks hasBlockInCtx
and either resolves after calling this._ctx.pub('*currentActivityParams', ...)
and this._ctx.pub('*currentActivity', ...) or rejects with an Error (instead of
only console.warn) so callers can await and handle ordering/errors.
In `@tests/plugins/activity-api.e2e.test.tsx`:
- Around line 65-94: Replace the two fixed setTimeout waits used to await plugin
cleanup with a deterministic wait using vi.waitFor: after clearing
paramsCallback and setting config.plugins = [], wait with vi.waitFor until
either paramsCallback has not been called for a short period or until the
plugin’s internal subscription list is empty (use available API/getters), then
proceed to call getApi().setCurrentActivity and assert paramsCallback was not
invoked; target the paramsCallback spy and the createTestPlugin/setup plugin
teardown flow so you remove the hardcoded setTimeout(resolve, 100) waits and use
vi.waitFor(...) with an explicit condition and a reasonable timeout instead.
In `@tests/plugins/uploader-api.e2e.test.tsx`:
- Around line 115-138: The test name claims it covers initFlow() and doneFlow()
but only calls initFlow(); modify the test to actually exercise doneFlow() by
invoking uploaderApi.doneFlow() from the plugin (same setup block where
initFlow() is called) or by triggering it from the test after clicking "Start
Flow", then add an assertion that the flow is completed/teardown occurs (e.g.,
check that page.getByTestId('uc-upload-list') is no longer visible or that the
flow-specific UI/state is cleared) so the test verifies both initFlow and
doneFlow behavior; update symbols referenced: createTestPlugin, setup,
uploaderApi.initFlow, uploaderApi.doneFlow, addSource, openModal, and the
expectation on page.getByTestId('uc-upload-list').
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lit/shared-instances.ts`:
- Around line 157-178: The when(...) implementation can hit a TDZ because
ctx.sub invokes the callback synchronously and may call unsub before it's
assigned, and it can miss an instance created between the pre-check (ctx.read)
and the subscription. Fix: in when (function when(...)) declare let unsub: () =>
void = () => {}; then assign unsub = ctx.sub(stateKey, (instance) => { if
(instance) { callback(instance as NonNullable<InstanceTypeMap[TName]>); unsub();
} }); after subscribing, immediately re-read via ctx.read(stateKey) (or ctx.has
then ctx.read) and if an instance exists call callback and then unsub(); keep
using getCtx(), instanceKeyMap, SharedInstancesState and the same callback
typing. This guarantees unsub is assigned before any synchronous callback runs
and avoids the race by re-checking after subscribing.
---
Duplicate comments:
In `@src/blocks/SourceList/SourceList.ts`:
- Around line 185-191: The onClick currently calls source.onSelect() directly in
_makePluginSourceConfig, allowing sync throws or promise rejections to bubble;
change onClick to call a guarded handler that wraps the call to
source.onSelect() (from PluginSourceRegistration) in try/catch and handles
returned promises with Promise.resolve(...).catch(...) so both sync errors and
async rejections are caught and handled (e.g., log or noop) before returning
from onClick in the SourceButtonConfig.
| when<TName extends InstanceName>( | ||
| name: TName, | ||
| callback: (instance: NonNullable<InstanceTypeMap[TName]>) => void, | ||
| ): () => void { | ||
| const stateKey = instanceKeyMap[name] as keyof SharedInstancesState; | ||
| const ctx = getCtx(); | ||
|
|
||
| const existingInstance = ctx.has(stateKey) ? ctx.read(stateKey) : undefined; | ||
| if (existingInstance) { | ||
| callback(existingInstance as NonNullable<InstanceTypeMap[TName]>); | ||
| return () => {}; | ||
| } | ||
|
|
||
| const unsub = ctx.sub(stateKey, (instance) => { | ||
| if (instance) { | ||
| callback(instance as NonNullable<InstanceTypeMap[TName]>); | ||
| unsub(); | ||
| } | ||
| }); | ||
|
|
||
| return unsub; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'PubSubCompat.ts' -o 'PubSub*.ts' | head -20Repository: uploadcare/file-uploader
Length of output: 207
🏁 Script executed:
fd -t f -e ts | rg "ctx\." | head -30Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
fd -t f 'shared-instances.ts' -x cat -nRepository: uploadcare/file-uploader
Length of output: 7633
🏁 Script executed:
fd -t f 'PubSubCompat.ts'Repository: uploadcare/file-uploader
Length of output: 91
🏁 Script executed:
fd -t f -e ts -path '*/PubSub*'Repository: uploadcare/file-uploader
Length of output: 236
🏁 Script executed:
cat -n src/lit/PubSubCompat.tsRepository: uploadcare/file-uploader
Length of output: 3262
🏁 Script executed:
rg -n "subscribeKeys|listenKeys" src/Repository: uploadcare/file-uploader
Length of output: 309
🏁 Script executed:
rg -n "ctx.sub" src/lit/shared-instances.tsRepository: uploadcare/file-uploader
Length of output: 125
🌐 Web query:
nanostores subscribeKeys synchronous callback behavior
💡 Result:
In Nano Stores, subscribeKeys(store, keys, cb) behaves like store.subscribe(cb) (vs listenKeys / store.listen):
- It calls your callback immediately during subscription (i.e., “eager”/synchronous initial call), not only on the next change. [1]
- On that initial call, both
oldValueandchangedareundefined. [1] - After that, it will call
cb(value, oldValue, changedKey)only when one of the specified keys changes. [1]
Sources
[1] Nano Stores README (maps / listenKeys + subscribeKeys behavior) (github.com)
Citations:
Guard against TDZ and race condition in when(...) subscriptions.
The ctx.sub() method uses nanostores' subscribeKeys, which invokes callbacks synchronously during subscription. If the instance is already available at subscription time, the callback immediately calls unsub() before the variable assignment completes, causing a TDZ error. Additionally, if the instance becomes available between the pre-check (line 164) and subscription (line 170), the callback is missed.
✅ Safer subscription pattern
when<TName extends InstanceName>(
name: TName,
callback: (instance: NonNullable<InstanceTypeMap[TName]>) => void,
): () => void {
const stateKey = instanceKeyMap[name] as keyof SharedInstancesState;
const ctx = getCtx();
- const existingInstance = ctx.has(stateKey) ? ctx.read(stateKey) : undefined;
- if (existingInstance) {
- callback(existingInstance as NonNullable<InstanceTypeMap[TName]>);
- return () => {};
- }
-
- const unsub = ctx.sub(stateKey, (instance) => {
- if (instance) {
- callback(instance as NonNullable<InstanceTypeMap[TName]>);
- unsub();
- }
- });
+ let resolved = false;
+ let unsub: () => void = () => {};
+ const resolve = (instance: InstanceTypeMap[TName] | undefined) => {
+ if (!resolved && instance) {
+ resolved = true;
+ callback(instance as NonNullable<InstanceTypeMap[TName]>);
+ unsub();
+ }
+ };
+
+ unsub = ctx.sub(stateKey, (instance) => {
+ resolve(instance as InstanceTypeMap[TName]);
+ });
+
+ // Re-check after subscribing to close the race window.
+ resolve(ctx.has(stateKey) ? (ctx.read(stateKey) as InstanceTypeMap[TName]) : undefined);
return unsub;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| when<TName extends InstanceName>( | |
| name: TName, | |
| callback: (instance: NonNullable<InstanceTypeMap[TName]>) => void, | |
| ): () => void { | |
| const stateKey = instanceKeyMap[name] as keyof SharedInstancesState; | |
| const ctx = getCtx(); | |
| const existingInstance = ctx.has(stateKey) ? ctx.read(stateKey) : undefined; | |
| if (existingInstance) { | |
| callback(existingInstance as NonNullable<InstanceTypeMap[TName]>); | |
| return () => {}; | |
| } | |
| const unsub = ctx.sub(stateKey, (instance) => { | |
| if (instance) { | |
| callback(instance as NonNullable<InstanceTypeMap[TName]>); | |
| unsub(); | |
| } | |
| }); | |
| return unsub; | |
| }, | |
| when<TName extends InstanceName>( | |
| name: TName, | |
| callback: (instance: NonNullable<InstanceTypeMap[TName]>) => void, | |
| ): () => void { | |
| const stateKey = instanceKeyMap[name] as keyof SharedInstancesState; | |
| const ctx = getCtx(); | |
| let resolved = false; | |
| let unsub: () => void = () => {}; | |
| const resolve = (instance: InstanceTypeMap[TName] | undefined) => { | |
| if (!resolved && instance) { | |
| resolved = true; | |
| callback(instance as NonNullable<InstanceTypeMap[TName]>); | |
| unsub(); | |
| } | |
| }; | |
| unsub = ctx.sub(stateKey, (instance) => { | |
| resolve(instance as InstanceTypeMap[TName]); | |
| }); | |
| // Re-check after subscribing to close the race window. | |
| resolve(ctx.has(stateKey) ? (ctx.read(stateKey) as InstanceTypeMap[TName]) : undefined); | |
| return unsub; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lit/shared-instances.ts` around lines 157 - 178, The when(...)
implementation can hit a TDZ because ctx.sub invokes the callback synchronously
and may call unsub before it's assigned, and it can miss an instance created
between the pre-check (ctx.read) and the subscription. Fix: in when (function
when(...)) declare let unsub: () => void = () => {}; then assign unsub =
ctx.sub(stateKey, (instance) => { if (instance) { callback(instance as
NonNullable<InstanceTypeMap[TName]>); unsub(); } }); after subscribing,
immediately re-read via ctx.read(stateKey) (or ctx.has then ctx.read) and if an
instance exists call callback and then unsub(); keep using getCtx(),
instanceKeyMap, SharedInstancesState and the same callback typing. This
guarantees unsub is assigned before any synchronous callback runs and avoids the
race by re-checking after subscribing.
nd0ut
force-pushed
the
chore/plugin-system
branch
from
20a47dc to
25fef8b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { page } from 'vitest/browser'; | ||
| import type { Config, UploadCtxProvider, UploaderPlugin } from '@/index.ts'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { cleanup, getCtxName, renderer } from '../utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
In general, to fix an unused import, either remove it or start using it meaningfully. To avoid changing behavior, we should prefer the minimal change: delete only the unused symbol from the import specifier while leaving the others intact.
Concretely, in tests/plugins/utils.tsx on line 6 we should stop importing renderer, but keep cleanup and getCtxName since they are used. No other parts of the file reference renderer, so no further edits are needed. We do not need any new methods, imports, or definitions; we’re only trimming the import list.
| @@ -3,7 +3,7 @@ | ||
| import type { Config, UploadCtxProvider, UploaderPlugin } from '@/index.ts'; | ||
| import { delay } from '@/utils/delay'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { cleanup, getCtxName, renderer } from '../utils/test-renderer'; | ||
| import { cleanup, getCtxName } from '../utils/test-renderer'; | ||
|
|
||
| export { cleanup }; | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/build.ts (2)
56-59: Settingoptions.splittinghere is redundant.The
splittingoption is already set at the tsup level on line 65, which tsup passes through to esbuild. Theoptions.supportedassignment for dynamic imports is valuable and should remain.♻️ Proposed simplification
if (buildItem.codeSplitting) { - options.splitting = true; options.supported = { 'dynamic-import': true }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.ts` around lines 56 - 59, Inside the buildItem.codeSplitting conditional remove the redundant assignment to options.splitting (tsup already sets splitting globally) but keep the options.supported = { 'dynamic-import': true } line; locate the conditional using the buildItem.codeSplitting check and the options variable to make this change so only the dynamic-import support is configured in that block.
56-65: Consider validating thatcodeSplittingis only used with ESM format.esbuild's code splitting feature only works with
format: 'esm'. If someone inadvertently enablescodeSplitting: truewithformat: 'iife', the build will fail with a less intuitive esbuild error. Adding an early validation would provide clearer feedback.🛡️ Proposed validation
Add this check at the start of the
buildfunction:async function build(buildItem: BuildItem) { if (buildItem.codeSplitting && buildItem.format !== 'esm') { throw new Error(`Code splitting requires ESM format, but '${buildItem.format}' was specified.`); } await tsupBuild({ // ... }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.ts` around lines 56 - 65, Add a guard at the start of the build function to validate that buildItem.codeSplitting is only enabled for ESM: check if buildItem.codeSplitting && buildItem.format !== 'esm' and throw a clear Error (e.g. "Code splitting requires ESM format, but '<format>' was specified.") before calling tsupBuild; reference the build function, the buildItem object, and the tsupBuild invocation so the validation runs early and prevents esbuild from failing with an obscure error.src/blocks/Config/computed-properties.ts (1)
148-152: Consider usingas conston the COMPUTED_PROPERTIES array to preserve literal types.The
@ts-expect-errorhides a real typing issue:argsis built asPartial<Record<ConfigKey, ConfigType[ConfigKey]>>, butcomputed.fnexpects the precise shape ofComputedPropertyArgs<TKey, TDeps>. Without type narrowing on the array, TypeScript widens each element's type and can't guarantee the shape match. Addingas constto the COMPUTED_PROPERTIES array would give TypeScript better information about each element's literal structure, potentially eliminating the need for suppression—though the dynamic loop construction may still require careful handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/Config/computed-properties.ts` around lines 148 - 152, The `@ts-expect-error` around the computed.fn call is hiding a typing mismatch because COMPUTED_PROPERTIES is being widened; change the COMPUTED_PROPERTIES declaration to use "as const" so each entry preserves literal types and TypeScript can narrow the element type, then remove the suppression and adjust the call site (where computed.fn is invoked with args and abortController.signal) to match the precise ComputedPropertyArgs<TKey, TDeps> shape (narrow or map args from Partial<Record<...>> into the expected exact shape) so the call to computed.fn no longer needs the `@ts-expect-error`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 49-54: The Node export for "./core" points to
"./dist/index.ssr.js" which doesn't export the core barrel, so either generate a
SSR build for core (emit "./dist/core.ssr.js" from src/core.ts and update the
"./core" node field to "./dist/core.ssr.js") or re-export core from src/index.ts
(add "export * from './core'" in src/index.ts) and rebuild so dist/index.ssr.js
includes core exports; update package.json "./core" node entry accordingly to
point to the artifact that actually exposes the core barrel.
In `@src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts`:
- Around line 65-67: initCallback currently calls _mountEditor() unconditionally
which can run before activityParams (via the activityParams getter) contains
internalId; change initCallback to defer mounting until params are ready by
either registering an onActivate callback (mirror the ExternalSource pattern)
and calling _mountEditor() there, or add a guard in initCallback that checks
activityParams/internalId is present before invoking _mountEditor(); ensure you
reference the same _mountEditor and activityParams getter so the mount only
occurs after activation or when internalId is available.
---
Nitpick comments:
In `@scripts/build.ts`:
- Around line 56-59: Inside the buildItem.codeSplitting conditional remove the
redundant assignment to options.splitting (tsup already sets splitting globally)
but keep the options.supported = { 'dynamic-import': true } line; locate the
conditional using the buildItem.codeSplitting check and the options variable to
make this change so only the dynamic-import support is configured in that block.
- Around line 56-65: Add a guard at the start of the build function to validate
that buildItem.codeSplitting is only enabled for ESM: check if
buildItem.codeSplitting && buildItem.format !== 'esm' and throw a clear Error
(e.g. "Code splitting requires ESM format, but '<format>' was specified.")
before calling tsupBuild; reference the build function, the buildItem object,
and the tsupBuild invocation so the validation runs early and prevents esbuild
from failing with an obscure error.
In `@src/blocks/Config/computed-properties.ts`:
- Around line 148-152: The `@ts-expect-error` around the computed.fn call is
hiding a typing mismatch because COMPUTED_PROPERTIES is being widened; change
the COMPUTED_PROPERTIES declaration to use "as const" so each entry preserves
literal types and TypeScript can narrow the element type, then remove the
suppression and adjust the call site (where computed.fn is invoked with args and
abortController.signal) to match the precise ComputedPropertyArgs<TKey, TDeps>
shape (narrow or map args from Partial<Record<...>> into the expected exact
shape) so the call to computed.fn no longer needs the `@ts-expect-error`.
package.json
Outdated
| "./core": { | ||
| "types": "./dist/core.d.ts", | ||
| "node": "./dist/index.ssr.js", | ||
| "browser": "./dist/core.js", | ||
| "default": "./dist/core.js" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find build/config files
fd -type f \( -name "tsup.config.*" -o -name "vite.config.*" -o -name "rollup.config.*" -o -name "webpack.config.*" \) --max-depth 2Repository: uploadcare/file-uploader
Length of output: 238
🏁 Script executed:
# Search for references to core exports and SSR bundles in build scripts
rg -n "core|ssr" -g "*.ts" -g "*.js" -g "*.mjs" -g "*.cjs" scripts/ build/ 2>/dev/null || trueRepository: uploadcare/file-uploader
Length of output: 395
🏁 Script executed:
# Look for build config in the root and scripts directory
ls -la | grep -E "^-.*\.(config|build)"
cat package.json | head -40Repository: uploadcare/file-uploader
Length of output: 1144
🏁 Script executed:
# Search for references to dist/core.ssr or index.ssr in the codebase
rg -n "(core\.ssr|index\.ssr)" --type ts --type jsRepository: uploadcare/file-uploader
Length of output: 239
🏁 Script executed:
# Check what exports are defined in src/core.ts
fd -name "core.ts" src/Repository: uploadcare/file-uploader
Length of output: 298
🏁 Script executed:
# Read build-ssr-stubs.ts to see what's included in index.ssr.js
cat scripts/build-ssr-stubs.tsRepository: uploadcare/file-uploader
Length of output: 3382
🏁 Script executed:
# Read vite.config.ts to understand the build configuration
cat vite.config.tsRepository: uploadcare/file-uploader
Length of output: 559
🏁 Script executed:
# Read build-items.ts to understand build setup
head -50 scripts/build-items.tsRepository: uploadcare/file-uploader
Length of output: 1414
🏁 Script executed:
# Check src/core.ts structure - what does it export?
head -50 src/core.tsRepository: uploadcare/file-uploader
Length of output: 2517
🏁 Script executed:
# Search for any core.ssr references in build scripts
rg "core\.ssr" . --max-count 10Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Check what src/index.ts exports to understand what goes into dist/index.js
head -100 src/index.tsRepository: uploadcare/file-uploader
Length of output: 3161
🏁 Script executed:
# Look for any re-exports of core in index.ts
rg "export.*core|from.*core" src/index.tsRepository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Check what src/core.ts exports
cat src/core.tsRepository: uploadcare/file-uploader
Length of output: 2881
🏁 Script executed:
# Search for test files that import from core to see expected usage
rg "from.*@uploadcare/file-uploader/core|import.*core" --type ts --type js -g "*.test.*" -g "*.spec.*"Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Check if core is used/imported anywhere in the codebase
rg "import.*from.*['\"]\.+/core['\"]|from.*['\"]@uploadcare/file-uploader/core" -g "*.ts" -g "*.js" | head -20Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Get src/core.ts - let's be explicit about the path
ls -la src/ | grep coreRepository: uploadcare/file-uploader
Length of output: 125
🏁 Script executed:
# Read the actual core file
head -100 src/core.tsRepository: uploadcare/file-uploader
Length of output: 2881
🏁 Script executed:
# Look for dist output configuration - does any build step generate core.ssr.js?
rg -A 5 -B 5 "core\.ssr" scripts/Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Read src/core.ts using sed or awk
sed -n '1,100p' src/core.tsRepository: uploadcare/file-uploader
Length of output: 2881
🏁 Script executed:
# Check build scripts for any tsup/build configuration that might generate core.ssr.js
cat scripts/build-items.ts | grep -A 10 "core"Repository: uploadcare/file-uploader
Length of output: 309
🏁 Script executed:
# Look for the main build script that orchestrates everything
fd -name "build.ts" scripts/Repository: uploadcare/file-uploader
Length of output: 298
🏁 Script executed:
# Check if there's a tsup config that might handle SSR output
fd -name "tsup.config*" .Repository: uploadcare/file-uploader
Length of output: 298
🏁 Script executed:
# Try reading core.ts with cat directly
cat src/core.tsRepository: uploadcare/file-uploader
Length of output: 2881
🏁 Script executed:
# Find the main build file
find scripts -name "*.ts" -type f | head -20Repository: uploadcare/file-uploader
Length of output: 213
🏁 Script executed:
# Look at package.json build scripts to understand the build process
rg '"build' package.json -A 1Repository: uploadcare/file-uploader
Length of output: 575
🏁 Script executed:
# Try using python to read src/core.ts
python3 << 'EOF'
with open('src/core.ts', 'r') as f:
content = f.read()
print(content)
EOFRepository: uploadcare/file-uploader
Length of output: 2882
🏁 Script executed:
# Look at build.ts to understand the full build process
cat scripts/build.tsRepository: uploadcare/file-uploader
Length of output: 4037
🏁 Script executed:
# Try reading with head to see first few lines only
head -20 src/core.tsRepository: uploadcare/file-uploader
Length of output: 975
🏁 Script executed:
# Let's check the complete build-items output to see all entries
cat scripts/build-items.tsRepository: uploadcare/file-uploader
Length of output: 3214
Fix node condition for ./core export to expose core barrel on Node.
"node": "./dist/index.ssr.js" does not contain core exports. Since dist/index.ssr.js is generated from dist/index.js (which does not re-export core), and no separate dist/core.ssr.js exists, Node consumers importing @uploadcare/file-uploader/core will not receive the expected core barrel.
Either create a dist/core.ssr.js build output from src/core.ts (similar to how dist/core.js is already generated), or re-export core from src/index.ts and ensure dist/index.ssr.js includes those exports.
Suggested fix (if core SSR bundle is generated)
"./core": {
"types": "./dist/core.d.ts",
- "node": "./dist/index.ssr.js",
+ "node": "./dist/core.ssr.js",
"browser": "./dist/core.js",
"default": "./dist/core.js"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 49 - 54, The Node export for "./core" points to
"./dist/index.ssr.js" which doesn't export the core barrel, so either generate a
SSR build for core (emit "./dist/core.ssr.js" from src/core.ts and update the
"./core" node field to "./dist/core.ssr.js") or re-export core from src/index.ts
(add "export * from './core'" in src/index.ts) and rebuild so dist/index.ssr.js
includes core exports; update package.json "./core" node entry accordingly to
point to the artifact that actually exposes the core barrel.
|
|
||
| this._mountEditor(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the CloudImageEditorActivity file around the mentioned lines
cat -n src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts | head -100Repository: uploadcare/file-uploader
Length of output: 3661
🏁 Script executed:
# Trace when activity params are set relative to initCallback()
rg -n "initCallback|activityParams|setCurrentActivity" src -g '*.ts' -A 2 -B 2Repository: uploadcare/file-uploader
Length of output: 23853
🏁 Script executed:
# Check PluginActivityRenderer, ActivityHost, registerActivity
rg -n "PluginActivityRenderer|ActivityHost|registerActivity" src/blocks -g '*.ts' -A 3Repository: uploadcare/file-uploader
Length of output: 3020
🏁 Script executed:
# Look for _mountEditor implementation
rg -n "_mountEditor" src -g '*.ts' -A 5Repository: uploadcare/file-uploader
Length of output: 1263
🏁 Script executed:
# Find where *currentActivityParams is set/assigned
rg -n "currentActivityParams" src -g '*.ts' -B 2 -A 2Repository: uploadcare/file-uploader
Length of output: 4704
🏁 Script executed:
# Check the initialization sequence in SymbioteCompatMixin
cat -n src/lit/SymbioteCompatMixin.ts | sed -n '300,400p'Repository: uploadcare/file-uploader
Length of output: 3883
🏁 Script executed:
# See full _mountEditor implementation and any guards around it
cat -n src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts | sed -n '99,125p'Repository: uploadcare/file-uploader
Length of output: 1017
🏁 Script executed:
# Check if there's a pattern in other activity blocks for accessing activityParams
rg -n "this.activityParams" src/blocks -g '*.ts' -B 3 -A 3 | head -60Repository: uploadcare/file-uploader
Length of output: 2703
Gate _mountEditor() until activity params are available.
_mountEditor() is called unconditionally in initCallback() but runs before *currentActivityParams is guaranteed to contain internalId. The default state is an empty object {}, causing activityParams getter to throw "Cloud Image Editor activity params not found".
Instead of calling _mountEditor() directly, defer mounting until the activity is actually activated by registering an onActivate callback (similar to ExternalSource pattern), or guard the call with a check that params are ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts` around lines
65 - 67, initCallback currently calls _mountEditor() unconditionally which can
run before activityParams (via the activityParams getter) contains internalId;
change initCallback to defer mounting until params are ready by either
registering an onActivate callback (mirror the ExternalSource pattern) and
calling _mountEditor() there, or add a guard in initCallback that checks
activityParams/internalId is present before invoking _mountEditor(); ensure you
reference the same _mountEditor and activityParams getter so the mount only
occurs after activation or when internalId is available.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
scripts/build.ts (1)
56-59: Merge pre-existingoptions.supportedvalues to avoid overwriting.Direct assignment to
options.supportedreplaces any previously set values. This is safe to merge using the spread operator to preserve any feature overrides set by tsup or other configuration.♻️ Proposed change
if (buildItem.codeSplitting) { options.splitting = true; - options.supported = { 'dynamic-import': true }; + options.supported = { ...(options.supported ?? {}), 'dynamic-import': true }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.ts` around lines 56 - 59, The code sets options.supported directly when buildItem.codeSplitting is true, overwriting any existing feature flags; change the assignment to merge into the existing options.supported instead (e.g., set options.splitting = true and then options.supported = { ...(options.supported || {}), 'dynamic-import': true }) so pre-existing overrides from tsup or other config are preserved; update the block around buildItem.codeSplitting, options.splitting, and options.supported accordingly.src/blocks/Config/computed-properties.ts (1)
148-153: Prefer removing the@ts-expect-erroronce the generics are stabilized.The suppression can hide real mismatches as new computed properties are added. If possible, tighten the generic typing around
ComputedPropertyArgssocomputed.fncan be invoked without suppression, then re-run typecheck to confirm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/Config/computed-properties.ts` around lines 148 - 153, Remove the `@ts-expect-error` by tightening the generics so computed.fn can be called with a properly-typed args object: update the ComputedPropertyArgs generic definition to correctly map from the computed.key and computed.deps types, ensure the call site uses ComputedPropertyArgs<typeof computed.key, typeof computed.deps> (or a narrowed alias) and that the return type of computed.fn aligns with ConfigValue<typeof computed.key> | Promise<ConfigValue<typeof computed.key>>; then remove the suppression from the invocation (the block around result, computed.fn, and abortController.signal) and run the typechecker to fix any remaining mismatches.src/blocks/SourceBtn/SourceBtn.ts (1)
8-45: Clarify whetherlabelis an i18n key or literal text.
labelis passed throughl10n(...), so a literal label like “Google Drive” will be treated as a translation key. Consider renaming tolabelKeyor bypassingl10nwhen the label is already localized to avoid confusing output.Also applies to: 54-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/SourceBtn/SourceBtn.ts` around lines 8 - 45, The SourceButtonConfig.label is ambiguous (treated as an i18n key by l10n); update the API and usage to make intent explicit: add a new property name labelKey (string) for translation keys and optionally localizedLabel (string) for pre-localized literal text, update the SourceButtonConfig type and the _applySource(source?: SourceButtonConfig) method to prefer localizedLabel if present, otherwise call l10n on labelKey (fall back to existing label for backward compatibility), and rename references where l10n(...) is invoked so the code uses labelKey/localizedLabel consistently (also apply the same change to the similar usage around the other block mentioned).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/abstract/managers/plugin/PluginManager.ts`:
- Around line 73-157: The setup flow in _registerPlugin can leak active
subscriptions when plugin.setup(...) throws because configSubscriptions are only
cleaned up when the plugin is later unregistered; fix by wrapping the await
plugin.setup(...) call in try/catch and on any exception iterate and call each
unsubscribe in configSubscriptions (and optionally clear registrations) before
rethrowing the error, ensuring the plugin is not left partially registered in
_plugins; reference the symbols _registerPlugin, configSubscriptions,
plugin.setup, _plugins, and _unregisterPlugin to locate and implement this
cleanup.
In `@src/blocks/Config/Config.ts`:
- Around line 107-129: _flushValueToAttribute currently always calls
value.toString(); update it to check for a custom config definition via
this._getCustomConfigDefinition(key) (you already call this in the earlier
check) and, if config?.toAttribute is a function, call config.toAttribute(value)
to produce the serialized attribute value; fall back to value.toString() only
when no toAttribute exists. Preserve the existing removal logic for
undefined/null and only call setAttribute when the serialized string differs
from this.getAttribute(attr). Use the same attribute list from
this._getAttributeNames(key) and keep checks for complex keys and attribute:
false as-is.
In `@src/blocks/FileItem/FileItem.ts`:
- Around line 278-301: The call to this.api.getOutputItem(this.uid) can throw
when the entry was removed, so wrap that call in a try/catch (inside
_updatePluginFileActions) and if it throws set this._pluginFileActions = [] and
return; then proceed to filter pluginManager.snapshot().fileActions only when a
valid outputFileEntry is obtained; reference the _updatePluginFileActions
method, this.api.getOutputItem / UploaderPublicApi.getOutputItem,
pluginManager.snapshot().fileActions, and this._pluginFileActions.
In `@src/blocks/SourceBtn/SourceBtn.ts`:
- Around line 47-50: The activate method currently calls this.source.onClick()
directly which can produce unhandled promise rejections if onClick is async;
change it to invoke the handler via
Promise.resolve(this.source.onClick()).catch(...) to observe failures and avoid
unhandled rejections, and inside the catch log or surface the error (e.g., call
an existing logger or a local error handler) so failures are visible without
crashing; update SourceBtn.activate to use this pattern when calling
this.source.onClick.
In `@src/lit/LitActivityBlock.ts`:
- Around line 71-72: In the error fallback inside LitActivityBlock, guard access
to this.$['*history'] before indexing it: ensure this.$['*history'] is an array
with length > 0 (or use optional chaining) when computing nextActivity so the
lookup doesn't throw; if history is missing or empty set
this.$['*currentActivity'] to null. Update the assignment that computes
nextActivity and the subsequent this.$['*currentActivity'] = nextActivity ??
null to first check existence/length of this.$['*history'] (or safely use
this.$['*history']?.[this.$['*history'].length - 1]) so the fallback no longer
throws on undefined/empty history.
In `@tests/plugins/custom-config.e2e.test.tsx`:
- Around line 122-153: The test unregisters the plugin but never mutates the
config afterwards, so the "not called" assertion is vacuous; after clearing the
mock and setting config.plugins = [] you should explicitly change the registered
config value 'cleanupOption' (e.g., via the uploader config API returned from
renderUploader or the appropriate config.set/update method) to a new value and
then assert that callback (the function passed to pluginApi.config.subscribe) is
not invoked; locate the subscription created in the plugin setup
(pluginApi.config.subscribe) and the config object returned by renderUploader to
perform the post-unregister change and the subsequent
expect(callback).not.toHaveBeenCalled() check.
In `@tests/plugins/utils.tsx`:
- Around line 1-6: The import statement includes an unused symbol "renderer" in
tests/plugins/utils.tsx; remove "renderer" from the named imports (keep cleanup,
getCtxName) in the import line or, if it was intended to be used, add the actual
usage where appropriate—locate the import that currently reads "import {
cleanup, getCtxName, renderer } from '../utils/test-renderer';" and delete
"renderer" to eliminate the unused import.
---
Duplicate comments:
In `@src/abstract/managers/LocaleManager.ts`:
- Around line 20-27: When pluginManager.onPluginsChange triggers and you call
this._applyPluginLocales(this._localeName), you must reapply any user-provided
localeDefinitionOverride for the current locale so plugin updates don't clobber
user overrides; after the this._applyPluginLocales(this._localeName) call, check
this.localeDefinitionOverride (or the manager's override storage) for an entry
for this._localeName and reapply it by invoking the existing
override-application logic (e.g., call the method that merges/apply locale
overrides or merge the override entries into the locale definitions) so user
overrides are applied last.
In `@src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts`:
- Around line 37-67: The initCallback currently calls _mountEditor()
unconditionally which can throw if activityParams aren't populated; update
initCallback to guard or defer mounting: check whether
super.activityParams?.internalId (or the minimal ready flag used in this
codebase) is present before calling this._mountEditor(), and if not, attach a
one-time listener or subscription to the activity/activation event that runs
this._mountEditor() once activityParams become available (or move the
_mountEditor() call into the activation handler). Modify initCallback to use the
guard (e.g., if (!super.activityParams?.internalId) return/subscribe; else
this._mountEditor()) so _mountEditor() only runs when activityParams are ready.
In `@src/blocks/FileItem/FileItem.ts`:
- Around line 303-318: The _handlePluginFileAction method currently calls
PluginFileActionRegistration.onClick inside a try/catch but doesn’t handle async
rejections; update the implementation to handle Promise results from
action.onClick by either making _handlePluginFileAction async and awaiting
action.onClick(outputFileEntry) inside try/catch, or by calling
Promise.resolve(action.onClick(outputFileEntry)).catch(...) to log errors; keep
the existing checks for this.uid and outputFileEntry and ensure errors are
logged with the same message format including action.id.
In `@src/blocks/Icon/Icon.ts`:
- Around line 65-69: The render method in Icon (render) uses
unsafeSVG(this._pluginSvg) which renders untrusted plugin SVGs and risks XSS;
modify flow so plugin SVGs are sanitized at registration or explicitly marked
trusted before calling unsafeSVG: add/consume a sanitization step where plugin
icons are registered (e.g., in any registerIcon/PluginIconRegistration path) to
run an SVG sanitizer and store the sanitized result or a trusted flag on the
icon descriptor, then update Icon.render to only call unsafeSVG(this._pluginSvg)
when the descriptor is marked trusted and otherwise render a sanitized-safe DOM
(or refuse to render); ensure references: Icon.render, this._pluginSvg,
unsafeSVG, and plugin registration routines are updated accordingly and
add/document the trust requirement where icons are registered.
In `@src/blocks/SourceList/SourceList.ts`:
- Around line 185-191: _wrap the plugin onSelect call in _makePluginSourceConfig
so it cannot throw to the window: change the onClick to call source.onSelect via
a guarded handler that catches sync exceptions and handles rejected promises
(e.g., use Promise.resolve(source.onSelect())...catch(...) or an async
try/catch) and surface or log the error instead of letting it propagate. Update
references in _makePluginSourceConfig and ensure
PluginSourceRegistration.onSelect invocations are guarded; keep the returned
shape as SourceButtonConfig with onClick remaining a function that swallows or
logs errors rather than allowing them to bubble.
In `@src/lit/shared-instances.ts`:
- Around line 158-176: The current when() can call unsub before unsub is
assigned if ctx.sub invokes the callback synchronously; fix by introducing a
noop/flag guard: declare let unsub: () => void = () => {}; let called = false;
then call unsub = ctx.sub(stateKey, (instance) => { if (instance) { called =
true; callback(instance as NonNullable<InstanceTypeMap[TName]>); unsub(); } });
and after assignment, if (called) unsub();—this ensures synchronous callbacks
don't run an uninitialized unsub and still unsubscribe correctly; update the
when function (references: when, stateKey, ctx.sub, unsub) accordingly.
---
Nitpick comments:
In `@scripts/build.ts`:
- Around line 56-59: The code sets options.supported directly when
buildItem.codeSplitting is true, overwriting any existing feature flags; change
the assignment to merge into the existing options.supported instead (e.g., set
options.splitting = true and then options.supported = { ...(options.supported ||
{}), 'dynamic-import': true }) so pre-existing overrides from tsup or other
config are preserved; update the block around buildItem.codeSplitting,
options.splitting, and options.supported accordingly.
In `@src/blocks/Config/computed-properties.ts`:
- Around line 148-153: Remove the `@ts-expect-error` by tightening the generics so
computed.fn can be called with a properly-typed args object: update the
ComputedPropertyArgs generic definition to correctly map from the computed.key
and computed.deps types, ensure the call site uses ComputedPropertyArgs<typeof
computed.key, typeof computed.deps> (or a narrowed alias) and that the return
type of computed.fn aligns with ConfigValue<typeof computed.key> |
Promise<ConfigValue<typeof computed.key>>; then remove the suppression from the
invocation (the block around result, computed.fn, and abortController.signal)
and run the typechecker to fix any remaining mismatches.
In `@src/blocks/SourceBtn/SourceBtn.ts`:
- Around line 8-45: The SourceButtonConfig.label is ambiguous (treated as an
i18n key by l10n); update the API and usage to make intent explicit: add a new
property name labelKey (string) for translation keys and optionally
localizedLabel (string) for pre-localized literal text, update the
SourceButtonConfig type and the _applySource(source?: SourceButtonConfig) method
to prefer localizedLabel if present, otherwise call l10n on labelKey (fall back
to existing label for backward compatibility), and rename references where
l10n(...) is invoked so the code uses labelKey/localizedLabel consistently (also
apply the same change to the similar usage around the other block mentioned).
| private _flushValueToAttribute(key: string, value: unknown) { | ||
| // Check if it's a complex built-in key | ||
| if (isComplexKey(key as keyof ConfigType)) { | ||
| return; // Complex keys can't be represented as attributes | ||
| } | ||
|
|
||
| // Check if it's a custom config with attribute: false | ||
| if (this._isCustomConfig(key)) { | ||
| const config = this._getCustomConfigDefinition(key); | ||
| // Skip if attribute is explicitly false (default is true, so flush unless false) | ||
| if (config?.attribute === false) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Flush the value to the DOM attributes (works for both built-in and custom configs) | ||
| const attrs = this._getAttributeNames(key); | ||
| for (const attr of attrs) { | ||
| if (typeof value === 'undefined' || value === null) { | ||
| this.removeAttribute(attr); | ||
| } else if (this.getAttribute(attr) !== value.toString()) { | ||
| this.setAttribute(attr, value.toString()); | ||
| } |
There was a problem hiding this comment.
Custom toAttribute() is ignored when syncing to DOM attributes.
CustomConfigDefinition.toAttribute() is defined but _flushValueToAttribute always uses value.toString(). Custom configs needing custom serialization won’t work.
🛠️ Suggested fix
- if (this._isCustomConfig(key)) {
- const config = this._getCustomConfigDefinition(key);
- // Skip if attribute is explicitly false (default is true, so flush unless false)
- if (config?.attribute === false) {
- return;
- }
- }
+ let customConfig: ReturnType<Config['_getCustomConfigDefinition']> | undefined;
+ if (this._isCustomConfig(key)) {
+ customConfig = this._getCustomConfigDefinition(key);
+ // Skip if attribute is explicitly false (default is true, so flush unless false)
+ if (customConfig?.attribute === false) {
+ return;
+ }
+ }
// Flush the value to the DOM attributes (works for both built-in and custom configs)
const attrs = this._getAttributeNames(key);
+ const attrValue =
+ value === undefined || value === null
+ ? null
+ : customConfig?.toAttribute
+ ? customConfig.toAttribute(value)
+ : value.toString();
for (const attr of attrs) {
- if (typeof value === 'undefined' || value === null) {
+ if (attrValue === null || attrValue === undefined) {
this.removeAttribute(attr);
- } else if (this.getAttribute(attr) !== value.toString()) {
- this.setAttribute(attr, value.toString());
+ } else if (this.getAttribute(attr) !== attrValue) {
+ this.setAttribute(attr, attrValue);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _flushValueToAttribute(key: string, value: unknown) { | |
| // Check if it's a complex built-in key | |
| if (isComplexKey(key as keyof ConfigType)) { | |
| return; // Complex keys can't be represented as attributes | |
| } | |
| // Check if it's a custom config with attribute: false | |
| if (this._isCustomConfig(key)) { | |
| const config = this._getCustomConfigDefinition(key); | |
| // Skip if attribute is explicitly false (default is true, so flush unless false) | |
| if (config?.attribute === false) { | |
| return; | |
| } | |
| } | |
| // Flush the value to the DOM attributes (works for both built-in and custom configs) | |
| const attrs = this._getAttributeNames(key); | |
| for (const attr of attrs) { | |
| if (typeof value === 'undefined' || value === null) { | |
| this.removeAttribute(attr); | |
| } else if (this.getAttribute(attr) !== value.toString()) { | |
| this.setAttribute(attr, value.toString()); | |
| } | |
| private _flushValueToAttribute(key: string, value: unknown) { | |
| // Check if it's a complex built-in key | |
| if (isComplexKey(key as keyof ConfigType)) { | |
| return; // Complex keys can't be represented as attributes | |
| } | |
| // Check if it's a custom config with attribute: false | |
| let customConfig: ReturnType<Config['_getCustomConfigDefinition']> | undefined; | |
| if (this._isCustomConfig(key)) { | |
| customConfig = this._getCustomConfigDefinition(key); | |
| // Skip if attribute is explicitly false (default is true, so flush unless false) | |
| if (customConfig?.attribute === false) { | |
| return; | |
| } | |
| } | |
| // Flush the value to the DOM attributes (works for both built-in and custom configs) | |
| const attrs = this._getAttributeNames(key); | |
| const attrValue = | |
| value === undefined || value === null | |
| ? null | |
| : customConfig?.toAttribute | |
| ? customConfig.toAttribute(value) | |
| : value.toString(); | |
| for (const attr of attrs) { | |
| if (attrValue === null || attrValue === undefined) { | |
| this.removeAttribute(attr); | |
| } else if (this.getAttribute(attr) !== attrValue) { | |
| this.setAttribute(attr, attrValue); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/blocks/Config/Config.ts` around lines 107 - 129, _flushValueToAttribute
currently always calls value.toString(); update it to check for a custom config
definition via this._getCustomConfigDefinition(key) (you already call this in
the earlier check) and, if config?.toAttribute is a function, call
config.toAttribute(value) to produce the serialized attribute value; fall back
to value.toString() only when no toAttribute exists. Preserve the existing
removal logic for undefined/null and only call setAttribute when the serialized
string differs from this.getAttribute(attr). Use the same attribute list from
this._getAttributeNames(key) and keep checks for complex keys and attribute:
false as-is.
| private _updatePluginFileActions(): void { | ||
| const pluginManager = this._sharedInstancesBag.pluginManager; | ||
| if (!pluginManager || !this.uid) { | ||
| this._pluginFileActions = []; | ||
| return; | ||
| } | ||
|
|
||
| const allFileActions = pluginManager.snapshot().fileActions; | ||
| const outputFileEntry = this.api.getOutputItem(this.uid); | ||
|
|
||
| if (!outputFileEntry) { | ||
| this._pluginFileActions = []; | ||
| return; | ||
| } | ||
|
|
||
| this._pluginFileActions = allFileActions.filter((action) => { | ||
| try { | ||
| return action.shouldRender(outputFileEntry); | ||
| } catch (error) { | ||
| console.error(`Error in plugin file action shouldRender (${action.id}):`, error); | ||
| return false; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard getOutputItem when entry is missing to avoid throw.
UploaderPublicApi.getOutputItem throws if the entry doesn’t exist, and _updatePluginFileActions can run when an entry has been removed. That can crash rendering.
Proposed fix
- const outputFileEntry = this.api.getOutputItem(this.uid);
-
- if (!outputFileEntry) {
- this._pluginFileActions = [];
- return;
- }
-
- this._pluginFileActions = allFileActions.filter((action) => {
- try {
- return action.shouldRender(outputFileEntry);
- } catch (error) {
- console.error(`Error in plugin file action shouldRender (${action.id}):`, error);
- return false;
- }
- });
+ try {
+ const outputFileEntry = this.api.getOutputItem(this.uid);
+ this._pluginFileActions = allFileActions.filter((action) => {
+ try {
+ return action.shouldRender(outputFileEntry);
+ } catch (error) {
+ console.error(`Error in plugin file action shouldRender (${action.id}):`, error);
+ return false;
+ }
+ });
+ } catch {
+ this._pluginFileActions = [];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _updatePluginFileActions(): void { | |
| const pluginManager = this._sharedInstancesBag.pluginManager; | |
| if (!pluginManager || !this.uid) { | |
| this._pluginFileActions = []; | |
| return; | |
| } | |
| const allFileActions = pluginManager.snapshot().fileActions; | |
| const outputFileEntry = this.api.getOutputItem(this.uid); | |
| if (!outputFileEntry) { | |
| this._pluginFileActions = []; | |
| return; | |
| } | |
| this._pluginFileActions = allFileActions.filter((action) => { | |
| try { | |
| return action.shouldRender(outputFileEntry); | |
| } catch (error) { | |
| console.error(`Error in plugin file action shouldRender (${action.id}):`, error); | |
| return false; | |
| } | |
| }); | |
| } | |
| private _updatePluginFileActions(): void { | |
| const pluginManager = this._sharedInstancesBag.pluginManager; | |
| if (!pluginManager || !this.uid) { | |
| this._pluginFileActions = []; | |
| return; | |
| } | |
| const allFileActions = pluginManager.snapshot().fileActions; | |
| try { | |
| const outputFileEntry = this.api.getOutputItem(this.uid); | |
| this._pluginFileActions = allFileActions.filter((action) => { | |
| try { | |
| return action.shouldRender(outputFileEntry); | |
| } catch (error) { | |
| console.error(`Error in plugin file action shouldRender (${action.id}):`, error); | |
| return false; | |
| } | |
| }); | |
| } catch { | |
| this._pluginFileActions = []; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/blocks/FileItem/FileItem.ts` around lines 278 - 301, The call to
this.api.getOutputItem(this.uid) can throw when the entry was removed, so wrap
that call in a try/catch (inside _updatePluginFileActions) and if it throws set
this._pluginFileActions = [] and return; then proceed to filter
pluginManager.snapshot().fileActions only when a valid outputFileEntry is
obtained; reference the _updatePluginFileActions method, this.api.getOutputItem
/ UploaderPublicApi.getOutputItem, pluginManager.snapshot().fileActions, and
this._pluginFileActions.
| public activate(): void { | ||
| if (!this.source) return; | ||
| void this.source.onClick(); | ||
| } |
There was a problem hiding this comment.
Handle async onClick failures to avoid unhandled promise rejections.
If a plugin’s onClick throws or rejects, it will surface as an unhandled rejection. Wrapping with Promise.resolve(...).catch(...) keeps failures observable without crashing.
🔧 Suggested fix
public activate(): void {
if (!this.source) return;
- void this.source.onClick();
+ void Promise.resolve(this.source.onClick()).catch((err) => {
+ console.error('[SourceBtn] onClick failed', err);
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public activate(): void { | |
| if (!this.source) return; | |
| void this.source.onClick(); | |
| } | |
| public activate(): void { | |
| if (!this.source) return; | |
| void Promise.resolve(this.source.onClick()).catch((err) => { | |
| console.error('[SourceBtn] onClick failed', err); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/blocks/SourceBtn/SourceBtn.ts` around lines 47 - 50, The activate method
currently calls this.source.onClick() directly which can produce unhandled
promise rejections if onClick is async; change it to invoke the handler via
Promise.resolve(this.source.onClick()).catch(...) to observe failures and avoid
unhandled rejections, and inside the catch log or surface the error (e.g., call
an existing logger or a local error handler) so failures are visible without
crashing; update SourceBtn.activate to use this pattern when calling
this.source.onClick.
tests/plugins/utils.tsx
Outdated
| import { beforeAll, expect } from 'vitest'; | ||
| import { page } from 'vitest/browser'; | ||
| import type { Config, UploadCtxProvider, UploaderPlugin } from '@/index.ts'; | ||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { cleanup, getCtxName, renderer } from '../utils/test-renderer'; | ||
|
|
There was a problem hiding this comment.
Drop unused renderer import.
Static analysis flags it as unused.
🧹 Suggested fix
-import { cleanup, getCtxName, renderer } from '../utils/test-renderer';
+import { cleanup, getCtxName } from '../utils/test-renderer';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { beforeAll, expect } from 'vitest'; | |
| import { page } from 'vitest/browser'; | |
| import type { Config, UploadCtxProvider, UploaderPlugin } from '@/index.ts'; | |
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | |
| import { cleanup, getCtxName, renderer } from '../utils/test-renderer'; | |
| import { beforeAll, expect } from 'vitest'; | |
| import { page } from 'vitest/browser'; | |
| import type { Config, UploadCtxProvider, UploaderPlugin } from '@/index.ts'; | |
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | |
| import { cleanup, getCtxName } from '../utils/test-renderer'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 5-5: Unused variable, import, function or class
Unused import renderer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/plugins/utils.tsx` around lines 1 - 6, The import statement includes an
unused symbol "renderer" in tests/plugins/utils.tsx; remove "renderer" from the
named imports (keep cleanup, getCtxName) in the import line or, if it was
intended to be used, add the actual usage where appropriate—locate the import
that currently reads "import { cleanup, getCtxName, renderer } from
'../utils/test-renderer';" and delete "renderer" to eliminate the unused import.
3da190e to
bbad0a8
Compare
… handle custom activities in a sync way
nd0ut
force-pushed
the
chore/plugin-system
branch
from
3208859 to
08648ba
Compare
…rgery Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…rgery Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
|
||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { renderer } from './utils/test-renderer'; | ||
| import { cleanup, renderer } from './utils/test-renderer'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
In general, unused imports should be removed, or, if the module is needed only for side effects, converted to a side-effect-only import (import './module';). Here, the file clearly wants to import ./utils/test-renderer for its side effects, as indicated by the comment, but does not actually use the renderer binding. To fix this without changing functionality, we should keep the import of cleanup if it is (or will be) used, and drop only the unused renderer binding.
Concretely, in tests/cloud-image-editor.e2e.test.tsx, on line 7 replace import { cleanup, renderer } from './utils/test-renderer'; with import { cleanup } from './utils/test-renderer';. This preserves the module import and any side effects, while removing the unused renderer symbol. No additional methods, imports, or definitions are required for this change.
| @@ -4,7 +4,7 @@ | ||
| import '../types/jsx'; | ||
|
|
||
| // biome-ignore lint/correctness/noUnusedImports: Used in JSX | ||
| import { cleanup, renderer } from './utils/test-renderer'; | ||
| import { cleanup } from './utils/test-renderer'; | ||
|
|
||
| beforeAll(async () => { | ||
| const UC = await import('@/index.js'); |
Description
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests